From 8876cda09bfa8f624b331488e2cbb68037ff5fc0 Mon Sep 17 00:00:00 2001 From: Chinmay Chhajed Date: Fri, 6 Sep 2019 11:02:36 +0800 Subject: [PATCH] esp_http_server : Bugfix in parsing of empty header values This MR is intended to fix incorrect parsing of HTTP requests when empty header values are present. The issue is was due to asymmetric behavior of `http_parser` library, which in case of: non-empty header values : invokes callbacks with the pointer to the start of a value empty header values : invokes callbacks with pointer to the start of next header or section Since HTTP server relies on this pointer (along with length of the value) to locate the end of a value, and replace the line terminators (CRLFs) with null characters, the second case needed to be handled correctly. Closes IDFGH-1539 Closes https://github.com/espressif/esp-idf/issues/3803 --- components/esp_http_server/src/httpd_parse.c | 17 ++++ .../http_server_advanced_test.py | 2 + .../http_server/advanced_tests/main/tests.c | 97 +++++++++++++++++++ .../advanced_tests/scripts/test.py | 44 ++++++++- 4 files changed, 159 insertions(+), 1 deletion(-) diff --git a/components/esp_http_server/src/httpd_parse.c b/components/esp_http_server/src/httpd_parse.c index 765510f0c..ec73bbb5f 100644 --- a/components/esp_http_server/src/httpd_parse.c +++ b/components/esp_http_server/src/httpd_parse.c @@ -267,6 +267,23 @@ static esp_err_t cb_header_value(http_parser *parser, const char *at, size_t len parser_data->last.at = at; parser_data->last.length = 0; parser_data->status = PARSING_HDR_VALUE; + + if (length == 0) { + /* As per behavior of http_parser, when length > 0, + * `at` points to the start of CRLF. But, in the + * case when header value is empty (zero length), + * then `at` points to the position right after + * the CRLF. Since for our purpose we need `last.at` + * to point to exactly where the CRLF starts, it + * needs to be adjusted by the right offset */ + char *at_adj = (char *)parser_data->last.at; + /* Find the end of header field string */ + while (*(--at_adj) != ':'); + /* Now skip leading spaces' */ + while (*(++at_adj) == ' '); + /* Now we are at the right position */ + parser_data->last.at = at_adj; + } } else if (parser_data->status != PARSING_HDR_VALUE) { ESP_LOGE(TAG, LOG_FMT("unexpected state transition")); parser_data->error = HTTPD_500_INTERNAL_SERVER_ERROR; diff --git a/examples/protocols/http_server/advanced_tests/http_server_advanced_test.py b/examples/protocols/http_server/advanced_tests/http_server_advanced_test.py index a27ad5ee0..f3e7033d7 100644 --- a/examples/protocols/http_server/advanced_tests/http_server_advanced_test.py +++ b/examples/protocols/http_server/advanced_tests/http_server_advanced_test.py @@ -133,6 +133,8 @@ def test_examples_protocol_http_server_advanced(env, extra_data): failed = True if not client.get_false_uri(got_ip, got_port): failed = True + if not client.get_test_headers(got_ip, got_port): + failed = True Utility.console_log("Error code tests...") if not client.code_500_server_error_test(got_ip, got_port): diff --git a/examples/protocols/http_server/advanced_tests/main/tests.c b/examples/protocols/http_server/advanced_tests/main/tests.c index c41810e61..a7a5df6a4 100644 --- a/examples/protocols/http_server/advanced_tests/main/tests.c +++ b/examples/protocols/http_server/advanced_tests/main/tests.c @@ -27,6 +27,96 @@ static esp_err_t hello_get_handler(httpd_req_t *req) #undef STR } +/* This handler is intended to check what happens in case of empty values of headers. + * Here `Header2` is an empty header and `Header1` and `Header3` will have `Value1` + * and `Value3` in them. */ +static esp_err_t test_header_get_handler(httpd_req_t *req) +{ + httpd_resp_set_type(req, HTTPD_TYPE_TEXT); + int buf_len; + char *buf; + + buf_len = httpd_req_get_hdr_value_len(req, "Header1"); + if (buf_len > 0) { + buf = malloc(++buf_len); + if (!buf) { + ESP_LOGE(TAG, "Failed to allocate memory of %d bytes!", buf_len); + httpd_resp_send_err(req, HTTPD_500_INTERNAL_SERVER_ERROR, "Memory allocation failed"); + return ESP_ERR_NO_MEM; + } + /* Copy null terminated value string into buffer */ + if (httpd_req_get_hdr_value_str(req, "Header1", buf, buf_len) == ESP_OK) { + ESP_LOGI(TAG, "Header1 content: %s", buf); + if (strcmp("Value1", buf) != 0) { + httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Wrong value of Header1 received"); + free(buf); + return ESP_ERR_INVALID_ARG; + } else { + ESP_LOGI(TAG, "Expected value and received value matched for Header1"); + } + } else { + ESP_LOGE(TAG, "Error in getting value of Header1"); + httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Error in getting value of Header1"); + free(buf); + return ESP_FAIL; + } + free(buf); + } else { + ESP_LOGE(TAG, "Header1 not found"); + httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Header1 not found"); + return ESP_ERR_NOT_FOUND; + } + + buf_len = httpd_req_get_hdr_value_len(req, "Header3"); + if (buf_len > 0) { + buf = malloc(++buf_len); + if (!buf) { + ESP_LOGE(TAG, "Failed to allocate memory of %d bytes!", buf_len); + httpd_resp_send_err(req, HTTPD_500_INTERNAL_SERVER_ERROR, "Memory allocation failed"); + return ESP_ERR_NO_MEM; + } + /* Copy null terminated value string into buffer */ + if (httpd_req_get_hdr_value_str(req, "Header3", buf, buf_len) == ESP_OK) { + ESP_LOGI(TAG, "Header3 content: %s", buf); + if (strcmp("Value3", buf) != 0) { + httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Wrong value of Header3 received"); + free(buf); + return ESP_ERR_INVALID_ARG; + } else { + ESP_LOGI(TAG, "Expected value and received value matched for Header3"); + } + } else { + ESP_LOGE(TAG, "Error in getting value of Header3"); + httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Error in getting value of Header3"); + free(buf); + return ESP_FAIL; + } + free(buf); + } else { + ESP_LOGE(TAG, "Header3 not found"); + httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Header3 not found"); + return ESP_ERR_NOT_FOUND; + } + + buf_len = httpd_req_get_hdr_value_len(req, "Header2"); + buf = malloc(++buf_len); + if (!buf) { + ESP_LOGE(TAG, "Failed to allocate memory of %d bytes!", buf_len); + httpd_resp_send_err(req, HTTPD_500_INTERNAL_SERVER_ERROR, "Memory allocation failed"); + return ESP_ERR_NO_MEM; + } + if (httpd_req_get_hdr_value_str(req, "Header2", buf, buf_len) == ESP_OK) { + ESP_LOGI(TAG, "Header2 content: %s", buf); + httpd_resp_send(req, buf, strlen(buf)); + } else { + ESP_LOGE(TAG, "Header2 not found"); + httpd_resp_send_err(req, HTTPD_400_BAD_REQUEST, "Header2 not found"); + return ESP_FAIL; + } + + return ESP_OK; +} + static esp_err_t hello_type_get_handler(httpd_req_t *req) { #define STR "Hello World!" @@ -217,6 +307,11 @@ static const httpd_uri_t basic_handlers[] = { .handler = hello_type_get_handler, .user_ctx = NULL, }, + { .uri = "/test_header", + .method = HTTP_GET, + .handler = test_header_get_handler, + .user_ctx = NULL, + }, { .uri = "/hello", .method = HTTP_GET, .handler = hello_get_handler, @@ -275,6 +370,8 @@ static httpd_handle_t test_httpd_start() pre_start_mem = esp_get_free_heap_size(); httpd_handle_t hd; httpd_config_t config = HTTPD_DEFAULT_CONFIG(); + /* Modify this setting to match the number of test URI handlers */ + config.max_uri_handlers = 9; config.server_port = 1234; /* This check should be a part of http_server */ diff --git a/examples/protocols/http_server/advanced_tests/scripts/test.py b/examples/protocols/http_server/advanced_tests/scripts/test.py index d1a923aed..4196ff532 100644 --- a/examples/protocols/http_server/advanced_tests/scripts/test.py +++ b/examples/protocols/http_server/advanced_tests/scripts/test.py @@ -142,7 +142,20 @@ import http.client import sys import string import random -import Utility + + +try: + import Utility +except ImportError: + import os + + # This environment variable is expected on the host machine + # > export TEST_FW_PATH=~/esp/esp-idf/tools/tiny-test-fw + test_fw_path = os.getenv("TEST_FW_PATH") + if test_fw_path and test_fw_path not in sys.path: + sys.path.insert(0, test_fw_path) + + import Utility _verbose_ = False @@ -427,6 +440,34 @@ def get_echo(dut, port): return True +def get_test_headers(dut, port): + # GET /test_header returns data of Header2' + Utility.console_log("[test] GET /test_header =>", end=' ') + conn = http.client.HTTPConnection(dut, int(port), timeout=15) + custom_header = {"Header1": "Value1", "Header3": "Value3"} + header2_values = ["", " ", "Value2", " Value2", "Value2 ", " Value2 "] + for val in header2_values: + custom_header["Header2"] = val + conn.request("GET", "/test_header", headers=custom_header) + resp = conn.getresponse() + if not test_val("status_code", 200, resp.status): + conn.close() + return False + hdr_val_start_idx = val.find("Value2") + if hdr_val_start_idx == -1: + if not test_val("header: Header2", "", resp.read().decode()): + conn.close() + return False + else: + if not test_val("header: Header2", val[hdr_val_start_idx:], resp.read().decode()): + conn.close() + return False + resp.read() + Utility.console_log("Success") + conn.close() + return True + + def get_hello_type(dut, port): # GET /hello/type_html returns text/html as Content-Type' Utility.console_log("[test] GET /hello/type_html has Content-Type of text/html =>", end=' ') @@ -966,6 +1007,7 @@ if __name__ == '__main__': get_hello_type(dut, port) get_hello_status(dut, port) get_false_uri(dut, port) + get_test_headers(dut, port) Utility.console_log("### Error code tests") code_500_server_error_test(dut, port)