fix(http1): reject content-lengths that have a plus sign prefix
The HTTP/1 content-length parser would accept lengths that were prefixed with a plus sign (for example, `+1234`). The specification restricts the content-length header to only allow DIGITs, making such a content-length illegal. Since some HTTP implementations protect against that, and others mis-interpret the length when the plus sign is present, this fixes hyper to always reject such content lengths. See GHSA-f3pg-qwvg-p99c
This commit is contained in:
		| @@ -30,7 +30,7 @@ fn connection_has(value: &HeaderValue, needle: &str) -> bool { | |||||||
|  |  | ||||||
| #[cfg(all(feature = "http1", feature = "server"))] | #[cfg(all(feature = "http1", feature = "server"))] | ||||||
| pub(super) fn content_length_parse(value: &HeaderValue) -> Option<u64> { | pub(super) fn content_length_parse(value: &HeaderValue) -> Option<u64> { | ||||||
|     value.to_str().ok().and_then(|s| s.parse().ok()) |     from_digits(value.as_bytes()) | ||||||
| } | } | ||||||
|  |  | ||||||
| pub(super) fn content_length_parse_all(headers: &HeaderMap) -> Option<u64> { | pub(super) fn content_length_parse_all(headers: &HeaderMap) -> Option<u64> { | ||||||
| @@ -46,7 +46,7 @@ pub(super) fn content_length_parse_all_values(values: ValueIter<'_, HeaderValue> | |||||||
|     for h in values { |     for h in values { | ||||||
|         if let Ok(line) = h.to_str() { |         if let Ok(line) = h.to_str() { | ||||||
|             for v in line.split(',') { |             for v in line.split(',') { | ||||||
|                 if let Some(n) = v.trim().parse().ok() { |                 if let Some(n) = from_digits(v.trim().as_bytes()) { | ||||||
|                     if content_length.is_none() { |                     if content_length.is_none() { | ||||||
|                         content_length = Some(n) |                         content_length = Some(n) | ||||||
|                     } else if content_length != Some(n) { |                     } else if content_length != Some(n) { | ||||||
| @@ -64,6 +64,33 @@ pub(super) fn content_length_parse_all_values(values: ValueIter<'_, HeaderValue> | |||||||
|     return content_length |     return content_length | ||||||
| } | } | ||||||
|  |  | ||||||
|  | fn from_digits(bytes: &[u8]) -> Option<u64> { | ||||||
|  |     // cannot use FromStr for u64, since it allows a signed prefix | ||||||
|  |     let mut result = 0u64; | ||||||
|  |     const RADIX: u64 = 10; | ||||||
|  |  | ||||||
|  |     if bytes.is_empty() { | ||||||
|  |         return None; | ||||||
|  |     } | ||||||
|  |  | ||||||
|  |     for &b in bytes { | ||||||
|  |         // can't use char::to_digit, since we haven't verified these bytes | ||||||
|  |         // are utf-8. | ||||||
|  |         match b { | ||||||
|  |             b'0'..=b'9' => { | ||||||
|  |                 result = result.checked_mul(RADIX)?; | ||||||
|  |                 result = result.checked_add((b - b'0') as u64)?; | ||||||
|  |             }, | ||||||
|  |             _ => { | ||||||
|  |                 // not a DIGIT, get outta here! | ||||||
|  |                 return None; | ||||||
|  |             } | ||||||
|  |         } | ||||||
|  |     } | ||||||
|  |  | ||||||
|  |     Some(result) | ||||||
|  | } | ||||||
|  |  | ||||||
| #[cfg(all(feature = "http2", feature = "client"))] | #[cfg(all(feature = "http2", feature = "client"))] | ||||||
| pub(super) fn method_has_defined_payload_semantics(method: &Method) -> bool { | pub(super) fn method_has_defined_payload_semantics(method: &Method) -> bool { | ||||||
|     match *method { |     match *method { | ||||||
|   | |||||||
| @@ -219,10 +219,8 @@ impl Http1Transaction for Server { | |||||||
|                     if is_te { |                     if is_te { | ||||||
|                         continue; |                         continue; | ||||||
|                     } |                     } | ||||||
|                     let len = value |                     let len = headers::content_length_parse(&value) | ||||||
|                         .to_str() |                         .ok_or_else(Parse::content_length_invalid)?; | ||||||
|                         .map_err(|_| Parse::content_length_invalid()) |  | ||||||
|                         .and_then(|s| s.parse().map_err(|_| Parse::content_length_invalid()))?; |  | ||||||
|                     if let Some(prev) = con_len { |                     if let Some(prev) = con_len { | ||||||
|                         if prev != len { |                         if prev != len { | ||||||
|                             debug!( |                             debug!( | ||||||
| @@ -1775,6 +1773,16 @@ mod tests { | |||||||
|             "multiple content-lengths", |             "multiple content-lengths", | ||||||
|         ); |         ); | ||||||
|  |  | ||||||
|  |         // content-length with prefix is not allowed | ||||||
|  |         parse_err( | ||||||
|  |             "\ | ||||||
|  |              POST / HTTP/1.1\r\n\ | ||||||
|  |              content-length: +10\r\n\ | ||||||
|  |              \r\n\ | ||||||
|  |              ", | ||||||
|  |             "prefixed content-length", | ||||||
|  |         ); | ||||||
|  |  | ||||||
|         // transfer-encoding that isn't chunked is an error |         // transfer-encoding that isn't chunked is an error | ||||||
|         parse_err( |         parse_err( | ||||||
|             "\ |             "\ | ||||||
| @@ -1958,6 +1966,14 @@ mod tests { | |||||||
|              ", |              ", | ||||||
|         ); |         ); | ||||||
|  |  | ||||||
|  |         parse_err( | ||||||
|  |             "\ | ||||||
|  |              HTTP/1.1 200 OK\r\n\ | ||||||
|  |              content-length: +8\r\n\ | ||||||
|  |              \r\n\ | ||||||
|  |              ", | ||||||
|  |         ); | ||||||
|  |  | ||||||
|         // transfer-encoding: chunked |         // transfer-encoding: chunked | ||||||
|         assert_eq!( |         assert_eq!( | ||||||
|             parse( |             parse( | ||||||
|   | |||||||
| @@ -405,6 +405,44 @@ fn get_chunked_response_with_ka() { | |||||||
|     read_until(&mut req, |buf| buf.ends_with(quux)).expect("reading 2"); |     read_until(&mut req, |buf| buf.ends_with(quux)).expect("reading 2"); | ||||||
| } | } | ||||||
|  |  | ||||||
|  | #[test] | ||||||
|  | fn post_with_content_length_body() { | ||||||
|  |     let server = serve(); | ||||||
|  |     let mut req = connect(server.addr()); | ||||||
|  |     req.write_all( | ||||||
|  |         b"\ | ||||||
|  |         POST / HTTP/1.1\r\n\ | ||||||
|  |         Content-Length: 5\r\n\ | ||||||
|  |         \r\n\ | ||||||
|  |         hello\ | ||||||
|  |     ", | ||||||
|  |     ) | ||||||
|  |     .unwrap(); | ||||||
|  |     req.read(&mut [0; 256]).unwrap(); | ||||||
|  |  | ||||||
|  |     assert_eq!(server.body(), b"hello"); | ||||||
|  | } | ||||||
|  |  | ||||||
|  | #[test] | ||||||
|  | fn post_with_invalid_prefix_content_length() { | ||||||
|  |     let server = serve(); | ||||||
|  |     let mut req = connect(server.addr()); | ||||||
|  |     req.write_all( | ||||||
|  |         b"\ | ||||||
|  |         POST / HTTP/1.1\r\n\ | ||||||
|  |         Content-Length: +5\r\n\ | ||||||
|  |         \r\n\ | ||||||
|  |         hello\ | ||||||
|  |     ", | ||||||
|  |     ) | ||||||
|  |     .unwrap(); | ||||||
|  |  | ||||||
|  |     let mut buf = [0; 256]; | ||||||
|  |     let _n = req.read(&mut buf).unwrap(); | ||||||
|  |     let expected = "HTTP/1.1 400 Bad Request\r\n"; | ||||||
|  |     assert_eq!(s(&buf[..expected.len()]), expected); | ||||||
|  | } | ||||||
|  |  | ||||||
| #[test] | #[test] | ||||||
| fn post_with_chunked_body() { | fn post_with_chunked_body() { | ||||||
|     let server = serve(); |     let server = serve(); | ||||||
|   | |||||||
		Reference in New Issue
	
	Block a user