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.

(cherry picked from commit 3dc50322db0efb92683b9578c7dccd1fae4b5cb2)

diff --git a/src/detect-dns-query.c b/src/detect-dns-query.c
index c8798fa651dddcd9a97b014c3885f7948ab61970..ac7c5f763ac9e3d37684bcf13449e9f0c0bc1e92 100644 (file)
--- a/src/detect-dns-query.c
+++ b/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)
@@ -90,8 +90,7 @@ static InspectionBuffer *DnsQueryGetData(DetectEngineThreadCtx *det_ctx,
     if (rs_dns_tx_get_query_name(cbdata->txv, cbdata->local_id, &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");
 }

diff --git a/src/detect-engine.c b/src/detect-engine.c
index cc06e73a59905658f00be32dab78bc412cc0339e..afbe56ca83da29330e0c076233cdfea8f175976b 100644 (file)
--- a/src/detect-engine.c
+++ b/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"
@@ -1072,12 +1073,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;
@@ -1099,16 +1114,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;
 }
 
@@ -1121,10 +1129,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)

diff --git a/src/detect-engine.h b/src/detect-engine.h
index 6ec729f68387d2036f178494d3d848c3926c45e8..190662e60489f713cb39b40c552fcb83fa5b5298 100644 (file)
--- a/src/detect-engine.h
+++ b/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);

diff --git a/src/detect-file-data.c b/src/detect-file-data.c
index 65b93d644fd9c361a0849b700395dbc525d52621..0dc2d65553fb14f5d7eaf316c84476d3c7fa8a18 100644 (file)
--- a/src/detect-file-data.c
+++ b/src/detect-file-data.c
@@ -467,8 +467,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;
@@ -478,9 +477,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");
 }
@@ -493,9 +491,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) {
@@ -547,7 +544,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);

diff --git a/src/detect-filemagic.c b/src/detect-filemagic.c
index 2f3c23a3b431cd516680d42bb94b39feade5bb52..252c6eed28e5e66394b0a78b69502bbbeae2daee 100644 (file)
--- a/src/detect-filemagic.c
+++ b/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");
 }

diff --git a/src/detect-filename.c b/src/detect-filename.c
index 2ed627f82faf12f7c37c7a9ce7d3b3f4a44c8050..cfc895a14964514757d9bf5c01e9e3992b648262 100644 (file)
--- a/src/detect-filename.c
+++ b/src/detect-filename.c
@@ -368,8 +368,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)
@@ -378,8 +377,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");
 }

diff --git a/src/detect-http2.c b/src/detect-http2.c
index 11846af5bc06ac7460a987d9fc43f27da8fc6461..d7bde85189a2624b4f0a742078db50bf43def074 100644 (file)
--- a/src/detect-http2.c
+++ b/src/detect-http2.c
@@ -693,8 +693,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)
@@ -827,8 +827,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)
@@ -842,8 +842,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");
 }

diff --git a/src/detect-krb5-cname.c b/src/detect-krb5-cname.c
index 3a71aaf5b476858fa79f29780d435b8af7b082df..9ae593a8f234e86f995829164f9d40400f1bd683 100644 (file)
--- a/src/detect-krb5-cname.c
+++ b/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");
 }

diff --git a/src/detect-krb5-sname.c b/src/detect-krb5-sname.c
index 0ae6cf209798ce25a6c7915d956daee96d4c0898..6adb73695f77c1e39ab0d55f11d6cb0206284ab0 100644 (file)
--- a/src/detect-krb5-sname.c
+++ b/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");
 }

diff --git a/src/detect-mqtt-subscribe-topic.c b/src/detect-mqtt-subscribe-topic.c
index 7132cad2bd52149f037c08c647ed6ad0a014078e..32d7b1608354ffaa1ca7b16b9f3ed6f2270d8336 100644 (file)
--- a/src/detect-mqtt-subscribe-topic.c
+++ b/src/detect-mqtt-subscribe-topic.c
@@ -71,8 +71,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)
@@ -84,8 +84,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");
 }
@@ -247,4 +246,4 @@ static int DetectMQTTSubscribeTopicSetup(DetectEngineCtx *de_ctx, Signature *s,
     if (DetectSignatureSetAppProto(s, ALPROTO_MQTT) < 0)
         return -1;
     return 0;
-}
\ No newline at end of file
+}

diff --git a/src/detect-mqtt-unsubscribe-topic.c b/src/detect-mqtt-unsubscribe-topic.c
index dc69a4652ae288b4dc190ea0dfd4809dea917e50..671cfebaf4e60db555bcbca557ce86ed54f77415 100644 (file)
--- a/src/detect-mqtt-unsubscribe-topic.c
+++ b/src/detect-mqtt-unsubscribe-topic.c
@@ -71,8 +71,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)
@@ -84,8 +84,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");
 }
@@ -247,4 +246,4 @@ static int DetectMQTTUnsubscribeTopicSetup(DetectEngineCtx *de_ctx, Signature *s
     if (DetectSignatureSetAppProto(s, ALPROTO_MQTT) < 0)
         return -1;
     return 0;
-}
\ No newline at end of file
+}

diff --git a/src/detect-tls-certs.c b/src/detect-tls-certs.c
index 92fb08840778511a28e214f4c4d07be822bee3ea..8e8fd8c0f73c864aa47e9981a8abcf13ab30a6d2 100644 (file)
--- a/src/detect-tls-certs.c
+++ b/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");
 }

diff --git a/src/detect.h b/src/detect.h
index 6d510b7f51c05c24244f0d50a25a7606278123e6..80958169466aa84ec27b45a667b1124a7222e72b 100644 (file)
--- a/src/detect.h
+++ b/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 */