Skip to content

net/http: reject bare LF in chunked encoding #71988

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

Closed
neild opened this issue Feb 26, 2025 · 17 comments
Closed

net/http: reject bare LF in chunked encoding #71988

neild opened this issue Feb 26, 2025 · 17 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. Critical A critical problem that affects the availability or correctness of production systems built using Go NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone

Comments

@neild
Copy link
Contributor

neild commented Feb 26, 2025

HTTP/1 uses CRLF as a line terminator, but permits implementations to accept a bare LF as a terminator in certain locations. It does not permit a bare LF to be used in the chunked encoding, however. (See https://www.rfc-editor.org/errata/eid7633, in particular the notes on why the proposed errata was rejected.)

We reject bare LFs ending chunk-data lines, but accept them in chunk-size lines. This can, if combined with an implementation that incorrectly permits a bare CR in a chunk-ext, permit request smuggling.

We should reject bare LFs in chunk-data lines.

This is a PUBLIC track security issue and CVE-2025-22871.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/652998 mentions this issue: net/http: reject newlines in chunk-size lines

@gabyhelp
Copy link

Related Code Changes

(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Feb 26, 2025
@neild
Copy link
Contributor Author

neild commented Feb 27, 2025

@gopherbot please open backport issues for this security fix.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #72010 (for 1.23), #72011 (for 1.24).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@dmitshur dmitshur added this to the Go1.25 milestone Feb 27, 2025
@dmitshur dmitshur added Security NeedsFix The path to resolution is known, but the work has not been done. labels Feb 27, 2025
@tsuna
Copy link
Contributor

tsuna commented Mar 5, 2025

Can you clarify what you meant by "This can, if combined with an implementation that incorrectly permits a bare CR in a chunk-ext, permit request smuggling." — which "implementation" are we talking about here?

@JunyangShao JunyangShao added the Critical A critical problem that affects the availability or correctness of production systems built using Go label Mar 5, 2025
@neild
Copy link
Contributor Author

neild commented Mar 5, 2025

If you have one HTTP implementation which permits a bare LF in the chunk-ext, and another that interprets a bare LF as a line terminator, you can smuggle a request through a proxy using one implementation to a server using the other.

You send a request body consisting of something like: length, ; (starting a chunk-ext), LF (implementation 1 thinks this is part of the chunk-ext, implementation 2 thinks this is the end of the chunk-size line), some carefully-chosen number of bytes (implementation 1 thinks this is the chunk-ext, implementation 2 thinks this is the body), CR LF 0 CR LF (implementation 1 thinks this is chunk-data, implementation 2 thinks this is the last-chunk), and headers for a new request (implementation 1 thinks this is chunk-data, implementation 2 thinks this is a new request).

Permitting an LF in the chunk-ext is a very uncommon bug.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/657216 mentions this issue: [release-branch.go1.23] net/http: reject newlines in chunk-size lines

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/657056 mentions this issue: [release-branch.go1.24] net/http: reject newlines in chunk-size lines

gopherbot pushed a commit that referenced this issue Mar 17, 2025
Unlike request headers, where we are allowed to leniently accept
a bare LF in place of a CRLF, chunked bodies must always use CRLF
line terminators. We were already enforcing this for chunk-data lines;
do so for chunk-size lines as well. Also reject bare CRs anywhere
other than as part of the CRLF terminator.

Fixes CVE-2025-22871
Fixes #72010
For #71988

Change-Id: Ib0e21af5a8ba28c2a1ca52b72af8e2265ec79e4a
Reviewed-on: https://go-review.googlesource.com/c/go/+/652998
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit d31c805)
Reviewed-on: https://go-review.googlesource.com/c/go/+/657216
gopherbot pushed a commit that referenced this issue Mar 18, 2025
Unlike request headers, where we are allowed to leniently accept
a bare LF in place of a CRLF, chunked bodies must always use CRLF
line terminators. We were already enforcing this for chunk-data lines;
do so for chunk-size lines as well. Also reject bare CRs anywhere
other than as part of the CRLF terminator.

Fixes CVE-2025-22871
Fixes #72011
For #71988

Change-Id: Ib0e21af5a8ba28c2a1ca52b72af8e2265ec79e4a
Reviewed-on: https://go-review.googlesource.com/c/go/+/652998
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit d31c805)
Reviewed-on: https://go-review.googlesource.com/c/go/+/657056
@FiloSottile
Copy link
Contributor

This can, if combined with an implementation that incorrectly permits a bare CR in a chunk-ext, permit request smuggling.

Just to make sure I am parsing this correctly (pun not intended), this was meant to be "bare LF", not "bare CF", right?

@neild
Copy link
Contributor Author

neild commented Apr 3, 2025

Just to make sure I am parsing this correctly (pun not intended), this was meant to be "bare LF", not "bare CF", right?

Yes, sorry. If an implementation permits a bare LF in the chunk-ext (very uncommon), then request smuggling can occur between it and pre-fix net/http.

@DemiMarie
Copy link
Contributor

Does permitting a bare LF to end the chunk data (instead of the required CRLF) also permit request smuggling?

@neild
Copy link
Contributor Author

neild commented Apr 6, 2025

Does permitting a bare LF to end the chunk data (instead of the required CRLF) also permit request smuggling?

I don't think so, but I might well be missing something.

LeSuisse added a commit to Enalean/tuleap that referenced this issue Apr 9, 2025
No security impact for our use cases, it can wait we bump our global pin
past this Go upgrade.

golang/go#71988

Change-Id: I9f55c6c772fda54d3964a99367144b613053c88a
@DemiMarie
Copy link
Contributor

@neild Can you provide any specific examples of reverse proxies that have this bug? NGINX allows chunk extensions to end with bare LF and I am wondering if there is a practically exploitable vulnerability here.

@neild
Copy link
Contributor Author

neild commented Apr 9, 2025

I'm afraid I don't have any examples I can share.

@jkaurredhat
Copy link

@neild Hey, Can this be backported to 1.22 as well ?

@seankhliao
Copy link
Member

No, only the 2 most recent major releases are supported.

@antbob
Copy link

antbob commented May 14, 2025

@jkaurredhat for what its worth the .23 patch Commit 15e01a2 applies cleanly on top of 1.20.14 and almost cleanly on top of 1.18.10 (needs manual apply in one of the tests) so you should have no problem with 1.22 either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. Critical A critical problem that affects the availability or correctness of production systems built using Go NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Development

No branches or pull requests

12 participants