From 6ece9e52d918cefa1e99b5f359a22b111bdced75 Mon Sep 17 00:00:00 2001 From: Patrick Griffis Date: Wed, 12 Aug 2026 14:26:15 -0500 Subject: [PATCH] message-headers: Validate Content-Range when parsing it A client runs this parser on a header the server chose, so the offsets it returns need checking before they are handed back as offsets into the response body. The three fields were parsed with g_ascii_strtoull() straight into signed goffsets with no overflow check, so a value above G_MAXINT64 became a negative offset while the function still reported success. Only the first field was guarded against a leading '-', which g_ascii_strtoull() wraps round into a large unsigned value, so the end and the total length could go negative that way too. Nothing checked that the end was not before the start, or that it fell within the total length. Parse each field through a helper which requires a plain run of digits fitting in a goffset, and reject a range which does not describe a real span of the resource. The out parameters are now only written on success, rather than being left holding a partial parse. --- libsoup/soup-message-headers.c | 58 +++++++++++++++++---- tests/range-test.c | 93 ++++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 10 deletions(-) diff --git a/libsoup/soup-message-headers.c b/libsoup/soup-message-headers.c index 7e2e6583..c8f2a598 100644 --- a/libsoup/soup-message-headers.c +++ b/libsoup/soup-message-headers.c @@ -1548,6 +1548,30 @@ soup_message_headers_set_range (SoupMessageHeaders *hdrs, soup_message_headers_set_ranges (hdrs, &range, 1); } +/* Parses one decimal offset from a Content-Range header, rejecting anything + * which isn't a plain run of digits fitting in a goffset. In particular a + * leading '-' must not be accepted: g_ascii_strtoull() would happily wrap it + * round into a large unsigned value. + */ +static gboolean +parse_content_range_offset (const char *in, + char **out_end, + goffset *out) +{ + guint64 value; + + if (!g_ascii_isdigit (*in)) + return FALSE; + + errno = 0; + value = g_ascii_strtoull (in, out_end, 10); + if (errno == ERANGE || value > G_MAXINT64) + return FALSE; + + *out = (goffset) value; + return TRUE; +} + /** * soup_message_headers_get_content_range: * @hdrs: a #SoupMessageHeaders @@ -1560,6 +1584,11 @@ soup_message_headers_set_range (SoupMessageHeaders *hdrs, * @end, and @total_length. If the total length field in the header * was specified as "*", then @total_length will be set to -1. * + * On success @start and @end are always non-negative, @end is not before + * @start, and @end is within @total_length if that was given, so they can be + * used directly as offsets into the response body. The out parameters are + * left untouched if the header cannot be parsed. + * * Returns: %TRUE if @hdrs contained a "Content-Range" header * containing a byte range which could be parsed, %FALSE otherwise. **/ @@ -1570,7 +1599,7 @@ soup_message_headers_get_content_range (SoupMessageHeaders *hdrs, goffset *total_length) { const char *header; - goffset length; + goffset first_pos, last_pos, length; char *p; g_return_val_if_fail (hdrs, FALSE); @@ -1583,25 +1612,34 @@ soup_message_headers_get_content_range (SoupMessageHeaders *hdrs, header += 6; while (g_ascii_isspace (*header)) header++; - if (!g_ascii_isdigit (*header)) - return FALSE; - *start = g_ascii_strtoull (header, &p, 10); - if (*p != '-') + if (!parse_content_range_offset (header, &p, &first_pos) || *p != '-') return FALSE; - *end = g_ascii_strtoull (p + 1, &p, 10); - if (*p != '/') + if (!parse_content_range_offset (p + 1, &p, &last_pos) || *p != '/') return FALSE; p++; if (*p == '*') { length = -1; p++; - } else - length = g_ascii_strtoull (p, &p, 10); + } else if (!parse_content_range_offset (p, &p, &length)) + return FALSE; + + if (*p != '\0') + return FALSE; + /* The range has to describe an actual span of the resource, otherwise + * it is not something a caller can use as offsets. + */ + if (last_pos < first_pos) + return FALSE; + if (length >= 0 && last_pos >= length) + return FALSE; + + *start = first_pos; + *end = last_pos; if (total_length) *total_length = length; - return *p == '\0'; + return TRUE; } /** diff --git a/tests/range-test.c b/tests/range-test.c index 9e4ed2c9..fe8cdfc5 100644 --- a/tests/range-test.c +++ b/tests/range-test.c @@ -746,6 +746,98 @@ do_range_count_test (void) } } +/* Tests for the Content-Range parser, which a client runs on a header chosen + * by the server. A successful parse must yield offsets the caller can safely + * use against a buffer of total_length bytes. + */ +typedef struct { + const char *description; + const char *content_range; + gboolean expected_result; + goffset expected_start, expected_end, expected_total_length; +} ContentRangeParsingTest; + +static const ContentRangeParsingTest content_range_parsing_tests[] = { + /* Valid. */ + { "simple range", "bytes 0-9/10", TRUE, 0, 9, 10 }, + { "partial range", "bytes 5-9/100", TRUE, 5, 9, 100 }, + { "single byte", "bytes 0-0/1", TRUE, 0, 0, 1 }, + { "final byte", "bytes 99-99/100", TRUE, 99, 99, 100 }, + { "unknown total length", "bytes 0-9/*", TRUE, 0, 9, -1 }, + { "extra space after the unit", "bytes 0-9/10", TRUE, 0, 9, 10 }, + { "large but representable", "bytes 0-9223372036854775805/9223372036854775806", + TRUE, 0, 9223372036854775805, 9223372036854775806 }, + + /* Malformed. */ + { "no unit", "0-9/10", FALSE, 0, 0, 0 }, + { "unknown unit", "horses 0-9/10", FALSE, 0, 0, 0 }, + { "no unit separator", "bytes", FALSE, 0, 0, 0 }, + { "missing total length", "bytes 0-9", FALSE, 0, 0, 0 }, + { "missing dash", "bytes 09/10", FALSE, 0, 0, 0 }, + { "missing slash", "bytes 0-9 10", FALSE, 0, 0, 0 }, + { "trailing garbage", "bytes 0-9/10 but with more content", FALSE, 0, 0, 0 }, + { "space before the end", "bytes 0- 9/10", FALSE, 0, 0, 0 }, + { "space before the total length", "bytes 0-9/ 10", FALSE, 0, 0, 0 }, + + /* Values a malicious server can use to drive the parsed offsets + * negative, or to make them inconsistent with each other. + */ + { "start overflowing gint64", "bytes 9223372036854775808-9223372036854775809/9223372036854775810", + FALSE, 0, 0, 0 }, + { "end overflowing gint64", "bytes 0-9223372036854775808/10", FALSE, 0, 0, 0 }, + { "total length overflowing gint64", "bytes 0-9/9223372036854775808", FALSE, 0, 0, 0 }, + { "all fields G_MAXUINT64", "bytes 18446744073709551615-18446744073709551615/18446744073709551615", + FALSE, 0, 0, 0 }, + { "all fields overflowing guint64", "bytes 99999999999999999999-99999999999999999999/99999999999999999999", + FALSE, 0, 0, 0 }, + { "negative start", "bytes -5-9/10", FALSE, 0, 0, 0 }, + { "negative end", "bytes 0--5/10", FALSE, 0, 0, 0 }, + { "negative total length", "bytes 0-9/-10", FALSE, 0, 0, 0 }, + { "end before start", "bytes 10-5/100", FALSE, 0, 0, 0 }, + { "end at the total length", "bytes 0-10/10", FALSE, 0, 0, 0 }, + { "end past the total length", "bytes 0-100/10", FALSE, 0, 0, 0 }, + { "start past the total length", "bytes 50-60/10", FALSE, 0, 0, 0 }, +}; + +static void +do_content_range_parsing_test (void) +{ + guint i; + + for (i = 0; i < G_N_ELEMENTS (content_range_parsing_tests); i++) { + const ContentRangeParsingTest *test = &content_range_parsing_tests[i]; + SoupMessageHeaders *hdrs; + goffset start = -1, end = -1, total_length = -1; + gboolean result; + + debug_printf (1, "%2u. %s: '%s'\n", i + 1, test->description, + test->content_range); + + hdrs = soup_message_headers_new (SOUP_MESSAGE_HEADERS_RESPONSE); + soup_message_headers_replace (hdrs, "Content-Range", test->content_range); + + result = soup_message_headers_get_content_range (hdrs, &start, &end, + &total_length); + g_assert_cmpint (result, ==, test->expected_result); + + if (result) { + g_assert_cmpint (start, ==, test->expected_start); + g_assert_cmpint (end, ==, test->expected_end); + g_assert_cmpint (total_length, ==, test->expected_total_length); + + /* Whatever the server sent, these must be usable as + * offsets into a buffer of total_length bytes. + */ + g_assert_cmpint (start, >=, 0); + g_assert_cmpint (end, >=, start); + if (total_length >= 0) + g_assert_cmpint (end, <, total_length); + } + + soup_message_headers_unref (hdrs); + } +} + #ifdef HAVE_APACHE static void do_apache_range_test (void) @@ -859,6 +951,7 @@ main (int argc, char **argv) g_test_add_func ("/ranges/libsoup", do_libsoup_range_test); g_test_add_func ("/ranges/parsing", do_range_parsing_test); g_test_add_func ("/ranges/count", do_range_count_test); + g_test_add_func ("/ranges/content-range", do_content_range_parsing_test); ret = g_test_run (); -- GitLab