Suricata

Problem comes from some code assuming bidirectional traffic.

Bug was found by this new fuzz target which aggressively frees memory

First commit should be :

transaction: do not call request code on response path

And handles closing connection in middle of request line

diff --git a/htp/htp_request.c b/htp/htp_request.c
index 18436e2..23e9540 100644
--- a/htp/htp_request.c
+++ b/htp/htp_request.c
@@ -816,6 +816,10 @@ htp_status_t htp_connp_REQ_LINE_complete(htp_connp_t *connp) {
 htp_status_t htp_connp_REQ_LINE(htp_connp_t *connp) {
     for (;;) {
         // Get one byte
+        IN_PEEK_NEXT(connp);
+        if (connp->in_status == HTP_STREAM_CLOSED && connp->in_next_byte == -1) {
+            return htp_connp_REQ_LINE_complete(connp);
+        }
         IN_COPY_BYTE_OR_RETURN(connp);

         // Have we reached the end of the line?
diff --git a/htp/htp_transaction.c b/htp/htp_transaction.c
index 2936b01..0080dfa 100644
--- a/htp/htp_transaction.c
+++ b/htp/htp_transaction.c
@@ -1350,11 +1350,6 @@ htp_status_t htp_tx_state_response_start(htp_tx_t *tx) {
      * or a overly long request */
     if (tx->request_method == HTP_M_UNKNOWN && tx->request_uri == NULL && tx->connp->in_state == htp_connp_REQ_LINE) {
         htp_log(tx->connp, HTP_LOG_MARK, HTP_LOG_WARNING, 0, "Request line incomplete");
-
-        if (htp_connp_REQ_LINE_complete(tx->connp) != HTP_OK) {
-            return HTP_ERROR;
-        }
     }

     return HTP_OK;

Bug happens when :
- we have a first normal request/response (so `htp_connp_req_data` sets a dangling pointer to `in_current_data`)
- we have a response without request and `htp_tx_state_response_start` calls `htp_connp_REQ_LINE_complete` which calls `htp_connp_req_consolidate_data` which uses dangling pointer

The patch fixes this by removing the call from `htp_tx_state_response_start` to `htp_connp_REQ_LINE_complete`
In order to maintain the good behavior with an incomplete request during the request line, `htp_connp_REQ_LINE` is modified to make the state progress when `connp->in_status == HTP_STREAM_CLOSED`

Second commit should be

headers : right terminator check according to next data

htp_connp_is_line_terminator handles both inbound and outbound data.
So, it cannot access directly out_current_data.
But its caller can specify another argument.

diff --git a/htp/htp_private.h b/htp/htp_private.h
index 3eca465..e87fc9e 100644
--- a/htp/htp_private.h
+++ b/htp/htp_private.h
@@ -157,7 +157,7 @@ int htp_is_line_whitespace(unsigned char *data, size_t len);

 int htp_connp_is_line_folded(unsigned char *data, size_t len);
 int htp_is_folding_char(int c);
-int htp_connp_is_line_terminator(htp_connp_t *connp, unsigned char *data, size_t len);
+int htp_connp_is_line_terminator(htp_connp_t *connp, unsigned char *data, size_t len, int nextNoLF);
 int htp_connp_is_line_ignorable(htp_connp_t *connp, unsigned char *data, size_t len);

 int htp_parse_uri(bstr *input, htp_uri_t **uri);
diff --git a/htp/htp_request.c b/htp/htp_request.c
index 3756de3..18436e2 100644
--- a/htp/htp_request.c
+++ b/htp/htp_request.c
@@ -647,7 +647,7 @@ htp_status_t htp_connp_REQ_HEADERS(htp_connp_t *connp) {
             #endif           

             // Should we terminate headers?
-            if (htp_connp_is_line_terminator(connp, data, len)) {
+            if (htp_connp_is_line_terminator(connp, data, len, 0)) {
                 // Parse previous header, if any.
                 if (connp->in_header != NULL) {
                     if (connp->cfg->process_request_header(connp, bstr_ptr(connp->in_header),
diff --git a/htp/htp_response.c b/htp/htp_response.c
index 9c74a07..1d6896b 100644
--- a/htp/htp_response.c
+++ b/htp/htp_response.c
@@ -826,8 +826,13 @@ htp_status_t htp_connp_RES_HEADERS(htp_connp_t *connp) {
             fprint_raw_data(stderr, __func__, data, len);
             #endif

+            int nextNoLF = 0;
+            if (connp->out_current_read_offset < connp->out_current_len &&
+                connp->out_current_data[connp->out_current_read_offset] != LF) {
+                nextNoLF = 1;
+            }
             // Should we terminate headers?
-            if (htp_connp_is_line_terminator(connp, data, len)) {
+            if (htp_connp_is_line_terminator(connp, data, len, nextNoLF)) {
                 // Parse previous header, if any.
                 if (connp->out_header != NULL) {
                     if (connp->cfg->process_response_header(connp, bstr_ptr(connp->out_header),
diff --git a/htp/htp_util.c b/htp/htp_util.c
index bfd679e..6def754 100644
--- a/htp/htp_util.c
+++ b/htp/htp_util.c
@@ -464,7 +464,7 @@ int htp_is_folding_char(int c) {
  * @param[in] len
  * @return 0 or 1
  */
-int htp_connp_is_line_terminator(htp_connp_t *connp, unsigned char *data, size_t len) {
+int htp_connp_is_line_terminator(htp_connp_t *connp, unsigned char *data, size_t len, int nextNoLF) {
     // Is this the end of request headers?
     switch (connp->cfg->server_personality) {
         case HTP_SERVER_IIS_5_1:
@@ -481,10 +481,7 @@ int htp_connp_is_line_terminator(htp_connp_t *connp, unsigned char *data, size_t
             }
             // Only space is terminator if terminator does not follow right away
             if (len == 2 && htp_is_lws(data[0]) && data[1] == LF) {
-                if (connp->out_current_read_offset < connp->out_current_len &&
-                    connp->out_current_data[connp->out_current_read_offset] != LF) {
-                    return 1;
-                }
+                return nextNoLF;
             }
             break;
     }
@@ -501,7 +498,7 @@ int htp_connp_is_line_terminator(htp_connp_t *connp, unsigned char *data, size_t
  * @return 0 or 1
  */
 int htp_connp_is_line_ignorable(htp_connp_t *connp, unsigned char *data, size_t len) {
-    return htp_connp_is_line_terminator(connp, data, len);
+    return htp_connp_is_line_terminator(connp, data, len, 0);
 }

 static htp_status_t htp_parse_port(unsigned char *data, size_t len, int *port, int *invalid) {

Bug happens when :
- we have a first normal request/response (so `htp_connp_res_data` sets a dangling pointer to `out_current_data`)
- we have a new request (without response yet) and `htp_connp_REQ_LINE_complete` calls `htp_connp_is_line_terminator` which uses dangling pointer

The patch fixes this by getting the needed data in `htp_connp_REQ_LINE_complete` and passing it to `htp_connp_is_line_terminator` as an argument
The other callers to `htp_connp_is_line_terminator` use 0 as default value for this argument