From 97ec9e13c7ca0bf80f6297e6b202c68ff35c7370 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Kugland?= Date: Wed, 13 May 2026 17:06:38 -0300 Subject: [PATCH 1/4] test: regression test for parse_field header smuggling Send a request whose Referer value contains the substring "X-Forwarded-For: " but no real X-Forwarded-For header. With the pre-fix parse_field (strcasestr over the whole buffer) the smuggled value was logged as the client IP; with the fix the real client IP is logged. --- devel/test_log_xff.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/devel/test_log_xff.py b/devel/test_log_xff.py index 9daad7b..89e0085 100644 --- a/devel/test_log_xff.py +++ b/devel/test_log_xff.py @@ -90,6 +90,30 @@ def test_xff_ipv6(self): self.assertTrue(line.startswith(ipv6 + " "), f"Expected log to start with IPv6 {ipv6}. Log line: {line}") + def test_xff_smuggled_in_referer(self): + """ + A client without a real X-Forwarded-For header must not be able to + spoof its logged IP by smuggling the field name inside another + header's value (here: Referer). The log must show the real client + IP, not the smuggled one. + """ + spoof_ip = "99.99.99.99" + time_marker = random_str() + url = f"/xff_smuggle-{time_marker}" + + # No X-Forwarded-For header is sent; the substring lives inside + # the Referer value. Pre-fix parse_field used strcasestr() over + # the whole request and would match this. + self.get(url, req_hdrs={"Referer": f"X-Forwarded-For: {spoof_ip}"}) + + line = self._wait_and_check_log(url) + self.assertFalse(line.startswith(spoof_ip + " "), + f"Spoofed IP {spoof_ip} smuggled via Referer was logged as the " + f"client address. Log line: {line}") + self.assertTrue(line.startswith("127.0.0.1 "), + f"Expected log to start with real client IP 127.0.0.1. " + f"Log line: {line}") + def test_xff_ipv6_list(self): """ Test a list containing IPv6 and IPv4. From b8680c568d04c8a03148b093ba002bb382dc9fbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Kugland?= Date: Wed, 13 May 2026 13:33:41 -0300 Subject: [PATCH 2/4] fix: match parse_field header names only at line starts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parse_field used strcasestr against the entire request buffer, so any substring matching the header name was accepted — including bytes from the request-target or another header's value. This let any client smuggle Authorization, Range, X-Forwarded-For, Host, If-Modified-Since, etc., into log entries and request handling (notably enabling X-Forwarded-For spoofing through the trusted-proxy path). Walk header lines and only match the field at the start of a line. --- darkhttpd.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/darkhttpd.c b/darkhttpd.c index bcfb248..b62edb6 100644 --- a/darkhttpd.c +++ b/darkhttpd.c @@ -1778,14 +1778,28 @@ static void redirect(struct connection *conn, const char *format, ...) { */ static char *parse_field(const struct connection *conn, const char *field) { size_t bound1, bound2; - char *pos; - - /* find start */ - pos = strcasestr(conn->request, field); + const size_t field_len = strlen(field); + char *pos = NULL; + + /* Walk header lines: only match [field] at the start of a line so that + * substrings inside the request-target or another header's value cannot + * be mistaken for a real header. The first line is the request line and + * will not match any of the header names we look for. */ + for (size_t i = 0; i + field_len <= conn->request_length; ) { + if (strncasecmp(conn->request + i, field, field_len) == 0) { + pos = conn->request + i; + break; + } + /* Advance to the byte after the next '\n', or stop if none. */ + const char *nl = memchr(conn->request + i, '\n', + conn->request_length - i); + if (nl == NULL) + break; + i = (size_t)(nl - conn->request) + 1; + } if (pos == NULL) return NULL; - assert(pos >= conn->request); - bound1 = (size_t)(pos - conn->request) + strlen(field); + bound1 = (size_t)(pos - conn->request) + field_len; /* find end */ for (bound2 = bound1; From 03a7771a7f3c4a705e2e40be64f5ec49ab42ef4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Kugland?= Date: Fri, 15 May 2026 21:29:46 -0300 Subject: [PATCH 3/4] test: cover false-negative in single-hit line-start check When the target field name appears in a prior header's value and the real header follows, a fix based on a single strcasestr call with a pos[-1] != '\n' guard returns NULL and drops the real header. The line-walking approach handles this correctly. --- devel/test_log_xff.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/devel/test_log_xff.py b/devel/test_log_xff.py index 89e0085..687ac6a 100644 --- a/devel/test_log_xff.py +++ b/devel/test_log_xff.py @@ -114,6 +114,34 @@ def test_xff_smuggled_in_referer(self): f"Expected log to start with real client IP 127.0.0.1. " f"Log line: {line}") + def test_xff_real_header_not_masked_by_prior_value_match(self): + """ + When the field name appears inside a prior header's value *and* a real + X-Forwarded-For header is also present, the real header must still be + found. A single-hit approach (find the first strcasestr match, check + pos[-1] != '\\n', stop) would reject that hit and return NULL, losing + the real header entirely. + """ + real_ip = "10.20.30.40" + spoof_ip = "99.99.99.99" + time_marker = random_str() + url = f"/xff_masked-{time_marker}" + + # Referer value contains "X-Forwarded-For: " as a substring. + # The real X-Forwarded-For header comes after. + self.get(url, req_hdrs={ + "Referer": f"http://evil.example/X-Forwarded-For: {spoof_ip}", + "X-Forwarded-For": real_ip, + }) + + line = self._wait_and_check_log(url) + self.assertTrue(line.startswith(real_ip + " "), + f"Expected log to start with real XFF IP {real_ip}. " + f"Log line: {line}") + self.assertFalse(line.startswith(spoof_ip + " "), + f"Spoofed IP {spoof_ip} (embedded in Referer value) was logged. " + f"Log line: {line}") + def test_xff_ipv6_list(self): """ Test a list containing IPv6 and IPv4. From 2fdd455b2fc44b397ea72ffe62edc13c41ac1389 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Kugland?= Date: Sat, 16 May 2026 21:19:12 -0300 Subject: [PATCH 4/4] fix: parse_field: prepend '\n' to anchor match to line start Instead of walking lines manually, prepend '\n' to the search string so strcasestr only matches the field name at the start of a line. Suggested by g-rden. --- darkhttpd.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/darkhttpd.c b/darkhttpd.c index b62edb6..174246a 100644 --- a/darkhttpd.c +++ b/darkhttpd.c @@ -1778,28 +1778,18 @@ static void redirect(struct connection *conn, const char *format, ...) { */ static char *parse_field(const struct connection *conn, const char *field) { size_t bound1, bound2; - const size_t field_len = strlen(field); - char *pos = NULL; - - /* Walk header lines: only match [field] at the start of a line so that - * substrings inside the request-target or another header's value cannot - * be mistaken for a real header. The first line is the request line and - * will not match any of the header names we look for. */ - for (size_t i = 0; i + field_len <= conn->request_length; ) { - if (strncasecmp(conn->request + i, field, field_len) == 0) { - pos = conn->request + i; - break; - } - /* Advance to the byte after the next '\n', or stop if none. */ - const char *nl = memchr(conn->request + i, '\n', - conn->request_length - i); - if (nl == NULL) - break; - i = (size_t)(nl - conn->request) + 1; - } + char *search, *pos; + + /* Prepend '\n' so strcasestr only matches [field] at the start of a line, + * preventing substrings inside the request-target or another header's + * value from being mistaken for a real header. */ + xasprintf(&search, "\n%s", field); + pos = strcasestr(conn->request, search); + free(search); if (pos == NULL) return NULL; - bound1 = (size_t)(pos - conn->request) + field_len; + pos++; /* skip the leading '\n' */ + bound1 = (size_t)(pos - conn->request) + strlen(field); /* find end */ for (bound2 = bound1;