diff --git a/crates/gatewayd/src/http.rs b/crates/gatewayd/src/http.rs index 8134836..aa4b6b4 100644 --- a/crates/gatewayd/src/http.rs +++ b/crates/gatewayd/src/http.rs @@ -171,8 +171,9 @@ pub fn read_head(stream: &mut dyn Read) -> Result { } fn parse_status_line(line: &str) -> Result<(&str, u16), HttpError> { - // "HTTP/1.1 200 OK": a 1.1 or 1.0 version, then a 3-digit status in 100..=599, then anything. - let mut parts = line.split_whitespace(); + // "HTTP/1.1 200 OK": a 1.1 or 1.0 version, one space, a 3-digit status in 100..=599, then + // optionally one space and a free-text reason. Any other spacing is refused. + let mut parts = line.splitn(3, ' '); let version = parts.next().unwrap_or(""); let code = parts.next().unwrap_or(""); if version != "HTTP/1.1" && version != "HTTP/1.0" { @@ -219,7 +220,8 @@ pub fn read_body(stream: &mut dyn Read, head: &Head) -> Result, HttpErro } None => { let mut buf = Vec::new(); - stream.take((MAX_BODY + 1) as u64).read_to_end(&mut buf)?; + let limit = u64::try_from(MAX_BODY).map_or(u64::MAX, |n| n.saturating_add(1)); + stream.take(limit).read_to_end(&mut buf)?; if buf.len() > MAX_BODY { return Err(HttpError::TooLarge("body")); } @@ -260,8 +262,11 @@ fn read_line(stream: &mut dyn Read, cap: usize) -> Result { debug_assert_eq!(n, 1); match byte[0] { b'\n' => { - if bytes.last() == Some(&b'\r') { - bytes.pop(); + // Lines end in CRLF; a bare LF is not accepted. + if bytes.pop() != Some(b'\r') { + return Err(HttpError::Protocol( + "a line is not ended by CRLF".to_string(), + )); } return String::from_utf8(bytes) .map_err(|_| HttpError::Protocol("a line is not UTF-8".to_string())); @@ -296,10 +301,11 @@ fn read_chunked(stream: &mut dyn Read) -> Result, HttpError> { } } } - if size > MAX_BODY as u128 - out.len() as u128 { - return Err(HttpError::TooLarge("body")); - } - let size = size as usize; + let room = MAX_BODY.saturating_sub(out.len()); + let size = usize::try_from(size) + .ok() + .filter(|s| *s <= room) + .ok_or(HttpError::TooLarge("body"))?; let mut chunk = vec![0u8; size]; stream.read_exact(&mut chunk)?; out.extend_from_slice(&chunk); @@ -318,9 +324,9 @@ fn parse_hex(s: &str) -> Result { let mut value: u128 = 0; for b in s.bytes() { let d = match b { - b'0'..=b'9' => (b - b'0') as u128, - b'a'..=b'f' => (b - b'a' + 10) as u128, - b'A'..=b'F' => (b - b'A' + 10) as u128, + b'0'..=b'9' => u128::from(b - b'0'), + b'a'..=b'f' => u128::from(b - b'a' + 10), + b'A'..=b'F' => u128::from(b - b'A' + 10), _ => return Err(HttpError::Protocol("a chunk size is not hex".to_string())), }; value = value diff --git a/crates/gatewayd/src/ws/handshake.rs b/crates/gatewayd/src/ws/handshake.rs index 3a584e6..238f8b8 100644 --- a/crates/gatewayd/src/ws/handshake.rs +++ b/crates/gatewayd/src/ws/handshake.rs @@ -14,24 +14,20 @@ const ALPHABET: &[u8; 64] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwx pub fn base64(bytes: &[u8]) -> String { // Standard alphabet (ALPHABET), "=" padding: each 3 bytes become 4 characters; a last group of // 1 or 2 bytes becomes 2 or 3 characters and 2 or 1 "=". No indexing that can go out of bounds. + let sextet = |n: u32, shift: u32| -> char { + let index = usize::try_from((n >> shift) & 0x3f).unwrap_or(0); + char::from(ALPHABET.get(index).copied().unwrap_or(b'A')) + }; let mut out = String::new(); for chunk in bytes.chunks(3) { let b0 = chunk.first().copied().unwrap_or(0); let b1 = chunk.get(1).copied().unwrap_or(0); let b2 = chunk.get(2).copied().unwrap_or(0); - let n = ((b0 as u32) << 16) | ((b1 as u32) << 8) | (b2 as u32); - out.push(ALPHABET[((n >> 18) & 0x3f) as usize] as char); - out.push(ALPHABET[((n >> 12) & 0x3f) as usize] as char); - out.push(if chunk.len() > 1 { - ALPHABET[((n >> 6) & 0x3f) as usize] as char - } else { - '=' - }); - out.push(if chunk.len() > 2 { - ALPHABET[(n & 0x3f) as usize] as char - } else { - '=' - }); + let n = (u32::from(b0) << 16) | (u32::from(b1) << 8) | u32::from(b2); + out.push(sextet(n, 18)); + out.push(sextet(n, 12)); + out.push(if chunk.len() > 1 { sextet(n, 6) } else { '=' }); + out.push(if chunk.len() > 2 { sextet(n, 0) } else { '=' }); } out } diff --git a/crates/gatewayd/tests/http_strict.rs b/crates/gatewayd/tests/http_strict.rs new file mode 100644 index 0000000..6d2d6ae --- /dev/null +++ b/crates/gatewayd/tests/http_strict.rs @@ -0,0 +1,57 @@ +//! The HTTP reader is strict where it can be: a status line with anything but single spaces, and a +//! chunk line not ended by CRLF, are protocol errors (M4a review, finding 3). + +use std::io::Cursor; + +use gatewayd::http::{HttpError, read_body, read_head}; + +fn read(response: &[u8]) -> Result, HttpError> { + let mut c = Cursor::new(response.to_vec()); + let head = read_head(&mut c)?; + read_body(&mut c, &head) +} + +#[test] +fn a_status_line_has_single_spaces() { + for line in [ + "HTTP/1.1 200 OK", + "HTTP/1.1 200 OK", + " HTTP/1.1 200 OK", + "HTTP/1.1\t200 OK", + ] { + let response = format!("{line}\r\nContent-Length: 0\r\n\r\n"); + let got = read(response.as_bytes()); + if line == "HTTP/1.1 200 OK" { + // The reason phrase is free text: two spaces inside it are allowed. + assert!(got.is_ok(), "{line:?}: {got:?}"); + } else { + assert!( + matches!(got, Err(HttpError::Protocol(_))), + "{line:?}: {got:?}" + ); + } + } + assert!( + read(b"HTTP/1.1 204\r\n\r\n").is_ok(), + "a status line without a reason phrase" + ); +} + +#[test] +fn chunk_lines_end_in_crlf() { + let head = "HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\n"; + let good = format!("{head}2\r\nab\r\n0\r\n\r\n"); + assert_eq!(read(good.as_bytes()).unwrap(), b"ab"); + for body in [ + "2\nab\r\n0\r\n\r\n", + "2\r\nab\n0\r\n\r\n", + "2\r\nab\r\n0\n\r\n", + "2\r\nab\r\n0\r\n\n", + ] { + let got = read(format!("{head}{body}").as_bytes()); + assert!( + matches!(got, Err(HttpError::Protocol(_))), + "{body:?}: {got:?}" + ); + } +} diff --git a/crates/proto/src/sha1.rs b/crates/proto/src/sha1.rs index 334dc79..f8a5ab3 100644 --- a/crates/proto/src/sha1.rs +++ b/crates/proto/src/sha1.rs @@ -45,7 +45,7 @@ impl Sha1 { // Use `split_at` and `get_mut(..)`, no indexing that can go out of bounds. self.length = self .length - .wrapping_add(8u64.wrapping_mul(data.len() as u64)); + .wrapping_add(8u64.wrapping_mul(u64::try_from(data.len()).unwrap_or(u64::MAX))); while !data.is_empty() { let space = 64 - self.filled; let take = data.len().min(space); diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 1448981..5815b8f 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -554,9 +554,9 @@ the milestone held. | # | Severity | Owner | Finding | Fix | |---|---|---|---|---| -| 1 | nit | implementer (06, 07) | `as` casts on values already bounded a few lines up: five in `http.rs` (chunk sizes, `MAX_BODY`), several in `handshake.rs`'s base64. None can lose data; tip I6 prefers `try_from`. | With the next change to those files | +| 1 | nit | implementer (06, 07) | `as` casts on values already bounded a few lines up: five in `http.rs` (chunk sizes, `MAX_BODY`), several in `handshake.rs`'s base64. None can lose data; tip I6 prefers `try_from`. | Fixed by the design model at the owner's request, with a third the review missed in `sha1.rs` | | 2 | low | spec, reference and implementer (04) | A secret file is checked with `symlink_metadata` and then opened by path, so someone who can write to its directory could swap it for a symbolic link in between. The reference had the same (tip T5). Opening once with `O_NOFOLLOW` and checking the open file closes it. | A follow-up task, if the owner wants it | -| 3 | nit | implementer (06) | The HTTP reader is more lenient than the reference: two spaces in the status line, and a bare `\n` ending a chunk line, are accepted. Every hostile case is still refused before any allocation. | None needed | +| 3 | nit | implementer (06) | The HTTP reader is more lenient than the reference: two spaces in the status line, and a bare `\n` ending a chunk line, are accepted. Every hostile case is still refused before any allocation. | Fixed by the design model at the owner's request: single spaces in the status line, CRLF on every chunk line (`tests/http_strict.rs`, red before) | | 4 | nit | plan (11) | The skeleton's `Pending.resume` was read only in a `todo!()` whose comment did not mention it; the implementer wrote `resume: true` directly, then removed the unused field and reported it (tip T26). | None needed | What was good: after the skeletons were split, every task finished on its first session; the