From 2cf2387e313cc42aac48dad81231fee4f832b86c Mon Sep 17 00:00:00 2001 From: Alexander Gozman Date: Sun, 21 Jan 2018 11:21:40 +0000 Subject: [PATCH] rules: optimize bidir rules with same src/dst As an optimization, reset bidirectional flag for rules with same src and dst. If one created bidirectional rule like 'alert tcp any any <> any any ...', the rule was checked twice (for each packet in every direction). This is suboptimal and may give duplicated alerts. To avoid this, bidirectional rules are now checked for the same src and dst (addresses and ports) and if it's the case, the rule is treated as unidirectional and a corresponding message is logged. --- src/detect-engine-address.c | 32 ++++++++++ src/detect-engine-address.h | 2 + src/detect-engine-port.c | 59 ++++++++++++++++++ src/detect-engine-port.h | 3 + src/detect-parse.c | 120 +++++++++++++++++++++++++++++++++++- 5 files changed, 213 insertions(+), 3 deletions(-) diff --git a/src/detect-engine-address.c b/src/detect-engine-address.c index c8cc1bea90..4394973502 100644 --- a/src/detect-engine-address.c +++ b/src/detect-engine-address.c @@ -445,6 +445,38 @@ int DetectAddressJoin(DetectEngineCtx *de_ctx, DetectAddress *target, return -1; } +/** + * \brief Checks if two address group lists are equal. + * + * \param list1 Pointer to the first address group list. + * \param list2 Pointer to the second address group list. + * + * \retval true On success. + * \retval false On failure. + */ +bool DetectAddressListsAreEqual(DetectAddress *list1, DetectAddress *list2) +{ + DetectAddress *item = list1; + DetectAddress *it = list2; + + // First, compare items one by one. + while (item != NULL && it != NULL) { + if (DetectAddressCmp(item, it) != ADDRESS_EQ) { + return false; + } + + item = item->next; + it = it->next; + } + + // Are the lists of the same size? + if (!(item == NULL && it == NULL)) { + return false; + } + + return true; +} + /** * \internal * \brief Creates a cidr ipv6 netblock, based on the cidr netblock value. diff --git a/src/detect-engine-address.h b/src/detect-engine-address.h index c01b20e6b2..a5e9b9acdb 100644 --- a/src/detect-engine-address.h +++ b/src/detect-engine-address.h @@ -44,6 +44,8 @@ void DetectAddressPrintList(DetectAddress *); int DetectAddressInsert(DetectEngineCtx *, DetectAddressHead *, DetectAddress *); int DetectAddressJoin(DetectEngineCtx *, DetectAddress *, DetectAddress *); +bool DetectAddressListsAreEqual(DetectAddress *list1, DetectAddress *list2); + DetectAddress *DetectAddressLookupInHead(const DetectAddressHead *, Address *); DetectAddress *DetectAddressLookupInList(DetectAddress *, DetectAddress *); int DetectAddressMatch(DetectAddress *, Address *); diff --git a/src/detect-engine-port.c b/src/detect-engine-port.c index 216fc296e7..36083e5bc1 100644 --- a/src/detect-engine-port.c +++ b/src/detect-engine-port.c @@ -752,6 +752,33 @@ DetectPortLookupGroup(DetectPort *dp, uint16_t port) return NULL; } +/** + * \brief Used to check if a DetectPort list contains an instance with + * a similar DetectPort. The comparison done is not the one that + * checks the memory for the same instance, but one that checks that the + * two instances hold the same content. + * + * \param head Pointer to the DetectPort list. + * \param ad Pointer to the DetectPort that has to be checked for in + * the DetectPort list. + * + * \retval cur Returns a pointer to the DetectPort on a match; NULL if + * no match. + */ +DetectPort *DetectPortLookupInList(DetectPort *head, DetectPort *gr) +{ + DetectPort *cur; + + if (head != NULL) { + for (cur = head; cur != NULL; cur = cur->next) { + if (DetectPortCmp(cur, gr) == PORT_EQ) + return cur; + } + } + + return NULL; +} + /** * \brief Function to join the source group to the target and its members * @@ -779,6 +806,38 @@ int DetectPortJoin(DetectEngineCtx *de_ctx, DetectPort *target, return 0; } +/** + * \brief Checks if two port group lists are equal. + * + * \param list1 Pointer to the first port group list. + * \param list2 Pointer to the second port group list. + * + * \retval true On success. + * \retval false On failure. + */ +bool DetectPortListsAreEqual(DetectPort *list1, DetectPort *list2) +{ + DetectPort *item = list1; + DetectPort *it = list2; + + // First, compare items one by one. + while (item != NULL && it != NULL) { + if (DetectPortCmp(item, it) != PORT_EQ) { + return false; + } + + item = item->next; + it = it->next; + } + + // Are the lists of the same size? + if (!(item == NULL && it == NULL)) { + return false; + } + + return true; +} + /******************* parsing routines ************************/ /** diff --git a/src/detect-engine-port.h b/src/detect-engine-port.h index f82e1c360e..b692c3e5bc 100644 --- a/src/detect-engine-port.h +++ b/src/detect-engine-port.h @@ -34,9 +34,12 @@ int DetectPortInsert(DetectEngineCtx *,DetectPort **, DetectPort *); void DetectPortCleanupList (const DetectEngineCtx *de_ctx, DetectPort *head); DetectPort *DetectPortLookupGroup(DetectPort *dp, uint16_t port); +DetectPort *DetectPortLookupInList(DetectPort *head, DetectPort *gr); int DetectPortJoin(DetectEngineCtx *,DetectPort *target, DetectPort *source); +bool DetectPortListsAreEqual(DetectPort *list1, DetectPort *list2); + void DetectPortPrint(DetectPort *); void DetectPortPrintList(DetectPort *head); int DetectPortCmp(DetectPort *, DetectPort *); diff --git a/src/detect-parse.c b/src/detect-parse.c index bd7fd1e2d4..dbb5dc6668 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -1878,6 +1878,39 @@ error: return NULL; } +/** + * \brief Checks if a signature has the same source and destination + * \param s parsed signature + * + * \retval true if source and destination are the same, false otherwise + */ +static bool SigHasSameSourceAndDestination(const Signature *s) +{ + if (!(s->flags & SIG_FLAG_SP_ANY) || !(s->flags & SIG_FLAG_DP_ANY)) { + if (!DetectPortListsAreEqual(s->sp, s->dp)) { + return false; + } + } + + if (!(s->flags & SIG_FLAG_SRC_ANY) || !(s->flags & SIG_FLAG_DST_ANY)) { + DetectAddress *src = s->init_data->src->ipv4_head; + DetectAddress *dst = s->init_data->dst->ipv4_head; + + if (!DetectAddressListsAreEqual(src, dst)) { + return false; + } + + src = s->init_data->src->ipv6_head; + dst = s->init_data->dst->ipv6_head; + + if (!DetectAddressListsAreEqual(src, dst)) { + return false; + } + } + + return true; +} + /** * \brief Parses a signature and adds it to the Detection Engine Context. * @@ -1900,9 +1933,16 @@ Signature *SigInit(DetectEngineCtx *de_ctx, const char *sigstr) } if (sig->init_data->init_flags & SIG_FLAG_INIT_BIDIREC) { - sig->next = SigInitHelper(de_ctx, sigstr, SIG_DIREC_SWITCHED); - if (sig->next == NULL) { - goto error; + if (SigHasSameSourceAndDestination(sig)) { + SCLogInfo("Rule with ID %u is bidirectional, but source and destination are the same, " + "treating the rule as unidirectional", sig->id); + + sig->init_data->init_flags &= ~SIG_FLAG_INIT_BIDIREC; + } else { + sig->next = SigInitHelper(de_ctx, sigstr, SIG_DIREC_SWITCHED); + if (sig->next == NULL) { + goto error; + } } } @@ -3886,6 +3926,75 @@ static int SigParseTestContentGtDsize02(void) PASS; } +static int SigParseBidirWithSameSrcAndDest01(void) +{ + DetectEngineCtx *de_ctx = DetectEngineCtxInit(); + FAIL_IF_NULL(de_ctx); + de_ctx->flags |= DE_QUIET; + + Signature *s = SigInit(de_ctx, + "alert tcp any any <> any any (sid:1; rev:1;)"); + FAIL_IF_NULL(s); + FAIL_IF_NOT_NULL(s->next); + FAIL_IF(s->init_data->init_flags & SIG_FLAG_INIT_BIDIREC); + + SigFree(s); + + s = SigInit(de_ctx, + "alert tcp any [80, 81] <> any [81, 80] (sid:1; rev:1;)"); + FAIL_IF_NULL(s); + FAIL_IF_NOT_NULL(s->next); + FAIL_IF(s->init_data->init_flags & SIG_FLAG_INIT_BIDIREC); + + SigFree(s); + + s = SigInit(de_ctx, + "alert tcp [1.2.3.4, 5.6.7.8] [80, 81] <> [5.6.7.8, 1.2.3.4] [81, 80] (sid:1; rev:1;)"); + FAIL_IF_NULL(s); + FAIL_IF_NOT_NULL(s->next); + FAIL_IF(s->init_data->init_flags & SIG_FLAG_INIT_BIDIREC); + + SigFree(s); + + PASS; +} + +static int SigParseBidirWithSameSrcAndDest02(void) +{ + DetectEngineCtx *de_ctx = DetectEngineCtxInit(); + FAIL_IF_NULL(de_ctx); + de_ctx->flags |= DE_QUIET; + + // Source is a subset of destination + Signature *s = SigInit(de_ctx, + "alert tcp 1.2.3.4 any <> [1.2.3.4, 5.6.7.8, ::1] any (sid:1; rev:1;)"); + FAIL_IF_NULL(s); + FAIL_IF_NULL(s->next); + FAIL_IF_NOT(s->init_data->init_flags & SIG_FLAG_INIT_BIDIREC); + + SigFree(s); + + // Source is a subset of destinationn + s = SigInit(de_ctx, + "alert tcp [1.2.3.4, ::1] [80, 81, 82] <> [1.2.3.4, ::1] [80, 81] (sid:1; rev:1;)"); + FAIL_IF_NULL(s); + FAIL_IF_NULL(s->next); + FAIL_IF_NOT(s->init_data->init_flags & SIG_FLAG_INIT_BIDIREC); + + SigFree(s); + + // Source intersects with destination + s = SigInit(de_ctx, + "alert tcp [1.2.3.4, ::1, ABCD:AAAA::1] [80] <> [1.2.3.4, ::1] [80, 81] (sid:1; rev:1;)"); + FAIL_IF_NULL(s); + FAIL_IF_NULL(s->next); + FAIL_IF_NOT(s->init_data->init_flags & SIG_FLAG_INIT_BIDIREC); + + SigFree(s); + + PASS; +} + #endif /* UNITTESTS */ void SigParseRegisterTests(void) @@ -3948,5 +4057,10 @@ void SigParseRegisterTests(void) SigParseTestContentGtDsize01); UtRegisterTest("SigParseTestContentGtDsize02", SigParseTestContentGtDsize02); + + UtRegisterTest("SigParseBidirWithSameSrcAndDest01", + SigParseBidirWithSameSrcAndDest01); + UtRegisterTest("SigParseBidirWithSameSrcAndDest02", + SigParseBidirWithSameSrcAndDest02); #endif /* UNITTESTS */ } -- 2.47.2