From a94c4fb5bd9ef0f6b47128f9808c45f2259f9409 Mon Sep 17 00:00:00 2001 From: Michael Catanzaro Date: Wed, 7 Jan 2026 14:50:33 -0600 Subject: [PATCH] Reject duplicate Host headers RFC 9112 section 3.2 says: A server MUST respond with a 400 (Bad Request) status code to any HTTP/1.1 request message that lacks a Host header field and to any request message that contains more than one Host header field line or a Host header field with an invalid field value. In addition to rejecting a duplicate header when parsing headers, also reject attempts to add the duplicate header using the soup_message_headers_append() API, and add tests for both cases. These checks will also apply to HTTP/2. I'm not sure whether this is actually desired or not, but the header processing code is not aware of which HTTP version is in use. (Note that while SoupMessageHeaders does not require the Host header to be present in an HTTP/1.1 request, SoupServer itself does. So we can't test the case of missing Host header via the header parsing test, but it really is enforced.) Fixes #472 --- libsoup/soup-headers.c | 3 +- libsoup/soup-message-headers-private.h | 4 +- libsoup/soup-message-headers.c | 78 ++++++++------ tests/header-parsing-test.c | 136 ++++++++++++++++--------- 4 files changed, 138 insertions(+), 83 deletions(-) diff --git a/libsoup/soup-headers.c b/libsoup/soup-headers.c index 44ea45cb5..28a4c998b 100644 --- a/libsoup/soup-headers.c +++ b/libsoup/soup-headers.c @@ -139,7 +139,8 @@ soup_headers_parse (const char *str, int len, SoupMessageHeaders *dest) for (p = strchr (value, '\r'); p; p = strchr (p, '\r')) *p = ' '; - soup_message_headers_append_untrusted_data (dest, name, value); + if (!soup_message_headers_append_untrusted_data (dest, name, value)) + goto done; } success = TRUE; diff --git a/libsoup/soup-message-headers-private.h b/libsoup/soup-message-headers-private.h index 981546454..770f3ef17 100644 --- a/libsoup/soup-message-headers-private.h +++ b/libsoup/soup-message-headers-private.h @@ -10,10 +10,10 @@ G_BEGIN_DECLS -void soup_message_headers_append_untrusted_data (SoupMessageHeaders *hdrs, +gboolean soup_message_headers_append_untrusted_data (SoupMessageHeaders *hdrs, const char *name, const char *value); -void soup_message_headers_append_common (SoupMessageHeaders *hdrs, +gboolean soup_message_headers_append_common (SoupMessageHeaders *hdrs, SoupHeaderName name, const char *value); const char *soup_message_headers_get_one_common (SoupMessageHeaders *hdrs, diff --git a/libsoup/soup-message-headers.c b/libsoup/soup-message-headers.c index d4260a8e0..ca6a69f58 100644 --- a/libsoup/soup-message-headers.c +++ b/libsoup/soup-message-headers.c @@ -273,12 +273,16 @@ soup_message_headers_clean_connection_headers (SoupMessageHeaders *hdrs) soup_header_free_list (tokens); } -void +gboolean soup_message_headers_append_common (SoupMessageHeaders *hdrs, SoupHeaderName name, const char *value) { SoupCommonHeader header; + if (name == SOUP_HEADER_HOST && soup_message_headers_get_one (hdrs, "Host")) { + g_warning ("Attempted to add duplicate Host header to a SoupMessageHeaders that already contains a Host header"); + return FALSE; + } if (!hdrs->common_headers) hdrs->common_headers = g_array_sized_new (FALSE, FALSE, sizeof (SoupCommonHeader), 6); @@ -290,33 +294,19 @@ soup_message_headers_append_common (SoupMessageHeaders *hdrs, g_hash_table_remove (hdrs->common_concat, GUINT_TO_POINTER (header.name)); soup_message_headers_set (hdrs, name, value); + return TRUE; } -/** - * soup_message_headers_append: - * @hdrs: a #SoupMessageHeaders - * @name: the header name to add - * @value: the new value of @name - * - * Appends a new header with name @name and value @value to @hdrs. - * - * (If there is an existing header with name @name, then this creates a second - * one, which is only allowed for list-valued headers; see also - * [method@MessageHeaders.replace].) - * - * The caller is expected to make sure that @name and @value are - * syntactically correct. - **/ -void -soup_message_headers_append (SoupMessageHeaders *hdrs, - const char *name, const char *value) +static gboolean +soup_message_headers_append_internal (SoupMessageHeaders *hdrs, + const char *name, const char *value) { SoupUncommonHeader header; SoupHeaderName header_name; - g_return_if_fail (hdrs); - g_return_if_fail (name != NULL); - g_return_if_fail (value != NULL); + g_return_val_if_fail (hdrs, FALSE); + g_return_val_if_fail (name != NULL, FALSE); + g_return_val_if_fail (value != NULL, FALSE); /* Setting a syntactically invalid header name or value is * considered to be a programming error. However, it can also @@ -324,23 +314,22 @@ soup_message_headers_append (SoupMessageHeaders *hdrs, * compiled with G_DISABLE_CHECKS. */ #ifndef G_DISABLE_CHECKS - g_return_if_fail (*name && strpbrk (name, " \t\r\n:") == NULL); - g_return_if_fail (strpbrk (value, "\r\n") == NULL); + g_return_val_if_fail (*name && strpbrk (name, " \t\r\n:") == NULL, FALSE); + g_return_val_if_fail (strpbrk (value, "\r\n") == NULL, FALSE); #else if (*name && strpbrk (name, " \t\r\n:")) { g_warning ("soup_message_headers_append: Ignoring bad name '%s'", name); - return; + return FALSE; } if (strpbrk (value, "\r\n")) { g_warning ("soup_message_headers_append: Ignoring bad value '%s'", value); - return; + return FALSE; } #endif header_name = soup_header_name_from_string (name); if (header_name != SOUP_HEADER_UNKNOWN) { - soup_message_headers_append_common (hdrs, header_name, value); - return; + return soup_message_headers_append_common (hdrs, header_name, value); } if (!hdrs->uncommon_headers) @@ -351,21 +340,48 @@ soup_message_headers_append (SoupMessageHeaders *hdrs, g_array_append_val (hdrs->uncommon_headers, header); if (hdrs->uncommon_concat) g_hash_table_remove (hdrs->uncommon_concat, header.name); + return TRUE; +} + +/** + * soup_message_headers_append: + * @hdrs: a #SoupMessageHeaders + * @name: the header name to add + * @value: the new value of @name + * + * Appends a new header with name @name and value @value to @hdrs. + * + * (If there is an existing header with name @name, then this creates a second + * one, which is only allowed for list-valued headers; see also + * [method@MessageHeaders.replace].) + * + * The caller is expected to make sure that @name and @value are + * syntactically correct. + **/ +void +soup_message_headers_append (SoupMessageHeaders *hdrs, + const char *name, const char *value) +{ + soup_message_headers_append_internal (hdrs, name, value); } /* - * Appends a header value ensuring that it is valid UTF8. + * Appends a header value ensuring that it is valid UTF-8, and also checking the + * return value of soup_message_headers_append_internal() to report whether the + * headers are invalid for various other reasons. */ -void +gboolean soup_message_headers_append_untrusted_data (SoupMessageHeaders *hdrs, const char *name, const char *value) { char *safe_value = g_utf8_make_valid (value, -1); char *safe_name = g_utf8_make_valid (name, -1); - soup_message_headers_append (hdrs, safe_name, safe_value); + gboolean result = soup_message_headers_append_internal (hdrs, safe_name, safe_value); + g_free (safe_value); g_free (safe_name); + return result; } void diff --git a/tests/header-parsing-test.c b/tests/header-parsing-test.c index e6a17bffb..fc6dd1a2f 100644 --- a/tests/header-parsing-test.c +++ b/tests/header-parsing-test.c @@ -24,6 +24,7 @@ static struct RequestTest { const char *method, *path; SoupHTTPVersion version; Header headers[10]; + GLogLevelFlags log_flags; } reqtests[] = { /**********************/ /*** VALID REQUESTS ***/ @@ -33,7 +34,7 @@ static struct RequestTest { "GET / HTTP/1.0\r\n", -1, SOUP_STATUS_OK, "GET", "/", SOUP_HTTP_1_0, - { { NULL } } + { { NULL } }, 0 }, { "Req w/ 1 header", NULL, @@ -42,7 +43,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Host", "example.com" }, { NULL } - } + }, 0 }, { "Req w/ 1 header, no leading whitespace", NULL, @@ -51,7 +52,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Host", "example.com" }, { NULL } - } + }, 0 }, { "Req w/ 1 header including trailing whitespace", NULL, @@ -60,7 +61,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Host", "example.com" }, { NULL } - } + }, 0 }, { "Req w/ 1 header, wrapped", NULL, @@ -69,7 +70,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Foo", "bar baz" }, { NULL } - } + }, 0 }, { "Req w/ 1 header, wrapped with additional whitespace", NULL, @@ -78,7 +79,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Foo", "bar baz" }, { NULL } - } + }, 0 }, { "Req w/ 1 header, wrapped with tab", NULL, @@ -87,7 +88,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Foo", "bar baz" }, { NULL } - } + }, 0 }, { "Req w/ 1 header, wrapped before value", NULL, @@ -96,7 +97,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Foo", "bar baz" }, { NULL } - } + }, 0 }, { "Req w/ 1 header with empty value", NULL, @@ -105,7 +106,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Host", "" }, { NULL } - } + }, 0 }, { "Req w/ 2 headers", NULL, @@ -115,7 +116,7 @@ static struct RequestTest { { { "Host", "example.com" }, { "Connection", "close" }, { NULL } - } + }, 0 }, { "Req w/ 3 headers", NULL, @@ -126,7 +127,7 @@ static struct RequestTest { { "Connection", "close" }, { "Blah", "blah" }, { NULL } - } + }, 0 }, { "Req w/ 3 headers, 1st wrapped", NULL, @@ -137,7 +138,7 @@ static struct RequestTest { { "Foo", "bar baz" }, { "Blah", "blah" }, { NULL } - } + }, 0 }, { "Req w/ 3 headers, 2nd wrapped", NULL, @@ -148,7 +149,7 @@ static struct RequestTest { { "Blah", "blah" }, { "Foo", "bar baz" }, { NULL } - } + }, 0 }, { "Req w/ 3 headers, 3rd wrapped", NULL, @@ -159,7 +160,7 @@ static struct RequestTest { { "Blah", "blah" }, { "Foo", "bar baz" }, { NULL } - } + }, 0 }, { "Req w/ same header multiple times", NULL, @@ -168,7 +169,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Foo", "bar, baz, quux" }, { NULL } - } + }, 0 }, { "Connection header on HTTP/1.0 message", NULL, @@ -178,21 +179,21 @@ static struct RequestTest { { { "Connection", "Bar, Quux" }, { "Foo", "bar" }, { NULL } - } + }, 0 }, { "GET with full URI", "667637", "GET http://example.com HTTP/1.1\r\n", -1, SOUP_STATUS_OK, "GET", "http://example.com", SOUP_HTTP_1_1, - { { NULL } } + { { NULL } }, 0 }, { "GET with full URI in upper-case", "667637", "GET HTTP://example.com HTTP/1.1\r\n", -1, SOUP_STATUS_OK, "GET", "HTTP://example.com", SOUP_HTTP_1_1, - { { NULL } } + { { NULL } }, 0 }, /* It's better for this to be passed through: this means a SoupServer @@ -202,7 +203,7 @@ static struct RequestTest { "GET AbOuT: HTTP/1.1\r\n", -1, SOUP_STATUS_OK, "GET", "AbOuT:", SOUP_HTTP_1_1, - { { NULL } } + { { NULL } }, 0 }, /****************************/ @@ -217,7 +218,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Host", "example.com" }, { NULL } - } + }, 0 }, /* RFC 2616 section 3.1 says we MUST accept this */ @@ -228,7 +229,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Host", "example.com" }, { NULL } - } + }, 0 }, /* RFC 2616 section 19.3 says we SHOULD accept these */ @@ -240,7 +241,7 @@ static struct RequestTest { { { "Host", "example.com" }, { "Connection", "close" }, { NULL } - } + }, 0 }, { "LF instead of CRLF after Request-Line", NULL, @@ -249,7 +250,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Host", "example.com" }, { NULL } - } + }, 0 }, { "Mixed CRLF/LF", "666316", @@ -261,7 +262,7 @@ static struct RequestTest { { "e", "f" }, { "g", "h" }, { NULL } - } + }, 0 }, { "Req w/ incorrect whitespace in Request-Line", NULL, @@ -270,7 +271,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Host", "example.com" }, { NULL } - } + }, 0 }, { "Req w/ incorrect whitespace after Request-Line", "475169", @@ -279,7 +280,7 @@ static struct RequestTest { "GET", "/", SOUP_HTTP_1_1, { { "Host", "example.com" }, { NULL } - } + }, 0 }, /* If the request/status line is parseable, then we @@ -293,7 +294,7 @@ static struct RequestTest { { { "Host", "example.com" }, { "Bar", "two" }, { NULL } - } + }, 0 }, { "First header line is continuation", "666316", @@ -303,7 +304,7 @@ static struct RequestTest { { { "Host", "example.com" }, { "c", "d" }, { NULL } - } + }, 0 }, { "Zero-length header name", "666316", @@ -313,7 +314,7 @@ static struct RequestTest { { { "a", "b" }, { "c", "d" }, { NULL } - } + }, 0 }, { "CR in header name", "666316", @@ -323,7 +324,7 @@ static struct RequestTest { { { "a", "b" }, { "c", "d" }, { NULL } - } + }, 0 }, { "CR in header value", "666316", @@ -336,7 +337,7 @@ static struct RequestTest { { "s", "t" }, /* CR at end is ignored */ { "c", "d" }, { NULL } - } + }, 0 }, { "Tab in header name", "666316", @@ -351,7 +352,7 @@ static struct RequestTest { { "p", "q z: w" }, { "c", "d" }, { NULL } - } + }, 0 }, { "Tab in header value", "666316", @@ -364,7 +365,7 @@ static struct RequestTest { { "z", "w" }, /* trailing tab ignored */ { "c", "d" }, { NULL } - } + }, 0 }, /************************/ --- libsoup-3.6.4/tests/header-parsing-test.c.old +++ libsoup-3.6.4/tests/header-parsing-test.c @@ -366,70 +366,70 @@ "GET /\r\n", -1, SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "HTTP 1.2 request (no such thing)", NULL, "GET / HTTP/1.2\r\n", -1, SOUP_STATUS_HTTP_VERSION_NOT_SUPPORTED, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "HTTP 2000 request (no such thing)", NULL, "GET / HTTP/2000.0\r\n", -1, SOUP_STATUS_HTTP_VERSION_NOT_SUPPORTED, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "Non-HTTP request", NULL, "GET / SOUP/1.1\r\nHost: example.com\r\n", -1, SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "Junk after Request-Line", NULL, "GET / HTTP/1.1 blah\r\nHost: example.com\r\n", -1, SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "NUL in Method", NULL, "G\x00T / HTTP/1.1\r\nHost: example.com\r\n", 37, SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "NUL at beginning of Method", "666316", "\x00 / HTTP/1.1\r\nHost: example.com\r\n", 35, SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "NUL in Path", NULL, "GET /\x00 HTTP/1.1\r\nHost: example.com\r\n", 38, SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "No terminating CRLF", NULL, "GET / HTTP/1.1\r\nHost: example.com", -1, SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "Unrecognized expectation", NULL, "GET / HTTP/1.1\r\nHost: example.com\r\nExpect: the-impossible\r\n", -1, SOUP_STATUS_EXPECTATION_FAILED, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, // https://gitlab.gnome.org/GNOME/libsoup/-/issues/377 @@ -437,14 +437,14 @@ "GET / HTTP/1.1\r\nHost\x00: example.com\r\n", 36, SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 }, { "NUL in header value", NULL, "HTTP/1.1 200 OK\r\nFoo: b\x00" "ar\r\n", 28, SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, - { { NULL } } + { { NULL } }, 0 } }; static const int num_reqtests = G_N_ELEMENTS (reqtests); @@ -933,10 +944,17 @@ do_request_tests (void) len = strlen (reqtests[i].request); else len = reqtests[i].length; + + if (reqtests[i].log_flags) + g_test_expect_message ("libsoup", reqtests[i].log_flags, "*"); + status = soup_headers_parse_request (reqtests[i].request, len, headers, &method, &path, &version); g_assert_cmpint (status, ==, reqtests[i].status); + if (reqtests[i].log_flags) + g_test_assert_expected_messages (); + if (SOUP_STATUS_IS_SUCCESSFUL (status)) { g_assert_cmpstr (method, ==, reqtests[i].method); g_assert_cmpstr (path, ==, reqtests[i].path); @@ -1357,6 +1375,25 @@ do_case_sensitive_header_tests (void) } } +static void +do_append_duplicate_host_test (void) +{ + SoupMessageHeaders *hdrs; + const char *list_value; + + hdrs = soup_message_headers_new (SOUP_MESSAGE_HEADERS_REQUEST); + soup_message_headers_append (hdrs, "Host", "a"); + g_test_expect_message ("libsoup", G_LOG_LEVEL_WARNING, + "Attempted to add duplicate Host header to a SoupMessageHeaders that already contains a Host header"); + soup_message_headers_append (hdrs, "Host", "b"); + g_test_assert_expected_messages (); + + list_value = soup_message_headers_get_list (hdrs, "Host"); + g_assert_cmpstr (list_value, ==, "a"); + + soup_message_headers_unref (hdrs); +} + int main (int argc, char **argv) { --- a/tests/header-parsing-test.c +++ b/tests/header-parsing-test.c @@ -447,6 +447,16 @@ SOUP_STATUS_BAD_REQUEST, NULL, NULL, -1, { { NULL } }, 0 - } + }, + { "Duplicate Host headers", + "https://gitlab.gnome.org/GNOME/libsoup/-/issues/472", + "GET / HTTP/1.1\r\nHost: example.com\r\nHost: example.org\r\n", + -1, + SOUP_STATUS_BAD_REQUEST, + NULL, NULL, -1, + { { NULL } }, + G_LOG_LEVEL_WARNING + } + }; static const int num_reqtests = G_N_ELEMENTS (reqtests); @@ -1356,6 +1366,7 @@ g_test_add_func ("/header-parsing/content-type", do_content_type_tests); g_test_add_func ("/header-parsing/append-param", do_append_param_tests); g_test_add_func ("/header-parsing/bad", do_bad_header_tests); + g_test_add_func ("/header-parsing/append-duplicate-host", do_append_duplicate_host_test); ret = g_test_run ();