gatewayd: strict status lines and chunk lines; no as casts (M4a review, findings 1 and 3)
A status line splits on single spaces only, and every chunk line must end in CRLF. The bounded `as` casts in http.rs, handshake.rs and proto's sha1.rs become try_from and from. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5.5
+18
-12
@@ -171,8 +171,9 @@ pub fn read_head(stream: &mut dyn Read) -> Result<Head, HttpError> {
|
||||
}
|
||||
|
||||
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<Vec<u8>, 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<String, HttpError> {
|
||||
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<Vec<u8>, 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<u128, HttpError> {
|
||||
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
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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<Vec<u8>, 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:?}"
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -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);
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user