Fix parsing of queries where the encoded queries contained \r, \n or +

svn:r1155

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

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 ||
+	if (strchr(key, '\r') != NULL || strchr(key, '\n') != NULL) {
-	    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;
+	char c;
-	int i, j, in_query = 0;
+	int i, j, in_query = always_decode_plus;
-	
-	ret = malloc(strlen(uri) + 1);
-	if (ret == NULL)
-		event_err(1, "%s: malloc(%lu)", __func__,
-			  (unsigned long)(strlen(uri) + 1));
 	
 	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);
+		if ((decoded_value = malloc(strlen(value) + 1)) == NULL)
-		event_debug(("Query Param: %s -> %s\n", key, value));
+			event_err(1, "%s: malloc", __func__);
-		evhttp_add_header(headers, key, value);
+
-		free(value);
+		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:

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 */);