From 5327605caa5863ec9593fd0899425cd971a9d525 Mon Sep 17 00:00:00 2001 From: Kevin Butler Date: Tue, 3 Sep 2013 01:14:43 +0100 Subject: [PATCH 1/2] Tougher validation for parsing urls from HTTP headers. Fixes #2767. --- changes/bug2767 | 2 ++ src/or/directory.c | 9 ++++++++- src/or/directory.h | 5 +++++ src/test/test_dir.c | 40 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 changes/bug2767 diff --git a/changes/bug2767 b/changes/bug2767 new file mode 100644 index 0000000000..974ce63267 --- /dev/null +++ b/changes/bug2767 @@ -0,0 +1,2 @@ + o Minor bugfixes: + - No longer accepting malformed http headers when parsing urls from headers, replies with Bad Request(400). Fixes #2767. \ No newline at end of file diff --git a/src/or/directory.c b/src/or/directory.c index 97305ae2a8..58ce0cf838 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -1387,7 +1387,7 @@ directory_send_command(dir_connection_t *conn, * so it does. Return 0. * Otherwise, return -1. */ -static int +STATIC int parse_http_url(const char *headers, char **url) { char *s, *start, *tmp; @@ -1416,6 +1416,13 @@ parse_http_url(const char *headers, char **url) } } + /* Check if the header is well formed (next sequence + * should be HTTP/1.X\r\n). Assumes we're supporting 1.0? */ + char *e = (char *)eat_whitespace_no_nl(s); + if (strcmpstart(e, "HTTP/1.") || !(*(e+8) == '\r')) { + return -1; + } + if (s-start < 5 || strcmpstart(start,"/tor/")) { /* need to rewrite it */ *url = tor_malloc(s - start + 5); strlcpy(*url,"/tor", s-start+5); diff --git a/src/or/directory.h b/src/or/directory.h index 41f18a1725..0453160f7a 100644 --- a/src/or/directory.h +++ b/src/or/directory.h @@ -118,5 +118,10 @@ download_status_mark_impossible(download_status_t *dl) int download_status_get_n_failures(const download_status_t *dls); +#ifdef TOR_UNIT_TESTS +/* Used only by directory.c and test_dir.c */ +STATIC int parse_http_url(const char *headers, char **url); +#endif + #endif diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 6c2915d094..05e13b5741 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -2364,6 +2364,45 @@ test_dir_fmt_control_ns(void *arg) tor_free(s); } +static void +test_dir_http_handling(void *args) +{ + (void)args; + char *url = NULL; + + /* Parse http url tests: */ + /* Good headers */ + test_eq(parse_http_url("GET /tor/a/b/c.txt HTTP/1.1\r\n" + "Host: example.com\r\n" + "User-Agent: Mozilla/5.0 (Windows;" + " U; Windows NT 6.1; en-US; rv:1.9.1.5)\r\n", + &url), 0); + test_streq(url, "/tor/a/b/c.txt"); + tor_free(url); + + /* Should prepends '/tor/' to url if required */ + test_eq(parse_http_url("GET /a/b/c.txt HTTP/1.1\r\n" + "Host: example.com\r\n" + "User-Agent: Mozilla/5.0 (Windows;" + " U; Windows NT 6.1; en-US; rv:1.9.1.5)\r\n", + &url), 0); + test_streq(url, "/tor/a/b/c.txt"); + tor_free(url); + + /* Bad headers */ + test_eq(parse_http_url("GET /a/b/c.txt\r\n" + "Host: example.com\r\n" + "User-Agent: Mozilla/5.0 (Windows;" + " U; Windows NT 6.1; en-US; rv:1.9.1.5)\r\n", + &url), -1); + tt_assert(!url); + + /* TODO: more http handling tests */ + + done: + ; +} + #define DIR_LEGACY(name) \ { #name, legacy_test_helper, TT_FORK, &legacy_setup, test_dir_ ## name } @@ -2386,6 +2425,7 @@ struct testcase_t dir_tests[] = { DIR_LEGACY(clip_unmeasured_bw_kb_alt), DIR(v2_dir, TT_FORK), DIR(fmt_control_ns, 0), + DIR(http_handling, 0), END_OF_TESTCASES }; From 270b4f030a07a0ff3ad80e4ae5495ad89b4096ef Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Tue, 3 Sep 2013 11:37:04 -0400 Subject: [PATCH 2/2] Handle HTTP minor versions greater than 9 (In practice they don't exist, but so long as we're making changes for standards compliance...) Also add several more unit tests for good and bad URL types. --- src/or/directory.c | 12 +++++++++--- src/test/test_dir.c | 37 +++++++++++++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/src/or/directory.c b/src/or/directory.c index 58ce0cf838..099df50d07 100644 --- a/src/or/directory.c +++ b/src/or/directory.c @@ -1417,10 +1417,16 @@ parse_http_url(const char *headers, char **url) } /* Check if the header is well formed (next sequence - * should be HTTP/1.X\r\n). Assumes we're supporting 1.0? */ + * should be HTTP/1.X\r\n). Assumes we're supporting 1.0? */ char *e = (char *)eat_whitespace_no_nl(s); - if (strcmpstart(e, "HTTP/1.") || !(*(e+8) == '\r')) { - return -1; + { + unsigned minor_ver; + char ch; + if (2 != tor_sscanf(e, "HTTP/1.%u%c", &minor_ver, &ch)) { + return -1; + } + if (ch != '\r') + return -1; } if (s-start < 5 || strcmpstart(start,"/tor/")) { /* need to rewrite it */ diff --git a/src/test/test_dir.c b/src/test/test_dir.c index 05e13b5741..0292cbd667 100644 --- a/src/test/test_dir.c +++ b/src/test/test_dir.c @@ -2380,7 +2380,15 @@ test_dir_http_handling(void *args) test_streq(url, "/tor/a/b/c.txt"); tor_free(url); - /* Should prepends '/tor/' to url if required */ + test_eq(parse_http_url("GET /tor/a/b/c.txt HTTP/1.0\r\n", &url), 0); + test_streq(url, "/tor/a/b/c.txt"); + tor_free(url); + + test_eq(parse_http_url("GET /tor/a/b/c.txt HTTP/1.600\r\n", &url), 0); + test_streq(url, "/tor/a/b/c.txt"); + tor_free(url); + + /* Should prepend '/tor/' to url if required */ test_eq(parse_http_url("GET /a/b/c.txt HTTP/1.1\r\n" "Host: example.com\r\n" "User-Agent: Mozilla/5.0 (Windows;" @@ -2389,6 +2397,14 @@ test_dir_http_handling(void *args) test_streq(url, "/tor/a/b/c.txt"); tor_free(url); + /* Bad headers -- no HTTP/1.x*/ + test_eq(parse_http_url("GET /a/b/c.txt\r\n" + "Host: example.com\r\n" + "User-Agent: Mozilla/5.0 (Windows;" + " U; Windows NT 6.1; en-US; rv:1.9.1.5)\r\n", + &url), -1); + tt_assert(!url); + /* Bad headers */ test_eq(parse_http_url("GET /a/b/c.txt\r\n" "Host: example.com\r\n" @@ -2397,10 +2413,23 @@ test_dir_http_handling(void *args) &url), -1); tt_assert(!url); - /* TODO: more http handling tests */ + test_eq(parse_http_url("GET /tor/a/b/c.txt", &url), -1); + tt_assert(!url); - done: - ; + test_eq(parse_http_url("GET /tor/a/b/c.txt HTTP/1.1", &url), -1); + tt_assert(!url); + + test_eq(parse_http_url("GET /tor/a/b/c.txt HTTP/1.1x\r\n", &url), -1); + tt_assert(!url); + + test_eq(parse_http_url("GET /tor/a/b/c.txt HTTP/1.", &url), -1); + tt_assert(!url); + + test_eq(parse_http_url("GET /tor/a/b/c.txt HTTP/1.\r", &url), -1); + tt_assert(!url); + + done: + tor_free(url); } #define DIR_LEGACY(name) \