detect: fix multi inspect buffer issue; clean up

Fix multi inspect buffer API causing cleanup logic in the single
inspect buffer paths. This could lead to a buffer overrun in the
"to clear" logic.

Multi buffers now use InspectionBufferSetupMulti instead of
InspectionBuffer. This is enforced by a check in debug validation.

Simplify the multi inspect buffer setup code and update the callers.

src/detect-dns-query.c

@@ -78,8 +78,8 @@ static InspectionBuffer *DnsQueryGetData(DetectEngineThreadCtx *det_ctx,
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, cbdata->local_id);
+    InspectionBuffer *buffer =
+            InspectionBufferMultipleForListGet(det_ctx, list_id, cbdata->local_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -91,8 +91,7 @@ static InspectionBuffer *DnsQueryGetData(DetectEngineThreadCtx *det_ctx,
                 &data, &data_len) == 0) {
         return NULL;
     }
-    InspectionBufferSetup(det_ctx, list_id, buffer, data, data_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, data, data_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }

src/detect-engine.c

@@ -76,6 +76,7 @@
 #include "util-device.h"
 #include "util-var-name.h"
 #include "util-profiling.h"
+#include "util-validate.h"
 
 #include "tm-threads.h"
 #include "runmodes.h"
@@ -1008,12 +1009,26 @@ InspectionBuffer *InspectionBufferGet(DetectEngineThreadCtx *det_ctx, const int
     return &det_ctx->inspect.buffers[list_id];
 }
 
+static InspectionBufferMultipleForList *InspectionBufferGetMulti(
+        DetectEngineThreadCtx *det_ctx, const int list_id)
+{
+    InspectionBufferMultipleForList *buffer = &det_ctx->multi_inspect.buffers[list_id];
+    if (!buffer->init) {
+        det_ctx->multi_inspect.to_clear_queue[det_ctx->multi_inspect.to_clear_idx++] = list_id;
+        buffer->init = 1;
+    }
+    return buffer;
+}
+
 /** \brief for a InspectionBufferMultipleForList get a InspectionBuffer
  *  \param fb the multiple buffer array
  *  \param local_id the index to get a buffer
  *  \param buffer the inspect buffer or NULL in case of error */
-InspectionBuffer *InspectionBufferMultipleForListGet(InspectionBufferMultipleForList *fb, uint32_t local_id)
+InspectionBuffer *InspectionBufferMultipleForListGet(
+        DetectEngineThreadCtx *det_ctx, const int list_id, const uint32_t local_id)
 {
+    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
+
     if (local_id >= fb->size) {
         uint32_t old_size = fb->size;
         uint32_t new_size = local_id + 1;
@@ -1035,16 +1050,9 @@ InspectionBuffer *InspectionBufferMultipleForListGet(InspectionBufferMultipleFor
     fb->max = MAX(fb->max, local_id);
     InspectionBuffer *buffer = &fb->inspection_buffers[local_id];
     SCLogDebug("using file_data buffer %p", buffer);
-    return buffer;
-}
-
-InspectionBufferMultipleForList *InspectionBufferGetMulti(DetectEngineThreadCtx *det_ctx, const int list_id)
-{
-    InspectionBufferMultipleForList *buffer = &det_ctx->multi_inspect.buffers[list_id];
-    if (!buffer->init) {
-        det_ctx->multi_inspect.to_clear_queue[det_ctx->multi_inspect.to_clear_idx++] = list_id;
-        buffer->init = 1;
-    }
+#ifdef DEBUG_VALIDATION
+    buffer->multi = true;
+#endif
     return buffer;
 }
 
@@ -1057,10 +1065,23 @@ void InspectionBufferInit(InspectionBuffer *buffer, uint32_t initial_size)
     }
 }
 
+/** \brief setup the buffer with our initial data */
+void InspectionBufferSetupMulti(InspectionBuffer *buffer, const DetectEngineTransforms *transforms,
+        const uint8_t *data, const uint32_t data_len)
+{
+    DEBUG_VALIDATE_BUG_ON(!buffer->multi);
+    buffer->inspect = buffer->orig = data;
+    buffer->inspect_len = buffer->orig_len = data_len;
+    buffer->len = 0;
+
+    InspectionBufferApplyTransforms(buffer, transforms);
+}
+
 /** \brief setup the buffer with our initial data */
 void InspectionBufferSetup(DetectEngineThreadCtx *det_ctx, const int list_id,
         InspectionBuffer *buffer, const uint8_t *data, const uint32_t data_len)
 {
+    DEBUG_VALIDATE_BUG_ON(buffer->multi);
     if (buffer->inspect == NULL) {
 #ifdef UNITTESTS
         if (det_ctx && list_id != -1)

src/detect-engine.h

@@ -1,4 +1,4 @@
-/* Copyright (C) 2007-2010 Open Information Security Foundation
+/* Copyright (C) 2007-2021 Open Information Security Foundation
  *
  * You can copy, redistribute or modify this Program under the terms of
  * the GNU General Public License version 2 as published by the Free
@@ -40,8 +40,10 @@ bool DetectBufferTypeValidateTransform(DetectEngineCtx *de_ctx, int sm_list,
         const uint8_t *content, uint16_t content_len, const char **namestr);
 void InspectionBufferClean(DetectEngineThreadCtx *det_ctx);
 InspectionBuffer *InspectionBufferGet(DetectEngineThreadCtx *det_ctx, const int list_id);
-InspectionBuffer *InspectionBufferMultipleForListGet(InspectionBufferMultipleForList *fb, uint32_t local_id);
-InspectionBufferMultipleForList *InspectionBufferGetMulti(DetectEngineThreadCtx *det_ctx, const int list_id);
+void InspectionBufferSetupMulti(InspectionBuffer *buffer, const DetectEngineTransforms *transforms,
+        const uint8_t *data, const uint32_t data_len);
+InspectionBuffer *InspectionBufferMultipleForListGet(
+        DetectEngineThreadCtx *det_ctx, const int list_id, uint32_t local_id);
 
 int DetectBufferTypeRegister(const char *name);
 int DetectBufferTypeGetByName(const char *name);

src/detect-file-data.c

@@ -488,8 +488,7 @@ static inline InspectionBuffer *FiledataWithXformsGetDataCallback(DetectEngineTh
         const DetectEngineTransforms *transforms, const int list_id, int local_file_id,
         InspectionBuffer *base_buffer, const bool first)
 {
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, local_file_id);
+    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(det_ctx, list_id, local_file_id);
     if (buffer == NULL) {
         SCLogDebug("list_id: %d: no buffer", list_id);
         return NULL;
@@ -499,9 +498,8 @@ static inline InspectionBuffer *FiledataWithXformsGetDataCallback(DetectEngineTh
         return buffer;
     }
 
-    InspectionBufferSetup(det_ctx, list_id, buffer, base_buffer->inspect, base_buffer->inspect_len);
+    InspectionBufferSetupMulti(buffer, transforms, base_buffer->inspect, base_buffer->inspect_len);
     buffer->inspect_offset = base_buffer->inspect_offset;
-    InspectionBufferApplyTransforms(buffer, transforms);
     SCLogDebug("xformed buffer %p size %u", buffer, buffer->inspect_len);
     SCReturnPtr(buffer, "InspectionBuffer");
 }
@@ -514,9 +512,8 @@ static InspectionBuffer *FiledataGetDataCallback(DetectEngineThreadCtx *det_ctx,
     SCLogDebug(
             "starting: list_id %d base_id %d first %s", list_id, base_id, first ? "true" : "false");
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, base_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, local_file_id);
-    SCLogDebug("base: fb %p buffer %p", fb, buffer);
+    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(det_ctx, base_id, local_file_id);
+    SCLogDebug("base: buffer %p", buffer);
     if (buffer == NULL)
         return NULL;
     if (base_id != list_id && buffer->inspect != NULL) {
@@ -568,7 +565,7 @@ static InspectionBuffer *FiledataGetDataCallback(DetectEngineThreadCtx *det_ctx,
     StreamingBufferGetDataAtOffset(cur_file->sb,
             &data, &data_len,
             cur_file->content_inspected);
-    InspectionBufferSetup(det_ctx, list_id, buffer, data, data_len);
+    InspectionBufferSetupMulti(buffer, NULL, data, data_len);
     SCLogDebug("[list %d] [before] buffer offset %" PRIu64 "; buffer len %" PRIu32
                "; data_len %" PRIu32 "; file_size %" PRIu64,
             list_id, buffer->inspect_offset, buffer->inspect_len, data_len, file_size);

src/detect-filemagic.c

@@ -438,8 +438,7 @@ static InspectionBuffer *FilemagicGetDataCallback(DetectEngineThreadCtx *det_ctx
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, local_file_id);
+    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(det_ctx, list_id, local_file_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -461,8 +460,7 @@ static InspectionBuffer *FilemagicGetDataCallback(DetectEngineThreadCtx *det_ctx
     const uint8_t *data = (const uint8_t *)cur_file->magic;
     uint32_t data_len = (uint32_t)strlen(cur_file->magic);
 
-    InspectionBufferSetup(det_ctx, list_id, buffer, data, data_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, data, data_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }

src/detect-filename.c

@@ -357,8 +357,7 @@ static InspectionBuffer *FilenameGetDataCallback(DetectEngineThreadCtx *det_ctx,
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, local_file_id);
+    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(det_ctx, list_id, local_file_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -367,8 +366,7 @@ static InspectionBuffer *FilenameGetDataCallback(DetectEngineThreadCtx *det_ctx,
     const uint8_t *data = cur_file->name;
     uint32_t data_len = cur_file->name_len;
 
-    InspectionBufferSetup(det_ctx, list_id, buffer, data, data_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, data, data_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }

src/detect-http2.c

@@ -691,8 +691,8 @@ static InspectionBuffer *GetHttp2HNameData(DetectEngineThreadCtx *det_ctx,
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, cbdata->local_id);
+    InspectionBuffer *buffer =
+            InspectionBufferMultipleForListGet(det_ctx, list_id, cbdata->local_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -825,8 +825,8 @@ static InspectionBuffer *GetHttp2HeaderData(DetectEngineThreadCtx *det_ctx,
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, cbdata->local_id);
+    InspectionBuffer *buffer =
+            InspectionBufferMultipleForListGet(det_ctx, list_id, cbdata->local_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -840,8 +840,7 @@ static InspectionBuffer *GetHttp2HeaderData(DetectEngineThreadCtx *det_ctx,
     if (b == NULL || b_len == 0)
         return NULL;
 
-    InspectionBufferSetup(det_ctx, list_id, buffer, b, b_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, b, b_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }

src/detect-ike-vendor.c

@@ -59,8 +59,8 @@ static InspectionBuffer *IkeVendorGetData(DetectEngineThreadCtx *det_ctx,
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, cbdata->local_id);
+    InspectionBuffer *buffer =
+            InspectionBufferMultipleForListGet(det_ctx, list_id, cbdata->local_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -72,8 +72,7 @@ static InspectionBuffer *IkeVendorGetData(DetectEngineThreadCtx *det_ctx,
         return NULL;
     }
 
-    InspectionBufferSetup(det_ctx, list_id, buffer, data, data_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, data, data_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }

src/detect-krb5-cname.c

@@ -61,8 +61,8 @@ static InspectionBuffer *GetKrb5CNameData(DetectEngineThreadCtx *det_ctx,
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, cbdata->local_id);
+    InspectionBuffer *buffer =
+            InspectionBufferMultipleForListGet(det_ctx, list_id, cbdata->local_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -76,8 +76,7 @@ static InspectionBuffer *GetKrb5CNameData(DetectEngineThreadCtx *det_ctx,
     if (b == NULL || b_len == 0)
         return NULL;
 
-    InspectionBufferSetup(det_ctx, list_id, buffer, b, b_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, b, b_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }

src/detect-krb5-sname.c

@@ -61,8 +61,8 @@ static InspectionBuffer *GetKrb5SNameData(DetectEngineThreadCtx *det_ctx,
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, cbdata->local_id);
+    InspectionBuffer *buffer =
+            InspectionBufferMultipleForListGet(det_ctx, list_id, cbdata->local_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -76,8 +76,7 @@ static InspectionBuffer *GetKrb5SNameData(DetectEngineThreadCtx *det_ctx,
     if (b == NULL || b_len == 0)
         return NULL;
 
-    InspectionBufferSetup(det_ctx, list_id, buffer, b, b_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, b, b_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }

src/detect-mqtt-subscribe-topic.c

@@ -69,8 +69,8 @@ static InspectionBuffer *MQTTSubscribeTopicGetData(DetectEngineThreadCtx *det_ct
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, cbdata->local_id);
+    InspectionBuffer *buffer =
+            InspectionBufferMultipleForListGet(det_ctx, list_id, cbdata->local_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -83,8 +83,7 @@ static InspectionBuffer *MQTTSubscribeTopicGetData(DetectEngineThreadCtx *det_ct
         return NULL;
     }
 
-    InspectionBufferSetup(det_ctx, list_id, buffer, data, data_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, data, data_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }
@@ -236,4 +235,4 @@ static int DetectMQTTSubscribeTopicSetup(DetectEngineCtx *de_ctx, Signature *s,
     if (DetectSignatureSetAppProto(s, ALPROTO_MQTT) < 0)
         return -1;
     return 0;
-}
+}

src/detect-mqtt-unsubscribe-topic.c

@@ -69,8 +69,8 @@ static InspectionBuffer *MQTTUnsubscribeTopicGetData(DetectEngineThreadCtx *det_
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, cbdata->local_id);
+    InspectionBuffer *buffer =
+            InspectionBufferMultipleForListGet(det_ctx, list_id, cbdata->local_id);
     if (buffer == NULL)
         return NULL;
     if (!first && buffer->inspect != NULL)
@@ -83,8 +83,7 @@ static InspectionBuffer *MQTTUnsubscribeTopicGetData(DetectEngineThreadCtx *det_
         return NULL;
     }
 
-    InspectionBufferSetup(det_ctx, list_id, buffer, data, data_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, data, data_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }
@@ -236,4 +235,4 @@ static int DetectMQTTUnsubscribeTopicSetup(DetectEngineCtx *de_ctx, Signature *s
     if (DetectSignatureSetAppProto(s, ALPROTO_MQTT) < 0)
         return -1;
     return 0;
-}
+}

src/detect-tls-certs.c

@@ -137,8 +137,8 @@ static InspectionBuffer *TlsCertsGetData(DetectEngineThreadCtx *det_ctx,
 {
     SCEnter();
 
-    InspectionBufferMultipleForList *fb = InspectionBufferGetMulti(det_ctx, list_id);
-    InspectionBuffer *buffer = InspectionBufferMultipleForListGet(fb, cbdata->local_id);
+    InspectionBuffer *buffer =
+            InspectionBufferMultipleForListGet(det_ctx, list_id, cbdata->local_id);
     if (buffer == NULL)
         return NULL;
 
@@ -151,16 +151,13 @@ static InspectionBuffer *TlsCertsGetData(DetectEngineThreadCtx *det_ctx,
     if (cbdata->cert == NULL) {
         cbdata->cert = TAILQ_FIRST(&ssl_state->server_connp.certs);
     } else {
-	cbdata->cert = TAILQ_NEXT(cbdata->cert, next);
+        cbdata->cert = TAILQ_NEXT(cbdata->cert, next);
     }
-
     if (cbdata->cert == NULL) {
         return NULL;
     }
 
-    InspectionBufferSetup(
-            det_ctx, list_id, buffer, cbdata->cert->cert_data, cbdata->cert->cert_len);
-    InspectionBufferApplyTransforms(buffer, transforms);
+    InspectionBufferSetupMulti(buffer, transforms, cbdata->cert->cert_data, cbdata->cert->cert_len);
 
     SCReturnPtr(buffer, "InspectionBuffer");
 }

src/detect.h

@@ -346,7 +346,9 @@ typedef struct InspectionBuffer {
     uint64_t inspect_offset;
     uint32_t inspect_len;   /**< size of active data. See to ::len or ::orig_len */
     uint8_t flags;          /**< DETECT_CI_FLAGS_* for use with DetectEngineContentInspection */
-
+#ifdef DEBUG_VALIDATION
+    bool multi;
+#endif
     uint32_t len;           /**< how much is in use */
     uint8_t *buf;
     uint32_t size;          /**< size of the memory allocation */