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

Fix RUSTSEC-2020-0031 #190

Merged
merged 5 commits into from Jan 30, 2021
Merged

Conversation

therealbstern
Copy link

@therealbstern therealbstern commented Jan 2, 2021

Without this fix:

HTTP/1.1 400 Bad Request
Server: tiny-http (Rust)
Date: Sat,  2 Jan 2021 20:23:02 GMT
Content-Type: application/json; charset=utf-8
Content-Length: 163

{"description":"could not read the body from the request, or could not execute the CGI program","cause":{"description":"Error while decoding chunks","cause":null}}

With this fix:

HTTP/1.1 400 Bad Request
Server: tiny-http (Rust)
Date: Sat,  9 Jan 2021 00:23:11 GMT
Content-Length: 0

Fixes #173.

Copy link
Collaborator

@rawler rawler left a comment

Nice! Thanks!

Could you please also add a test to the unit-tests in the same file, with the RUSTSEC-reference to highlight why never breaking this test is important?

@therealbstern
Copy link
Author

therealbstern commented Jan 8, 2021

Unit test added with a comment; please let me know if you want more detail.

I waffled for a long time about making from_str return Err(()) if s.to_lowercase.trim() != s.to_lowercase() but ultimately decided that the HTTP spec is really vague on whether or not whitespace can appear in header names. It implies strongly that it cannot, but doesn't really come out and say it, so the best I think http_tiny can do is pass along the whole header.

@therealbstern therealbstern requested a review from rawler Jan 8, 2021
@rawler
Copy link
Collaborator

rawler commented Jan 8, 2021

I read up on the spec. From "https://tools.ietf.org/html/rfc7230#section-3.2.3":

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.

It also declares that a "header-name" = "token", which is a sequence of symbols, where whitespace is not accepted.

In short; I think the proper mitigation is to error out if header-name has any whitespace. Sorry for not catching this in previous review.

@therealbstern
Copy link
Author

therealbstern commented Jan 9, 2021

No, that's fine, I'm happy to error out - I mentioned the weirdness because I was concerned, but I wasn't sure how firm the RFC was being.

In one part of the RFC, token is defined as 1*tchar and tchar is defined as a whole bunch of stuff that isn't valid for header fields. I thought breaking stuff without drawing attention to the breakage would cause problems later. New PR coming soon.

@therealbstern
Copy link
Author

therealbstern commented Jan 9, 2021

Now returns Err(()) as discussed above, whenever there's any whitespace (Unicode definition, not ASCII definition). I chose the Unicode definition because:

  • a consumer may be naive about whitespace, and it seemed safer to reject anything that might be considered blank, especially as Unicode is the default interpretation in Rust
  • I can't see the IETF deciding ASCII whitespace is bad but Unicode whitespace is okay (i.e., not deciding that invisible spaces in headers are good while tabs are bad).

Please let me know if you want any other changes made.

@rawler
Copy link
Collaborator

rawler commented Jan 9, 2021

Super! Could we perhaps also add the following to input-test.rs to verify the recommended RFC-behavior? (I don't think I can add to your PR?)

 }
+
+#[test]
+fn invalid_header_name() {
+    let mut client = support::new_client_to_hello_world_server();
+
+    (write!(client, "GET / HTTP/1.1\r\nHost: localhost\r\nConnection: close\r\nContent-Type: text/plain; charset=utf8\r\nContent-Length : 5\r\n\r\nhello")).unwrap();
+
+    let mut content = String::new();
+    client.read_to_string(&mut content).unwrap();
+    assert!(&content[9..].starts_with("400 Bad Request")); // 417 status code
+}

@therealbstern
Copy link
Author

therealbstern commented Jan 9, 2021

I thought you deserved the credit for that test, since all I did was add a comment and change a typo in another comment. :-)

rawler
rawler approved these changes Jan 10, 2021
Copy link
Collaborator

@rawler rawler left a comment

This now looks good to me, but I'll wait a few days before merging, to allow some other (real) maintainer chime in.

bradfier added a commit to PassFort/tiny-http that referenced this issue Jan 21, 2021
As identified in RUSTSEC-2020-0031, normalizing the value of a header
field (through the use of `str::trim`) can make applications based on
this library vulnerable to HTTP request smuggling if the immediate
upstream load balancer interprets the malformed header in a different
way.

This backported patch based on a [PR] opened on the main tiny-http repo.

[PR]: tiny-http#190
bradfier added a commit to PassFort/tiny-http that referenced this issue Jan 21, 2021
As identified in RUSTSEC-2020-0031, normalizing the value of a header
field (through the use of `str::trim`) can make applications based on
this library vulnerable to HTTP request smuggling if the immediate
upstream load balancer interprets the malformed header in a different
way.

This backported patch based on a PR opened on the main tiny-http repo. [1]

[1]: tiny-http#190
bradfier added a commit to bradfier/tiny-http that referenced this issue Jan 21, 2021
As identified in RUSTSEC-2020-0031, normalizing the value of a header
field (through the use of `str::trim`) can make applications based on
this library vulnerable to HTTP request smuggling if the immediate
upstream load balancer interprets the malformed header in a different
way.

This backported patch based on a PR opened against master. [1]

[1]: tiny-http#190
bradfier added a commit to bradfier/tiny-http that referenced this issue Jan 21, 2021
As identified in RUSTSEC-2020-0031, normalizing the value of a header
field (through the use of `str::trim`) can make applications based on
this library vulnerable to HTTP request smuggling if the immediate
upstream load balancer interprets the malformed header in a different
way.

This backported patch based on a PR opened against master. [1]

[1]: tiny-http#190

Co-authored-by: Ben Stern <bstern@fortian.com>
@bradfier bradfier mentioned this pull request Jan 21, 2021
@therealbstern
Copy link
Author

therealbstern commented Jan 29, 2021

Is there anything else that should be done here?

therealbstern pushed a commit to fortian/tiny-http that referenced this issue Jan 29, 2021
@frewsxcv
Copy link
Member

frewsxcv commented Jan 30, 2021

Thanks! @rawler In the future, feel free to merge! No need to wait on me or tomaka.

@frewsxcv frewsxcv merged commit 172ca81 into tiny-http:master Jan 30, 2021
1 check passed
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 this pull request may close these issues.

3 participants