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

@@ -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.

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 *);

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 ************************/
 
 /**

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 *);

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 */
 }