Crash in esp_http_server request parsing (IDFGH-748) #3182
Comments
github-actions
bot
changed the title
|
@jimparis Thanks for reporting and thoroughly tracing the issue.
You are right, and that needs to be corrected (though as per the parsing logic, the The source of the problem is this line in This assumes that, for packets with no body, the final header will be followed by two CRLFs, though I see your request sends only LFs, that is why this assumption fails: Initially I was puzzled why the http_parser library (which is used in esp_http_server) doesn't check for CRLFs (as described in the HTTP 1.1 RFC 2616), but it turns out there's a catch : And as per 19.3 So the parsing logic followed in http_server breaks as this assumption doesn't hold always, for example in this case the browser you are using must be sending out LFs only. |
|
Thanks @anurag-kar for finding it. For the record these requests are being generated by Chrome's connectivity diagnostics tool, and I've reported their nonconforming line endings here: https://crbug.com/942528 |
|
This isn't the world's cleanest or most robust solution but it seems to work here: diff --git a/components/esp_http_server/src/httpd_parse.c b/components/esp_http_server/src/httpd_parse.c
index 2837778e..f7938c7f 100644
--- a/components/esp_http_server/src/httpd_parse.c
+++ b/components/esp_http_server/src/httpd_parse.c
@@ -52,6 +52,7 @@ typedef struct {
struct {
const char *at;
size_t length;
+ char replaced_char;
} last;
/* State variables */
@@ -217,6 +218,8 @@ static esp_err_t cb_header_field(http_parser *parser, const char *at, size_t len
} else if (parser_data->status == PARSING_HDR_VALUE) {
/* NULL terminate last header (key: value) pair */
size_t offset = parser_data->last.at - ra->scratch;
+ parser_data->last.replaced_char =
+ ra->scratch[offset + parser_data->last.length];
ra->scratch[offset + parser_data->last.length] = '\0';
/* Store current values of the parser callback arguments */
@@ -290,6 +293,8 @@ static esp_err_t cb_headers_complete(http_parser *parser)
} else if (parser_data->status == PARSING_HDR_VALUE) {
/* NULL terminate last header (key: value) pair */
size_t offset = parser_data->last.at - ra->scratch;
+ parser_data->last.replaced_char =
+ ra->scratch[offset + parser_data->last.length];
ra->scratch[offset + parser_data->last.length] = '\0';
/* Reach end of last header */
@@ -379,8 +384,11 @@ static esp_err_t cb_no_body(http_parser *parser)
return ESP_FAIL;
}
- /* Get end of packet */
- at += strlen("\r\n\r\n");
+ /* Get end of packet. Skip over \r\n\r\n, or \n\n */
+ if (parser_data->last.replaced_char == '\r')
+ at += strlen("\r\n\r\n");
+ else
+ at += strlen("\n\n");
/* Pause parsing so that if part of another packet
* is in queue then it doesn't get parsed, which |
|
And it would also need to support bare LF at the end of header lines with something like this diff --git a/components/esp_http_server/src/httpd_parse.c b/components/esp_http_server/src/httpd_parse.c
index f7938c7f..54a91d0e 100644
--- a/components/esp_http_server/src/httpd_parse.c
+++ b/components/esp_http_server/src/httpd_parse.c
@@ -844,7 +844,9 @@ size_t httpd_req_get_hdr_value_len(httpd_req_t *r, const char *field)
*/
if ((val_ptr - hdr_ptr != strlen(field)) ||
(strncasecmp(hdr_ptr, field, strlen(field)))) {
- hdr_ptr += strlen(hdr_ptr) + strlen("\r\n");
+ hdr_ptr += strlen(hdr_ptr) + strlen("\r");
+ if (*hdr_ptr == '\n')
+ hdr_ptr++;
continue;
}
/* Skip ':' */
@@ -890,7 +892,9 @@ esp_err_t httpd_req_get_hdr_value_str(httpd_req_t *r, const char *field, char *v
*/
if ((val_ptr - hdr_ptr != strlen(field)) ||
(strncasecmp(hdr_ptr, field, strlen(field)))) {
- hdr_ptr += strlen(hdr_ptr) + strlen("\r\n");
+ hdr_ptr += strlen(hdr_ptr) + strlen("\r");
+ if (*hdr_ptr == '\n')
+ hdr_ptr++;
continue;
} |
That's great! @jimparis I really appreciate your efforts in solving this issue, though we have already started working on the bugfix (along with tests for checking support for LF/CRLF, so that we don't end up breaking this delicate logic in the future), but I am pretty sure it will not be very different from the logic that you are using in your version of the fix. Hope to have it merged as soon as possible. Till then please bear with us and thank you for cooperating. |
List of changes: * When parsing requests, count termination from LF characters only * Correct memcpy() length parameter in httpd_unrecv() (pointed out by jimparis in GitHub issue thread) * Use ssize_t to store results of length subtractions during parsing * Modify some comments to reduce ambiguity Closes #3182
List of changes: * When parsing requests, count termination from LF characters only * Correct memcpy() length parameter in httpd_unrecv() (pointed out by jimparis in GitHub issue thread) * Use ssize_t to store results of length subtractions during parsing * Modify some comments to reduce ambiguity Closes #3182
List of changes: * When parsing requests, count termination from LF characters only * Correct memcpy() length parameter in httpd_unrecv() (pointed out by jimparis in GitHub issue thread) * Use ssize_t to store results of length subtractions during parsing * Modify some comments to reduce ambiguity Closes espressif/esp-idf#3182
Running esp-idf recent master (ff020c3)
I'm seeing a crash in
esp_http_serverthat was a giant stack overflow. I neededCONFIG_FREERTOS_WATCHPOINT_END_OF_STACK=yin order to track it down. The cause was an unbounded memcpy in inhttpd_unrecv:Here,
buf_lenwas4294967294 == (unsigned)-2. Which is probably a bug in itself, but either way, that memcpy should be of lengthra->sd->pending_len, right?Changing it results in this error, instead of a crash:
Full debug output from
httpd_txrx.candhttpd_parse.cleading up to this error is as follows:Looking a bit further in the original crash, the huge
buf_lencame fromparse_parsing, which had this data:Here is the full backtrace at the time:
The text was updated successfully, but these errors were encountered: