From 22731762d9fe2c98d9e6c3942b1568266b23c69f Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Wed, 9 Aug 2023 11:02:34 +0200 Subject: [PATCH] BUG/MINOR: http: skip leading zeroes in content-length values Ben Kallus also noticed that we preserve leading zeroes on content-length values. While this is totally valid, it would be safer to at least trim them before passing the value, because a bogus server written to parse using "strtol(value, NULL, 0)" could inadvertently take a leading zero as a prefix for an octal value. While there is not much that can be done to protect such servers in general (e.g. lack of check for overflows etc), at least it's quite cheap to make sure the transmitted value is normalized and not taken for an octal one. This is not really a bug, rather a missed opportunity to sanitize the input, but is marked as a bug so that we don't forget to backport it to stable branches. A combined regtest was added to h1or2_to_h1c which already validates end-to-end syntax consistency on aggregate headers. --- reg-tests/http-rules/h1or2_to_h1c.vtc | 16 ++++++++++++---- src/h1.c | 8 ++++++++ src/http.c | 8 ++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/reg-tests/http-rules/h1or2_to_h1c.vtc b/reg-tests/http-rules/h1or2_to_h1c.vtc index 90834c562..3dd907e16 100644 --- a/reg-tests/http-rules/h1or2_to_h1c.vtc +++ b/reg-tests/http-rules/h1or2_to_h1c.vtc @@ -26,11 +26,11 @@ server s1 { -body "This is a body" expect req.method == "GET" - expect req.http.fe-sl1-crc == 992395575 - expect req.http.fe-sl2-crc == 1270056220 + expect req.http.fe-sl1-crc == 1874847043 + expect req.http.fe-sl2-crc == 1142278307 expect req.http.fe-hdr-crc == 1719311923 - expect req.http.be-sl1-crc == 2604236007 - expect req.http.be-sl2-crc == 4181358964 + expect req.http.be-sl1-crc == 3455320059 + expect req.http.be-sl2-crc == 2509326257 expect req.http.be-hdr-crc == 3634102538 } -repeat 2 -start @@ -51,6 +51,7 @@ haproxy h1 -conf { http-request set-var(req.path) path http-request set-var(req.query) query http-request set-var(req.param) url_param(qs_arg) + http-request set-var(req.cl) req.fhdr(content-length) http-request set-header sl1 "sl1: " @@ -63,8 +64,10 @@ haproxy h1 -conf { http-request set-header sl1 "%[req.fhdr(sl1)] method=<%[var(req.method)]>; uri=<%[var(req.uri)]>; path=<%[var(req.path)]>;" http-request set-header sl1 "%[req.fhdr(sl1)] query=<%[var(req.query)]>; param=<%[var(req.param)]>" + http-request set-header sl1 "%[req.fhdr(sl1)] cl=<%[var(req.cl)]>" http-request set-header sl2 "%[req.fhdr(sl2)] method=<%[method]>; uri=<%[url]>; path=<%[path]>; " http-request set-header sl2 "%[req.fhdr(sl2)] query=<%[query]>; param=<%[url_param(qs_arg)]>" + http-request set-header sl2 "%[req.fhdr(sl2)] cl=<%[req.fhdr(content-length)]>" http-request set-header hdr "%[req.fhdr(hdr)] hdr1=<%[req.hdr(hdr1)]>; fhdr1=<%[req.fhdr(hdr1)]>;" http-request set-header hdr "%[req.fhdr(hdr)] hdr2=<%[req.hdr(hdr2)]>; fhdr2=<%[req.fhdr(hdr2)]>;" http-request set-header hdr "%[req.fhdr(hdr)] hdr3=<%[req.hdr(hdr3)]>; fhdr3=<%[req.fhdr(hdr3)]>;" @@ -118,6 +121,7 @@ haproxy h1 -conf { http-request set-var(req.path) path http-request set-var(req.query) query http-request set-var(req.param) url_param(qs_arg) + http-request set-var(req.cl) req.fhdr(content-length) http-request set-header sl1 "sl1: " @@ -130,8 +134,10 @@ haproxy h1 -conf { http-request set-header sl1 "%[req.fhdr(sl1)] method=<%[var(req.method)]>; uri=<%[var(req.uri)]>; path=<%[var(req.path)]>;" http-request set-header sl1 "%[req.fhdr(sl1)] query=<%[var(req.query)]>; param=<%[var(req.param)]>" + http-request set-header sl1 "%[req.fhdr(sl1)] cl=<%[var(req.cl)]>" http-request set-header sl2 "%[req.fhdr(sl2)] method=<%[method]>; uri=<%[url]>; path=<%[path]>; " http-request set-header sl2 "%[req.fhdr(sl2)] query=<%[query]>; param=<%[url_param(QS_arg,,i)]>" + http-request set-header sl2 "%[req.fhdr(sl2)] cl=<%[req.fhdr(content-length)]>" http-request set-header hdr "%[req.fhdr(hdr)] hdr1=<%[req.hdr(hdr1)]>; fhdr1=<%[req.fhdr(hdr1)]>;" http-request set-header hdr "%[req.fhdr(hdr)] hdr2=<%[req.hdr(hdr2)]>; fhdr2=<%[req.fhdr(hdr2)]>;" http-request set-header hdr "%[req.fhdr(hdr)] hdr3=<%[req.hdr(hdr3)]>; fhdr3=<%[req.fhdr(hdr3)]>;" @@ -169,6 +175,7 @@ client c1h1 -connect ${h1_feh1_sock} { txreq \ -req GET \ -url /path/to/file.extension?qs_arg=qs_value \ + -hdr "content-length: 000, 00" \ -hdr "hdr1: val1" \ -hdr "hdr2: val2a" \ -hdr "hdr2: val2b" \ @@ -203,6 +210,7 @@ client c1h2 -connect ${h1_feh2_sock} { -req GET \ -scheme "https" \ -url /path/to/file.extension?qs_arg=qs_value \ + -hdr "content-length" "000, 00" \ -hdr "hdr1" "val1" \ -hdr "hdr2" " val2a" \ -hdr "hdr2" " val2b" \ diff --git a/src/h1.c b/src/h1.c index 92ec96bfe..38b73cd95 100644 --- a/src/h1.c +++ b/src/h1.c @@ -58,6 +58,14 @@ int h1_parse_cont_len_header(struct h1m *h1m, struct ist *value) goto fail; break; } + + if (unlikely(!cl && n > word.ptr)) { + /* There was a leading zero before this digit, + * let's trim it. + */ + word.ptr = n; + } + if (unlikely(cl > ULLONG_MAX / 10ULL)) goto fail; /* multiply overflow */ cl = cl * 10ULL; diff --git a/src/http.c b/src/http.c index 85cd2f2e1..2436292b2 100644 --- a/src/http.c +++ b/src/http.c @@ -731,6 +731,14 @@ int http_parse_cont_len_header(struct ist *value, unsigned long long *body_len, goto fail; break; } + + if (unlikely(!cl && n > word.ptr)) { + /* There was a leading zero before this digit, + * let's trim it. + */ + word.ptr = n; + } + if (unlikely(cl > ULLONG_MAX / 10ULL)) goto fail; /* multiply overflow */ cl = cl * 10ULL;