From c1bd9385624a16497aabcafd6cb97274ed845c9e Mon Sep 17 00:00:00 2001 From: Niels Provos Date: Sat, 11 Apr 2009 04:12:46 +0000 Subject: [PATCH] Fix parsing of queries where the encoded queries contained \r, \n or + svn:r1155 --- ChangeLog | 1 + http.c | 89 ++++++++++++++++++++++++++++++++++----------- test/regress_http.c | 65 +++++++++++++++++++++++++++++++-- 3 files changed, 131 insertions(+), 24 deletions(-) diff --git a/ChangeLog b/ChangeLog index 07fa2446..08b1665b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -7,6 +7,7 @@ Changes in 1.4.10-stable: o const-ify some arguments to evdns functions. o off-by-one error in epoll_recalc; reported by Victor Goya o include Doxyfile in tar ball; from Jeff Garzik + o correctly parse queries with encoded \r, \n or + characters Changes in 1.4.9-stable: o event_add would not return error for some backends; from Dean McNamee diff --git a/http.c b/http.c index 0abb9a87..491e3c3e 100644 --- a/http.c +++ b/http.c @@ -211,6 +211,10 @@ static void evhttp_read_firstline(struct evhttp_connection *evcon, struct evhttp_request *req); static void evhttp_read_header(struct evhttp_connection *evcon, struct evhttp_request *req); +static int evhttp_add_header_internal(struct evkeyvalq *headers, + const char *key, const char *value); +static int evhttp_decode_uri_internal(const char *uri, size_t length, + char *ret, int always_decode_plus); void evhttp_read(int, short, void *); void evhttp_write(int, short, void *); @@ -1362,22 +1366,46 @@ evhttp_remove_header(struct evkeyvalq *headers, const char *key) return (0); } +static int +evhttp_header_is_valid_value(const char *value) +{ + const char *p = value; + + while ((p = strpbrk(p, "\r\n")) != NULL) { + /* we really expect only one new line */ + p += strspn(p, "\r\n"); + /* we expect a space or tab for continuation */ + if (*p != ' ' && *p != '\t') + return (0); + } + return (1); +} + int evhttp_add_header(struct evkeyvalq *headers, const char *key, const char *value) { - struct evkeyval *header = NULL; - event_debug(("%s: key: %s val: %s\n", __func__, key, value)); - if (strchr(value, '\r') != NULL || strchr(value, '\n') != NULL || - strchr(key, '\r') != NULL || strchr(key, '\n') != NULL) { + if (strchr(key, '\r') != NULL || strchr(key, '\n') != NULL) { /* drop illegal headers */ - event_debug(("%s: dropping illegal header\n", __func__)); + event_debug(("%s: dropping illegal header key\n", __func__)); + return (-1); + } + + if (!evhttp_header_is_valid_value(value)) { + event_debug(("%s: dropping illegal header value\n", __func__)); return (-1); } - header = calloc(1, sizeof(struct evkeyval)); + return (evhttp_add_header_internal(headers, key, value)); +} + +static int +evhttp_add_header_internal(struct evkeyvalq *headers, + const char *key, const char *value) +{ + struct evkeyval *header = calloc(1, sizeof(struct evkeyval)); if (header == NULL) { event_warn("%s: calloc", __func__); return (-1); @@ -2039,16 +2067,16 @@ evhttp_encode_uri(const char *uri) return (p); } -char * -evhttp_decode_uri(const char *uri) +/* + * @param always_decode_plus: when true we transform plus to space even + * if we have not seen a ?. + */ +static int +evhttp_decode_uri_internal( + const char *uri, size_t length, char *ret, int always_decode_plus) { - char c, *ret; - int i, j, in_query = 0; - - ret = malloc(strlen(uri) + 1); - if (ret == NULL) - event_err(1, "%s: malloc(%lu)", __func__, - (unsigned long)(strlen(uri) + 1)); + char c; + int i, j, in_query = always_decode_plus; for (i = j = 0; uri[i] != '\0'; i++) { c = uri[i]; @@ -2065,7 +2093,22 @@ evhttp_decode_uri(const char *uri) ret[j++] = c; } ret[j] = '\0'; - + + return (j); +} + +char * +evhttp_decode_uri(const char *uri) +{ + char *ret; + + if ((ret = malloc(strlen(uri) + 1)) == NULL) + event_err(1, "%s: malloc(%lu)", __func__, + (unsigned long)(strlen(uri) + 1)); + + evhttp_decode_uri_internal(uri, strlen(uri), + ret, 0 /*always_decode_plus*/); + return (ret); } @@ -2099,7 +2142,7 @@ evhttp_parse_query(const char *uri, struct evkeyvalq *headers) p = argument; while (p != NULL && *p != '\0') { - char *key, *value; + char *key, *value, *decoded_value; argument = strsep(&p, "&"); value = argument; @@ -2107,10 +2150,14 @@ evhttp_parse_query(const char *uri, struct evkeyvalq *headers) if (value == NULL) goto error; - value = evhttp_decode_uri(value); - event_debug(("Query Param: %s -> %s\n", key, value)); - evhttp_add_header(headers, key, value); - free(value); + if ((decoded_value = malloc(strlen(value) + 1)) == NULL) + event_err(1, "%s: malloc", __func__); + + evhttp_decode_uri_internal(value, strlen(value), + decoded_value, 1 /*always_decode_plus*/); + event_debug(("Query Param: %s -> %s\n", key, decoded_value)); + evhttp_add_header_internal(headers, key, decoded_value); + free(decoded_value); } error: diff --git a/test/regress_http.c b/test/regress_http.c index 9091f89a..1e2a1eb0 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -995,13 +995,16 @@ http_bad_header_test(void) if (evhttp_add_header(&headers, "One\r", "Two") != -1) goto fail; - + if (evhttp_add_header(&headers, "One", "Two") != 0) + goto fail; + if (evhttp_add_header(&headers, "One", "Two\r\n Three") != 0) + goto fail; + if (evhttp_add_header(&headers, "One\r", "Two") != -1) + goto fail; if (evhttp_add_header(&headers, "One\n", "Two") != -1) goto fail; - if (evhttp_add_header(&headers, "One", "Two\r") != -1) goto fail; - if (evhttp_add_header(&headers, "One", "Two\n") != -1) goto fail; @@ -1014,6 +1017,61 @@ fail: exit(1); } +static int validate_header( + const struct evkeyvalq* headers, + const char *key, const char *value) +{ + const char *real_val = evhttp_find_header(headers, key); + if (real_val == NULL) + return (-1); + if (strcmp(real_val, value) != 0) + return (-1); + return (0); +} + +static void +http_parse_query_test(void) +{ + struct evkeyvalq headers; + + fprintf(stdout, "Testing HTTP query parsing: "); + + TAILQ_INIT(&headers); + + evhttp_parse_query("http://www.test.com/?q=test", &headers); + if (validate_header(&headers, "q", "test") != 0) + goto fail; + evhttp_clear_headers(&headers); + + evhttp_parse_query("http://www.test.com/?q=test&foo=bar", &headers); + if (validate_header(&headers, "q", "test") != 0) + goto fail; + if (validate_header(&headers, "foo", "bar") != 0) + goto fail; + evhttp_clear_headers(&headers); + + evhttp_parse_query("http://www.test.com/?q=test+foo", &headers); + if (validate_header(&headers, "q", "test foo") != 0) + goto fail; + evhttp_clear_headers(&headers); + + evhttp_parse_query("http://www.test.com/?q=test%0Afoo", &headers); + if (validate_header(&headers, "q", "test\nfoo") != 0) + goto fail; + evhttp_clear_headers(&headers); + + evhttp_parse_query("http://www.test.com/?q=test%0Dfoo", &headers); + if (validate_header(&headers, "q", "test\rfoo") != 0) + goto fail; + evhttp_clear_headers(&headers); + + fprintf(stdout, "OK\n"); + return; +fail: + fprintf(stdout, "FAILED\n"); + exit(1); +} + static void http_base_test(void) { @@ -1400,6 +1458,7 @@ http_suite(void) { http_base_test(); http_bad_header_test(); + http_parse_query_test(); http_basic_test(); http_connection_test(0 /* not-persistent */); http_connection_test(1 /* persistent */);