From 92c27bbda9e809683537e5cc48adad0d5c390587 Mon Sep 17 00:00:00 2001 From: Dirk Engling Date: Fri, 6 Nov 2026 18:27:44 +0100 Subject: Gives urlencoded_parse a destination bound --- src/ot_http.c | 34 ++++++++++++++------------- src/ot_urlencoded.c | 12 ++++++---- src/ot_urlencoded.h | 4 ++-- tests/t_urlencoded.c | 66 ++++++++++++++++++++++++++++++++++------------------ 4 files changed, 72 insertions(+), 44 deletions(-) diff --git a/src/ot_http.c b/src/ot_http.c index 29f41a3..fc49333 100644 --- a/src/ot_http.c +++ b/src/ot_http.c @@ -313,7 +313,7 @@ static ssize_t http_handle_stats(const int64_t sock, struct ot_workstruct *ws, c #endif while (scanon) { - switch (urlencoded_match_keyword(keywords_main, &read_ptr, SCAN_SEARCHPATH_PARAM)) { + switch (urlencoded_match_keyword(keywords_main, &read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_PARAM)) { case -2: scanon = 0; break; /* TERMINATOR */ @@ -323,11 +323,11 @@ static ssize_t http_handle_stats(const int64_t sock, struct ot_workstruct *ws, c urlencoded_skip_value(&read_ptr); break; case 1: /* matched "mode" */ - if ((mode = urlencoded_match_keyword(keywords_mode, &read_ptr, SCAN_SEARCHPATH_VALUE)) <= 0) + if ((mode = urlencoded_match_keyword(keywords_mode, &read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_VALUE)) <= 0) HTTPERROR_400_PARAM; break; case 2: /* matched "format" */ - if ((format = urlencoded_match_keyword(keywords_format, &read_ptr, SCAN_SEARCHPATH_VALUE)) <= 0) + if ((format = urlencoded_match_keyword(keywords_format, &read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_VALUE)) <= 0) HTTPERROR_400_PARAM; break; case 3: @@ -505,7 +505,7 @@ static ssize_t http_handle_scrape(const int64_t sock, struct ot_workstruct *ws, } while (scanon) { - switch (urlencoded_match_keyword(keywords_scrape, &read_ptr, SCAN_SEARCHPATH_PARAM)) { + switch (urlencoded_match_keyword(keywords_scrape, &read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_PARAM)) { case -2: scanon = 0; break; /* TERMINATOR */ @@ -515,10 +515,12 @@ static ssize_t http_handle_scrape(const int64_t sock, struct ot_workstruct *ws, urlencoded_skip_value(&read_ptr); break; case 1: /* matched "info_hash" */ + { /* ignore this, when we have less than 20 bytes */ - if (urlencoded_parse(&read_ptr, (char *)(multiscrape_buf + numwant++), SCAN_SEARCHPATH_VALUE) != (ssize_t)sizeof(ot_hash)) + ot_hash *slot = multiscrape_buf + numwant++; + if (urlencoded_parse(&read_ptr, (char *)slot, (char *)(slot + 1), SCAN_SEARCHPATH_VALUE) != (ssize_t)sizeof(ot_hash)) HTTPERROR_400_PARAM; - break; + } break; } } @@ -622,7 +624,7 @@ static ssize_t http_handle_announce(const int64_t sock, struct ot_workstruct scanon = 1; while (scanon) { - switch (urlencoded_match_keyword(keywords_announce, &read_ptr, SCAN_SEARCHPATH_PARAM)) { + switch (urlencoded_match_keyword(keywords_announce, &read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_PARAM)) { case -2: scanon = 0; break; /* TERMINATOR */ @@ -632,14 +634,14 @@ static ssize_t http_handle_announce(const int64_t sock, struct ot_workstruct urlencoded_skip_value(&read_ptr); break; case 1: /* matched "port" */ - len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, SCAN_SEARCHPATH_VALUE); + len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_VALUE); if ((len <= 0) || scan_int(write_ptr, len, &tmp) || (tmp > 0xffff)) HTTPERROR_400_PARAM; port = htons((uint16_t)tmp); OT_SETPORT(&ws->peer, &port); break; case 2: /* matched "left" */ - if ((len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, SCAN_SEARCHPATH_VALUE)) <= 0) + if ((len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_VALUE)) <= 0) HTTPERROR_400_PARAM; if (scan_int(write_ptr, len, &tmp)) tmp = 0; @@ -647,7 +649,7 @@ static ssize_t http_handle_announce(const int64_t sock, struct ot_workstruct OT_PEERFLAG(&ws->peer) |= PEER_FLAG_SEEDING; break; case 3: /* matched "event" */ - switch (urlencoded_match_keyword(keywords_announce_event, &read_ptr, SCAN_SEARCHPATH_VALUE)) { + switch (urlencoded_match_keyword(keywords_announce_event, &read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_VALUE)) { case -1: HTTPERROR_400_PARAM; case 1: /* matched "completed" */ @@ -661,7 +663,7 @@ static ssize_t http_handle_announce(const int64_t sock, struct ot_workstruct } break; case 4: /* matched "numwant" */ - len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, SCAN_SEARCHPATH_VALUE); + len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_VALUE); if ((len <= 0) || scan_int(write_ptr, len, &tmp)) HTTPERROR_400_PARAM; if (tmp < 0) @@ -671,7 +673,7 @@ static ssize_t http_handle_announce(const int64_t sock, struct ot_workstruct numwant = (size_t)tmp; break; case 5: /* matched "compact" */ - len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, SCAN_SEARCHPATH_VALUE); + len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_VALUE); if ((len <= 0) || scan_int(write_ptr, len, &tmp)) HTTPERROR_400_PARAM; if (!tmp) @@ -681,7 +683,7 @@ static ssize_t http_handle_announce(const int64_t sock, struct ot_workstruct if (ws->hash) HTTPERROR_400_DOUBLEHASH; /* ignore this, when we have less than 20 bytes */ - if (urlencoded_parse(&read_ptr, write_ptr = read_ptr, SCAN_SEARCHPATH_VALUE) != 20) + if (urlencoded_parse(&read_ptr, write_ptr = read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_VALUE) != 20) HTTPERROR_400_PARAM; ws->hash = (ot_hash *)write_ptr; break; @@ -689,7 +691,7 @@ static ssize_t http_handle_announce(const int64_t sock, struct ot_workstruct case 7: /* matched "ip" */ { char *tmp_buf1 = ws->reply, *tmp_buf2 = ws->reply + 16; - len = urlencoded_parse(&read_ptr, tmp_buf2, SCAN_SEARCHPATH_VALUE); + len = urlencoded_parse(&read_ptr, tmp_buf2, ws->outbuf + G_OUTBUF_SIZE, SCAN_SEARCHPATH_VALUE); tmp_buf2[len] = 0; if ((len <= 0) || ip_parse(tmp_buf2, tmp_buf1) != (size_t)len) HTTPERROR_400_PARAM; @@ -698,7 +700,7 @@ static ssize_t http_handle_announce(const int64_t sock, struct ot_workstruct #endif case 8: /* matched "peer_id" */ /* ignore this, when we have less than 20 bytes */ - if (urlencoded_parse(&read_ptr, write_ptr = read_ptr, SCAN_SEARCHPATH_VALUE) != 20) + if (urlencoded_parse(&read_ptr, write_ptr = read_ptr, ws->request + ws->request_size, SCAN_SEARCHPATH_VALUE) != 20) HTTPERROR_400_PARAM; ws->peer_id = write_ptr; break; @@ -772,7 +774,7 @@ ssize_t http_handle_request(const int64_t sock, struct ot_workstruct *ws) { In reality we abandoned requiring the url to be correct. This now only decodes url encoded characters, we check for announces and scrapes by looking for "a*" or "sc" */ - len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, SCAN_PATH); + len = urlencoded_parse(&read_ptr, write_ptr = read_ptr, ws->request + ws->request_size, SCAN_PATH); /* If parsing returned an error, leave with not found */ if (g_redirecturl && (len == -2)) diff --git a/src/ot_urlencoded.c b/src/ot_urlencoded.c index 4109bb5..c977adb 100644 --- a/src/ot_urlencoded.c +++ b/src/ot_urlencoded.c @@ -68,17 +68,18 @@ void urlencoded_skip_value(char **string) { *string = (char *)s; } -int urlencoded_match_keyword(const ot_keywords *keywords, char **string, SCAN_SEARCHPATH_FLAG flags) { +int urlencoded_match_keyword(const ot_keywords *keywords, char **string, const char *deststring_end, SCAN_SEARCHPATH_FLAG flags) { char *deststring = *string; - ssize_t match_length = urlencoded_parse(string, deststring, flags); + ssize_t match_length = urlencoded_parse(string, deststring, deststring_end, flags); if (match_length < 0) return match_length; if (match_length == 0) return -3; + /* Compare on the exact decoded length. */ while (keywords->key) { - if (!strncmp(keywords->key, deststring, match_length) && !keywords->key[match_length]) + if ((size_t)match_length == strlen(keywords->key) && !memcmp(keywords->key, deststring, (size_t)match_length)) return keywords->value; keywords++; } @@ -86,7 +87,7 @@ int urlencoded_match_keyword(const ot_keywords *keywords, char **string, SCAN_SE return -3; } -ssize_t urlencoded_parse(char **string, char *deststring, SCAN_SEARCHPATH_FLAG flags) { +ssize_t urlencoded_parse(char **string, char *deststring, const char *deststring_end, SCAN_SEARCHPATH_FLAG flags) { const unsigned char *s = *(const unsigned char **)string; unsigned char *d = (unsigned char *)deststring; unsigned char b, c; @@ -106,6 +107,9 @@ ssize_t urlencoded_parse(char **string, char *deststring, SCAN_SEARCHPATH_FLAG f c |= (b << 4); } + if ((const char *)d >= deststring_end) + return -1; + /* Write (possibly decoded) character to output */ *d++ = c; } diff --git a/src/ot_urlencoded.h b/src/ot_urlencoded.h index 9882468..053e079 100644 --- a/src/ot_urlencoded.h +++ b/src/ot_urlencoded.h @@ -28,7 +28,7 @@ typedef enum { or -1 for parse error or -2 for terminator found */ -ssize_t urlencoded_parse(char **string, char *deststring, SCAN_SEARCHPATH_FLAG flags); +ssize_t urlencoded_parse(char **string, char *deststring, const char *deststring_end, SCAN_SEARCHPATH_FLAG flags); /* string in: pointer to source out: pointer to next scan position @@ -38,7 +38,7 @@ ssize_t urlencoded_parse(char **string, char *deststring, SCAN_SEARCHPATH_FLAG f or -2 for terminator found or -3 for no keyword matched */ -int urlencoded_match_keyword(const ot_keywords *keywords, char **string, SCAN_SEARCHPATH_FLAG flags); +int urlencoded_match_keyword(const ot_keywords *keywords, char **string, const char *deststring_end, SCAN_SEARCHPATH_FLAG flags); /* string in: pointer to value of a param=value pair to skip out: pointer to next scan position on return diff --git a/tests/t_urlencoded.c b/tests/t_urlencoded.c index 3b3e6c2..9935c1c 100644 --- a/tests/t_urlencoded.c +++ b/tests/t_urlencoded.c @@ -21,7 +21,7 @@ static void case_path(void) { ssize_t n; test_case("path stops at the question mark"); - n = urlencoded_parse(&p, dest, SCAN_PATH); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_PATH); CHECK_INT(n, 9); dest[n] = 0; CHECK_STR(dest, "/announce"); @@ -35,22 +35,22 @@ static void case_param_and_value(void) { ssize_t n; test_case("parameter stops at '=', value stops at '&'"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_PARAM); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_PARAM); CHECK_INT(n, 9); dest[n] = 0; CHECK_STR(dest, "info_hash"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, 3); dest[n] = 0; CHECK_STR(dest, "abc"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_PARAM); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_PARAM); CHECK_INT(n, 4); dest[n] = 0; CHECK_STR(dest, "port"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, 2); dest[n] = 0; CHECK_STR(dest, "80"); @@ -63,7 +63,7 @@ static void case_percent_decoding(void) { ssize_t n; test_case("percent escapes decode"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, 3); CHECK_MEM(dest, "ABC", 3); } @@ -77,7 +77,7 @@ static void case_binary_info_hash(void) { ssize_t n; test_case("twenty byte binary value survives decoding"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, 20); CHECK_MEM(dest, want, 20); } @@ -88,7 +88,7 @@ static void case_in_place(void) { ssize_t n; test_case("decoding in place over the source is safe"); - n = urlencoded_parse(&p, query, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, query, query + sizeof(query), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, 2); CHECK_MEM(query, "AB", 2); } @@ -100,7 +100,7 @@ static void case_bad_escape(void) { ssize_t n; test_case("a non hexadecimal escape is a parse error"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, -1); } @@ -111,7 +111,7 @@ static void case_truncated_escape(void) { ssize_t n; test_case("an escape cut short by the terminator is a parse error"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, -1); } @@ -123,11 +123,11 @@ static void case_terminator(void) { ssize_t n; test_case("a hard terminator before any output reports end of input"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_PARAM); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_PARAM); CHECK_INT(n, -2); p = space; - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_PARAM); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_PARAM); CHECK_INT(n, -2); } @@ -138,10 +138,10 @@ static void case_terminator_after_output(void) { ssize_t n; test_case("a hard terminator after output is left for the next call"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, 3); CHECK_INT(*p, 0); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, -2); } @@ -157,14 +157,14 @@ static void case_separators_are_contextual(void) { ssize_t n; test_case("a question mark is literal inside a value"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, 3); dest[n] = 0; CHECK_STR(dest, "a?b"); test_case("an equals sign is literal inside a value"); p = equals; - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, 3); dest[n] = 0; CHECK_STR(dest, "a=b"); @@ -177,7 +177,7 @@ static void case_ampersand_in_path(void) { ssize_t n; test_case("an ampersand in a path is a parse error"); - n = urlencoded_parse(&p, dest, SCAN_PATH); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_PATH); CHECK_INT(n, -1); } @@ -188,7 +188,7 @@ static void case_raw_high_byte(void) { ssize_t n; test_case("an undecoded byte above ASCII is a parse error"); - n = urlencoded_parse(&p, dest, SCAN_SEARCHPATH_VALUE); + n = urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE); CHECK_INT(n, -1); } @@ -215,15 +215,15 @@ static void case_find_keywords(void) { char *p = match; test_case("a known keyword returns its value"); - CHECK_INT(urlencoded_match_keyword(keywords, &p, SCAN_SEARCHPATH_PARAM), 2); + CHECK_INT(urlencoded_match_keyword(keywords, &p, match + sizeof(match), SCAN_SEARCHPATH_PARAM), 2); test_case("an unknown keyword returns -3"); p = nomatch; - CHECK_INT(urlencoded_match_keyword(keywords, &p, SCAN_SEARCHPATH_PARAM), -3); + CHECK_INT(urlencoded_match_keyword(keywords, &p, nomatch + sizeof(nomatch), SCAN_SEARCHPATH_PARAM), -3); test_case("a prefix of a keyword does not match it"); p = prefix; - CHECK_INT(urlencoded_match_keyword(keywords, &p, SCAN_SEARCHPATH_PARAM), -3); + CHECK_INT(urlencoded_match_keyword(keywords, &p, prefix + sizeof(prefix), SCAN_SEARCHPATH_PARAM), -3); } /* The encoding direction. Which bytes survive unescaped is fixed by what @@ -313,10 +313,30 @@ static void case_fmt_decoder_disagrees_on_dollar(void) { dest[n] = 0; CHECK_STR(dest, "a$b"); - got = urlencoded_parse(&p, scanned, SCAN_SEARCHPATH_VALUE); + got = urlencoded_parse(&p, scanned, scanned + sizeof(scanned), SCAN_SEARCHPATH_VALUE); CHECK_INT(got, -1); } +static void case_value_stops_at_dest_end(void) { + /* A value that decodes to more than the destination holds is a parse error, + not a write past the buffer. (ip= used to overflow ws->outbuf this way.) */ + char query[] = "AAAAAAAAAA&x=1"; /* ten A's then a terminator */ + char dest[4]; + char *p = query; + test_case("a value longer than the destination returns -1"); + CHECK_INT((int)urlencoded_parse(&p, dest, dest + sizeof(dest), SCAN_SEARCHPATH_VALUE), -1); +} + +static void case_keyword_with_decoded_nul_does_not_match(void) { + /* A percent-encoded NUL used to make a value match a shorter keyword and + then index the keyword past its end. */ + static const ot_keywords keywords[] = {{"port", 1}, {NULL, -3}}; + char query[] = "port%00xxxx=1"; + char *p = query; + test_case("a keyword followed by a decoded NUL does not match the keyword"); + CHECK_INT(urlencoded_match_keyword(keywords, &p, query + sizeof(query), SCAN_SEARCHPATH_PARAM), -3); +} + void test_urlencoded(void) { case_path(); case_param_and_value(); @@ -332,6 +352,8 @@ void test_urlencoded(void) { case_raw_high_byte(); case_skipvalue(); case_find_keywords(); + case_value_stops_at_dest_end(); + case_keyword_with_decoded_nul_does_not_match(); case_fmt_passes_safe_bytes(); case_fmt_escapes_the_rest(); case_fmt_hex_is_lower_case(); -- cgit v1.3