1
0
mirror of https://github.com/fafhrd91/actix-web synced 2024-11-27 17:52:56 +01:00

fix parsing ambiguities for HTTP/1.0 requests (#2794)

* fix HRS vuln when first CL header is 0

* ignore TE headers in http/1.0 reqs

* update changelog

* disallow HTTP/1.0 requests without a CL header

* fix test

* broken fix for http1.0 post requests
This commit is contained in:
Rob Ede 2022-07-01 08:23:40 +01:00 committed by GitHub
parent c6eba2da9b
commit 8f9a12ed5d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 142 additions and 18 deletions

View File

@ -1,6 +1,10 @@
# Changes # Changes
## Unreleased - 2022-xx-xx ## Unreleased - 2022-xx-xx
### Fixed
- Fix parsing ambiguity in Transfer-Encoding and Content-Length headers for HTTP/1.0 requests. [#2794]
[#2794]: https://github.com/actix/actix-web/pull/2794
## 3.2.0 - 2022-06-30 ## 3.2.0 - 2022-06-30

View File

@ -46,6 +46,23 @@ pub(crate) enum PayloadLength {
None, None,
} }
impl PayloadLength {
/// Returns true if variant is `None`.
fn is_none(&self) -> bool {
matches!(self, Self::None)
}
/// Returns true if variant is represents zero-length (not none) payload.
fn is_zero(&self) -> bool {
matches!(
self,
PayloadLength::Payload(PayloadType::Payload(PayloadDecoder {
kind: Kind::Length(0)
}))
)
}
}
pub(crate) trait MessageType: Sized { pub(crate) trait MessageType: Sized {
fn set_connection_type(&mut self, conn_type: Option<ConnectionType>); fn set_connection_type(&mut self, conn_type: Option<ConnectionType>);
@ -59,6 +76,7 @@ pub(crate) trait MessageType: Sized {
&mut self, &mut self,
slice: &Bytes, slice: &Bytes,
raw_headers: &[HeaderIndex], raw_headers: &[HeaderIndex],
version: Version,
) -> Result<PayloadLength, ParseError> { ) -> Result<PayloadLength, ParseError> {
let mut ka = None; let mut ka = None;
let mut has_upgrade_websocket = false; let mut has_upgrade_websocket = false;
@ -87,21 +105,23 @@ pub(crate) trait MessageType: Sized {
return Err(ParseError::Header); return Err(ParseError::Header);
} }
header::CONTENT_LENGTH => match value.to_str() { header::CONTENT_LENGTH => match value.to_str().map(str::trim) {
Ok(s) if s.trim().starts_with('+') => { Ok(val) if val.starts_with('+') => {
debug!("illegal Content-Length: {:?}", s); debug!("illegal Content-Length: {:?}", val);
return Err(ParseError::Header); return Err(ParseError::Header);
} }
Ok(s) => {
if let Ok(len) = s.parse::<u64>() { Ok(val) => {
if len != 0 { if let Ok(len) = val.parse::<u64>() {
// accept 0 lengths here and remove them in `decode` after all
// headers have been processed to prevent request smuggling issues
content_length = Some(len); content_length = Some(len);
}
} else { } else {
debug!("illegal Content-Length: {:?}", s); debug!("illegal Content-Length: {:?}", val);
return Err(ParseError::Header); return Err(ParseError::Header);
} }
} }
Err(_) => { Err(_) => {
debug!("illegal Content-Length: {:?}", value); debug!("illegal Content-Length: {:?}", value);
return Err(ParseError::Header); return Err(ParseError::Header);
@ -114,22 +134,23 @@ pub(crate) trait MessageType: Sized {
return Err(ParseError::Header); return Err(ParseError::Header);
} }
header::TRANSFER_ENCODING => { header::TRANSFER_ENCODING if version == Version::HTTP_11 => {
seen_te = true; seen_te = true;
if let Ok(s) = value.to_str().map(str::trim) { if let Ok(val) = value.to_str().map(str::trim) {
if s.eq_ignore_ascii_case("chunked") { if val.eq_ignore_ascii_case("chunked") {
chunked = true; chunked = true;
} else if s.eq_ignore_ascii_case("identity") { } else if val.eq_ignore_ascii_case("identity") {
// allow silently since multiple TE headers are already checked // allow silently since multiple TE headers are already checked
} else { } else {
debug!("illegal Transfer-Encoding: {:?}", s); debug!("illegal Transfer-Encoding: {:?}", val);
return Err(ParseError::Header); return Err(ParseError::Header);
} }
} else { } else {
return Err(ParseError::Header); return Err(ParseError::Header);
} }
} }
// connection keep-alive state // connection keep-alive state
header::CONNECTION => { header::CONNECTION => {
ka = if let Ok(conn) = value.to_str().map(str::trim) { ka = if let Ok(conn) = value.to_str().map(str::trim) {
@ -146,6 +167,7 @@ pub(crate) trait MessageType: Sized {
None None
}; };
} }
header::UPGRADE => { header::UPGRADE => {
if let Ok(val) = value.to_str().map(str::trim) { if let Ok(val) = value.to_str().map(str::trim) {
if val.eq_ignore_ascii_case("websocket") { if val.eq_ignore_ascii_case("websocket") {
@ -153,19 +175,23 @@ pub(crate) trait MessageType: Sized {
} }
} }
} }
header::EXPECT => { header::EXPECT => {
let bytes = value.as_bytes(); let bytes = value.as_bytes();
if bytes.len() >= 4 && &bytes[0..4] == b"100-" { if bytes.len() >= 4 && &bytes[0..4] == b"100-" {
expect = true; expect = true;
} }
} }
_ => {} _ => {}
} }
headers.append(name, value); headers.append(name, value);
} }
} }
self.set_connection_type(ka); self.set_connection_type(ka);
if expect { if expect {
self.set_expect() self.set_expect()
} }
@ -249,7 +275,22 @@ impl MessageType for Request {
let mut msg = Request::new(); let mut msg = Request::new();
// convert headers // convert headers
let length = msg.set_headers(&src.split_to(len).freeze(), &headers[..h_len])?; let mut length =
msg.set_headers(&src.split_to(len).freeze(), &headers[..h_len], ver)?;
// disallow HTTP/1.0 POST requests that do not contain a Content-Length headers
// see https://datatracker.ietf.org/doc/html/rfc1945#section-7.2.2
if ver == Version::HTTP_10 && method == Method::POST && length.is_none() {
debug!("no Content-Length specified for HTTP/1.0 POST request");
return Err(ParseError::Header);
}
// Remove CL value if 0 now that all headers and HTTP/1.0 special cases are processed.
// Protects against some request smuggling attacks.
// See https://github.com/actix/actix-web/issues/2767.
if length.is_zero() {
length = PayloadLength::None;
}
// payload decoder // payload decoder
let decoder = match length { let decoder = match length {
@ -337,7 +378,15 @@ impl MessageType for ResponseHead {
msg.version = ver; msg.version = ver;
// convert headers // convert headers
let length = msg.set_headers(&src.split_to(len).freeze(), &headers[..h_len])?; let mut length =
msg.set_headers(&src.split_to(len).freeze(), &headers[..h_len], ver)?;
// Remove CL value if 0 now that all headers and HTTP/1.0 special cases are processed.
// Protects against some request smuggling attacks.
// See https://github.com/actix/actix-web/issues/2767.
if length.is_zero() {
length = PayloadLength::None;
}
// message payload // message payload
let decoder = if let PayloadLength::Payload(pl) = length { let decoder = if let PayloadLength::Payload(pl) = length {
@ -606,14 +655,40 @@ mod tests {
} }
#[test] #[test]
fn test_parse_post() { fn parse_h10_post() {
let mut buf = BytesMut::from("POST /test2 HTTP/1.0\r\n\r\n"); let mut buf = BytesMut::from(
"POST /test1 HTTP/1.0\r\n\
Content-Length: 3\r\n\
\r\n\
abc",
);
let mut reader = MessageDecoder::<Request>::default();
let (req, _) = reader.decode(&mut buf).unwrap().unwrap();
assert_eq!(req.version(), Version::HTTP_10);
assert_eq!(*req.method(), Method::POST);
assert_eq!(req.path(), "/test1");
let mut buf = BytesMut::from(
"POST /test2 HTTP/1.0\r\n\
Content-Length: 0\r\n\
\r\n",
);
let mut reader = MessageDecoder::<Request>::default(); let mut reader = MessageDecoder::<Request>::default();
let (req, _) = reader.decode(&mut buf).unwrap().unwrap(); let (req, _) = reader.decode(&mut buf).unwrap().unwrap();
assert_eq!(req.version(), Version::HTTP_10); assert_eq!(req.version(), Version::HTTP_10);
assert_eq!(*req.method(), Method::POST); assert_eq!(*req.method(), Method::POST);
assert_eq!(req.path(), "/test2"); assert_eq!(req.path(), "/test2");
let mut buf = BytesMut::from(
"POST /test3 HTTP/1.0\r\n\
\r\n",
);
let mut reader = MessageDecoder::<Request>::default();
let err = reader.decode(&mut buf).unwrap_err();
assert!(err.to_string().contains("Header"))
} }
#[test] #[test]
@ -981,6 +1056,17 @@ mod tests {
); );
expect_parse_err!(&mut buf); expect_parse_err!(&mut buf);
let mut buf = BytesMut::from(
"GET / HTTP/1.1\r\n\
Host: example.com\r\n\
Content-Length: 0\r\n\
Content-Length: 2\r\n\
\r\n\
ab",
);
expect_parse_err!(&mut buf);
} }
#[test] #[test]
@ -996,6 +1082,40 @@ mod tests {
expect_parse_err!(&mut buf); expect_parse_err!(&mut buf);
} }
#[test]
fn hrs_te_http10() {
// in HTTP/1.0 transfer encoding is ignored and must therefore contain a CL header
let mut buf = BytesMut::from(
"POST / HTTP/1.0\r\n\
Host: example.com\r\n\
Transfer-Encoding: chunked\r\n\
\r\n\
3\r\n\
aaa\r\n\
0\r\n\
",
);
expect_parse_err!(&mut buf);
}
#[test]
fn hrs_cl_and_te_http10() {
// in HTTP/1.0 transfer encoding is simply ignored so it's fine to have both
let mut buf = BytesMut::from(
"GET / HTTP/1.0\r\n\
Host: example.com\r\n\
Content-Length: 3\r\n\
Transfer-Encoding: chunked\r\n\
\r\n\
000",
);
parse_ready!(&mut buf);
}
#[test] #[test]
fn hrs_unknown_transfer_encoding() { fn hrs_unknown_transfer_encoding() {
let mut buf = BytesMut::from( let mut buf = BytesMut::from(