From 8545ef2e568554ca1364d55e18992d0e83bd6da8 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Fri, 18 Apr 2025 16:13:27 +0200 Subject: [PATCH] detect: factorize code for DetectSetupDirection Ticket: 7665 Instead of each keyword calling DetectSetupDirection, use a new flag SIGMATCH_SUPPORT_DIR so that DetectSetupDirection gets called, before parsing the rest of the keyword. Allows to support filesize keyword in transactional signatures --- src/detect-file-data.c | 9 +- src/detect-filemagic.c | 7 +- src/detect-filename.c | 7 +- src/detect-filesize.c | 1 + src/detect-http-headers-stub.h | 11 +- src/detect-parse.c | 194 ++++++++++++++++++++++++++------- src/detect-parse.h | 1 - src/detect.h | 2 + 8 files changed, 167 insertions(+), 65 deletions(-) diff --git a/src/detect-file-data.c b/src/detect-file-data.c index fb79b2bf50..f55b59ddf2 100644 --- a/src/detect-file-data.c +++ b/src/detect-file-data.c @@ -79,7 +79,7 @@ void DetectFiledataRegister(void) #ifdef UNITTESTS sigmatch_table[DETECT_FILE_DATA].RegisterTests = DetectFiledataRegisterTests; #endif - sigmatch_table[DETECT_FILE_DATA].flags = SIGMATCH_OPTIONAL_OPT; + sigmatch_table[DETECT_FILE_DATA].flags = SIGMATCH_OPTIONAL_OPT | SIGMATCH_SUPPORT_DIR; filehandler_table[DETECT_FILE_DATA].name = "file_data"; filehandler_table[DETECT_FILE_DATA].priority = 2; @@ -141,11 +141,6 @@ static int DetectFiledataSetup (DetectEngineCtx *de_ctx, Signature *s, const cha return -1; } - if (DetectSetupDirection(s, str) < 0) { - SCLogError("file.data failed to setup direction"); - return -1; - } - if (s->alproto == ALPROTO_SMTP && (s->init_data->init_flags & SIG_FLAG_INIT_FLOW) && !(s->flags & SIG_FLAG_TOSERVER) && (s->flags & SIG_FLAG_TOCLIENT)) { SCLogError("The 'file-data' keyword cannot be used with SMTP flow:to_client or " @@ -167,8 +162,6 @@ static int DetectFiledataSetup (DetectEngineCtx *de_ctx, Signature *s, const cha } s->init_data->init_flags |= SIG_FLAG_INIT_TXDIR_STREAMING_TOSERVER; } - s->init_data->init_flags &= ~SIG_FLAG_INIT_FORCE_TOSERVER; - s->init_data->init_flags &= ~SIG_FLAG_INIT_FORCE_TOCLIENT; SetupDetectEngineConfig(de_ctx); return 0; diff --git a/src/detect-filemagic.c b/src/detect-filemagic.c index 26ce0cc7cf..2a33be7570 100644 --- a/src/detect-filemagic.c +++ b/src/detect-filemagic.c @@ -114,7 +114,8 @@ void DetectFilemagicRegister(void) sigmatch_table[DETECT_FILE_MAGIC].desc = "sticky buffer to match on the file magic"; sigmatch_table[DETECT_FILE_MAGIC].url = "/rules/file-keywords.html#filemagic"; sigmatch_table[DETECT_FILE_MAGIC].Setup = DetectFilemagicSetupSticky; - sigmatch_table[DETECT_FILE_MAGIC].flags = SIGMATCH_OPTIONAL_OPT | SIGMATCH_INFO_STICKY_BUFFER; + sigmatch_table[DETECT_FILE_MAGIC].flags = + SIGMATCH_OPTIONAL_OPT | SIGMATCH_INFO_STICKY_BUFFER | SIGMATCH_SUPPORT_DIR; filehandler_table[DETECT_FILE_MAGIC].name = "file.magic", filehandler_table[DETECT_FILE_MAGIC].priority = 2; @@ -250,10 +251,6 @@ static int DetectFilemagicSetup (DetectEngineCtx *de_ctx, Signature *s, const ch */ static int DetectFilemagicSetupSticky(DetectEngineCtx *de_ctx, Signature *s, const char *str) { - if (DetectSetupDirection(s, str) < 0) { - SCLogError("file.magic failed to setup direction"); - return -1; - } if (SCDetectBufferSetActiveList(de_ctx, s, g_file_magic_buffer_id) < 0) return -1; diff --git a/src/detect-filename.c b/src/detect-filename.c index bf23e23916..5a4321c198 100644 --- a/src/detect-filename.c +++ b/src/detect-filename.c @@ -100,7 +100,8 @@ void DetectFilenameRegister(void) sigmatch_table[DETECT_FILE_NAME].desc = "sticky buffer to match on the file name"; sigmatch_table[DETECT_FILE_NAME].url = "/rules/file-keywords.html#filename"; sigmatch_table[DETECT_FILE_NAME].Setup = DetectFilenameSetupSticky; - sigmatch_table[DETECT_FILE_NAME].flags = SIGMATCH_OPTIONAL_OPT | SIGMATCH_INFO_STICKY_BUFFER; + sigmatch_table[DETECT_FILE_NAME].flags = + SIGMATCH_OPTIONAL_OPT | SIGMATCH_INFO_STICKY_BUFFER | SIGMATCH_SUPPORT_DIR; DetectBufferTypeSetDescriptionByName("file.name", "file name"); @@ -208,10 +209,6 @@ static int DetectFilenameSetup (DetectEngineCtx *de_ctx, Signature *s, const cha */ static int DetectFilenameSetupSticky(DetectEngineCtx *de_ctx, Signature *s, const char *str) { - if (DetectSetupDirection(s, str) < 0) { - SCLogError("file.name failed to setup direction"); - return -1; - } if (SCDetectBufferSetActiveList(de_ctx, s, g_file_name_buffer_id) < 0) return -1; s->file_flags |= (FILE_SIG_NEED_FILE | FILE_SIG_NEED_FILENAME); diff --git a/src/detect-filesize.c b/src/detect-filesize.c index e0b430bbfd..8128b2f634 100644 --- a/src/detect-filesize.c +++ b/src/detect-filesize.c @@ -66,6 +66,7 @@ void DetectFilesizeRegister(void) sigmatch_table[DETECT_FILESIZE].FileMatch = DetectFilesizeMatch; sigmatch_table[DETECT_FILESIZE].Setup = DetectFilesizeSetup; sigmatch_table[DETECT_FILESIZE].Free = DetectFilesizeFree; + sigmatch_table[DETECT_FILESIZE].flags = SIGMATCH_SUPPORT_DIR; #ifdef UNITTESTS sigmatch_table[DETECT_FILESIZE].RegisterTests = DetectFilesizeRegisterTests; #endif diff --git a/src/detect-http-headers-stub.h b/src/detect-http-headers-stub.h index ce95371c41..6a3828f437 100644 --- a/src/detect-http-headers-stub.h +++ b/src/detect-http-headers-stub.h @@ -162,17 +162,9 @@ static InspectionBuffer *GetResponseData2(DetectEngineThreadCtx *det_ctx, */ static int DetectHttpHeadersSetupSticky(DetectEngineCtx *de_ctx, Signature *s, const char *str) { - if (DetectSetupDirection(s, str) < 0) { - SCLogError(KEYWORD_NAME " failed to setup direction"); - return -1; - } - if (SCDetectBufferSetActiveList(de_ctx, s, g_buffer_id) < 0) return -1; - s->init_data->init_flags &= ~SIG_FLAG_INIT_FORCE_TOSERVER; - s->init_data->init_flags &= ~SIG_FLAG_INIT_FORCE_TOCLIENT; - if (DetectSignatureSetAppProto(s, ALPROTO_HTTP) < 0) return -1; @@ -189,7 +181,8 @@ static void DetectHttpHeadersRegisterStub(void) sigmatch_table[KEYWORD_ID].url = "/rules/" KEYWORD_DOC; sigmatch_table[KEYWORD_ID].Setup = DetectHttpHeadersSetupSticky; #if defined(KEYWORD_TOSERVER) && defined(KEYWORD_TOSERVER) - sigmatch_table[KEYWORD_ID].flags |= SIGMATCH_OPTIONAL_OPT | SIGMATCH_INFO_STICKY_BUFFER; + sigmatch_table[KEYWORD_ID].flags |= + SIGMATCH_OPTIONAL_OPT | SIGMATCH_INFO_STICKY_BUFFER | SIGMATCH_SUPPORT_DIR; #else sigmatch_table[KEYWORD_ID].flags |= SIGMATCH_NOOPT | SIGMATCH_INFO_STICKY_BUFFER; #endif diff --git a/src/detect-parse.c b/src/detect-parse.c index 01c5efd64a..0263ea3d5e 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -530,6 +530,12 @@ SigMatch *SigMatchAppendSMToList( /* buffer set up by sigmatch is tracked in case we add a stickybuffer for the * same list. */ s->init_data->curbuf->sm_init = true; + if (s->init_data->init_flags & SIG_FLAG_INIT_FORCE_TOCLIENT) { + s->init_data->curbuf->only_tc = true; + } + if (s->init_data->init_flags & SIG_FLAG_INIT_FORCE_TOSERVER) { + s->init_data->curbuf->only_ts = true; + } SCLogDebug("s->init_data->buffer_index %u", s->init_data->buffer_index); } } @@ -858,6 +864,87 @@ int SigMatchListSMBelongsTo(const Signature *s, const SigMatch *key_sm) return -1; } +/** + * \brief Parse and setup a direction + * + * \param s signature + * \param str argument to the keyword + * \param only_dir argument wether the keyword only accepts a direction + * + * \retval 0 on success, -1 on failure + */ +static int DetectSetupDirection(Signature *s, char **str, bool only_dir) +{ + char *orig = *str; + if (strncmp(*str, "to_client", strlen("to_client")) == 0) { + *str += strlen("to_client"); + // skip space + while (**str && isblank(**str)) { + (*str)++; + } + // check comma or nothing + if (**str) { + if (only_dir) { + SCLogError("unknown option: only accepts to_server or to_client"); + return -1; + } + if (**str != ',') { + // leave to_client_something for next parser if not only_dir + *str = orig; + return 0; + } else { + (*str)++; + } + while (**str && isblank(**str)) { + (*str)++; + } + } + s->init_data->init_flags |= SIG_FLAG_INIT_FORCE_TOCLIENT; + if ((s->flags & SIG_FLAG_TXBOTHDIR) == 0) { + if (s->flags & SIG_FLAG_TOSERVER) { + SCLogError("contradictory directions"); + return -1; + } + s->flags |= SIG_FLAG_TOCLIENT; + } + } else if (strncmp(*str, "to_server", strlen("to_server")) == 0) { + *str += strlen("to_server"); + // skip space + while (**str && isblank(**str)) { + (*str)++; + } + // check comma or nothing + if (**str) { + if (only_dir) { + SCLogError("unknown option: only accepts to_server or to_client"); + return -1; + } + if (**str != ',') { + // leave to_client_something for next parser if not only_dir + *str = orig; + return 0; + } else { + (*str)++; + } + while (**str && isblank(**str)) { + (*str)++; + } + } + s->init_data->init_flags |= SIG_FLAG_INIT_FORCE_TOSERVER; + if ((s->flags & SIG_FLAG_TXBOTHDIR) == 0) { + if (s->flags & SIG_FLAG_TOCLIENT) { + SCLogError("contradictory directions"); + return -1; + } + s->flags |= SIG_FLAG_TOSERVER; + } + } else if (only_dir) { + SCLogError("unknown option: only accepts to_server or to_client"); + return -1; + } + return 0; +} + static int SigParseOptions(DetectEngineCtx *de_ctx, Signature *s, char *optstr, char *output, size_t output_size, bool requires) { @@ -1045,7 +1132,15 @@ static int SigParseOptions(DetectEngineCtx *de_ctx, Signature *s, char *optstr, } } /* setup may or may not add a new SigMatch to the list */ + if (st->flags & SIGMATCH_SUPPORT_DIR) { + if (DetectSetupDirection(s, &ptr, st->flags & SIGMATCH_OPTIONAL_OPT) < 0) { + SCLogError("%s failed to setup direction", st->name); + goto error; + } + } setup_ret = st->Setup(de_ctx, s, ptr); + s->init_data->init_flags &= ~SIG_FLAG_INIT_FORCE_TOSERVER; + s->init_data->init_flags &= ~SIG_FLAG_INIT_FORCE_TOCLIENT; } else { /* setup may or may not add a new SigMatch to the list */ setup_ret = st->Setup(de_ctx, s, NULL); @@ -3528,43 +3623,6 @@ void DetectSetupParseRegexes(const char *parse_str, DetectParseRegex *detect_par } } -/** - * \brief Parse and setup a direction - * - * \param s siganture - * \param str argument to the keyword - * - * \retval 0 on success, -1 on failure - */ -int DetectSetupDirection(Signature *s, const char *str) -{ - if (str) { - if (strcmp(str, "to_client") == 0) { - s->init_data->init_flags |= SIG_FLAG_INIT_FORCE_TOCLIENT; - if ((s->flags & SIG_FLAG_TXBOTHDIR) == 0) { - if (s->flags & SIG_FLAG_TOSERVER) { - SCLogError("contradictory directions"); - return -1; - } - s->flags |= SIG_FLAG_TOCLIENT; - } - } else if (strcmp(str, "to_server") == 0) { - s->init_data->init_flags |= SIG_FLAG_INIT_FORCE_TOSERVER; - if ((s->flags & SIG_FLAG_TXBOTHDIR) == 0) { - if (s->flags & SIG_FLAG_TOCLIENT) { - SCLogError("contradictory directions"); - return -1; - } - s->flags |= SIG_FLAG_TOSERVER; - } - } else { - SCLogError("unknown option: only accepts to_server or to_client"); - return -1; - } - } - return 0; -} - /* * TESTS */ @@ -5303,6 +5361,63 @@ static int SigSetMultiAppProto(void) PASS; } +static int DetectSetupDirection01(void) +{ + Signature *s = SigAlloc(); + FAIL_IF_NULL(s); + // Basic case : ok + char *str = (char *)"to_client"; + FAIL_IF(DetectSetupDirection(s, &str, true) < 0); + SigFree(NULL, s); + PASS; +} + +static int DetectSetupDirection02(void) +{ + Signature *s = SigAlloc(); + FAIL_IF_NULL(s); + char *str = (char *)"to_server"; + FAIL_IF(DetectSetupDirection(s, &str, true) < 0); + // ok so far + str = (char *)"to_client"; + FAIL_IF(DetectSetupDirection(s, &str, true) >= 0); + // fails because we cannot have both to_client and to_server for same signature + SigFree(NULL, s); + PASS; +} + +static int DetectSetupDirection03(void) +{ + Signature *s = SigAlloc(); + FAIL_IF_NULL(s); + char *str = (char *)"to_client , something"; + FAIL_IF(DetectSetupDirection(s, &str, false) < 0); + FAIL_IF(strcmp(str, "something") != 0); + str = (char *)"to_client,something"; + FAIL_IF(DetectSetupDirection(s, &str, false) < 0); + FAIL_IF(strcmp(str, "something") != 0); + SigFree(NULL, s); + PASS; +} + +static int DetectSetupDirection04(void) +{ + Signature *s = SigAlloc(); + FAIL_IF_NULL(s); + // invalid case + char *str = (char *)"to_client_toto"; + FAIL_IF(DetectSetupDirection(s, &str, true) >= 0); + // test we do not change the string pointer if only_dir is false + str = (char *)"to_client_toto"; + FAIL_IF(DetectSetupDirection(s, &str, false) < 0); + FAIL_IF(strcmp(str, "to_client_toto") != 0); + str = (char *)"to_client,something"; + // fails because we call with only_dir=true + FAIL_IF(DetectSetupDirection(s, &str, true) >= 0); + SigFree(NULL, s); + PASS; +} + #endif /* UNITTESTS */ #ifdef UNITTESTS @@ -5381,5 +5496,10 @@ void SigParseRegisterTests(void) UtRegisterTest("SigSetMultiAppProto", SigSetMultiAppProto); + UtRegisterTest("DetectSetupDirection01", DetectSetupDirection01); + UtRegisterTest("DetectSetupDirection02", DetectSetupDirection02); + UtRegisterTest("DetectSetupDirection03", DetectSetupDirection03); + UtRegisterTest("DetectSetupDirection04", DetectSetupDirection04); + #endif /* UNITTESTS */ } diff --git a/src/detect-parse.h b/src/detect-parse.h index 18c998f3e2..ddc0b11984 100644 --- a/src/detect-parse.h +++ b/src/detect-parse.h @@ -122,7 +122,6 @@ int SC_Pcre2SubstringCopy( int SC_Pcre2SubstringGet(pcre2_match_data *match_data, uint32_t number, PCRE2_UCHAR **bufferptr, PCRE2_SIZE *bufflen); -int DetectSetupDirection(Signature *s, const char *str); void DetectRegisterAppLayerHookLists(void); #endif /* SURICATA_DETECT_PARSE_H */ diff --git a/src/detect.h b/src/detect.h index 94ae258b4b..148245442a 100644 --- a/src/detect.h +++ b/src/detect.h @@ -1649,6 +1649,8 @@ typedef struct SigGroupHead_ { #define SIGMATCH_STRICT_PARSING BIT_U16(11) /** keyword supported by firewall rules */ #define SIGMATCH_SUPPORT_FIREWALL BIT_U16(12) +/** keyword supporting setting an optional direction */ +#define SIGMATCH_SUPPORT_DIR BIT_U16(13) enum DetectEngineTenantSelectors { -- 2.47.2