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

Netty shouldn't allow the invalid fold. #9866

Closed
ZeddYu opened this issue Dec 10, 2019 · 12 comments · Fixed by #9871
Closed

Netty shouldn't allow the invalid fold. #9866

ZeddYu opened this issue Dec 10, 2019 · 12 comments · Fixed by #9871
Milestone

Comments

@ZeddYu
Copy link

ZeddYu commented Dec 10, 2019

Expected behavior

Netty shouldn't allow the invalid fold. According to RFC7230, https://tools.ietf.org/html/rfc7230#section-3.2.

header-field   = field-name ":" OWS field-value OWS

     field-name     = token
     field-value    = *( field-content / obs-fold )
     field-content  = field-vchar [ 1*( SP / HTAB ) field-vchar ]
     field-vchar    = VCHAR / obs-text

     obs-fold       = CRLF 1*( SP / HTAB )
                    ; obsolete line folding
                    ; see Section 3.2.4

A Sp or HTAB should follow the CRLF. But Netty allow a CRLF without a SP or HTAB.
2019-12-10下午7 58 09

Actual behavior

Netty accept invalid fold. This may casue http smuggling.

Steps to reproduce

Send a request like this:

POST / HTTP/1.1
Host: example.com
Connection: close
Content-Length: 5
Content-Length:
6

0

Minimal yet complete reproducer code (or URL to code)

Netty version

all

JVM version (e.g. java -version)

java version "1.8.0_181"

OS version (e.g. uname -a)

Darwin ZedddeMacBook-Pro.local 19.0.0 Darwin Kernel Version 19.0.0: Thu Oct 17 16:17:15 PDT 2019; root:xnu-6153.41.3~29/RELEASE_X86_64 x86_64

@normanmaurer
Copy link
Member

@ZeddYu while I agree that there is an issue I can't see how this could lead to http smuggling as netty will just create a header name with value 6 here.

normanmaurer added a commit that referenced this issue Dec 11, 2019
Motivation:

Technical speaking its valid to have http headers with no values so we should support it. That said we need to detect if these are "generated" because of an "invalid" fold.

Modifications:

- Detect if a colon is missing when parsing headers.
- Add unit test

Result:

Fixes #9866
@normanmaurer
Copy link
Member

Should be fixed by #9871

@ZeddYu
Copy link
Author

ZeddYu commented Dec 11, 2019

@ZeddYu while I agree that there is an issue I can't see how this could lead to http smuggling as netty will just create a header name with value 6 here.

@normanmaurer This maybe a little complex. According to the end of https://portswigger.net/research/http-desync-attacks-request-smuggling-reborn, we can use a invalid CRLF to bypass some situation like this.
截屏2019-12-11下午5 50 42

@normanmaurer
Copy link
Member

@ZeddYu IHMO how we currently handling it (which is a bug) does not have the same effect as explained there and so I can't see how it would result in smuggling

@ZeddYu
Copy link
Author

ZeddYu commented Dec 11, 2019

@normanmaurer I think we can use this bug to do like this.

POST / HTTP/1.1
Host: example.com
Content-length:41
Content-Type: application/x-www-form-urlencoded
Transfer-Encoding: 
chunked

0

GET /tmp HTTP/1.1
Host:localhost

GET / HTTP/1.1
Host:localhost

And I get 2 responses. :P

@normanmaurer
Copy link
Member

I see ... I would use the same CVE for all of the reported things as these are all related tho @ZeddYu

normanmaurer added a commit that referenced this issue Dec 11, 2019
Motivation:

Technical speaking its valid to have http headers with no values so we should support it. That said we need to detect if these are "generated" because of an "invalid" fold.

Modifications:

- Detect if a colon is missing when parsing headers.
- Add unit test

Result:

Fixes #9866
normanmaurer added a commit that referenced this issue Dec 11, 2019
Motivation:

Technical speaking its valid to have http headers with no values so we should support it. That said we need to detect if these are "generated" because of an "invalid" fold.

Modifications:

- Detect if a colon is missing when parsing headers.
- Add unit test

Result:

Fixes #9866
@normanmaurer normanmaurer added this to the 4.1.44.Final milestone Dec 11, 2019
@bertjwregeer
Copy link

@normanmaurer I think we can use this bug to do like this.

POST / HTTP/1.1
Host: example.com
Content-length:41
Content-Type: application/x-www-form-urlencoded
Transfer-Encoding: 
chunked

0

GET /tmp HTTP/1.1
Host:localhost

GET / HTTP/1.1
Host:localhost

And I get 2 responses. :P

This example includes two requests... so that makes sense.

You set the Content-Length to 41, which is the length of the following (with CRLF)

0

GET /tmp HTTP/1.1
Host:localhost

So that's the body of the first request.

Then

GET / HTTP/1.1
Host:localhost

Is simply pipelined because it's not part of the body of the first request.

This is because your example includes invalid header folding for the chunked part of the header.

Header folding requires that the next line includes a space or a tab character, your example does neither, so the Transfer-Encoding header is left blank and the Content-Length header is used.

@ZeddYu
Copy link
Author

ZeddYu commented Dec 22, 2019 via email

@bertjwregeer
Copy link

Please explain how this allows a request to be smuggled.

Transfer-Encoding:
chunked

Is an empty header named Transfer-Encoding, and a bad header named chunked. However according to the HTTP standard those are not invalid folds, nor should they cause any issues since at this point the Content-Length will be used instead to parse the request.

@carnil
Copy link

carnil commented Feb 8, 2020

CVE-2019-20444 was assigned for this issue apparently.

@ssennath92
Copy link

I am getting 400 bad request on giving invalid fold.

@ssennath92
Copy link

Can HTTP request smuggling be solved by disabling the reuse of back-end connections

dalaro pushed a commit to dalaro/netty that referenced this issue Mar 30, 2020
)

Motivation:

Technical speaking its valid to have http headers with no values so we should support it. That said we need to detect if these are "generated" because of an "invalid" fold.

Modifications:

- Detect if a colon is missing when parsing headers.
- Add unit test

Result:

Fixes netty#9866

(cherry picked from commit a7c18d4)

(After the default cherry-pick algorithm finished, I hand-resolved some
compile errors related to refactoring between the 4.0 and 4.1 branches)
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
)

Motivation:

Technical speaking its valid to have http headers with no values so we should support it. That said we need to detect if these are "generated" because of an "invalid" fold.

Modifications:

- Detect if a colon is missing when parsing headers.
- Add unit test

Result:

Fixes netty#9866

(cherry picked from commit a7c18d4)
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 Mar 30, 2020
This version is equivalent to upstream's 4.0.54.Final, but with one
upstream commit (for CVE-2019-20444) cherry-picked backwards from 4.1.

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

	a7c18d4
dalaro added a commit to dalaro/netty that referenced this issue Apr 7, 2020
This version is equivalent to upstream's 4.0.54.Final, but with one
upstream commit (for CVE-2019-20444) cherry-picked backwards from 4.1.

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

	a7c18d4
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
ihanyong pushed a commit to ihanyong/netty that referenced this issue Jul 31, 2020
)

Motivation:

Technical speaking its valid to have http headers with no values so we should support it. That said we need to detect if these are "generated" because of an "invalid" fold.

Modifications:

- Detect if a colon is missing when parsing headers.
- Add unit test

Result:

Fixes netty#9866
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.

5 participants