Fix RUSTSEC-2020-0031

[therealbstern]

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.

[rawler]

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]

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.

[rawler]

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]

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]

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]

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]

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]

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

[therealbstern]

Is there anything else that should be done here?

[frewsxcv]

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