From 9aec2307f56db2a34441ce6525ce999af3885fc2 Mon Sep 17 00:00:00 2001 From: Jeff Lucovsky Date: Tue, 17 Jun 2025 10:17:12 -0400 Subject: [PATCH 1/2] detect/var: Restrict var usage to single buffer in strict mode Issue: 1412 When in strict mode, issue an error and refuse to load the rule if variables produced from a different buffer are used with a separate buffer. When not in strict mode (default), issue a warning and load the rule. Only consider sig matches with compatible ids/lists. --- src/detect-byte-extract.c | 33 +++++++++++++++------------ src/detect-byte-extract.h | 2 +- src/detect-byte.c | 47 +++++++++++++++++++++++++++++++++------ src/detect-byte.h | 4 ++-- src/detect-bytejump.c | 6 +++-- src/detect-bytemath.c | 42 +++++++++++++++++++--------------- src/detect-bytemath.h | 2 +- src/detect-bytetest.c | 13 ++++++----- src/detect-depth.c | 3 ++- src/detect-distance.c | 3 ++- src/detect-isdataat.c | 3 ++- src/detect-offset.c | 3 ++- src/detect-within.c | 3 ++- src/util-lua-bytevarlib.c | 2 +- 14 files changed, 110 insertions(+), 56 deletions(-) diff --git a/src/detect-byte-extract.c b/src/detect-byte-extract.c index 3b691373261e..76951ab49b55 100644 --- a/src/detect-byte-extract.c +++ b/src/detect-byte-extract.c @@ -26,6 +26,7 @@ #include "detect.h" #include "detect-engine.h" +#include "detect-byte.h" #include "detect-content.h" #include "detect-pcre.h" #include "detect-bytejump.h" @@ -364,6 +365,15 @@ static void DetectByteExtractFree(DetectEngineCtx *de_ctx, void *ptr) SCByteExtractFree(ptr); } +static inline bool DetectByteExtractSMNameMatch(const SigMatch *sm, const char *arg) +{ + if (sm->type == DETECT_BYTE_EXTRACT) { + const SCDetectByteExtractData *bed = (const SCDetectByteExtractData *)sm->ctx; + return strcmp(bed->name, arg) == 0; + } + return false; +} + /** * \brief Lookup the SigMatch for a named byte_extract variable. * @@ -372,30 +382,25 @@ static void DetectByteExtractFree(DetectEngineCtx *de_ctx, void *ptr) * * \retval A pointer to the SigMatch if found, otherwise NULL. */ -SigMatch *DetectByteExtractRetrieveSMVar(const char *arg, int sm_list, const Signature *s) +const SigMatch *DetectByteExtractRetrieveSMVar(const char *arg, int *found_list, const Signature *s) { for (uint32_t x = 0; x < s->init_data->buffer_index; x++) { - SigMatch *sm = s->init_data->buffers[x].head; + const SigMatch *sm = s->init_data->buffers[x].head; while (sm != NULL) { - if (sm->type == DETECT_BYTE_EXTRACT) { - const SCDetectByteExtractData *bed = (const SCDetectByteExtractData *)sm->ctx; - if (strcmp(bed->name, arg) == 0) { - return sm; - } + if (DetectByteExtractSMNameMatch(sm, arg)) { + *found_list = s->init_data->buffers[x].id; + return sm; } sm = sm->next; } } for (int list = 0; list < DETECT_SM_LIST_MAX; list++) { - SigMatch *sm = s->init_data->smlists[list]; + const SigMatch *sm = s->init_data->smlists[list]; while (sm != NULL) { - // Make sure that the linked buffers ore on the same list - if (sm->type == DETECT_BYTE_EXTRACT && (sm_list == -1 || sm_list == list)) { - const SCDetectByteExtractData *bed = (const SCDetectByteExtractData *)sm->ctx; - if (strcmp(bed->name, arg) == 0) { - return sm; - } + if (DetectByteExtractSMNameMatch(sm, arg)) { + *found_list = list; + return sm; } sm = sm->next; } diff --git a/src/detect-byte-extract.h b/src/detect-byte-extract.h index 2214e81bbb87..9a69bfec01fa 100644 --- a/src/detect-byte-extract.h +++ b/src/detect-byte-extract.h @@ -26,7 +26,7 @@ void DetectByteExtractRegister(void); -SigMatch *DetectByteExtractRetrieveSMVar(const char *, int sm_list, const Signature *); +const SigMatch *DetectByteExtractRetrieveSMVar(const char *, int *found_list, const Signature *); int DetectByteExtractDoMatch(DetectEngineThreadCtx *, const SigMatchData *, const Signature *, const uint8_t *, uint32_t, uint64_t *, uint8_t); diff --git a/src/detect-byte.c b/src/detect-byte.c index 3f8735b11979..6e4561866b93 100644 --- a/src/detect-byte.c +++ b/src/detect-byte.c @@ -1,4 +1,4 @@ -/* Copyright (C) 2020 Open Information Security Foundation +/* Copyright (C) 2020-2025 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 @@ -27,30 +27,63 @@ #include "detect-byte-extract.h" #include "detect-bytemath.h" +// Once a variable is found, apply strict semantics (error) else warning +static bool DetectByteListMismatch(bool any, int sm_list, int found_list, bool strict, + const char *arg, const DetectEngineCtx *de_ctx) +{ + if (any || sm_list == found_list) + return false; + + if (strict) { + return true; + } + + if (de_ctx) { + SCLogWarning("Using byte variable from a different buffer may produce indeterminate " + "results; variable: \"%s\" at line %" PRId32 " from file %s; see issue #1412", + arg, de_ctx->rule_line, de_ctx->rule_file); + } + return false; +} + /** * \brief Used to retrieve args from BM. * * \param arg The name of the variable being sought * \param s The signature to check for the variable + * \param strict Match if and only iff the list sought and the list found equal. * \param sm_list The caller's matching buffer * \param index When found, the value of the slot within the byte vars * * \retval true A match for the variable was found. * \retval false */ -bool DetectByteRetrieveSMVar( - const char *arg, const Signature *s, int sm_list, DetectByteIndexType *index) +bool DetectByteRetrieveSMVar(const char *arg, const Signature *s, bool strict, int sm_list, + DetectByteIndexType *index, const DetectEngineCtx *de_ctx) { - SigMatch *bed_sm = DetectByteExtractRetrieveSMVar(arg, sm_list, s); + bool any = sm_list == -1; + int found_list; + const SigMatch *bed_sm = DetectByteExtractRetrieveSMVar(arg, &found_list, s); if (bed_sm != NULL) { + if (DetectByteListMismatch(any, sm_list, found_list, strict, arg, de_ctx)) { + return false; + } + + SCLogDebug("[buf] found %s; list wanted: sm_list: %d, list found: %d", arg, sm_list, + found_list); *index = ((SCDetectByteExtractData *)bed_sm->ctx)->local_id; return true; } - SigMatch *bmd_sm = DetectByteMathRetrieveSMVar(arg, sm_list, s); + const SigMatch *bmd_sm = DetectByteMathRetrieveSMVar(arg, &found_list, s); if (bmd_sm != NULL) { - *index = ((DetectByteMathData *)bmd_sm->ctx)->local_id; - return true; + if (!DetectByteListMismatch(any, sm_list, found_list, strict, arg, de_ctx)) { + *index = ((DetectByteMathData *)bmd_sm->ctx)->local_id; + SCLogDebug("[list] found %s; list wanted: sm_list: %d, list found: %d", arg, sm_list, + found_list); + return true; + } } + return false; } diff --git a/src/detect-byte.h b/src/detect-byte.h index fd251885120f..d73a6b4edada 100644 --- a/src/detect-byte.h +++ b/src/detect-byte.h @@ -27,6 +27,6 @@ typedef uint8_t DetectByteIndexType; -bool DetectByteRetrieveSMVar(const char *, const Signature *, int sm_list, DetectByteIndexType *); - +bool DetectByteRetrieveSMVar(const char *, const Signature *, bool strict, int sm_list, + DetectByteIndexType *, const DetectEngineCtx *); #endif /* SURICATA_DETECT_BYTE_H */ diff --git a/src/detect-bytejump.c b/src/detect-bytejump.c index 6b0363f0985f..c1fb2863dc89 100644 --- a/src/detect-bytejump.c +++ b/src/detect-bytejump.c @@ -544,7 +544,8 @@ static int DetectBytejumpSetup(DetectEngineCtx *de_ctx, Signature *s, const char if (nbytes != NULL) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(nbytes, s, sm_list, &index)) { + if (!DetectByteRetrieveSMVar( + nbytes, s, SigMatchStrictEnabled(DETECT_BYTEJUMP), sm_list, &index, de_ctx)) { SCLogError("Unknown byte_extract var " "seen in byte_jump - %s", nbytes); @@ -557,7 +558,8 @@ static int DetectBytejumpSetup(DetectEngineCtx *de_ctx, Signature *s, const char if (offset != NULL) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(offset, s, sm_list, &index)) { + if (!DetectByteRetrieveSMVar( + offset, s, SigMatchStrictEnabled(DETECT_BYTEJUMP), sm_list, &index, de_ctx)) { SCLogError("Unknown byte_extract var " "seen in byte_jump - %s", offset); diff --git a/src/detect-bytemath.c b/src/detect-bytemath.c index fe88d69e4aef..694d032e7c41 100644 --- a/src/detect-bytemath.c +++ b/src/detect-bytemath.c @@ -360,7 +360,8 @@ static int DetectByteMathSetup(DetectEngineCtx *de_ctx, Signature *s, const char if (nbytes != NULL) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(nbytes, s, sm_list, &index)) { + if (!DetectByteRetrieveSMVar( + nbytes, s, SigMatchStrictEnabled(DETECT_BYTEMATH), sm_list, &index, de_ctx)) { SCLogError("unknown byte_ keyword var seen in byte_math - %s", nbytes); goto error; } @@ -372,7 +373,8 @@ static int DetectByteMathSetup(DetectEngineCtx *de_ctx, Signature *s, const char if (rvalue != NULL) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(rvalue, s, sm_list, &index)) { + if (!DetectByteRetrieveSMVar( + rvalue, s, SigMatchStrictEnabled(DETECT_BYTEMATH), sm_list, &index, de_ctx)) { SCLogError("unknown byte_ keyword var seen in byte_math - %s", rvalue); goto error; } @@ -434,6 +436,15 @@ static void DetectByteMathFree(DetectEngineCtx *de_ctx, void *ptr) SCByteMathFree(ptr); } +static inline bool DetectByteMathSMNameMatch(const SigMatch *sm, const char *arg) +{ + if (sm->type == DETECT_BYTEMATH) { + const DetectByteMathData *bmd = (const DetectByteMathData *)sm->ctx; + return strcmp(bmd->result, arg) == 0; + } + return false; +} + /** * \brief Lookup the SigMatch for a named byte_math variable. * @@ -442,32 +453,27 @@ static void DetectByteMathFree(DetectEngineCtx *de_ctx, void *ptr) * * \retval A pointer to the SigMatch if found, otherwise NULL. */ -SigMatch *DetectByteMathRetrieveSMVar(const char *arg, int sm_list, const Signature *s) +const SigMatch *DetectByteMathRetrieveSMVar(const char *arg, int *found_list, const Signature *s) { for (uint32_t x = 0; x < s->init_data->buffer_index; x++) { - SigMatch *sm = s->init_data->buffers[x].head; + const SigMatch *sm = s->init_data->buffers[x].head; while (sm != NULL) { - if (sm->type == DETECT_BYTEMATH) { - const DetectByteMathData *bmd = (const DetectByteMathData *)sm->ctx; - if (strcmp(bmd->result, arg) == 0) { - SCLogDebug("Retrieved SM for \"%s\"", arg); - return sm; - } + if (DetectByteMathSMNameMatch(sm, arg)) { + SCLogDebug("Retrieved SM for \"%s\"", arg); + *found_list = s->init_data->buffers[x].id; + return sm; } sm = sm->next; } } for (int list = 0; list < DETECT_SM_LIST_MAX; list++) { - SigMatch *sm = s->init_data->smlists[list]; + const SigMatch *sm = s->init_data->smlists[list]; while (sm != NULL) { - // Make sure that the linked buffers ore on the same list - if (sm->type == DETECT_BYTEMATH && (sm_list == -1 || sm_list == list)) { - const DetectByteMathData *bmd = (const DetectByteMathData *)sm->ctx; - if (strcmp(bmd->result, arg) == 0) { - SCLogDebug("Retrieved SM for \"%s\"", arg); - return sm; - } + if (DetectByteMathSMNameMatch(sm, arg)) { + SCLogDebug("Retrieved SM for \"%s\"", arg); + *found_list = list; + return sm; } sm = sm->next; } diff --git a/src/detect-bytemath.h b/src/detect-bytemath.h index 85ac9077eed9..5cce444746af 100644 --- a/src/detect-bytemath.h +++ b/src/detect-bytemath.h @@ -26,7 +26,7 @@ void DetectBytemathRegister(void); -SigMatch *DetectByteMathRetrieveSMVar(const char *, int sm_list, const Signature *); +const SigMatch *DetectByteMathRetrieveSMVar(const char *, int *found_list, const Signature *); int DetectByteMathDoMatch(DetectEngineThreadCtx *, const DetectByteMathData *, const Signature *, const uint8_t *, const uint32_t, uint8_t, uint64_t, uint64_t *, uint8_t); diff --git a/src/detect-bytetest.c b/src/detect-bytetest.c index e73b9bdd1b60..be095b023089 100644 --- a/src/detect-bytetest.c +++ b/src/detect-bytetest.c @@ -231,8 +231,7 @@ int DetectBytetestDoMatch(DetectEngineThreadCtx *det_ctx, const Signature *s, SCLogDebug("comparing base %d string 0x%" PRIx64 " %s%u 0x%" PRIx64, data->base, val, (neg ? "!" : ""), data->op, data->value); - } - else { + } else { int endianness = (flags & DETECT_BYTETEST_LITTLE) ? BYTE_LITTLE_ENDIAN : BYTE_BIG_ENDIAN; extbytes = ByteExtractUint64(&val, endianness, (uint16_t)nbytes, ptr); @@ -259,6 +258,7 @@ int DetectBytetestDoMatch(DetectEngineThreadCtx *det_ctx, const Signature *s, /* Compare using the configured operator */ match = 0; + SCLogDebug("val: %ld, value: %ld", val, value); switch (data->op) { case DETECT_BYTETEST_OP_EQ: if (val == value) { @@ -640,7 +640,8 @@ static int DetectBytetestSetup(DetectEngineCtx *de_ctx, Signature *s, const char if (value != NULL) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(value, s, sm_list, &index)) { + if (!DetectByteRetrieveSMVar( + value, s, SigMatchStrictEnabled(DETECT_BYTETEST), sm_list, &index, de_ctx)) { SCLogError("Unknown byte_extract var " "seen in byte_test - %s", value); @@ -654,7 +655,8 @@ static int DetectBytetestSetup(DetectEngineCtx *de_ctx, Signature *s, const char if (offset != NULL) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(offset, s, sm_list, &index)) { + if (!DetectByteRetrieveSMVar( + offset, s, SigMatchStrictEnabled(DETECT_BYTETEST), sm_list, &index, de_ctx)) { SCLogError("Unknown byte_extract var " "seen in byte_test - %s", offset); @@ -668,7 +670,8 @@ static int DetectBytetestSetup(DetectEngineCtx *de_ctx, Signature *s, const char if (nbytes != NULL) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(nbytes, s, sm_list, &index)) { + if (!DetectByteRetrieveSMVar( + nbytes, s, SigMatchStrictEnabled(DETECT_BYTETEST), sm_list, &index, de_ctx)) { SCLogError("Unknown byte_extract var " "seen in byte_test - %s", nbytes); diff --git a/src/detect-depth.c b/src/detect-depth.c index 7c0bf5bfe5f3..138653267cb7 100644 --- a/src/detect-depth.c +++ b/src/detect-depth.c @@ -106,7 +106,8 @@ static int DetectDepthSetup (DetectEngineCtx *de_ctx, Signature *s, const char * } if (str[0] != '-' && isalpha((unsigned char)str[0])) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(str, s, -1, &index)) { + if (!DetectByteRetrieveSMVar( + str, s, SigMatchStrictEnabled(DETECT_DEPTH), -1, &index, de_ctx)) { SCLogError("unknown byte_ keyword var " "seen in depth - %s.", str); diff --git a/src/detect-distance.c b/src/detect-distance.c index 43b48ba799cc..3fed2e54ac7f 100644 --- a/src/detect-distance.c +++ b/src/detect-distance.c @@ -108,7 +108,8 @@ static int DetectDistanceSetup (DetectEngineCtx *de_ctx, Signature *s, } if (str[0] != '-' && isalpha((unsigned char)str[0])) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(str, s, -1, &index)) { + if (!DetectByteRetrieveSMVar( + str, s, SigMatchStrictEnabled(DETECT_DISTANCE), -1, &index, de_ctx)) { SCLogError("unknown byte_ keyword var " "seen in distance - %s", str); diff --git a/src/detect-isdataat.c b/src/detect-isdataat.c index 7709568159b7..c5491a484389 100644 --- a/src/detect-isdataat.c +++ b/src/detect-isdataat.c @@ -347,7 +347,8 @@ int DetectIsdataatSetup (DetectEngineCtx *de_ctx, Signature *s, const char *isda if (offset != NULL) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(offset, s, -1, &index)) { + if (!DetectByteRetrieveSMVar( + offset, s, SigMatchStrictEnabled(DETECT_ISDATAAT), -1, &index, de_ctx)) { SCLogError("Unknown byte_extract var " "seen in isdataat - %s\n", offset); diff --git a/src/detect-offset.c b/src/detect-offset.c index 91d02b2882e3..3644a39a52e1 100644 --- a/src/detect-offset.c +++ b/src/detect-offset.c @@ -92,7 +92,8 @@ int DetectOffsetSetup (DetectEngineCtx *de_ctx, Signature *s, const char *offset } if (str[0] != '-' && isalpha((unsigned char)str[0])) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(str, s, -1, &index)) { + if (!DetectByteRetrieveSMVar( + str, s, SigMatchStrictEnabled(DETECT_OFFSET), -1, &index, de_ctx)) { SCLogError("unknown byte_ keyword var " "seen in offset - %s.", str); diff --git a/src/detect-within.c b/src/detect-within.c index f18a9347db8d..03908e552fac 100644 --- a/src/detect-within.c +++ b/src/detect-within.c @@ -104,7 +104,8 @@ static int DetectWithinSetup(DetectEngineCtx *de_ctx, Signature *s, const char * } if (str[0] != '-' && isalpha((unsigned char)str[0])) { DetectByteIndexType index; - if (!DetectByteRetrieveSMVar(str, s, -1, &index)) { + if (!DetectByteRetrieveSMVar( + str, s, SigMatchStrictEnabled(DETECT_WITHIN), -1, &index, de_ctx)) { SCLogError("unknown byte_ keyword var " "seen in within - %s", str); diff --git a/src/util-lua-bytevarlib.c b/src/util-lua-bytevarlib.c index bea9a4ce9481..02014c3f3c79 100644 --- a/src/util-lua-bytevarlib.c +++ b/src/util-lua-bytevarlib.c @@ -55,7 +55,7 @@ static int LuaBytevarMap(lua_State *L) } DetectByteIndexType idx; - if (!DetectByteRetrieveSMVar(name, s, -1, &idx)) { + if (!DetectByteRetrieveSMVar(name, s, false, -1, &idx, NULL)) { luaL_error(L, "unknown byte_extract or byte_math variable: %s", name); } From 17da193bb3d90c28ec10cabefb98aba7d0713988 Mon Sep 17 00:00:00 2001 From: Jeff Lucovsky Date: Fri, 20 Jun 2025 08:24:32 -0400 Subject: [PATCH 2/2] doc/upgrade: Highlight byte_ variable scope Issue 1412 Add mention of byte_{extract,math,test,jump} variable usage and buffer scope and include how the command line option strict-rule-keywords affects validation. --- doc/userguide/upgrade.rst | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/doc/userguide/upgrade.rst b/doc/userguide/upgrade.rst index c6697a7576c8..cf0e33665807 100644 --- a/doc/userguide/upgrade.rst +++ b/doc/userguide/upgrade.rst @@ -176,6 +176,13 @@ Major changes content that matched just because it fell in the inspection chunk without wholly belonging to any one request/response may not match any longer. +- If a byte keyword (such as ``byte_extract`` or ``byte_math``, etc) is used with + a variable, and that variable usage is with a buffer other than the one used + to create the variable, a warning is printed and the rule is loaded. The + command-line option ``--strict-rule-keywords`` should be used to produce + an error message and prevent the rule from loading. See + https://redmine.openinfosecfoundation.org/issues/1412 for more information. + Removals ~~~~~~~~ - The ssh keywords ``ssh.protoversion`` and ``ssh.softwareversion`` have been removed.