Skip to content

Conversation

@Manishakumari-hc
Copy link
Collaborator

@Manishakumari-hc Manishakumari-hc commented Oct 10, 2025

Description

  • Validates the content-length & req.Body conditions .
  • Procecces request only when :
    * content-length is greater than 0 & less than int64(s.agent.config.KVMaxValueSize)
    OR
    * when content-length is not passed(=-1) && req.Body size is less than int64(s.agent.config.KVMaxValueSize) .
    • Incase of requests with large body size or content-length the Error is thrown.

Test case:
Test case updated to check the added swich cases in KVSPUT code block .
Screenshot 2025-10-14 at 10 01 51 PM

Signed-off-by: Manisha Kumari <[email protected]>
@Manishakumari-hc Manishakumari-hc requested review from a team as code owners October 10, 2025 11:04
@Manishakumari-hc Manishakumari-hc changed the title SECVULN-29092 Dos handled SECVULN-29092 DOS handled Oct 10, 2025
@Manishakumari-hc Manishakumari-hc changed the title SECVULN-29092 DOS handled SECVULN-29092 DoS handled for kvs_endpoint.go Oct 10, 2025
@Manishakumari-hc Manishakumari-hc added backport/all Apply backports for all active releases per .release/versions.hcl pr/no-changelog PR does not need a corresponding .changelog entry labels Oct 10, 2025
@Manishakumari-hc Manishakumari-hc self-assigned this Oct 10, 2025
Signed-off-by: Manisha Kumari <[email protected]>
StatusCode: http.StatusRequestEntityTooLarge,
Reason: fmt.Sprintf("Request body too large, max allowed is %d bytes.", maxSize),
}
}
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.

Reason: fmt.Sprintf("Request body too large, max allowed is %d bytes.", maxSize),
}
}

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.

buf := bytes.NewBuffer(nil)
if _, err := io.Copy(buf, req.Body); err != nil {
// LimitReader to limit copy of large requests with no Content-Length
limitedReader := io.LimitReader(req.Body, maxSize+1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is maxSize+1 needed?

}

// Reject request if actual read size exceeds allowed limit
if copiedBytes > maxSize {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are already restricting copiedBytes then why do we need this condition?

Copy link
Collaborator

@dduzgun-security dduzgun-security left a comment

Choose a reason for hiding this comment

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

I'm not sure this resolves the issue. The problem here is when the Content-Length is omitted. The condition should check that the Content-Length is present, greater than zero and bellow the KVMaxValueSize and if not, it throws an error message. Please include unit tests to this too.

We also need a changelog for this that mentions the CVE this fix will resolve: CVE-2025-11374.

maxSize := int64(s.agent.config.KVMaxValueSize)

switch {
case req.ContentLength < 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
case req.ContentLength < 0:
case !req.ContentLength || req.ContentLength <= 0:

CHANGELOG.md Outdated

SECURITY:

* security: Fix Consul's KV endpoint is vulnerable to denial of service address CVE-2025-11374 [[GH-22916](https://github.com/hashicorp/consul/pull/22916)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not where we add changelog.

maxSize := int64(s.agent.config.KVMaxValueSize)

switch {
case req.ContentLength <= 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Check if contentlength is omitted. If omitted, raise error

Copy link
Collaborator Author

@Manishakumari-hc Manishakumari-hc Oct 13, 2025

Choose a reason for hiding this comment

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

In the case of no content-length it is taken as content-lenth=-1 .So it is handled here @sanikachavan5

Signed-off-by: Manisha Kumari <[email protected]>
@@ -0,0 +1,3 @@
```release-note:security
security: Consul's KV endpoint is vulnerable to denial of service [CVE-2025-11374]()
Copy link
Contributor

Choose a reason for hiding this comment

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

Improved validation of the Content-Length header in the Consul KV endpoint to prevent potential denial of service attacks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added.

@Manishakumari-hc Manishakumari-hc removed the pr/no-changelog PR does not need a corresponding .changelog entry label Oct 13, 2025
@Manishakumari-hc
Copy link
Collaborator Author

I'm not sure this resolves the issue. The problem here is when the Content-Length is omitted. The condition should check that the Content-Length is present, greater than zero and bellow the KVMaxValueSize and if not, it throws an error message. Please include unit tests to this too.

We also need a changelog for this that mentions the CVE this fix will resolve: CVE-2025-11374.

@eastebry There is no URL for this CVE yet. Do you want me to modify the changelog ?

eastebry
eastebry previously approved these changes Oct 13, 2025
Copy link

@eastebry eastebry left a comment

Choose a reason for hiding this comment

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

Thanks for making this change!

I believe this check is correct, and this will fix this issue (more details on that at the bottom), however I am wondering if we would be better off using a MaxByteReader to read the body and enforce the limit. There may be trade offs to this approach, so I'll leave the final decision to you all.

Regarding this specific check, I believe this is correct. Here are the cases I was concerned about and explicitly tested:

  • If a Content-Length header is not set, it appears that Go will infer based on the body, or set it to -1 if the request is chunked.
  • If Content-Length is set to a different value than the size of the body, the server will either truncate the body, or hang while it waits for the additional bytes to be sent.

Based on that, this code appears to work properly. I do wonder if we would be better off not performing this check ourselves, and relying on a MaxByteReader.

@Manishakumari-hc
Copy link
Collaborator Author

Thanks for making this change!

I believe this check is correct, and this will fix this issue (more details on that at the bottom), however I am wondering if we would be better off using a MaxByteReader to read the body and enforce the limit. There may be trade offs to this approach, so I'll leave the final decision to you all.

Regarding this specific check, I believe this is correct. Here are the cases I was concerned about and explicitly tested:

  • If a Content-Length header is not set, it appears that Go will infer based on the body, or set it to -1 if the request is chunked.
  • If Content-Length is set to a different value than the size of the body, the server will either truncate the body, or hang while it waits for the additional bytes to be sent.

Based on that, this code appears to work properly. I do wonder if we would be better off not performing this check ourselves, and relying on a MaxByteReader.

Updated to MaxByteReader

@sanikachavan5 sanikachavan5 added backport/ent/1.18 Changes are backported to 1.18 ent backport/ent/1.20 backport to ent 1.20 backport/ent/1.21 changes are backported to 1.21 ent backport/1.22 Changes are backported to 1.22 labels Oct 14, 2025
@Manishakumari-hc Manishakumari-hc removed the request for review from dduzgun-security October 14, 2025 16:59
@Manishakumari-hc Manishakumari-hc dismissed dduzgun-security’s stale review October 14, 2025 17:21

addressed the change and other reviewrs have reviewd.

@Manishakumari-hc Manishakumari-hc merged commit 72a358c into main Oct 14, 2025
189 of 195 checks passed
@hc-github-team-consul-core hc-github-team-consul-core added backport/1.21 This release series is longer active on CE, use backport/ent/1.21 backport/ent/1.19 Changes are backported to 1.19 ent labels Oct 14, 2025
@sanikachavan5 sanikachavan5 added backport/1.22 Changes are backported to 1.22 and removed backport/1.22 Changes are backported to 1.22 labels Oct 14, 2025
@hc-github-team-consul-core
Copy link
Collaborator

📣 Hi @Manishakumari-hc! a backport is missing for this PR [22916] for versions [1.18,1.19,1.20,1.21] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

5 similar comments
@hc-github-team-consul-core
Copy link
Collaborator

📣 Hi @Manishakumari-hc! a backport is missing for this PR [22916] for versions [1.18,1.19,1.20,1.21] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@hc-github-team-consul-core
Copy link
Collaborator

📣 Hi @Manishakumari-hc! a backport is missing for this PR [22916] for versions [1.18,1.19,1.20,1.21] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@hc-github-team-consul-core
Copy link
Collaborator

📣 Hi @Manishakumari-hc! a backport is missing for this PR [22916] for versions [1.18,1.19,1.20,1.21] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@hc-github-team-consul-core
Copy link
Collaborator

📣 Hi @Manishakumari-hc! a backport is missing for this PR [22916] for versions [1.18,1.19,1.20,1.21] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@hc-github-team-consul-core
Copy link
Collaborator

📣 Hi @Manishakumari-hc! a backport is missing for this PR [22916] for versions [1.18,1.19,1.20,1.21] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

@sarah-oloumi
Copy link

@dduzgun-security Where is CVE-2025-11374 coming from? I'm reading the release notes for the pre-release and when googling CVE-2025-11374 I don't see anything. Is this a consul specific CVE? There is not even a NIST page on it... https://nvd.nist.gov/vuln/detail/CVE-2025-11374 leads to nothing

Same thing for https://nvd.nist.gov/vuln/detail/CVE-2025-11375 and for https://nvd.nist.gov/vuln/detail/CVE-2025-11392 @hc-github-team-consul-core

How can we get more info on the CVSS for these CVEs?

@dduzgun-security
Copy link
Collaborator

Hi @sarah-oloumi , the CVE is reserved by us under the HashiCorp CNA and will be published at the same time as the next Consul release of version 1.22.

@hc-github-team-consul-core
Copy link
Collaborator

📣 Hi @Manishakumari-hc! a backport is missing for this PR [22916] for versions [1.18,1.19,1.20,1.21] please perform the backport manually and add the following snippet to your backport PR description:

<details>
	<summary> Overview of commits </summary>
		- <<backport commit 1>>
		- <<backport commit 2>>
		...
</details>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/all Apply backports for all active releases per .release/versions.hcl backport/ent/1.18 Changes are backported to 1.18 ent backport/ent/1.19 Changes are backported to 1.19 ent backport/ent/1.20 backport to ent 1.20 backport/ent/1.21 changes are backported to 1.21 ent backport/1.21 This release series is longer active on CE, use backport/ent/1.21 backport/1.22 Changes are backported to 1.22

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants