Skip to content

Commit 47d47d4

Browse files
georgeglarsonclaude
andcommitted
Fix 11 audit issues: smuggling flag, HPACK safety, TLS hardening, memory caps
H1: Flag HTTP request smuggling when both Content-Length and Transfer-Encoding are present (smuggling_risk field on HttpMessage). M1: Reduce H2Tracker limits (10K→2K connections, 16→2 MB buffers). M2: Skip HPACK decode on truncated header blocks (header_overflow flag). M3: Track multiple discarded H2 streams via HashMap (was single Option). M4: Cap HTTP messages per stream parse at 200. M5: Optimize find_next_http_start with per-first-byte method groups. M6: Apply MAX_BODY_SIZE cap to Content-Length bodies. M7: Discard TLS keys after KeyUpdate instead of futile retries. M8: Reassemble fragmented TLS handshakes across records (64 KB buffer). M9: Zeroize keylog file contents after parsing. M10: Per-direction TLS 1.3 phase flags to eliminate fragile dual-try. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 1e1af63 commit 47d47d4

7 files changed

Lines changed: 275 additions & 85 deletions

File tree

src/output/mod.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -699,6 +699,7 @@ mod tests {
699699
},
700700
headers: vec![("Host".into(), "example.com".into())],
701701
body: String::new(),
702+
smuggling_risk: false,
702703
};
703704
let (header, body) = f.format_http_text("stream-1", &msg, &None);
704705
assert!(header.contains("HTTP"));
@@ -718,6 +719,7 @@ mod tests {
718719
},
719720
headers: vec![],
720721
body: "hello".into(),
722+
smuggling_risk: false,
721723
};
722724
let (header, body) = f.format_http_text("stream-2", &msg, &None);
723725
assert!(header.contains("200 OK"));
@@ -815,6 +817,7 @@ mod tests {
815817
},
816818
headers: vec![],
817819
body: String::new(),
820+
smuggling_risk: false,
818821
};
819822
let j = f.format_http_json("stream-1", &msg);
820823
assert_eq!(j["type"], "http");

src/protocol/http.rs

Lines changed: 82 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ pub struct HttpMessage {
66
pub kind: HttpKind,
77
pub headers: Vec<(String, String)>,
88
pub body: String,
9+
/// H1: True when both Content-Length and Transfer-Encoding are present,
10+
/// indicating a potential HTTP request smuggling vector.
11+
#[serde(skip_serializing_if = "std::ops::Not::not")]
12+
pub smuggling_risk: bool,
913
}
1014

1115
#[derive(Debug, Serialize)]
@@ -50,6 +54,13 @@ impl HttpMessage {
5054
let _ = write!(out, "{}: {}\r\n", k, v);
5155
}
5256

57+
if self.smuggling_risk {
58+
let _ = write!(
59+
out,
60+
"*** WARNING: Both Content-Length and Transfer-Encoding present (potential request smuggling) ***\r\n"
61+
);
62+
}
63+
5364
out.push_str("\r\n");
5465

5566
if !self.body.is_empty() {
@@ -86,42 +97,74 @@ const KNOWN_METHODS: &[&str] = &[
8697
/// M11: Maximum chunked body size (10 MB).
8798
const MAX_CHUNKED_BODY: usize = 10 * 1024 * 1024;
8899

89-
/// Maximum close-delimited response body size (1 MB).
90-
/// Independent of stream reassembly limits for defense in depth.
100+
/// Maximum response body size (1 MB) for both Content-Length and
101+
/// close-delimited bodies. Independent of stream reassembly limits
102+
/// for defense in depth.
91103
const MAX_BODY_SIZE: usize = 1_048_576;
92104

105+
/// Maximum number of HTTP messages parsed from a single stream.
106+
const MAX_MESSAGES_PER_PARSE: usize = 200;
107+
93108
/// H1: Find the start of the next HTTP message in a byte slice.
94109
/// Looks for request methods and "HTTP/" response starts.
95110
/// M7: Uses first-byte filtering to reduce scanning — only bytes matching
96111
/// a method's first character trigger the inner starts_with comparisons.
97112
/// M6: Requires full "HTTP/x.y NNN" pattern for response starts to reduce
98113
/// false splits in close-delimited response bodies.
99114
fn find_next_http_start(data: &[u8]) -> Option<usize> {
115+
// Map each method's first byte to the methods starting with that byte,
116+
// so we only check relevant methods per byte (avoids quadratic scan).
117+
const METHOD_G: &[&str] = &["GET"];
118+
const METHOD_P: &[&str] = &["POST", "PUT", "PATCH"];
119+
const METHOD_D: &[&str] = &["DELETE"];
120+
const METHOD_C: &[&str] = &["CONNECT"];
121+
const METHOD_O: &[&str] = &["OPTIONS"];
122+
const METHOD_T: &[&str] = &["TRACE"];
123+
100124
for i in 0..data.len() {
101-
// M7: Filter by first byte before attempting starts_with comparisons
102-
match data[i] {
125+
let remaining = &data[i..];
126+
match remaining[0] {
103127
b'H' => {
104128
// M6: Require "HTTP/" followed by digit.digit space digit
105-
// to avoid false-matching "HTTP/" in body content.
106-
if data.len() >= i + 10
107-
&& data[i..].starts_with(b"HTTP/")
108-
&& data[i + 5].is_ascii_digit()
109-
&& data[i + 6] == b'.'
110-
&& data[i + 7].is_ascii_digit()
111-
&& data[i + 8] == b' '
112-
&& data[i + 9].is_ascii_digit()
129+
if remaining.len() >= 10
130+
&& remaining.starts_with(b"HTTP/")
131+
&& remaining[5].is_ascii_digit()
132+
&& remaining[6] == b'.'
133+
&& remaining[7].is_ascii_digit()
134+
&& remaining[8] == b' '
135+
&& remaining[9].is_ascii_digit()
113136
{
114137
return Some(i);
115138
}
116139
}
117-
b'G' | b'P' | b'D' | b'C' | b'O' | b'T' => {
118-
for method in KNOWN_METHODS {
119-
if data[i..].starts_with(method.as_bytes()) {
120-
let after = i + method.len();
121-
if after < data.len() && data[after] == b' ' {
122-
return Some(i);
123-
}
124-
}
140+
b'G' => {
141+
if check_methods(remaining, METHOD_G) {
142+
return Some(i);
143+
}
144+
}
145+
b'P' => {
146+
if check_methods(remaining, METHOD_P) {
147+
return Some(i);
148+
}
149+
}
150+
b'D' => {
151+
if check_methods(remaining, METHOD_D) {
152+
return Some(i);
153+
}
154+
}
155+
b'C' => {
156+
if check_methods(remaining, METHOD_C) {
157+
return Some(i);
158+
}
159+
}
160+
b'O' => {
161+
if check_methods(remaining, METHOD_O) {
162+
return Some(i);
163+
}
164+
}
165+
b'T' => {
166+
if check_methods(remaining, METHOD_T) {
167+
return Some(i);
125168
}
126169
}
127170
_ => {}
@@ -130,14 +173,26 @@ fn find_next_http_start(data: &[u8]) -> Option<usize> {
130173
None
131174
}
132175

176+
/// Check if remaining data starts with any of the given methods followed by a space.
177+
#[inline]
178+
fn check_methods(data: &[u8], methods: &[&str]) -> bool {
179+
for method in methods {
180+
let mb = method.as_bytes();
181+
if data.len() > mb.len() && data.starts_with(mb) && data[mb.len()] == b' ' {
182+
return true;
183+
}
184+
}
185+
false
186+
}
187+
133188
/// Try to parse one or more HTTP messages from a stream payload.
134189
/// Returns all successfully parsed messages.
135190
/// Operates on raw bytes to avoid UTF-8 offset mismatches with binary bodies.
136191
pub fn parse_http(data: &[u8]) -> Vec<HttpMessage> {
137192
let mut messages = Vec::new();
138193
let mut remaining = data;
139194

140-
while !remaining.is_empty() {
195+
while !remaining.is_empty() && messages.len() < MAX_MESSAGES_PER_PARSE {
141196
let (header_end, sep_len) = match find_header_end(remaining) {
142197
Some(result) => result,
143198
None => break,
@@ -206,6 +261,10 @@ pub fn parse_http(data: &[u8]) -> Vec<HttpMessage> {
206261

207262
let is_response = matches!(kind, HttpKind::Response { .. });
208263

264+
// H1: Flag potential HTTP request smuggling when both Content-Length
265+
// and Transfer-Encoding are present (RFC 7230 Section 3.3.3).
266+
let smuggling_risk = is_chunked && content_length.is_some();
267+
209268
let (body, consumed) = if is_chunked {
210269
match decode_chunked(after_headers) {
211270
Some((decoded, bytes_consumed)) => (
@@ -215,7 +274,7 @@ pub fn parse_http(data: &[u8]) -> Vec<HttpMessage> {
215274
None => (String::new(), 0),
216275
}
217276
} else if let Some(cl) = content_length {
218-
let len = cl.min(after_headers.len());
277+
let len = cl.min(after_headers.len()).min(MAX_BODY_SIZE);
219278
(
220279
String::from_utf8_lossy(&after_headers[..len]).into_owned(),
221280
len,
@@ -240,6 +299,7 @@ pub fn parse_http(data: &[u8]) -> Vec<HttpMessage> {
240299
kind,
241300
headers,
242301
body,
302+
smuggling_risk,
243303
});
244304
}
245305

src/protocol/http2.rs

Lines changed: 46 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -25,9 +25,12 @@ const MAX_FRAME_PAYLOAD: usize = 16_777_215; // 2^24 - 1
2525
const MAX_STREAMS_PER_CONN: usize = 1_000;
2626
const MAX_HEADER_BLOCK: usize = 65_536;
2727
const MAX_DATA_PER_STREAM: usize = 1_048_576; // 1 MB
28-
const MAX_CONNECTIONS: usize = 10_000;
29-
/// M1: Cap per-direction buffer to prevent unbounded memory growth.
30-
const MAX_DIRECTION_BUF: usize = 16 * 1024 * 1024; // 16 MB
28+
/// M1: Reduced from 10,000 to 2,000 to cap compound memory usage.
29+
/// 2,000 connections * 2 MB * 2 directions = ~8 GB worst case.
30+
const MAX_CONNECTIONS: usize = 2_000;
31+
/// M1: Cap per-direction buffer. Reduced from 16 MB to 2 MB to keep
32+
/// aggregate memory bounded (~8 GB worst case with MAX_CONNECTIONS).
33+
const MAX_DIRECTION_BUF: usize = 2 * 1024 * 1024; // 2 MB
3134

3235
/// Direction of data flow within a connection.
3336
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
@@ -55,12 +58,12 @@ struct H2Connection {
5558
detected: Option<bool>,
5659
/// Monotonic tick for LRU eviction.
5760
last_active: u64,
58-
/// Accumulates header block fragments for discarded streams (rejected at
61+
/// M3: Accumulates header block fragments for discarded streams (rejected at
5962
/// stream limit) or PUSH_PROMISE frames that span multiple CONTINUATION
6063
/// frames. HPACK is stateful — every header block must be decoded even if
6164
/// the headers are discarded, otherwise the dynamic table becomes corrupted.
62-
/// Format: (stream_id, direction, accumulated_header_bytes).
63-
discard_header_buf: Option<(u32, H2Direction, Vec<u8>)>,
65+
/// Uses a HashMap to track multiple concurrent discarded streams.
66+
discard_header_bufs: HashMap<u32, (H2Direction, Vec<u8>)>,
6467
}
6568

6669
struct H2Stream {
@@ -78,6 +81,10 @@ struct H2Stream {
7881
end_stream_headers: bool,
7982
/// Whether END_STREAM was set on a DATA frame.
8083
end_stream_data: bool,
84+
/// M2: Set when accumulated header block exceeds MAX_HEADER_BLOCK.
85+
/// When true, HPACK decoding is skipped to avoid partial dynamic table
86+
/// mutation from a truncated header block.
87+
header_overflow: bool,
8188
}
8289

8390
impl H2Stream {
@@ -90,6 +97,7 @@ impl H2Stream {
9097
end_headers: false,
9198
end_stream_headers: false,
9299
end_stream_data: false,
100+
header_overflow: false,
93101
}
94102
}
95103
}
@@ -104,7 +112,7 @@ impl H2Connection {
104112
server_buf: Vec::new(),
105113
detected: None,
106114
last_active: 0,
107-
discard_header_buf: None,
115+
discard_header_bufs: HashMap::new(),
108116
}
109117
}
110118
}
@@ -288,15 +296,18 @@ impl H2Tracker {
288296
if header_block.len() <= MAX_HEADER_BLOCK {
289297
buf.extend_from_slice(header_block);
290298
}
291-
conn.discard_header_buf = Some((stream_id, direction, buf));
299+
conn.discard_header_bufs.insert(stream_id, (direction, buf));
292300
}
293301
continue;
294302
}
295303

296304
let stream = conn.streams.entry(stream_id).or_insert_with(H2Stream::new);
297305

306+
// M2: Track header block overflow to avoid partial HPACK decode
298307
if stream.header_buf.len() + header_block.len() <= MAX_HEADER_BLOCK {
299308
stream.header_buf.extend_from_slice(header_block);
309+
} else {
310+
stream.header_overflow = true;
300311
}
301312

302313
if end_stream {
@@ -305,11 +316,13 @@ impl H2Tracker {
305316

306317
if end_headers {
307318
stream.end_headers = true;
308-
let decoder = match direction {
309-
H2Direction::ClientToServer => &mut conn.client_decoder,
310-
H2Direction::ServerToClient => &mut conn.server_decoder,
311-
};
312-
decode_headers(stream, decoder);
319+
if !stream.header_overflow {
320+
let decoder = match direction {
321+
H2Direction::ClientToServer => &mut conn.client_decoder,
322+
H2Direction::ServerToClient => &mut conn.server_decoder,
323+
};
324+
decode_headers(stream, decoder);
325+
}
313326

314327
if stream.end_stream_headers {
315328
if let Some(msg) = build_message(stream) {
@@ -322,18 +335,25 @@ impl H2Tracker {
322335
FRAME_CONTINUATION => {
323336
if let Some(stream) = conn.streams.get_mut(&stream_id) {
324337
if !stream.end_headers {
325-
if stream.header_buf.len() + frame_payload.len() <= MAX_HEADER_BLOCK {
338+
// M2: Track overflow instead of silently dropping
339+
if stream.header_buf.len() + frame_payload.len() <= MAX_HEADER_BLOCK
340+
&& !stream.header_overflow
341+
{
326342
stream.header_buf.extend_from_slice(&frame_payload);
343+
} else {
344+
stream.header_overflow = true;
327345
}
328346

329347
let end_headers = flags & FLAG_END_HEADERS != 0;
330348
if end_headers {
331349
stream.end_headers = true;
332-
let decoder = match direction {
333-
H2Direction::ClientToServer => &mut conn.client_decoder,
334-
H2Direction::ServerToClient => &mut conn.server_decoder,
335-
};
336-
decode_headers(stream, decoder);
350+
if !stream.header_overflow {
351+
let decoder = match direction {
352+
H2Direction::ClientToServer => &mut conn.client_decoder,
353+
H2Direction::ServerToClient => &mut conn.server_decoder,
354+
};
355+
decode_headers(stream, decoder);
356+
}
337357

338358
if stream.end_stream_headers {
339359
if let Some(msg) = build_message(stream) {
@@ -347,10 +367,11 @@ impl H2Tracker {
347367
// CONTINUATION for a discarded stream or PUSH_PROMISE —
348368
// accumulate fragments and decode HPACK when complete.
349369
let end_headers = flags & FLAG_END_HEADERS != 0;
350-
if let Some((discard_sid, discard_dir, ref mut buf)) =
351-
conn.discard_header_buf
352-
&& discard_sid == stream_id
370+
// M3: Use HashMap to track multiple discarded streams
371+
if let Some((discard_dir, buf)) =
372+
conn.discard_header_bufs.get_mut(&stream_id)
353373
{
374+
let discard_dir = *discard_dir;
354375
if buf.len() + frame_payload.len() <= MAX_HEADER_BLOCK {
355376
buf.extend_from_slice(&frame_payload);
356377
}
@@ -360,7 +381,7 @@ impl H2Tracker {
360381
H2Direction::ServerToClient => &mut conn.server_decoder,
361382
};
362383
let _ = decoder.decode(buf);
363-
conn.discard_header_buf = None;
384+
conn.discard_header_bufs.remove(&stream_id);
364385
}
365386
continue;
366387
}
@@ -449,7 +470,7 @@ impl H2Tracker {
449470
if header_block.len() <= MAX_HEADER_BLOCK {
450471
buf.extend_from_slice(header_block);
451472
}
452-
conn.discard_header_buf = Some((stream_id, direction, buf));
473+
conn.discard_header_bufs.insert(stream_id, (direction, buf));
453474
}
454475
}
455476
FRAME_SETTINGS => {
@@ -575,6 +596,7 @@ fn build_message(stream: &H2Stream) -> Option<HttpMessage> {
575596
kind,
576597
headers: stream.headers.clone(),
577598
body,
599+
smuggling_risk: false,
578600
})
579601
}
580602

src/tls/handshake.rs

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,8 @@ pub(crate) struct HandshakeResult {
2121
/// used to preserve ClientHello's supported_versions detection over ServerHello's
2222
/// legacy version field.
2323
///
24-
/// **Limitation (L25):** This assumes the handshake message fits in a single TLS record.
25-
/// Handshake messages spanning multiple records (fragmented handshakes) are not reassembled
26-
/// and will fail to parse. This is acceptable for typical deployments where ClientHello and
27-
/// ServerHello fit in one record.
24+
/// M8: Callers should use `TlsDecryptor::accumulate_handshake` to reassemble
25+
/// handshake messages spanning multiple TLS records before calling this function.
2826
pub(crate) fn parse_handshake(
2927
data: &[u8],
3028
src_ip: IpAddr,

0 commit comments

Comments
 (0)