detect: fixes use-after-free with http.request_header

Ticket: #6441

This keyword and the response one use a multiple inspection buffer.
But the different instances point to the same memory address
that comes from HttpHeaderGetBufferSpace and is not owned
by the transaction, and is rebuilt, which is a functional
bug in itself.

As it gets crafted, it can get reallocated if one header
is over 1024 bytes, while the previous freed pointer will still get
used for the previous headers.

diff --git a/src/detect-http-header.c b/src/detect-http-header.c
index 93942c06fa7a589ea917b7fddfc83793e060acd0..98e438c21111d552ca5b0fc477c1c571b2eb2450 100644 (file)
--- a/src/detect-http-header.c
+++ b/src/detect-http-header.c
@@ -48,6 +48,7 @@
 #include "util-print.h"
 #include "util-memcmp.h"
 #include "util-profiling.h"
+#include "util-validate.h"
 
 #include "app-layer.h"
 #include "app-layer-parser.h"
@@ -462,6 +463,8 @@ void DetectHttpHeaderRegister(void)
 
 static int g_http_request_header_buffer_id = 0;
 static int g_http_response_header_buffer_id = 0;
+static int g_request_header_thread_id = 0;
+static int g_response_header_thread_id = 0;
 
 static InspectionBuffer *GetHttp2HeaderData(DetectEngineThreadCtx *det_ctx, const uint8_t flags,
         const DetectEngineTransforms *transforms, Flow *_f, const struct MpmListIdDataArgs *cbdata,
@@ -570,6 +573,36 @@ static int PrefilterMpmHttp2HeaderRegister(DetectEngineCtx *de_ctx, SigGroupHead
             mpm_reg->app_v2.tx_min_progress, pectx, PrefilterMpmHttpHeaderFree, mpm_reg->name);
 }
 
+typedef struct HttpMultiBufItem {
+    uint8_t *buffer;
+    size_t len;
+} HttpMultiBufItem;
+
+typedef struct HttpMultiBufHeaderThreadData {
+    // array of items, being defined as a buffer with its length just above
+    HttpMultiBufItem *items;
+    // capacity of items (size of allocation)
+    size_t cap;
+    // length of items (number in use)
+    size_t len;
+} HttpMultiBufHeaderThreadData;
+
+static void *HttpMultiBufHeaderThreadDataInit(void *data)
+{
+    HttpMultiBufHeaderThreadData *td = SCCalloc(1, sizeof(*td));
+    return td;
+}
+
+static void HttpMultiBufHeaderThreadDataFree(void *data)
+{
+    HttpMultiBufHeaderThreadData *td = data;
+    for (size_t i = 0; i < td->cap; i++) {
+        SCFree(td->items[i].buffer);
+    }
+    SCFree(td->items);
+    SCFree(td);
+}
+
 static InspectionBuffer *GetHttp1HeaderData(DetectEngineThreadCtx *det_ctx, const uint8_t flags,
         const DetectEngineTransforms *transforms, Flow *f, const struct MpmListIdDataArgs *cbdata,
         int list_id)
@@ -583,10 +616,15 @@ static InspectionBuffer *GetHttp1HeaderData(DetectEngineThreadCtx *det_ctx, cons
     if (buffer->initialized)
         return buffer;
 
-    HttpHeaderThreadData *hdr_td = NULL;
-    HttpHeaderBuffer *buf =
-            HttpHeaderGetBufferSpace(det_ctx, f, flags, g_keyword_thread_id, &hdr_td);
-    if (unlikely(buf == NULL)) {
+    int kw_thread_id;
+    if (flags & STREAM_TOSERVER) {
+        kw_thread_id = g_request_header_thread_id;
+    } else {
+        kw_thread_id = g_response_header_thread_id;
+    }
+    HttpMultiBufHeaderThreadData *hdr_td =
+            DetectThreadCtxGetGlobalKeywordThreadCtx(det_ctx, kw_thread_id);
+    if (unlikely(hdr_td == NULL)) {
         return NULL;
     }
 
@@ -597,33 +635,53 @@ static InspectionBuffer *GetHttp1HeaderData(DetectEngineThreadCtx *det_ctx, cons
     } else {
         headers = tx->response_headers;
     }
-    if (cbdata->local_id < htp_table_size(headers)) {
-        htp_header_t *h = htp_table_get_index(headers, cbdata->local_id, NULL);
-        size_t size1 = bstr_size(h->name);
-        size_t size2 = bstr_size(h->value);
-        size_t b_len = size1 + 2 + size2;
-        if (b_len > buf->size) {
-            if (HttpHeaderExpandBuffer(hdr_td, buf, b_len) != 0) {
+    size_t no_of_headers = htp_table_size(headers);
+    if (cbdata->local_id == 0) {
+        // We initialize a big buffer on first item
+        // Then, we will just use parts of it
+        hdr_td->len = 0;
+        if (hdr_td->cap < no_of_headers) {
+            void *new_buffer = SCRealloc(hdr_td->items, no_of_headers * sizeof(HttpMultiBufItem));
+            if (unlikely(new_buffer == NULL)) {
                 return NULL;
             }
+            hdr_td->items = new_buffer;
+            // zeroes the new part of the items
+            memset(hdr_td->items + hdr_td->cap, 0,
+                    (no_of_headers - hdr_td->cap) * sizeof(HttpMultiBufItem));
+            hdr_td->cap = no_of_headers;
         }
-        memcpy(buf->buffer, bstr_ptr(h->name), bstr_size(h->name));
-        buf->buffer[size1] = ':';
-        buf->buffer[size1 + 1] = ' ';
-        memcpy(buf->buffer + size1 + 2, bstr_ptr(h->value), bstr_size(h->value));
-        buf->len = b_len;
-    } else {
-        InspectionBufferSetupMultiEmpty(buffer);
-        return NULL;
-    }
-    if (buf->len == 0) {
-        InspectionBufferSetupMultiEmpty(buffer);
-        return NULL;
+        for (size_t i = 0; i < no_of_headers; i++) {
+            htp_header_t *h = htp_table_get_index(headers, i, NULL);
+            size_t size1 = bstr_size(h->name);
+            size_t size2 = bstr_size(h->value);
+            size_t size = size1 + size2 + 2;
+            if (hdr_td->items[i].len < size) {
+                // Use realloc, as this pointer is not freed until HttpMultiBufHeaderThreadDataFree
+                hdr_td->items[i].buffer = SCRealloc(hdr_td->items[i].buffer, size);
+                if (unlikely(hdr_td->items[i].buffer == NULL)) {
+                    return NULL;
+                }
+            }
+            memcpy(hdr_td->items[i].buffer, bstr_ptr(h->name), size1);
+            hdr_td->items[i].buffer[size1] = ':';
+            hdr_td->items[i].buffer[size1 + 1] = ' ';
+            memcpy(hdr_td->items[i].buffer + size1 + 2, bstr_ptr(h->value), size2);
+            hdr_td->items[i].len = size;
+        }
+        hdr_td->len = no_of_headers;
     }
 
-    InspectionBufferSetupMulti(buffer, transforms, buf->buffer, buf->len);
-
-    SCReturnPtr(buffer, "InspectionBuffer");
+    // cbdata->local_id is the index of the requested header buffer
+    // hdr_td->len is the number of header buffers
+    if (cbdata->local_id < hdr_td->len) {
+        // we have one valid header buffer
+        InspectionBufferSetupMulti(buffer, transforms, hdr_td->items[cbdata->local_id].buffer,
+                hdr_td->items[cbdata->local_id].len);
+        SCReturnPtr(buffer, "InspectionBuffer");
+    } // else there are no more header buffer to get
+    InspectionBufferSetupMultiEmpty(buffer);
+    return NULL;
 }
 
 static void PrefilterTxHttp1Header(DetectEngineThreadCtx *det_ctx, const void *pectx, Packet *p,
@@ -736,6 +794,8 @@ void DetectHttpRequestHeaderRegister(void)
     DetectBufferTypeSetDescriptionByName("http_request_header", "HTTP header name and value");
     g_http_request_header_buffer_id = DetectBufferTypeGetByName("http_request_header");
     DetectBufferTypeSupportsMultiInstance("http_request_header");
+    g_request_header_thread_id = DetectRegisterThreadCtxGlobalFuncs("http_request_header",
+            HttpMultiBufHeaderThreadDataInit, NULL, HttpMultiBufHeaderThreadDataFree);
 }
 
 static int DetectHTTPResponseHeaderSetup(DetectEngineCtx *de_ctx, Signature *s, const char *arg)
@@ -771,6 +831,8 @@ void DetectHttpResponseHeaderRegister(void)
     DetectBufferTypeSetDescriptionByName("http_response_header", "HTTP header name and value");
     g_http_response_header_buffer_id = DetectBufferTypeGetByName("http_response_header");
     DetectBufferTypeSupportsMultiInstance("http_response_header");
+    g_response_header_thread_id = DetectRegisterThreadCtxGlobalFuncs("http_response_header",
+            HttpMultiBufHeaderThreadDataInit, NULL, HttpMultiBufHeaderThreadDataFree);
 }
 
 /************************************Unittests*********************************/