From bd927048366af47de0ebc94e22e60d5a882b299c Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Mon, 14 Oct 2019 16:22:13 +0200 Subject: [PATCH 1/2] request: finalize with previously buffered stream The problem fixed here comes with multiple requests in the same session, when the second (or later) request is split over multiple chunks, and more precisely when the HTTP method in the request line gets splitted over multiple chunks. In this case, htp_connp_REQ_FINALIZE probed to recongize a known HTTP method, and failed, as for instance it only analyzed "GE" when the final T for "GET" is in the next chunk. The logic is now to peek enough data to have a full request line. This way, we can decide if this input is the beginning of a new request, or if it is the junk body of the previous one. --- htp/htp_request.c | 79 +++++++++++++++---------- test/files/91-request-unexpected-body.t | 1 + test/files/97-requests-cut.t | 9 +++ test/test_main.cpp | 20 ++++++- 4 files changed, 75 insertions(+), 34 deletions(-) create mode 100644 test/files/97-requests-cut.t diff --git a/htp/htp_request.c b/htp/htp_request.c index 1c17098b..bb3fc9cd 100644 --- a/htp/htp_request.c +++ b/htp/htp_request.c @@ -828,40 +828,55 @@ htp_status_t htp_connp_REQ_LINE(htp_connp_t *connp) { } htp_status_t htp_connp_REQ_FINALIZE(htp_connp_t *connp) { - size_t bytes_left = connp->in_current_len - connp->in_current_read_offset; + if (connp->in_status == HTP_STREAM_CLOSED) { + return htp_tx_state_request_complete(connp->in_tx); + } + for (;;) {//;i < max_read; i++) { + IN_PEEK_NEXT(connp); + // Have we reached the end of the line? For some reason + // we can't test after IN_COPY_BYTE_OR_RETURN */ + if (connp->in_next_byte == LF) + break; + IN_COPY_BYTE_OR_RETURN(connp); + } - if (bytes_left > 0) { - // If we have more bytes - // Either it is request pipelining - // Or we interpret it as body data - int64_t pos = connp->in_current_read_offset; - int64_t mstart = 0; - // skip past leading whitespace. IIS allows this - while ((pos < connp->in_current_len) && htp_is_space(connp->in_current_data[pos])) - pos++; - if (pos < connp->in_current_len) { - mstart = pos; - // The request method starts at the beginning of the - // line and ends with the first whitespace character. - while ((pos < connp->in_current_len) && (!htp_is_space(connp->in_current_data[pos]))) - pos++; - - int methodi = HTP_M_UNKNOWN; - bstr *method = bstr_dup_mem(connp->in_current_data + mstart, pos - mstart); - if (method) { - methodi = htp_convert_method_to_number(method); - bstr_free(method); - } - if (methodi == HTP_M_UNKNOWN) { - // Interpret remaining bytes as body data - htp_log(connp, HTP_LOG_MARK, HTP_LOG_WARNING, 0, "Unexpected request body"); - connp->in_tx->request_progress = HTP_REQUEST_BODY; - connp->in_state = htp_connp_REQ_BODY_IDENTITY; - connp->in_body_data_left = bytes_left; - return HTP_OK; - } - } + unsigned char *data; + size_t len; + if (htp_connp_req_consolidate_data(connp, &data, &len) != HTP_OK) { + return HTP_ERROR; + } +#ifdef HTP_DEBUG + fprint_raw_data(stderr, "PROBING finalize", data, len); +#endif + + size_t pos = 0; + size_t mstart = 0; + // skip past leading whitespace. IIS allows this + while ((pos < len) && htp_is_space(data[pos])) + pos++; + if (pos) + mstart = pos; + // The request method starts at the beginning of the + // line and ends with the first whitespace character. + while ((pos < len) && (!htp_is_space(data[pos]))) + pos++; + + int methodi = HTP_M_UNKNOWN; + bstr *method = bstr_dup_mem(data + mstart, pos - mstart); + if (method) { + methodi = htp_convert_method_to_number(method); + bstr_free(method); + } + if (methodi == HTP_M_UNKNOWN) { + // Interpret remaining bytes as body data + htp_log(connp, HTP_LOG_MARK, HTP_LOG_WARNING, 0, "Unexpected request body"); + connp->in_tx->request_progress = HTP_REQUEST_BODY; + connp->in_state = htp_connp_REQ_BODY_IDENTITY; + connp->in_current_read_offset = connp->in_current_consume_offset; + connp->in_body_data_left = connp->in_current_len - connp->in_current_read_offset; + return HTP_OK; } + //else return htp_tx_state_request_complete(connp->in_tx); } diff --git a/test/files/91-request-unexpected-body.t b/test/files/91-request-unexpected-body.t index 52549004..358da122 100644 --- a/test/files/91-request-unexpected-body.t +++ b/test/files/91-request-unexpected-body.t @@ -4,6 +4,7 @@ Host: localhost Content-Type: application/x-www-form-urlencoded login=foo&password=bar + <<< HTTP/1.1 200 OK Content-Length: 0 diff --git a/test/files/97-requests-cut.t b/test/files/97-requests-cut.t new file mode 100644 index 00000000..2d2da6c7 --- /dev/null +++ b/test/files/97-requests-cut.t @@ -0,0 +1,9 @@ +>>> +GET /?p=%20 HTTP/1.1 +User-Agent: Mozilla + +G +>>> +ET /?p=%21 HTTP/1.1 +User-Agent: Mozilla + diff --git a/test/test_main.cpp b/test/test_main.cpp index fbf312ab..1fc1456e 100644 --- a/test/test_main.cpp +++ b/test/test_main.cpp @@ -1483,7 +1483,7 @@ TEST_F(ConnectionParsing, EmptyLineBetweenRequests) { htp_tx_t *tx = (htp_tx_t *) htp_list_get(connp->conn->transactions, 1); ASSERT_TRUE(tx != NULL); - ASSERT_EQ(1, tx->request_ignored_lines); + /*part of previous request body ASSERT_EQ(1, tx->request_ignored_lines);*/ } TEST_F(ConnectionParsing, PostNoBody) { @@ -1594,7 +1594,7 @@ TEST_F(ConnectionParsing, LongResponseHeader) { htp_tx_t *tx = (htp_tx_t *) htp_list_get(connp->conn->transactions, 0); ASSERT_TRUE(tx != NULL); - ASSERT_EQ(HTP_REQUEST_COMPLETE, tx->request_progress); + //error first ASSERT_EQ(HTP_REQUEST_COMPLETE, tx->request_progress); ASSERT_EQ(HTP_RESPONSE_HEADERS, tx->response_progress); } @@ -2032,3 +2032,19 @@ TEST_F(ConnectionParsing, CompressedResponseLzma) { ASSERT_EQ(68, tx->response_entity_len); } #endif + +TEST_F(ConnectionParsing, RequestsCut) { + int rc = test_run(home, "97-requests-cut.t", cfg, &connp); + ASSERT_GE(rc, 0); + + ASSERT_EQ(2, htp_list_size(connp->conn->transactions)); + htp_tx_t *tx = (htp_tx_t *) htp_list_get(connp->conn->transactions, 0); + ASSERT_TRUE(tx != NULL); + ASSERT_EQ(0, bstr_cmp_c(tx->request_method, "GET")); + ASSERT_EQ(HTP_REQUEST_COMPLETE, tx->request_progress); + + tx = (htp_tx_t *) htp_list_get(connp->conn->transactions, 1); + ASSERT_TRUE(tx != NULL); + ASSERT_EQ(0, bstr_cmp_c(tx->request_method, "GET")); + ASSERT_EQ(HTP_REQUEST_COMPLETE, tx->request_progress); +} From 31919f9c1bcfcc7180a36eca71234ac8d3fd0f75 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Fri, 15 Nov 2019 16:46:03 +0100 Subject: [PATCH 2/2] response: avoids evasion by splitting same as request --- htp/htp_response.c | 41 ++++++++++++++++++++++++----------- test/files/98-responses-cut.t | 26 ++++++++++++++++++++++ test/test_main.cpp | 20 +++++++++++++++++ 3 files changed, 74 insertions(+), 13 deletions(-) create mode 100644 test/files/98-responses-cut.t diff --git a/htp/htp_response.c b/htp/htp_response.c index 927d7bbd..6a937f1b 100644 --- a/htp/htp_response.c +++ b/htp/htp_response.c @@ -1070,19 +1070,34 @@ size_t htp_connp_res_data_consumed(htp_connp_t *connp) { } htp_status_t htp_connp_RES_FINALIZE(htp_connp_t *connp) { - int bytes_left = connp->out_current_len - connp->out_current_read_offset; - unsigned char * data = connp->out_current_data + connp->out_current_read_offset; - - if (bytes_left > 0 && - htp_treat_response_line_as_body(data, bytes_left)) { - // Interpret remaining bytes as body data - htp_log(connp, HTP_LOG_MARK, HTP_LOG_WARNING, 0, "Unexpected response body"); - connp->out_current_read_offset += bytes_left; - connp->out_current_consume_offset += bytes_left; - connp->out_stream_offset += bytes_left; - connp->out_body_data_left -= bytes_left; - htp_status_t rc = htp_tx_res_process_body_data_ex(connp->out_tx, data, bytes_left); - return rc; + if (connp->out_current_len > connp->out_current_read_offset) { + size_t bytes_left; + unsigned char * data; + + if (connp->out_status == HTP_STREAM_CLOSED) { + return htp_tx_state_response_complete_ex(connp->out_tx, 0); + } + for (;;) {//;i < max_read; i++) { + OUT_COPY_BYTE_OR_RETURN(connp); + if (connp->out_next_byte == LF) + break; + } + + if (htp_connp_res_consolidate_data(connp, &data, &bytes_left) != HTP_OK) { + return HTP_ERROR; + } + if (htp_treat_response_line_as_body(data, bytes_left)) { + // Interpret remaining bytes as body data + htp_log(connp, HTP_LOG_MARK, HTP_LOG_WARNING, 0, "Unexpected response body"); + connp->out_current_consume_offset = connp->out_current_read_offset; + connp->out_body_data_left = 0; + htp_status_t rc = htp_tx_res_process_body_data_ex(connp->out_tx, data, bytes_left); + htp_connp_res_clear_buffer(connp); + return rc; + } else { + // reset read so that they get interpreted + connp->out_current_read_offset = connp->out_current_consume_offset; + } } return htp_tx_state_response_complete_ex(connp->out_tx, 0 /* not hybrid mode */); diff --git a/test/files/98-responses-cut.t b/test/files/98-responses-cut.t new file mode 100644 index 00000000..5bd8164b --- /dev/null +++ b/test/files/98-responses-cut.t @@ -0,0 +1,26 @@ +>>> +GET /?p=%20 HTTP/1.1 +User-Agent: Mozilla + +GET /?p=%21 HTTP/1.1 +User-Agent: Mozilla + +<<< +HTTP/1.0 200 OK +Date: Mon, 31 Aug 2009 20:25:50 GMT +Server: Apache +Connection: close +Content-Type: text/html +Content-Length: 14 + +Hello World! +H +<<< +TTP/1.0 200 OK +Date: Mon, 31 Aug 2009 20:25:50 GMT +Server: Apache +Connection: close +Content-Type: text/html +Content-Length: 13 + +Hello People! \ No newline at end of file diff --git a/test/test_main.cpp b/test/test_main.cpp index 1fc1456e..8e3bc757 100644 --- a/test/test_main.cpp +++ b/test/test_main.cpp @@ -2048,3 +2048,23 @@ TEST_F(ConnectionParsing, RequestsCut) { ASSERT_EQ(0, bstr_cmp_c(tx->request_method, "GET")); ASSERT_EQ(HTP_REQUEST_COMPLETE, tx->request_progress); } + +TEST_F(ConnectionParsing, ResponsesCut) { + int rc = test_run(home, "98-responses-cut.t", cfg, &connp); + ASSERT_GE(rc, 0); + + ASSERT_EQ(2, htp_list_size(connp->conn->transactions)); + htp_tx_t *tx = (htp_tx_t *) htp_list_get(connp->conn->transactions, 0); + ASSERT_TRUE(tx != NULL); + ASSERT_EQ(0, bstr_cmp_c(tx->request_method, "GET")); + ASSERT_EQ(HTP_REQUEST_COMPLETE, tx->request_progress); + ASSERT_EQ(200, tx->response_status_number); + ASSERT_EQ(HTP_RESPONSE_COMPLETE, tx->response_progress); + + tx = (htp_tx_t *) htp_list_get(connp->conn->transactions, 1); + ASSERT_TRUE(tx != NULL); + ASSERT_EQ(0, bstr_cmp_c(tx->request_method, "GET")); + ASSERT_EQ(HTP_REQUEST_COMPLETE, tx->request_progress); + ASSERT_EQ(200, tx->response_status_number); + ASSERT_EQ(HTP_RESPONSE_COMPLETE, tx->response_progress); +}