Skip to content
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

net/http: drop support for any other TransferEncoding value than "chunked" #38867

Open
FiloSottile opened this issue May 5, 2020 · 1 comment
Open
Labels
Milestone

Comments

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented May 5, 2020

As of RFC 7230, "identity" is not a thing anymore, and we are probably off-spec for any other value. nginx only supports "chunked" anyway. There is a lot of code that can be removed or simplified in the sending path. (The parsing path is already being made strict as security hardening.)

However, we document setting TransferEncoding to "identity" as the way to affect some automatic chunking behavior, so we should probably still catch it at a shallow level, and behave accordingly, even if we don't send it on the wire.

@FiloSottile FiloSottile added the NeedsFix label May 5, 2020
@FiloSottile FiloSottile added this to the Unplanned milestone May 5, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 5, 2020

Change https://golang.org/cl/231418 mentions this issue: net/http: only support "chunked" in inbound Transfer-Encoding headers

gopherbot pushed a commit that referenced this issue May 6, 2020
This is a security hardening measure against HTTP request smuggling.
Thank you to ZeddYu for reporting this issue.

We weren't parsing things correctly anyway, allowing "identity" to be
combined with "chunked", and ignoring any Transfer-Encoding header past
the first. This is a delicate security surface that already broke
before, just be strict and don't add complexity to support cases not
observed in the wild (nginx removed "identity" support [1] and multiple
TE header support [2]) and removed by RFC 7230 (see page 81).

It'd probably be good to also drop support for anything other than
"chunked" in outbound TE headers, as "identity" is not a thing anymore,
and we are probably off-spec for anything other than "chunked", but it
should not be a security concern, so leaving it for now. See #38867.

[1]: https://hg.nginx.org/nginx/rev/fe5976aae0e3
[2]: https://hg.nginx.org/nginx/rev/aca005d232ff

Change-Id: If17d0827f9c6167a0b19a158e2bc5844ec803288
Reviewed-on: https://go-review.googlesource.com/c/go/+/231418
Reviewed-by: Katie Hockman <katie@golang.org>
xujianhai666 added a commit to xujianhai666/go-1 that referenced this issue May 21, 2020
This is a security hardening measure against HTTP request smuggling.
Thank you to ZeddYu for reporting this issue.

We weren't parsing things correctly anyway, allowing "identity" to be
combined with "chunked", and ignoring any Transfer-Encoding header past
the first. This is a delicate security surface that already broke
before, just be strict and don't add complexity to support cases not
observed in the wild (nginx removed "identity" support [1] and multiple
TE header support [2]) and removed by RFC 7230 (see page 81).

It'd probably be good to also drop support for anything other than
"chunked" in outbound TE headers, as "identity" is not a thing anymore,
and we are probably off-spec for anything other than "chunked", but it
should not be a security concern, so leaving it for now. See golang#38867.

[1]: https://hg.nginx.org/nginx/rev/fe5976aae0e3
[2]: https://hg.nginx.org/nginx/rev/aca005d232ff

Change-Id: If17d0827f9c6167a0b19a158e2bc5844ec803288
Reviewed-on: https://go-review.googlesource.com/c/go/+/231418
Reviewed-by: Katie Hockman <katie@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.