Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/22916.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:security
security: Improved validation of the Content-Length header in the Consul KV endpoint to prevent potential denial of service attacks[CVE-2025-11374]()
```
41 changes: 34 additions & 7 deletions agent/kvs_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package agent

import (
"bytes"
"errors"
"fmt"
"io"
"net/http"
Expand Down Expand Up @@ -234,19 +235,45 @@ func (s *HTTPHandlers) KVSPut(resp http.ResponseWriter, req *http.Request, args
}

// Check the content-length
if req.ContentLength > int64(s.agent.config.KVMaxValueSize) {
maxSize := int64(s.agent.config.KVMaxValueSize)
var buf *bytes.Buffer

switch {
case req.ContentLength <= 0 && req.Body == nil:
return "Request has no content-length & no body", nil

case req.ContentLength <= 0 && req.Body != nil:
// LimitReader to limit copy of large requests with no Content-Length
//+1 ensures we can detect if the body is too large
byteReader := http.MaxBytesReader(nil, req.Body, maxSize+1)
buf = new(bytes.Buffer)
if _, err := io.Copy(buf, byteReader); err != nil {
var bodyTooLargeErr *http.MaxBytesError
if errors.As(err, &bodyTooLargeErr) {
return nil, HTTPError{
StatusCode: http.StatusRequestEntityTooLarge,
Reason: fmt.Sprintf("Request body too large. Max allowed is %d bytes.", maxSize),
}
}
return nil, err
}

case req.ContentLength > maxSize:
// Throw error if Content-Length is greater than max size
return nil, HTTPError{
StatusCode: http.StatusRequestEntityTooLarge,
Reason: fmt.Sprintf("Request body(%d bytes) too large, max size: %d bytes. See %s.",
req.ContentLength, s.agent.config.KVMaxValueSize, "https://developer.hashicorp.com/docs/agent/config/config-files#kv_max_value_size"),
req.ContentLength, maxSize, "https://developer.hashicorp.com/docs/agent/config/config-files#kv_max_value_size"),
}
}

// Copy the value
buf := bytes.NewBuffer(nil)
if _, err := io.Copy(buf, req.Body); err != nil {
return nil, err
default:
// Copy the value
buf = bytes.NewBuffer(nil)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion was to make of MaxBytesReader down here, so that we are only reading the body once.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code still read it only once right ?
Either in the first case req.ContentLength <= 0 && req.Body != nil:
OR default:
I think this is more readable !
Can you please confirm ?

Copy link
Collaborator Author

@Manishakumari-hc Manishakumari-hc Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eastebry if the code looks fine to meet the functionality. Can we go ahead with the merge today ? As the code freeze starts today .
We have only this release before due date.
If you ask we can do the code optimizations in 1.22.1 release in mid-nov

Copy link

@eastebry eastebry Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m sorry, I’m not being very clear here.

At the moment, you are reading the body at two places in the switch statement, once within a case where you check the content length is -1 (indicating chunking), and once in the default case, after the size check has been performed.

I am trying to ask if there is a need for the switch statement at all? If we remove all cases and just use the MaxByteReader to enforce that we never read more than the specified number of bytes, we don't need to check to content length header at all. If the MaxByteReader throws an error, then we raise an HttpError.

Does that make sense?

if _, err := io.Copy(buf, req.Body); err != nil {
return nil, err
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add jira tickets to oss repo prs.


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests for this.

applyReq.DirEnt.Value = buf.Bytes()

// Make the RPC
Expand Down
92 changes: 92 additions & 0 deletions agent/kvs_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package agent
import (
"bytes"
"fmt"
"io"
"net/http"
"net/http/httptest"
"path"
Expand Down Expand Up @@ -37,6 +38,7 @@ func TestKVSEndpoint_PUT_GET_DELETE(t *testing.T) {
for _, key := range keys {
buf := bytes.NewBuffer([]byte("test"))
Copy link

@santoshpulluri santoshpulluri Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure test cases are covered for all the scenarios (basically 3 switch cases as well as default case.)

req, _ := http.NewRequest("PUT", "/v1/kv/"+key, buf)
req.ContentLength = int64(buf.Len())
resp := httptest.NewRecorder()
obj, err := a.srv.KVSEndpoint(resp, req)
if err != nil {
Expand Down Expand Up @@ -554,6 +556,96 @@ func TestKVSEndpoint_GET(t *testing.T) {
}
}

func TestKVSPUT_SwitchCases(t *testing.T) {
if testing.Short() {
t.Skip("skipping test in short mode")
}

t.Parallel()
a := NewTestAgent(t, "")
defer a.Shutdown()

maxSize := int(a.srv.agent.config.KVMaxValueSize)

tests := []struct {
name string
body string
contentLength int64
expectErr bool
expectMsg string
expectHTTPMsg string
}{
{
name: "Case 2: No Content-Length but Body exists (allowed size)",
body: "small-value",
contentLength: 0,
expectErr: false,
},
{
name: "Case 2b: No Content-Length but Body exists (too large)",
body: strings.Repeat("x", maxSize+50),
contentLength: 0,
expectErr: true,
expectHTTPMsg: fmt.Sprintf("Request body too large. Max allowed is %d bytes.", maxSize),
},
{
name: "Case 3: Content-Length greater than max allowed limit",
body: strings.Repeat("x", maxSize+10),
contentLength: int64(maxSize) + 10,
expectErr: true,
expectHTTPMsg: fmt.Sprintf("Request body(%d bytes) too large, max size: %d bytes.", int64(maxSize)+10, maxSize),
},
{
name: "Case 4: Normal body within allowed limit",
body: "tiny",
contentLength: 4,
expectErr: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var bodyReader io.Reader
if tt.body != "" {
bodyReader = bytes.NewBufferString(tt.body)
} else {
bodyReader = nil
}

req := httptest.NewRequest(http.MethodPut, "/v1/kv/switch-test", bodyReader)
req.ContentLength = tt.contentLength
resp := httptest.NewRecorder()

obj, err := a.srv.KVSEndpoint(resp, req)

// Expected error cases
if tt.expectErr {
if err == nil {
t.Fatalf("expected error, got nil")
}
httpErr, ok := err.(HTTPError)
if !ok {
t.Fatalf("expected HTTPError, got %T", err)
}
if !strings.Contains(httpErr.Reason, tt.expectHTTPMsg[:20]) { // partial match
t.Fatalf("expected HTTPError reason to contain %q, got %q", tt.expectHTTPMsg, httpErr.Reason)
}
return
}

// Unexpected error
if err != nil {
t.Fatalf("unexpected error: %v", err)
}

// Normal successful PUT result
if res, ok := obj.(bool); !ok || !res {
t.Fatalf("expected successful PUT result, got %v", obj)
}
})
}
}

func TestKVSEndpoint_DELETE_ConflictingFlags(t *testing.T) {
if testing.Short() {
t.Skip("too slow for testing.Short")
Expand Down
Loading