Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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 @@ -234,19 +234,46 @@ 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
limitedReader := io.LimitReader(req.Body, maxSize+1)
buf = new(bytes.Buffer)
copiedBytes, err := io.Copy(buf, limitedReader)
if err != nil {
return nil, err
}
// Reject request if actual read size exceeds allowed maxsize limit
if copiedBytes > maxSize {
return nil, HTTPError{
StatusCode: http.StatusRequestEntityTooLarge,
Reason: fmt.Sprintf("Request body too large.Max body and content-length allowed is %d bytes.", maxSize),
}
}

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
1 change: 1 addition & 0 deletions agent/kvs_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,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
Loading