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

http request smuggling, cause by obfuscating TE header #9571

Closed
axeBig opened this issue Sep 16, 2019 · 37 comments · Fixed by #9585
Closed

http request smuggling, cause by obfuscating TE header #9571

axeBig opened this issue Sep 16, 2019 · 37 comments · Fixed by #9585
Milestone

Comments

@axeBig
Copy link

axeBig commented Sep 16, 2019

Expected behavior

ignore obfuscating TE header("Transfer-Encoding : chunked" vs "Transfer-Encoding: chunked")

Actual behavior

use Transfer-Encoding[space] as Transfer-Encoding

Steps to reproduce

1、topology: client→elb→nettyServer
2、client send a request with both content-length and trunked-encoded[space]
3、elb ignored trunked-encoded[space], but use content-length
4、netty use trunked-encoded[space]

Minimal yet complete reproducer code (or URL to code)

when header field end with space but not colon, shoud the space be ignored?
can not found proof in https://greenbytes.de/tech/webdav/rfc7230.html#header.fields.

code in io.netty.handler.codec.http.HttpObjectDecoder#splitHeader

for (nameEnd = nameStart; nameEnd < length; nameEnd ++) {
            char ch = sb.charAt(nameEnd);
            if (ch == ':' || Character.isWhitespace(ch)) {
                break;
            }
 }

Netty version

all

JVM version (e.g. java -version)

OS version (e.g. uname -a)

@amizurov
Copy link
Contributor

amizurov commented Sep 18, 2019

Hi, it think the "Transfer-Encoding : chunked" header name with trailing whitespace is incorrect, but I'm not sure, maybe @slandelle can help.

@normanmaurer
Copy link
Member

Also generally speaking I would upgrade to the latest netty version first and try to reproduce with it. The version you are using is very old.

@slandelle
Copy link
Contributor

when header field end with space but not colon, should the space be ignored?
can not found proof in https://greenbytes.de/tech/webdav/rfc7230.html#header.fields.

Actual source: https://tools.ietf.org/html/rfc7230#page-22):

Each header field consists of a case-insensitive field name followed by a colon (":"), optional leading whitespace, the field value, and optional trailing whitespace.

It looks clear to me that your header name must be parsed as "Transfer-Encoding " with a trailing white space, so it's expected to not be handled as "Transfer-Encoding" by Netty (or any other solution that follows standards).

IMHO, this is a bug in your client and you should get this fixed there.

@axeBig
Copy link
Author

axeBig commented Sep 18, 2019

Also generally speaking I would upgrade to the latest netty version first and try to reproduce with it. The version you are using is very old.

4.1.32.Final is in my project, and 4.1.41.final(https://github.com/netty/netty/blob/netty-4.1.41.Final/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java) is the same like ch == ':' || Character.isWhitespace(ch))

@axeBig
Copy link
Author

axeBig commented Sep 18, 2019

when header field end with space but not colon, should the space be ignored?
can not found proof in https://greenbytes.de/tech/webdav/rfc7230.html#header.fields.

Actual source: https://tools.ietf.org/html/rfc7230#page-22):

Each header field consists of a case-insensitive field name followed by a colon (":"), optional leading whitespace, the field value, and optional trailing whitespace.

It looks clear to me that your header name must be parsed as "Transfer-Encoding " with a trailing white space, so it's expected to not be handled as "Transfer-Encoding" by Netty (or any other solution that follows standards).

IMHO, this is a bug in your client and you should get this fixed there.

some guy attack elb by both having TE[space] and CL. elb decode request by CL but netty decoded by TE[space]. this case some tail of the attack request inflect the next request(https://portswigger.net/web-security/request-smuggling).

@slandelle
Copy link
Contributor

Ah OK!!! Sorry, I originally didn't get it.

So I agree Netty's HttpObjectDecoder shouldn't trim header name.

I don't know why it removes this whitespace, and I couldn't find way as things have been this way since the project has a git history.

I'm in favor of removing this || Character.isWhitespace(ch).

@normanmaurer WDYT? Have you even seen legit user agents crafting HTTP headers with a whitespace before the :?

@normanmaurer
Copy link
Member

@slandelle nope... Can someone just do a PR with a unit test ? I currently super busy :(

@Bi3g0
Copy link

Bi3g0 commented Sep 19, 2019

In order to fix the problem easily, I drew a topology of vulnerability principle below.

image
And how did elb deal with Incorrect TE
1)Attack request

POST /getusers HTTP/1.1
Host: www.backend.com
Content-Length: 64
Transfer-Encoding : chunked

0

GET /hacker HTTP/1.1
Host: www.hacker.com
hacker: hacker

2)ELB processed request
image

@axeBig
Copy link
Author

axeBig commented Sep 19, 2019

after a attemp, keep the space will colfict the validate rule in defaultHttpHeader(like header can not contain s=,;: ...), and the left body will also inflect the next request.
simple change TE[space] to other name like(Illegal-TE) will help.

by the way, we(me and @Bi3g0) are a team, can we record this on CVE

@slandelle
Copy link
Contributor

It looks to me having a trailing white space in the header name is legit as per RFC7230, so IMHO, it shouldn't be renamed.
The core issue is to leave such header name as is so it's not interpreted as the trimmed version.

@normanmaurer
Copy link
Member

yeah... @slandelle can you provide a pr ?

@Bi3g0
Copy link

Bi3g0 commented Sep 19, 2019

It looks to me having a trailing white space in the header name is legit as per RFC7230, so IMHO, it shouldn't be renamed.
The core issue is to leave such header name as is so it's not interpreted as the trimmed version.

Hi, I think we can reference rfc7230#section-3.2.4

https://tools.ietf.org/html/rfc7230#section-3.2.4

No whitespace is allowed between the header field-name and colon. In
the past, differences in the handling of such whitespace have led to
security vulnerabilities in request routing and response handling. A
server MUST reject any received request message that contains
whitespace between a header field-name and colon with a response code
of 400 (Bad Request). A proxy MUST remove any such whitespace from a
response message before forwarding the message downstream.

@normanmaurer
Copy link
Member

@Bi3g0 so what you think we should do ? I'm a bit confused atm :/

@Bi3g0
Copy link

Bi3g0 commented Sep 19, 2019

@normanmaurer
Hi,
I think Netty can identify TE[space]: field value as an incorrect header or a custom header.

1)Netty can identify TE[space]: field value as a custom header, Netty should sperate requests by CL and forward the whole header TE[space]: field value , like elb.
#9571 (comment)

2)Or Netty can identifyTE[space]: field value as an incorrect header, return a response code of 400 (Bad Request).

We(@axeBig and me) just renamed TE[space] as an emergency measure, this helps us cope with the current situation.

@slandelle
Copy link
Contributor

@Bi3g0 I stand corrected, I wasn’t aware of this update :P.
So the thing is the expected behavior depends on if such header happens in request or response. Maybe leaving as is in the parser and handling in HttpHeaders validation would do the trick? The main thing is to be able to automatically reply with 400 for requests, not just silently ditch invalid header.
Will try to have a look later today.

@trustin
Copy link
Member

trustin commented Sep 19, 2019

We have a property (DcoderResult?) that indicates whether the decoding was successful or not IIRC. How about using that, instead of automatically responding with 400? That way, a user could choose to 1) send a custom 400 response page, 2) just disconnect, or 3) do something interesting (like looking into the bad headers).

@Bi3g0
Copy link

Bi3g0 commented Sep 19, 2019

@trustin
Hi,
I probably understand what you mean. This is a nice alternative with more options. I think netty preferably has a default option to handle incorrect TE, such as forwarding the incorrect TE to the backend or automatically returning 400, because not all netty users can use the property (DcoderResult?) correctly and safely.

@normanmaurer
Copy link
Member

I agree with @trustin ... I think we should just send a "InvalidMessage" down the pipeline... @slandelle WDYT ?

@slandelle
Copy link
Contributor

Do you mean having a special MalformedHttpHeaderException passed through DecoderResult?
If so, lgtm.

@normanmaurer
Copy link
Member

normanmaurer commented Sep 19, 2019 via email

@normanmaurer
Copy link
Member

@slandelle can you take this one ?

@slandelle
Copy link
Contributor

@normanmaurer Yes if you can wait a few days.

normanmaurer added a commit that referenced this issue Sep 20, 2019
…30#section-3.2.4

Motivation:

When parsing HTTP headers special care needs to be taken when a whitespace is detected in the header name.

Modifications:

- Ignore whitespace when decoding response (just like before)
- Throw exception when whitespace is detected during parsing
- Add unit tests

Result:

Fixes #9571
@normanmaurer
Copy link
Member

@trustin @slandelle @axeBig @Bi3g0 please check #9585

@normanmaurer
Copy link
Member

normanmaurer commented Sep 20, 2019 via email

@axeBig
Copy link
Author

axeBig commented Sep 20, 2019

when decode fail, we user decide what to do. looks fine to me.

@Bi3g0
Copy link

Bi3g0 commented Sep 20, 2019

We just did a test and the problem has been fixed. When I sent an incorrect TE, netty returned 400. But by default netty will not disconnect the TCP connection, which may cause other problems, such as denial of service.

When users of netty do not deal with 400, other requests within TCP connections will not be responded to, or may consume additional network connections from servers.

#9571 (comment)

@slandelle
Copy link
Contributor

When users of netty do not deal with 400

IMHO, Netty is a low level IO framework, not a web framework and it's the user's responsibility to deal with this situation.

@Bi3g0
Copy link

Bi3g0 commented Sep 20, 2019

Okay, then there's no problem.

normanmaurer added a commit that referenced this issue Sep 20, 2019
…30#section-3.2.4 (#9585)

Motivation:

When parsing HTTP headers special care needs to be taken when a whitespace is detected in the header name.

Modifications:

- Ignore whitespace when decoding response (just like before)
- Throw exception when whitespace is detected during parsing
- Add unit tests

Result:

Fixes #9571
normanmaurer added a commit that referenced this issue Sep 20, 2019
…30#section-3.2.4 (#9585)

Motivation:

When parsing HTTP headers special care needs to be taken when a whitespace is detected in the header name.

Modifications:

- Ignore whitespace when decoding response (just like before)
- Throw exception when whitespace is detected during parsing
- Add unit tests

Result:

Fixes #9571
@normanmaurer normanmaurer added this to the 4.1.42.Final milestone Sep 25, 2019
@abergmann
Copy link

CVE-2019-16869 was assigned to this issue.

@sunweaver
Copy link

@abergmann: I am currently investigating this CVE for Debian LTS, I found another location in the code that looks pretty similar. Does it need that fix, too?
https://github.com/netty/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java#L1451

@ddillard
Copy link

Can someone explain why the CVE description says this affects versions before 4.1.42, but the CPE says it affects versions before 4.1.36???

@sbstnc
Copy link

sbstnc commented Oct 7, 2019

@ddillard The description is correct. The bugfix is included in 4.1.42. See: 39cafcb

@slandelle
Copy link
Contributor

Looks like CVE is incorrect and reports fixed in 4.1.36.

cve

@SirMaster
Copy link

Question, will / could there be fix for the 3.10.6 version coming out as well?

@normanmaurer
Copy link
Member

normanmaurer commented Nov 9, 2019 via email

@fabianedl777
Copy link

Hi,

I am using spring webflux 2.2.2.RELEASE with netty 4.1.43.Final, in an application that has 2 instances and is deployed in kubernetes and I have found this vulnerability when the TE is the first header and this header has a blank before the header name for example [space] Transfer-Encoding: chunked; reviewing the source code on the next line


, blanks at the beginning of the header are excluded and for this reason the request continues and the vulnerability is subsequently presented; I ran this test on Akka Http and in this scenario he sends a 400 mentioning that the header name has an invalid name.

Thank you

@normanmaurer
Copy link
Member

@fabianedl777 can you please try to update to latest netty version (4.1.45.Final) and if you still be able to reproduce the issue send me a reproducer to my email (you can find it via GitHub).

dalaro added a commit to dalaro/netty that referenced this issue Mar 30, 2020
Compared against 4.1.25.6.dse, this tag cherry-picks upstream commits
that fixed bugs in HttpObjectDecoder/HttpRequestDecoder, plus two
intermediate refactoring commits that indirectly affect those bugfix
commits.

What follows is a list of PR links, issue links, CVE links, and hashes
associated with the cherry-picked commits.

Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#9865)
	https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-7238
	netty#9861
	netty#9865

	8494b04

Detect missing colon when parsing http headers with no value (netty#9871)
	GHSA-cqqj-4p63-rrmm
	netty#9866
	netty#9871

	a7c18d4

Fix typos in javadocs (netty#9527)
	skipped

Correctly handle whitespaces in HTTP header names as defined by RFC7230#section-3.2.4 (netty#9585)
	https://nvd.nist.gov/vuln/detail/CVE-2019-16869
	netty#9571
	netty#9585

	39cafcb

Use `AppendableCharSequence.charAtUnsafe(int)` in `HttpObjectDecoder` (netty#9492)
	netty#9492

	85fcf4e

use checkPositive/checkPositiveOrZero (netty#8835)
	netty#8835

	4c64c98

HttpObjectDecoder ignores HTTP trailer header when empty line is rece… (netty#8799)
	netty#8736
	netty#8799

	91d3920
dalaro pushed a commit to dalaro/netty that referenced this issue Mar 30, 2020
…30#section-3.2.4 (netty#9585)

Motivation:

When parsing HTTP headers special care needs to be taken when a whitespace is detected in the header name.

Modifications:

- Ignore whitespace when decoding response (just like before)
- Throw exception when whitespace is detected during parsing
- Add unit tests

Result:

Fixes netty#9571

(cherry picked from commit 39cafcb)
dalaro added a commit to dalaro/netty that referenced this issue Mar 30, 2020
Compared against 4.1.34.2.dse, this tag cherry-picks upstream commits
that fixed bugs in HttpObjectDecoder/HttpRequestDecoder, plus two
intermediate refactoring commits that indirectly affect those bugfix
commits.

What follows is a list of PR links, issue links, CVE links, and hashes
associated with the cherry-picked commits.

Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#9865)
	https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-7238
	netty#9861
	netty#9865

	8494b04

Detect missing colon when parsing http headers with no value (netty#9871)
	https://nvd.nist.gov/vuln/detail/CVE-2019-20444
	netty#9866
	netty#9871

	a7c18d4

Fix typos in javadocs (netty#9527)
	skipped

Correctly handle whitespaces in HTTP header names as defined by RFC7230#section-3.2.4 (netty#9585)
	https://nvd.nist.gov/vuln/detail/CVE-2019-16869
	netty#9571
	netty#9585

	39cafcb

Use `AppendableCharSequence.charAtUnsafe(int)` in `HttpObjectDecoder` (netty#9492)
	netty#9492

	85fcf4e
dalaro added a commit to dalaro/netty that referenced this issue Apr 7, 2020
Compared against 4.1.25.6.dse, this tag cherry-picks upstream commits
that fixed bugs in HttpObjectDecoder/HttpRequestDecoder, plus two
intermediate refactoring commits that indirectly affect those bugfix
commits.

What follows is a list of PR links, issue links, CVE links, and hashes
associated with the cherry-picked commits.

Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#9865)
	https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-7238
	netty#9861
	netty#9865

	8494b04

Detect missing colon when parsing http headers with no value (netty#9871)
	GHSA-cqqj-4p63-rrmm
	netty#9866
	netty#9871

	a7c18d4

Fix typos in javadocs (netty#9527)
	skipped

Correctly handle whitespaces in HTTP header names as defined by RFC7230#section-3.2.4 (netty#9585)
	https://nvd.nist.gov/vuln/detail/CVE-2019-16869
	netty#9571
	netty#9585

	39cafcb

Use `AppendableCharSequence.charAtUnsafe(int)` in `HttpObjectDecoder` (netty#9492)
	netty#9492

	85fcf4e

use checkPositive/checkPositiveOrZero (netty#8835)
	netty#8835

	4c64c98

HttpObjectDecoder ignores HTTP trailer header when empty line is rece… (netty#8799)
	netty#8736
	netty#8799

	91d3920
dalaro added a commit to dalaro/netty that referenced this issue Apr 7, 2020
Compared against 4.1.34.2.dse, this tag cherry-picks upstream commits
that fixed bugs in HttpObjectDecoder/HttpRequestDecoder, plus two
intermediate refactoring commits that indirectly affect those bugfix
commits.

What follows is a list of PR links, issue links, CVE links, and hashes
associated with the cherry-picked commits.

Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (netty#9865)
	https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-7238
	netty#9861
	netty#9865

	8494b04

Detect missing colon when parsing http headers with no value (netty#9871)
	https://nvd.nist.gov/vuln/detail/CVE-2019-20444
	netty#9866
	netty#9871

	a7c18d4

Fix typos in javadocs (netty#9527)
	skipped

Correctly handle whitespaces in HTTP header names as defined by RFC7230#section-3.2.4 (netty#9585)
	https://nvd.nist.gov/vuln/detail/CVE-2019-16869
	netty#9571
	netty#9585

	39cafcb

Use `AppendableCharSequence.charAtUnsafe(int)` in `HttpObjectDecoder` (netty#9492)
	netty#9492

	85fcf4e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.