diff --git a/ChangeLog b/ChangeLog index e74c864e..65810395 100644 --- a/ChangeLog +++ b/ChangeLog @@ -145,6 +145,7 @@ Changes in current version: o New bind-to option to allow DNS clients to bind to an arbitrary port for outgoing requests. o evbuffers can now be "frozen" to prevent operations at one or both ends. o Bufferevents now notice external attempts to add data to an inbuf or remove it from an outbuf, and stop them. + o Fix parsing of queries where the encoded queries contained \r, \n or + Changes in 1.4.0: o allow \r or \n individually to separate HTTP headers instead of the standard "\r\n"; from Charles Kerr. diff --git a/http.c b/http.c index 18984368..4917cbdd 100644 --- a/http.c +++ b/http.c @@ -211,14 +211,16 @@ static void evhttp_request_dispatch(struct evhttp_connection* evcon); 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); + struct evhttp_request *req); +static int evhttp_add_header_internal(struct evkeyvalq *headers, + const char *key, const char *value); /* callbacks for bufferevent */ static void evhttp_read_cb(struct bufferevent *, void *); static void evhttp_write_cb(struct bufferevent *, void *); static void evhttp_error_cb(struct bufferevent *bufev, short what, void *arg); static int evhttp_decode_uri_internal(const char *uri, size_t length, - char *ret); + char *ret, int always_decode_plus); #ifndef _EVENT_HAVE_STRSEP /* strsep replacement for platforms that lack it. Only works if @@ -1372,22 +1374,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 vakye\n", __func__)); return (-1); } - header = mm_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 = mm_calloc(1, sizeof(struct evkeyval)); if (header == NULL) { event_warn("%s: calloc", __func__); return (-1); @@ -2112,11 +2138,16 @@ evhttp_encode_uri(const char *uri) return (p); } +/* + * @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) +evhttp_decode_uri_internal( + const char *uri, size_t length, char *ret, int always_decode_plus) { char c; - int i, j, in_query = 0; + int i, j, in_query = always_decode_plus; for (i = j = 0; i < length; i++) { c = uri[i]; @@ -2146,7 +2177,8 @@ evhttp_decode_uri(const char *uri) event_err(1, "%s: malloc(%lu)", __func__, (unsigned long)(strlen(uri) + 1)); - evhttp_decode_uri_internal(uri, strlen(uri), ret); + evhttp_decode_uri_internal(uri, strlen(uri), + ret, 1 /*always_decode_plus*/); return (ret); } @@ -2191,7 +2223,7 @@ evhttp_parse_query(const char *uri, struct evkeyvalq *headers) value = evhttp_decode_uri(value); event_debug(("Query Param: %s -> %s\n", key, value)); - evhttp_add_header(headers, key, value); + evhttp_add_header_internal(headers, key, value); mm_free(value); } @@ -2214,7 +2246,8 @@ evhttp_dispatch_callback(struct httpcbq *callbacks, struct evhttp_request *req) if ((translated = mm_malloc(offset + 1)) == NULL) return (NULL); - offset = evhttp_decode_uri_internal(req->uri, offset, translated); + offset = evhttp_decode_uri_internal(req->uri, offset, + translated, 0 /* always_decode_plus */); TAILQ_FOREACH(cb, callbacks, next) { int res = 0; diff --git a/test/regress_http.c b/test/regress_http.c index 1f6947e1..7cbba72a 100644 --- a/test/regress_http.c +++ b/test/regress_http.c @@ -1351,6 +1351,7 @@ http_bad_header_test(void *ptr) TAILQ_INIT(&headers); tt_want(evhttp_add_header(&headers, "One", "Two") == 0); + tt_want(evhttp_add_header(&headers, "One", "Two\r\n Three") == 0); tt_want(evhttp_add_header(&headers, "One\r", "Two") == -1); tt_want(evhttp_add_header(&headers, "One\n", "Two") == -1); tt_want(evhttp_add_header(&headers, "One", "Two\r") == -1); @@ -1359,6 +1360,46 @@ http_bad_header_test(void *ptr) evhttp_clear_headers(&headers); } +static int validate_header( + const struct evkeyvalq* headers, + const char *key, const char *value) +{ + const char *real_val = evhttp_find_header(headers, key); + tt_assert(real_val != NULL); + tt_want(strcmp(real_val, value) == 0); +end: + return (0); +} + +static void +http_parse_query_test(void *ptr) +{ + struct evkeyvalq headers; + + TAILQ_INIT(&headers); + + evhttp_parse_query("http://www.test.com/?q=test", &headers); + tt_want(validate_header(&headers, "q", "test") == 0); + evhttp_clear_headers(&headers); + + evhttp_parse_query("http://www.test.com/?q=test&foo=bar", &headers); + tt_want(validate_header(&headers, "q", "test") == 0); + tt_want(validate_header(&headers, "foo", "bar") == 0); + evhttp_clear_headers(&headers); + + evhttp_parse_query("http://www.test.com/?q=test+foo", &headers); + tt_want(validate_header(&headers, "q", "test foo") == 0); + evhttp_clear_headers(&headers); + + evhttp_parse_query("http://www.test.com/?q=test%0Afoo", &headers); + tt_want(validate_header(&headers, "q", "test\nfoo") == 0); + evhttp_clear_headers(&headers); + + evhttp_parse_query("http://www.test.com/?q=test%0Dfoo", &headers); + tt_want(validate_header(&headers, "q", "test\rfoo") == 0); + evhttp_clear_headers(&headers); +} + static void http_base_test(void) { @@ -2148,6 +2189,7 @@ struct testcase_t http_testcases[] = { { "primitives", http_primitives, 0, NULL, NULL }, HTTP_LEGACY(base), { "bad_headers", http_bad_header_test, 0, NULL, NULL }, + { "parse_query", http_parse_query_test, 0, NULL, NULL }, HTTP_LEGACY(basic), HTTP_LEGACY(cancel), HTTP_LEGACY(virtual_host),