From 38b1c937642bdc68aef48aae62360e4211b9f637 Mon Sep 17 00:00:00 2001 From: Chinmay Date: Mon, 9 Sep 2019 12:05:40 +0530 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 | 99 ++++++++++++++++++- .../advanced_tests/scripts/test.py | 44 ++++++++- 4 files changed, 160 insertions(+), 2 deletions(-) diff --git a/components/esp_http_server/src/httpd_parse.c b/components/esp_http_server/src/httpd_parse.c index 765510f0c2..ec73bbb5f5 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 1509fa5410..24ae36300b 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 589bf96469..d7d003e085 100644 --- a/examples/protocols/http_server/advanced_tests/main/tests.c +++ b/examples/protocols/http_server/advanced_tests/main/tests.c @@ -28,7 +28,97 @@ esp_err_t hello_get_handler(httpd_req_t *req) #undef STR } -esp_err_t hello_type_get_handler(httpd_req_t *req) +/* 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!" httpd_resp_set_type(req, HTTPD_TYPE_TEXT); @@ -217,6 +307,11 @@ 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, @@ -274,6 +369,8 @@ 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 d1a923aeda..4196ff532a 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)