-
Notifications
You must be signed in to change notification settings - Fork 4.5k
SECVULN-29092 DoS handled for kvs_endpoint.go #22916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
635cda0
14f630c
9ebb5e0
db69840
b8fa0b4
6ed93a9
ebec61b
37727e3
3a0d477
870f498
1542fbf
91475c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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]() | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ package agent | |
|
|
||
| import ( | ||
| "bytes" | ||
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
|
|
@@ -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) | ||
| if _, err := io.Copy(buf, req.Body); err != nil { | ||
| return nil, err | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't add jira tickets to oss repo prs. |
||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ package agent | |
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "io" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "path" | ||
|
|
@@ -37,6 +38,7 @@ func TestKVSEndpoint_PUT_GET_DELETE(t *testing.T) { | |
| for _, key := range keys { | ||
| buf := bytes.NewBuffer([]byte("test")) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
|
@@ -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") | ||
|
|
||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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?