From 93e5c373a56a41fdf2f639ee654895995a92134a Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Fri, 4 Oct 2024 14:53:02 +0200 Subject: [PATCH 1/2] detect: clean support for multi-protocol keywords such as ja4. Why ? We do not want to see hard-coded protocol constants such as ALPROTO_QUIC directly used in generic code in detect-parse.c How ? From the keyword point of view, this commit adds the function DetectSignatureSetMultiAppProto which is similar to DetectSignatureSetAppProto but takes multiple alprotos. It restricts the signature alprotos to a set of possible alprotos and errors out if the interstion gets empty. The data structure SignatureInitData gets extended with a fixed-length array, as the use case is a sparse number of protocols Ticket: 7304 --- src/detect-parse.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++ src/detect-parse.h | 1 + src/detect.h | 5 +++ 3 files changed, 99 insertions(+) diff --git a/src/detect-parse.c b/src/detect-parse.c index 3b03dfb92b36..973c1087d4d2 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -1735,6 +1735,84 @@ int DetectSignatureAddTransform(Signature *s, int transform, void *options) SCReturnInt(0); } +// alprotos parameter is expected as an array terminated by ALPROTO_UNKNOWN +int DetectSignatureSetMultiAppProto(Signature *s, const AppProto *alprotos) +{ + if (s->alproto != ALPROTO_UNKNOWN) { + // One alproto was set, check if it matches the new ones proposed + while (*alprotos != ALPROTO_UNKNOWN) { + if (s->alproto == *alprotos) { + // alproto already set to only one + return 0; + } + alprotos++; + } + // alproto already set and not matching the new set of alprotos + return -1; + } + if (s->init_data->alprotos[0] != ALPROTO_UNKNOWN) { + // check intersection of already used alprotos and new ones + for (size_t i = 0; i < SIG_ALPROTO_MAX; i++) { + if (s->init_data->alprotos[i] == ALPROTO_UNKNOWN) { + break; + } + // first disable the ones that do not match + bool found = false; + while (*alprotos != ALPROTO_UNKNOWN) { + if (s->init_data->alprotos[i] == *alprotos) { + found = true; + break; + } + alprotos++; + } + if (!found) { + s->init_data->alprotos[i] = ALPROTO_UNKNOWN; + } + } + // Then put at the beginning every defined protocol + for (size_t i = 0; i < SIG_ALPROTO_MAX; i++) { + if (s->init_data->alprotos[i] == ALPROTO_UNKNOWN) { + for (size_t j = SIG_ALPROTO_MAX - 1; j > i; j--) { + if (s->init_data->alprotos[j] != ALPROTO_UNKNOWN) { + s->init_data->alprotos[i] = s->init_data->alprotos[j]; + s->init_data->alprotos[j] = ALPROTO_UNKNOWN; + break; + } + } + if (s->init_data->alprotos[i] == ALPROTO_UNKNOWN) { + if (i == 0) { + // there was no intersection + return -1; + } else if (i == 1) { + // intersection is singleton, set it as usual + AppProto alproto = s->init_data->alprotos[0]; + s->init_data->alprotos[0] = ALPROTO_UNKNOWN; + return DetectSignatureSetAppProto(s, alproto); + } + break; + } + } + } + } else { + if (alprotos[0] == ALPROTO_UNKNOWN) { + // do not allow empty set + return -1; + } + if (alprotos[1] == ALPROTO_UNKNOWN) { + // allow singleton, but call traditional setter + return DetectSignatureSetAppProto(s, alprotos[0]); + } + // first time we enforce alprotos + for (size_t i = 0; i < SIG_ALPROTO_MAX; i++) { + if (alprotos[i] == ALPROTO_UNKNOWN) { + break; + } + s->init_data->alprotos[i] = alprotos[i]; + } + } + return 0; +} + int DetectSignatureSetAppProto(Signature *s, AppProto alproto) { if (alproto == ALPROTO_UNKNOWN || @@ -1743,6 +1821,21 @@ int DetectSignatureSetAppProto(Signature *s, AppProto alproto) return -1; } + if (s->init_data->alprotos[0] != ALPROTO_UNKNOWN) { + // Multiple protocols were set, check if we restrict to one + bool found = false; + for (size_t i = 0; i < SIG_ALPROTO_MAX; i++) { + if (s->init_data->alprotos[i] == alproto) { + found = true; + break; + } + } + if (!found) { + return -1; + } + s->init_data->alprotos[0] = ALPROTO_UNKNOWN; + } + if (s->alproto != ALPROTO_UNKNOWN) { alproto = AppProtoCommon(s->alproto, alproto); if (alproto == ALPROTO_FAILED) { diff --git a/src/detect-parse.h b/src/detect-parse.h index ec2c204c0f42..c6b69a1bb2ac 100644 --- a/src/detect-parse.h +++ b/src/detect-parse.h @@ -102,6 +102,7 @@ SigMatch *DetectGetLastSMByListId(const Signature *s, int list_id, ...); int DetectSignatureAddTransform(Signature *s, int transform, void *options); int WARN_UNUSED DetectSignatureSetAppProto(Signature *s, AppProto alproto); +int WARN_UNUSED DetectSignatureSetMultiAppProto(Signature *s, const AppProto *alprotos); /* parse regex setup and free util funcs */ diff --git a/src/detect.h b/src/detect.h index e88b5540ac79..bf7f771b9be0 100644 --- a/src/detect.h +++ b/src/detect.h @@ -538,6 +538,8 @@ typedef struct SignatureInitDataBuffer_ { SigMatch *tail; } SignatureInitDataBuffer; +#define SIG_ALPROTO_MAX 4 + typedef struct SignatureInitData_ { /** Number of sigmatches. Used for assigning SigMatch::idx */ uint16_t sm_cnt; @@ -555,6 +557,9 @@ typedef struct SignatureInitData_ { uint32_t init_flags; /* coccinelle: SignatureInitData:init_flags:SIG_FLAG_INIT_ */ + /* alproto mask if multiple protocols are possible */ + AppProto alprotos[SIG_ALPROTO_MAX]; + /* used at init to determine max dsize */ SigMatch *dsize_sm; From 7b24101d10d34dcff82e8771f0577a8b5f1df586 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Mon, 25 Nov 2024 09:30:51 +0100 Subject: [PATCH 2/2] detect/ja: use multi-protocol support instead of hardcoding list : removes usage of ALPROTO_QUIC and ALPROTO_TLS in generic SigValidate Ticket: 7304 --- src/detect-ja4-hash.c | 70 ++++++++++++++++++++++++++++++++++-- src/detect-parse.c | 20 ++++++++--- src/detect-tls-ja3-hash.c | 4 +-- src/detect-tls-ja3-string.c | 4 +-- src/detect-tls-ja3s-hash.c | 4 +-- src/detect-tls-ja3s-string.c | 4 +-- src/detect.h | 3 +- 7 files changed, 92 insertions(+), 17 deletions(-) diff --git a/src/detect-ja4-hash.c b/src/detect-ja4-hash.c index ebddc6b6d060..e5d95fd3b407 100644 --- a/src/detect-ja4-hash.c +++ b/src/detect-ja4-hash.c @@ -55,6 +55,9 @@ int Ja4IsDisabled(const char *type); static InspectionBuffer *Ja4DetectGetHash(DetectEngineThreadCtx *det_ctx, const DetectEngineTransforms *transforms, Flow *_f, const uint8_t _flow_flags, void *txv, const int list_id); +#ifdef UNITTESTS +static void DetectJa4RegisterTests(void); +#endif static int g_ja4_hash_buffer_id = 0; #endif @@ -70,6 +73,9 @@ void DetectJa4HashRegister(void) sigmatch_table[DETECT_AL_JA4_HASH].url = "/rules/ja4-keywords.html#ja4-hash"; #ifdef HAVE_JA4 sigmatch_table[DETECT_AL_JA4_HASH].Setup = DetectJa4HashSetup; +#ifdef UNITTESTS + sigmatch_table[DETECT_AL_JA4_HASH].RegisterTests = DetectJa4RegisterTests; +#endif #else /* HAVE_JA4 */ sigmatch_table[DETECT_AL_JA4_HASH].Setup = DetectJA4SetupNoSupport; #endif /* HAVE_JA4 */ @@ -111,7 +117,8 @@ static int DetectJa4HashSetup(DetectEngineCtx *de_ctx, Signature *s, const char if (DetectBufferSetActiveList(de_ctx, s, g_ja4_hash_buffer_id) < 0) return -1; - if (s->alproto != ALPROTO_UNKNOWN && s->alproto != ALPROTO_TLS && s->alproto != ALPROTO_QUIC) { + AppProto alprotos[] = { ALPROTO_TLS, ALPROTO_QUIC, ALPROTO_UNKNOWN }; + if (DetectSignatureSetMultiAppProto(s, alprotos) < 0) { SCLogError("rule contains conflicting protocols."); return -1; } @@ -126,7 +133,6 @@ static int DetectJa4HashSetup(DetectEngineCtx *de_ctx, Signature *s, const char } return -2; } - s->init_data->init_flags |= SIG_FLAG_INIT_JA; return 0; } @@ -174,4 +180,64 @@ static InspectionBuffer *Ja4DetectGetHash(DetectEngineThreadCtx *det_ctx, } return buffer; } + +#ifdef UNITTESTS +static int DetectJa4TestParse01(void) +{ + DetectEngineCtx *de_ctx = DetectEngineCtxInit(); + + // invalid tests + Signature *s = + SigInit(de_ctx, "alert ip any any -> any any (sid: 1; file.data; content: \"toto\"; " + "ja4.hash; content: \"q13d0310h3_55b375c5d22e_cd85d2d88918\";)"); + // cannot have file.data with ja4.hash (quic or tls) + FAIL_IF_NOT_NULL(s); + s = SigInit(de_ctx, "alert ip any any -> any any (sid: 1; " + "ja4.hash; content: \"q13d0310h3_55b375c5d22e_cd85d2d88918\"; file.data; " + "content: \"toto\";)"); + // cannot have file.data with ja4.hash (quic or tls) + FAIL_IF_NOT_NULL(s); + s = SigInit(de_ctx, "alert smb any any -> any any (sid: 1; " + "ja4.hash; content: \"q13d0310h3_55b375c5d22e_cd85d2d88918\";)"); + // cannot have alproto=smb with ja4.hash (quic or tls) + FAIL_IF_NOT_NULL(s); + s = SigInit(de_ctx, "alert ip any any -> any any (sid: 1; " + "ja4.hash; content: \"q13d0310h3_55b375c5d22e_cd85d2d88918\"; smb.share; " + "content:\"toto\";)"); + // cannot have a smb keyword with ja4.hash (quic or tls) + FAIL_IF_NOT_NULL(s); + s = SigInit(de_ctx, "alert ip any any -> any any (sid: 1; " + "smb.share; content:\"toto\"; ja4.hash; content: " + "\"q13d0310h3_55b375c5d22e_cd85d2d88918\";)"); + // cannot have a smb keyword with ja4.hash (quic or tls) + FAIL_IF_NOT_NULL(s); + + // valid tests + s = SigInit(de_ctx, "alert ip any any -> any any (sid: 1; " + "ja4.hash; content: \"q13d0310h3_55b375c5d22e_cd85d2d88918\";)"); + // just ja4.hash any proto + FAIL_IF_NULL(s); + s = SigInit(de_ctx, "alert quic any any -> any any (sid: 1; " + "ja4.hash; content: \"q13d0310h3_55b375c5d22e_cd85d2d88918\";)"); + // just ja4.hash only quic + FAIL_IF_NULL(s); + s = SigInit(de_ctx, "alert tls any any -> any any (sid: 1; " + "ja4.hash; content: \"q13d0310h3_55b375c5d22e_cd85d2d88918\";)"); + // just ja4.hash only tls + FAIL_IF_NULL(s); + s = SigInit(de_ctx, "alert ip any any -> any any (sid: 1; " + "ja4.hash; content: \"q13d0310h3_55b375c5d22e_cd85d2d88918\"; " + "quic.version; content:\"|00|\";)"); + // ja4.hash and a quic keyword + FAIL_IF_NULL(s); + DetectEngineCtxFree(de_ctx); + PASS; +} + +static void DetectJa4RegisterTests(void) +{ + UtRegisterTest("DetectJa4TestParse01", DetectJa4TestParse01); +} #endif + +#endif // HAVE_JA4 diff --git a/src/detect-parse.c b/src/detect-parse.c index 973c1087d4d2..be56cb428039 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -2159,11 +2159,6 @@ static int SigValidate(DetectEngineCtx *de_ctx, Signature *s) } DetectLuaPostSetup(s); - if ((s->init_data->init_flags & SIG_FLAG_INIT_JA) && s->alproto != ALPROTO_UNKNOWN && - s->alproto != ALPROTO_TLS && s->alproto != ALPROTO_QUIC) { - SCLogError("Cannot have ja3/ja4 with protocol %s.", AppProtoToString(s->alproto)); - SCReturnInt(0); - } if ((s->flags & SIG_FLAG_FILESTORE) || s->file_flags != 0 || (s->init_data->init_flags & SIG_FLAG_INIT_FILEDATA)) { if (s->alproto != ALPROTO_UNKNOWN && @@ -2173,6 +2168,21 @@ static int SigValidate(DetectEngineCtx *de_ctx, Signature *s) "support file matching", AppProtoToString(s->alproto)); SCReturnInt(0); + } else if (s->init_data->alprotos[0] != ALPROTO_UNKNOWN) { + bool found = false; + for (size_t i = 0; i < SIG_ALPROTO_MAX; i++) { + if (s->init_data->alprotos[i] == ALPROTO_UNKNOWN) { + break; + } + if (AppLayerParserSupportsFiles(IPPROTO_TCP, s->init_data->alprotos[i])) { + found = true; + break; + } + } + if (!found) { + SCLogError("No protocol support file matching"); + SCReturnInt(0); + } } if (s->alproto == ALPROTO_HTTP2 && (s->file_flags & FILE_SIG_NEED_FILENAME)) { SCLogError("protocol HTTP2 doesn't support file name matching"); diff --git a/src/detect-tls-ja3-hash.c b/src/detect-tls-ja3-hash.c index 57b0e55edeb5..ef62776c742d 100644 --- a/src/detect-tls-ja3-hash.c +++ b/src/detect-tls-ja3-hash.c @@ -136,7 +136,8 @@ static int DetectTlsJa3HashSetup(DetectEngineCtx *de_ctx, Signature *s, const ch if (DetectBufferSetActiveList(de_ctx, s, g_tls_ja3_hash_buffer_id) < 0) return -1; - if (s->alproto != ALPROTO_UNKNOWN && s->alproto != ALPROTO_TLS && s->alproto != ALPROTO_QUIC) { + AppProto alprotos[] = { ALPROTO_TLS, ALPROTO_QUIC, ALPROTO_UNKNOWN }; + if (DetectSignatureSetMultiAppProto(s, alprotos) < 0) { SCLogError("rule contains conflicting protocols."); return -1; } @@ -151,7 +152,6 @@ static int DetectTlsJa3HashSetup(DetectEngineCtx *de_ctx, Signature *s, const ch } return -2; } - s->init_data->init_flags |= SIG_FLAG_INIT_JA; return 0; } diff --git a/src/detect-tls-ja3-string.c b/src/detect-tls-ja3-string.c index 1ec289c6e9d1..9fafd9daaedc 100644 --- a/src/detect-tls-ja3-string.c +++ b/src/detect-tls-ja3-string.c @@ -125,7 +125,8 @@ static int DetectTlsJa3StringSetup(DetectEngineCtx *de_ctx, Signature *s, const if (DetectBufferSetActiveList(de_ctx, s, g_tls_ja3_str_buffer_id) < 0) return -1; - if (s->alproto != ALPROTO_UNKNOWN && s->alproto != ALPROTO_TLS && s->alproto != ALPROTO_QUIC) { + AppProto alprotos[] = { ALPROTO_TLS, ALPROTO_QUIC, ALPROTO_UNKNOWN }; + if (DetectSignatureSetMultiAppProto(s, alprotos) < 0) { SCLogError("rule contains conflicting protocols."); return -1; } @@ -140,7 +141,6 @@ static int DetectTlsJa3StringSetup(DetectEngineCtx *de_ctx, Signature *s, const } return -2; } - s->init_data->init_flags |= SIG_FLAG_INIT_JA; return 0; } diff --git a/src/detect-tls-ja3s-hash.c b/src/detect-tls-ja3s-hash.c index 6d3d42e5edf8..9117ffeec36c 100644 --- a/src/detect-tls-ja3s-hash.c +++ b/src/detect-tls-ja3s-hash.c @@ -134,7 +134,8 @@ static int DetectTlsJa3SHashSetup(DetectEngineCtx *de_ctx, Signature *s, const c if (DetectBufferSetActiveList(de_ctx, s, g_tls_ja3s_hash_buffer_id) < 0) return -1; - if (s->alproto != ALPROTO_UNKNOWN && s->alproto != ALPROTO_TLS && s->alproto != ALPROTO_QUIC) { + AppProto alprotos[] = { ALPROTO_TLS, ALPROTO_QUIC, ALPROTO_UNKNOWN }; + if (DetectSignatureSetMultiAppProto(s, alprotos) < 0) { SCLogError("rule contains conflicting protocols."); return -1; } @@ -149,7 +150,6 @@ static int DetectTlsJa3SHashSetup(DetectEngineCtx *de_ctx, Signature *s, const c } return -2; } - s->init_data->init_flags |= SIG_FLAG_INIT_JA; return 0; } diff --git a/src/detect-tls-ja3s-string.c b/src/detect-tls-ja3s-string.c index 0104560627d5..95a502011f3e 100644 --- a/src/detect-tls-ja3s-string.c +++ b/src/detect-tls-ja3s-string.c @@ -125,7 +125,8 @@ static int DetectTlsJa3SStringSetup(DetectEngineCtx *de_ctx, Signature *s, const if (DetectBufferSetActiveList(de_ctx, s, g_tls_ja3s_str_buffer_id) < 0) return -1; - if (s->alproto != ALPROTO_UNKNOWN && s->alproto != ALPROTO_TLS && s->alproto != ALPROTO_QUIC) { + AppProto alprotos[] = { ALPROTO_TLS, ALPROTO_QUIC, ALPROTO_UNKNOWN }; + if (DetectSignatureSetMultiAppProto(s, alprotos) < 0) { SCLogError("rule contains conflicting protocols."); return -1; } @@ -140,7 +141,6 @@ static int DetectTlsJa3SStringSetup(DetectEngineCtx *de_ctx, Signature *s, const } return -2; } - s->init_data->init_flags |= SIG_FLAG_INIT_JA; return 0; } diff --git a/src/detect.h b/src/detect.h index bf7f771b9be0..49351d3edf1b 100644 --- a/src/detect.h +++ b/src/detect.h @@ -293,8 +293,7 @@ typedef struct DetectPort_ { #define SIG_FLAG_INIT_NEED_FLUSH BIT_U32(7) #define SIG_FLAG_INIT_PRIO_EXPLICIT \ BIT_U32(8) /**< priority is explicitly set by the priority keyword */ -#define SIG_FLAG_INIT_FILEDATA BIT_U32(9) /**< signature has filedata keyword */ -#define SIG_FLAG_INIT_JA BIT_U32(10) /**< signature has ja3/ja4 keyword */ +#define SIG_FLAG_INIT_FILEDATA BIT_U32(9) /**< signature has filedata keyword */ /* signature mask flags */ /** \note: additions should be added to the rule analyzer as well */