From b6a10e5c1e8f3181e9c7a30d907ae4c27f33c271 Mon Sep 17 00:00:00 2001 From: Marko Lalic Date: Tue, 6 Jan 2015 20:28:36 +0100 Subject: [PATCH 1/3] Fix chunk size parsing: handle invalid chunk sizes Currently, when the chunk size was an invalid hex number (i.e. contained a non-HEX octet), hyper would halt its parsing at the last valid hex digit and report this as the chunk size. For example, the following would be the chunk sizes reported for some strings: "X" => 0 "1X" => 1 "aY" => 10 "a;a" => 10 (only this should be correct!) This fix makes it so that any chunk size, which cannot be parsed as a valid hex number, causes an IoError. Additionally, the case where the chunk size is followed by a chunk extension is also handled (since the ";" token is a valid delimiter between the chunk size and, as such, along with a LWS octet, represents a valid terminal character of the chunk size). Regression tests for the `get_chunk_size` function and included. --- src/http.rs | 69 ++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 63 insertions(+), 6 deletions(-) diff --git a/src/http.rs b/src/http.rs index d0c63e3a..fb1cf0f7 100644 --- a/src/http.rs +++ b/src/http.rs @@ -137,17 +137,18 @@ fn read_chunk_size(rdr: &mut R) -> IoResult { let mut size = 0u; let radix = 16; let mut in_ext = false; + let mut in_chunk_size = true; loop { match try!(rdr.read_byte()) { - b@b'0'...b'9' if !in_ext => { + b@b'0'...b'9' if in_chunk_size => { size *= radix; size += (b - b'0') as uint; }, - b@b'a'...b'f' if !in_ext => { + b@b'a'...b'f' if in_chunk_size => { size *= radix; size += (b + 10 - b'a') as uint; }, - b@b'A'...b'F' if !in_ext => { + b@b'A'...b'F' if in_chunk_size => { size *= radix; size += (b + 10 - b'A') as uint; }, @@ -157,9 +158,28 @@ fn read_chunk_size(rdr: &mut R) -> IoResult { _ => return Err(io::standard_error(io::InvalidInput)) } }, - ext => { + // If we weren't in the extension yet, the ";" signals its start + b';' if !in_ext => { in_ext = true; + in_chunk_size = false; + }, + // "Linear white space" is ignored between the chunk size and the + // extension separator token (";") due to the "implied *LWS rule". + b'\t' | b' ' if !in_ext & !in_chunk_size => {}, + // LWS can follow the chunk size, but no more digits can come + b'\t' | b' ' if in_chunk_size => in_chunk_size = false, + // We allow any arbitrary octet once we are in the extension, since + // they all get ignored anyway. According to the HTTP spec, valid + // extensions would have a more strict syntax: + // (token ["=" (token | quoted-string)]) + // but we gain nothing by rejecting an otherwise valid chunk size. + ext if in_ext => { todo!("chunk extension byte={}", ext); + }, + // Finally, if we aren't in the extension and we're reading any + // other octet, the chunk size line is invalid! + _ => { + return Err(io::standard_error(io::InvalidInput)); } } } @@ -689,7 +709,7 @@ fn expect(r: IoResult, expected: u8) -> HttpResult<()> { #[cfg(test)] mod tests { - use std::io::{self, MemReader, MemWriter}; + use std::io::{self, MemReader, MemWriter, IoResult}; use std::borrow::Cow::{Borrowed, Owned}; use test::Bencher; use uri::RequestUri; @@ -702,7 +722,7 @@ mod tests { use url::Url; use super::{read_method, read_uri, read_http_version, read_header, - RawHeaderLine, read_status, RawStatus}; + RawHeaderLine, read_status, RawStatus, read_chunk_size}; fn mem(s: &str) -> MemReader { MemReader::new(s.as_bytes().to_vec()) @@ -811,6 +831,43 @@ mod tests { assert_eq!(s, "foo barb"); } + #[test] + fn test_read_chunk_size() { + fn read(s: &str, result: IoResult) { + assert_eq!(read_chunk_size(&mut mem(s)), result); + } + + read("1\r\n", Ok(1)); + read("01\r\n", Ok(1)); + read("0\r\n", Ok(0)); + read("00\r\n", Ok(0)); + read("A\r\n", Ok(10)); + read("a\r\n", Ok(10)); + read("Ff\r\n", Ok(255)); + read("Ff \r\n", Ok(255)); + // Missing LF or CRLF + read("F\rF", Err(io::standard_error(io::InvalidInput))); + read("F", Err(io::standard_error(io::EndOfFile))); + // Invalid hex digit + read("X\r\n", Err(io::standard_error(io::InvalidInput))); + read("1X\r\n", Err(io::standard_error(io::InvalidInput))); + read("-\r\n", Err(io::standard_error(io::InvalidInput))); + read("-1\r\n", Err(io::standard_error(io::InvalidInput))); + // Acceptable (if not fully valid) extensions do not influence the size + read("1;extension\r\n", Ok(1)); + read("a;ext name=value\r\n", Ok(10)); + read("1;extension;extension2\r\n", Ok(1)); + read("1;;; ;\r\n", Ok(1)); + read("2; extension...\r\n", Ok(2)); + read("3 ; extension=123\r\n", Ok(3)); + read("3 ;\r\n", Ok(3)); + read("3 ; \r\n", Ok(3)); + // Invalid extensions cause an error + read("1 invalid extension\r\n", Err(io::standard_error(io::InvalidInput))); + read("1 A\r\n", Err(io::standard_error(io::InvalidInput))); + read("1;no CRLF", Err(io::standard_error(io::EndOfFile))); + } + #[bench] fn bench_read_method(b: &mut Bencher) { b.bytes = b"CONNECT ".len() as u64; From 2357995cc930add309b5406610d1bf5582f74373 Mon Sep 17 00:00:00 2001 From: Marko Lalic Date: Tue, 6 Jan 2015 21:04:22 +0100 Subject: [PATCH 2/3] Add regression tests for chunked response parsing The tests assert that `Transport-Encoding: chunked` responses are correctly handled both when the chunk sizes are valid, as well as invalid. --- src/client/response.rs | 93 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/src/client/response.rs b/src/client/response.rs index f87dd936..49ab4ea2 100644 --- a/src/client/response.rs +++ b/src/client/response.rs @@ -100,6 +100,8 @@ mod tests { use std::io::BufferedReader; use header::Headers; + use header::common::TransferEncoding; + use header::common::transfer_encoding::Encoding; use http::HttpReader::EofReader; use http::RawStatus; use mock::MockStream; @@ -124,4 +126,95 @@ mod tests { assert_eq!(b, box MockStream::new()); } + + #[test] + fn test_parse_chunked_response() { + let stream = MockStream::with_input(b"\ + HTTP/1.1 200 OK\r\n\ + Transfer-Encoding: chunked\r\n\ + \r\n\ + 1\r\n\ + q\r\n\ + 2\r\n\ + we\r\n\ + 2\r\n\ + rt\r\n\ + 0\r\n\ + \r\n" + ); + + let mut res = Response::new(box stream).unwrap(); + + // The status line is correct? + assert_eq!(res.status, status::StatusCode::Ok); + assert_eq!(res.version, version::HttpVersion::Http11); + // The header is correct? + match res.headers.get::() { + Some(encodings) => { + assert_eq!(1, encodings.len()); + assert_eq!(Encoding::Chunked, encodings[0]); + }, + None => panic!("Transfer-Encoding: chunked expected!"), + }; + // The body is correct? + let body = res.read_to_string().unwrap(); + assert_eq!("qwert", body); + } + + /// Tests that when a chunk size is not a valid radix-16 number, an error + /// is returned. + #[test] + fn test_invalid_chunk_size_not_hex_digit() { + let stream = MockStream::with_input(b"\ + HTTP/1.1 200 OK\r\n\ + Transfer-Encoding: chunked\r\n\ + \r\n\ + X\r\n\ + 1\r\n\ + 0\r\n\ + \r\n" + ); + + let mut res = Response::new(box stream).unwrap(); + + assert!(res.read_to_string().is_err()); + } + + /// Tests that when a chunk size contains an invalid extension, an error is + /// returned. + #[test] + fn test_invalid_chunk_size_extension() { + let stream = MockStream::with_input(b"\ + HTTP/1.1 200 OK\r\n\ + Transfer-Encoding: chunked\r\n\ + \r\n\ + 1 this is an invalid extension\r\n\ + 1\r\n\ + 0\r\n\ + \r\n" + ); + + let mut res = Response::new(box stream).unwrap(); + + assert!(res.read_to_string().is_err()); + } + + /// Tests that when a valid extension that contains a digit is appended to + /// the chunk size, the chunk is correctly read. + #[test] + fn test_chunk_size_with_extension() { + let stream = MockStream::with_input(b"\ + HTTP/1.1 200 OK\r\n\ + Transfer-Encoding: chunked\r\n\ + \r\n\ + 1;this is an extension with a digit 1\r\n\ + 1\r\n\ + 0\r\n\ + \r\n" + ); + + let mut res = Response::new(box stream).unwrap(); + + assert_eq!("1", res.read_to_string().unwrap()) + } } From f327a7e737e0a8528f4239e5a57db88ce47a0fa7 Mon Sep 17 00:00:00 2001 From: Marko Lalic Date: Tue, 6 Jan 2015 21:01:35 +0100 Subject: [PATCH 3/3] Add regression tests for chunked request parsing The tests assert that requests with a `Transfer-Encoding: chunked` are correctly parsed both when the chunks are valid, as well as invalid. --- src/server/request.rs | 101 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/src/server/request.rs b/src/server/request.rs index 5c808441..b434ce1b 100644 --- a/src/server/request.rs +++ b/src/server/request.rs @@ -75,6 +75,8 @@ impl<'a> Reader for Request<'a> { #[cfg(test)] mod tests { + use header::common::{Host, TransferEncoding}; + use header::common::transfer_encoding::Encoding; use mock::MockStream; use super::Request; @@ -122,4 +124,103 @@ mod tests { let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap(); assert_eq!(req.read_to_string(), Ok("".to_string())); } + + #[test] + fn test_parse_chunked_request() { + let mut stream = MockStream::with_input(b"\ + POST / HTTP/1.1\r\n\ + Host: example.domain\r\n\ + Transfer-Encoding: chunked\r\n\ + \r\n\ + 1\r\n\ + q\r\n\ + 2\r\n\ + we\r\n\ + 2\r\n\ + rt\r\n\ + 0\r\n\ + \r\n" + ); + + let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap(); + + // The headers are correct? + match req.headers.get::() { + Some(host) => { + assert_eq!("example.domain", host.hostname); + }, + None => panic!("Host header expected!"), + }; + match req.headers.get::() { + Some(encodings) => { + assert_eq!(1, encodings.len()); + assert_eq!(Encoding::Chunked, encodings[0]); + } + None => panic!("Transfer-Encoding: chunked expected!"), + }; + // The content is correctly read? + let body = req.read_to_string().unwrap(); + assert_eq!("qwert", body); + } + + /// Tests that when a chunk size is not a valid radix-16 number, an error + /// is returned. + #[test] + fn test_invalid_chunk_size_not_hex_digit() { + let mut stream = MockStream::with_input(b"\ + POST / HTTP/1.1\r\n\ + Host: example.domain\r\n\ + Transfer-Encoding: chunked\r\n\ + \r\n\ + X\r\n\ + 1\r\n\ + 0\r\n\ + \r\n" + ); + + let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap(); + + assert!(req.read_to_string().is_err()); + } + + /// Tests that when a chunk size contains an invalid extension, an error is + /// returned. + #[test] + fn test_invalid_chunk_size_extension() { + let mut stream = MockStream::with_input(b"\ + POST / HTTP/1.1\r\n\ + Host: example.domain\r\n\ + Transfer-Encoding: chunked\r\n\ + \r\n\ + 1 this is an invalid extension\r\n\ + 1\r\n\ + 0\r\n\ + \r\n" + ); + + let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap(); + + assert!(req.read_to_string().is_err()); + } + + /// Tests that when a valid extension that contains a digit is appended to + /// the chunk size, the chunk is correctly read. + #[test] + fn test_chunk_size_with_extension() { + let mut stream = MockStream::with_input(b"\ + POST / HTTP/1.1\r\n\ + Host: example.domain\r\n\ + Transfer-Encoding: chunked\r\n\ + \r\n\ + 1;this is an extension with a digit 1\r\n\ + 1\r\n\ + 0\r\n\ + \r\n" + ); + + let mut req = Request::new(&mut stream, sock("127.0.0.1:80")).unwrap(); + + assert_eq!("1", req.read_to_string().unwrap()) + } + }