Skip to content

Commit 0ac1fbd

Browse files
latenighthackathonlatenighthackathon
authored
fix(l7): reject duplicate Content-Length headers to prevent request smuggling (CWE-444) (#663)
* fix(l7): reject duplicate Content-Length headers to prevent request smuggling Both parse_body_length() in rest.rs and try_parse_http_request() in inference.rs silently accepted multiple Content-Length headers, overwriting with the last value seen. Per RFC 7230 Section 3.3.3, a message with multiple Content-Length headers with differing values must be rejected to prevent HTTP request smuggling (CWE-444). An attacker could send conflicting Content-Length values causing the proxy and downstream server to disagree on message boundaries. Fix: - rest.rs: detect duplicate CL headers with differing values and return an error before forwarding - inference.rs: add ParseResult::Invalid variant; detect duplicate CL headers and return Invalid with a descriptive reason - proxy.rs: handle ParseResult::Invalid by sending HTTP 400 and denying the connection Closes #637 Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> * fix(l7): address review feedback on Content-Length smuggling defense - inference.rs: reject unparseable Content-Length values instead of silently defaulting to 0 via unwrap_or(0) - rest.rs: reject unparseable Content-Length values so a valid+invalid duplicate pair cannot bypass the differing-values check - rest.rs: fix Transfer-Encoding substring match (.contains("chunked") → split/trim exact match) to align with inference.rs and prevent false positives on values like "chunkedx" - proxy.rs: log parsing details server-side via tracing::warn and return generic "Bad Request" body instead of leaking internal parsing reasons to sandboxed code - Add tests for all new rejection paths in inference.rs and rest.rs Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> * style(l7): apply cargo fmt formatting Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> --------- Signed-off-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com>
1 parent a7ebf3a commit 0ac1fbd

File tree

3 files changed

+129
-6
lines changed

3 files changed

+129
-6
lines changed

‎crates/openshell-sandbox/src/l7/inference.rs‎

Lines changed: 57 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,8 @@ pub enum ParseResult {
9696
Complete(ParsedHttpRequest, usize),
9797
/// Headers are incomplete — caller should read more data.
9898
Incomplete,
99+
/// The request is malformed and must be rejected (e.g., duplicate Content-Length).
100+
Invalid(String),
99101
}
100102

101103
/// Try to parse an HTTP/1.1 request from raw bytes.
@@ -125,6 +127,7 @@ pub fn try_parse_http_request(buf: &[u8]) -> ParseResult {
125127

126128
let mut headers = Vec::new();
127129
let mut content_length: usize = 0;
130+
let mut has_content_length = false;
128131
let mut is_chunked = false;
129132
for line in lines {
130133
if line.is_empty() {
@@ -134,7 +137,21 @@ pub fn try_parse_http_request(buf: &[u8]) -> ParseResult {
134137
let name = name.trim().to_string();
135138
let value = value.trim().to_string();
136139
if name.eq_ignore_ascii_case("content-length") {
137-
content_length = value.parse().unwrap_or(0);
140+
let new_len: usize = match value.parse() {
141+
Ok(v) => v,
142+
Err(_) => {
143+
return ParseResult::Invalid(format!(
144+
"invalid Content-Length value: {value}"
145+
));
146+
}
147+
};
148+
if has_content_length && new_len != content_length {
149+
return ParseResult::Invalid(format!(
150+
"duplicate Content-Length headers with differing values ({content_length} vs {new_len})"
151+
));
152+
}
153+
content_length = new_len;
154+
has_content_length = true;
138155
}
139156
if name.eq_ignore_ascii_case("transfer-encoding")
140157
&& value
@@ -552,4 +569,43 @@ mod tests {
552569
};
553570
assert_eq!(parsed.body.len(), 100);
554571
}
572+
573+
// ---- SEC: Content-Length validation ----
574+
575+
#[test]
576+
fn reject_differing_duplicate_content_length() {
577+
let request = b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nContent-Length: 0\r\nContent-Length: 50\r\n\r\n";
578+
assert!(matches!(
579+
try_parse_http_request(request),
580+
ParseResult::Invalid(reason) if reason.contains("differing values")
581+
));
582+
}
583+
584+
#[test]
585+
fn accept_identical_duplicate_content_length() {
586+
let request = b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nContent-Length: 5\r\nContent-Length: 5\r\n\r\nhello";
587+
let ParseResult::Complete(parsed, _) = try_parse_http_request(request) else {
588+
panic!("expected Complete for identical duplicate CL");
589+
};
590+
assert_eq!(parsed.body, b"hello");
591+
}
592+
593+
#[test]
594+
fn reject_non_numeric_content_length() {
595+
let request =
596+
b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nContent-Length: abc\r\n\r\n";
597+
assert!(matches!(
598+
try_parse_http_request(request),
599+
ParseResult::Invalid(reason) if reason.contains("invalid Content-Length")
600+
));
601+
}
602+
603+
#[test]
604+
fn reject_two_non_numeric_content_lengths() {
605+
let request = b"POST /v1/chat/completions HTTP/1.1\r\nHost: x\r\nContent-Length: abc\r\nContent-Length: def\r\n\r\n";
606+
assert!(matches!(
607+
try_parse_http_request(request),
608+
ParseResult::Invalid(_)
609+
));
610+
}
555611
}

‎crates/openshell-sandbox/src/l7/rest.rs‎

Lines changed: 66 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -242,14 +242,22 @@ fn parse_body_length(headers: &str) -> Result<BodyLength> {
242242
let lower = line.to_ascii_lowercase();
243243
if lower.starts_with("transfer-encoding:") {
244244
let val = lower.split_once(':').map_or("", |(_, v)| v.trim());
245-
if val.contains("chunked") {
245+
if val.split(',').any(|enc| enc.trim() == "chunked") {
246246
has_te_chunked = true;
247247
}
248248
}
249-
if lower.starts_with("content-length:")
250-
&& let Some(val) = lower.split_once(':').map(|(_, v)| v.trim())
251-
&& let Ok(len) = val.parse::<u64>()
252-
{
249+
if lower.starts_with("content-length:") {
250+
let val = lower.split_once(':').map_or("", |(_, v)| v.trim());
251+
let len: u64 = val
252+
.parse()
253+
.map_err(|_| miette!("Request contains invalid Content-Length value"))?;
254+
if let Some(prev) = cl_value {
255+
if prev != len {
256+
return Err(miette!(
257+
"Request contains multiple Content-Length headers with differing values ({prev} vs {len})"
258+
));
259+
}
260+
}
253261
cl_value = Some(len);
254262
}
255263
}
@@ -702,6 +710,59 @@ mod tests {
702710
);
703711
}
704712

713+
/// SEC: Reject differing duplicate Content-Length headers.
714+
#[test]
715+
fn reject_differing_duplicate_content_length() {
716+
let headers =
717+
"POST /api HTTP/1.1\r\nHost: x\r\nContent-Length: 0\r\nContent-Length: 50\r\n\r\n";
718+
assert!(
719+
parse_body_length(headers).is_err(),
720+
"Must reject differing duplicate Content-Length"
721+
);
722+
}
723+
724+
/// SEC: Accept identical duplicate Content-Length headers.
725+
#[test]
726+
fn accept_identical_duplicate_content_length() {
727+
let headers =
728+
"POST /api HTTP/1.1\r\nHost: x\r\nContent-Length: 42\r\nContent-Length: 42\r\n\r\n";
729+
match parse_body_length(headers).unwrap() {
730+
BodyLength::ContentLength(42) => {}
731+
other => panic!("Expected ContentLength(42), got {other:?}"),
732+
}
733+
}
734+
735+
/// SEC: Reject non-numeric Content-Length values.
736+
#[test]
737+
fn reject_non_numeric_content_length() {
738+
let headers = "POST /api HTTP/1.1\r\nHost: x\r\nContent-Length: abc\r\n\r\n";
739+
assert!(
740+
parse_body_length(headers).is_err(),
741+
"Must reject non-numeric Content-Length"
742+
);
743+
}
744+
745+
/// SEC: Reject when second Content-Length is non-numeric (bypass test).
746+
#[test]
747+
fn reject_valid_then_invalid_content_length() {
748+
let headers =
749+
"POST /api HTTP/1.1\r\nHost: x\r\nContent-Length: 42\r\nContent-Length: abc\r\n\r\n";
750+
assert!(
751+
parse_body_length(headers).is_err(),
752+
"Must reject when any Content-Length is non-numeric"
753+
);
754+
}
755+
756+
/// SEC: Transfer-Encoding substring match must not match partial tokens.
757+
#[test]
758+
fn te_substring_not_chunked() {
759+
let headers = "POST /api HTTP/1.1\r\nHost: x\r\nTransfer-Encoding: chunkedx\r\n\r\n";
760+
match parse_body_length(headers).unwrap() {
761+
BodyLength::None => {}
762+
other => panic!("Expected None for non-matching TE, got {other:?}"),
763+
}
764+
}
765+
705766
/// SEC-009: Bare LF in headers enables header injection.
706767
#[tokio::test]
707768
async fn reject_bare_lf_in_headers() {

‎crates/openshell-sandbox/src/proxy.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -943,6 +943,12 @@ async fn handle_inference_interception(
943943
buf.resize((buf.len() * 2).min(MAX_INFERENCE_BUF), 0);
944944
}
945945
}
946+
ParseResult::Invalid(reason) => {
947+
warn!(reason = %reason, "rejecting malformed inference request");
948+
let response = format_http_response(400, &[], b"Bad Request");
949+
write_all(&mut tls_client, &response).await?;
950+
return Ok(InferenceOutcome::Denied { reason });
951+
}
946952
}
947953
}
948954

0 commit comments

Comments
 (0)