]> git.ipfire.org Git - thirdparty/suricata.git/commitdiff
detect: fix multi inspect buffer issue; clean up
authorVictor Julien <victor@inliniac.net>
Thu, 13 May 2021 05:50:12 +0000 (07:50 +0200)
committerVictor Julien <victor@inliniac.net>
Tue, 15 Jun 2021 09:25:24 +0000 (11:25 +0200)
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.

14 files changed:
src/detect-dns-query.c
src/detect-engine.c
src/detect-engine.h
src/detect-file-data.c
src/detect-filemagic.c
src/detect-filename.c
src/detect-http2.c
src/detect-ike-vendor.c
src/detect-krb5-cname.c
src/detect-krb5-sname.c
src/detect-mqtt-subscribe-topic.c
src/detect-mqtt-unsubscribe-topic.c
src/detect-tls-certs.c
src/detect.h

index d3c941d792975ba9bc85c18d488feec5665bb595..36a7a996b16e03793ba339100034a999666280a8 100644 (file)
@@ -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");
 }
index d1402194c911512caab9c437faa8ac367f664b7f..00f3242ce3326a720ed31ceff3d285ea5454f5fc 100644 (file)
@@ -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)
index 8f6ef4aaead768e68cf1fa4356f4029ea65585a0..d4047e44dbb59f2a0ec312fdde5b7121ba899a8a 100644 (file)
@@ -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);
index 49c297ab80fe35cb84074e5c924da95eebe505a4..4ec99bd968cadba2606034a5a2145edd65004c0c 100644 (file)
@@ -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);
index 806c391fad9976efa480b67eb532a4b3cfeae311..49b0d125fd96388b32f46c7bfcc8174cfc115e84 100644 (file)
@@ -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");
 }
index f990f0dcda78e78e84095968b603396ad5b47260..a5ac0b2ae054314f3cce559497f3e41fcd944a4a 100644 (file)
@@ -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");
 }
index 4e89214d673c039e1167afa0b6a2d0ba8c9e6c67..917230bba102431fe62c93e24cc1cf5501a3c600 100644 (file)
@@ -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");
 }
index 800ab0d5363acb5b0dbf6c4b679f0267bb6a07cb..c336f8c594cad04a460eb7eec61f4b6e56ace76c 100644 (file)
@@ -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");
 }
index e7ca0fe84a898be83e50c0a93867fccc80ce5b08..7e90d679c2a09c259f9f77a972d5a3510f3f0ead 100644 (file)
@@ -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");
 }
index fd33bb49bfbbcc9c7db13406c7e5a5b5bf549555..a791c658d4158c4af62f8c9fc8291775c7a61607 100644 (file)
@@ -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");
 }
index 57ab2c4c03d612954415bc0b0a9b821ea1009537..0c035f2c83d814e4c762db699fcf180d5286853e 100644 (file)
@@ -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;
-}
\ No newline at end of file
+}
index 307f843ea41e5bea17b03248231efed9dc684aad..d8ae6a7b511022bd9e98e4ffc7cb700e92dedb48 100644 (file)
@@ -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;
-}
\ No newline at end of file
+}
index 5eb7be2898af7a8081c89e60889f30707da84dee..08f0a706c60cf40ff2cc36c2e4a1267f45ebef37 100644 (file)
@@ -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");
 }
index 51afd7cd70ed71d59fec3d88ca5b8e1beee1aa17..9036243759605305b12a18fe1e0ba89a39706424 100644 (file)
@@ -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 */