Gives urlencoded_parse a destination bound

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();