detect: bidirectional rules can use dir-ambiguous keywords

By using keywords bidir.toclient and bidir.toserver, the following
keywords are forced to the direction even if they can match
on both directions.

Ticket: 5665

doc/userguide/rules/intro.rst

@@ -266,12 +266,18 @@ And it will match on a full transaction, using both the uri from the request,
 and the stat_code from the response.
 As such, it will match only when Suricata got both request and response.
 
+Bidirectional rules can use direction-abmibuous keywords, by first using
+``bidir.toclient`` or ``bidir.toserver`` keywords.
+
+.. container:: example-rule
+
+    alert http any any => 5.6.7.8 80 (msg:"matching json to server and xml to client"; sid: 1;  :example-rule-emphasis:`bidir.toserver;` http.content_type; content: "json";  :example-rule-emphasis:`bidir.toclient;` http.content_type; content: "xml";)
+
 Bidirectional rules have some limitations :
 - They are only meant to work on transactions with first a request to the server,
 and then a response to the client, and not the other way around.
 - They cannot have ``fast_pattern`` or ``prefilter`` on a keyword which is on
 the direction to client.
-- They will not work with ambiguous keywords, which work for both directions.
 - They will refuse to load if a single directional rule is enough.
 
 Rule options

src/detect-engine-mpm.c

@@ -1056,15 +1056,20 @@ int g_skip_prefilter = 0;
 
 bool DetectBufferToClient(const DetectEngineCtx *de_ctx, int buf_id, AppProto alproto)
 {
+    bool r = false;
     const DetectEngineAppInspectionEngine *app = de_ctx->app_inspect_engines;
     for (; app != NULL; app = app->next) {
         if (app->sm_list == buf_id && (AppProtoEquals(alproto, app->alproto) || alproto == 0)) {
             if (app->dir == 1) {
-                return true;
+                // do not return yet in case we have app engines on both sides
+                r = true;
+            } else {
+                // ambiguous keywords have a app-engine to server
+                return false;
             }
         }
     }
-    return false;
+    return r;
 }
 
 void RetrieveFPForSig(const DetectEngineCtx *de_ctx, Signature *s)

src/detect-engine-register.c

@@ -577,6 +577,7 @@ void SigTableSetup(void)
     DetectOffsetRegister();
     DetectReplaceRegister();
     DetectFlowRegister();
+    DetectBidirRegister();
     DetectFlowAgeRegister();
     DetectFlowPktsToClientRegister();
     DetectFlowPktsToServerRegister();

src/detect-engine-register.h

@@ -324,6 +324,9 @@ enum DetectKeywordId {
 
     DETECT_PREFILTER,
 
+    DETECT_BIDIR_TOCLIENT,
+    DETECT_BIDIR_TOSERVER,
+
     DETECT_TRANSFORM_COMPRESS_WHITESPACE,
     DETECT_TRANSFORM_STRIP_WHITESPACE,
     DETECT_TRANSFORM_STRIP_PSEUDO_HEADERS,

src/detect-engine.c

@@ -788,6 +788,15 @@ int DetectEngineAppInspectionEngine2Signature(DetectEngineCtx *de_ctx, Signature
             for (const DetectEngineAppInspectionEngine *t = de_ctx->app_inspect_engines; t != NULL;
                     t = t->next) {
                 if (t->sm_list == s->init_data->buffers[x].id) {
+                    if (s->flags & SIG_FLAG_BOTHDIR) {
+                        // ambiguous keywords have app engines in both directions
+                        // so we skip the wrong direction for this buffer
+                        if (s->init_data->buffers[x].only_tc && t->dir == 0) {
+                            continue;
+                        } else if (s->init_data->buffers[x].only_ts && t->dir == 1) {
+                            continue;
+                        }
+                    }
                     AppendAppInspectEngine(
                             de_ctx, t, s, smd, mpm_list, files_id, &last_id, &head_is_mpm);
                 }
@@ -1417,7 +1426,12 @@ int DetectBufferSetActiveList(DetectEngineCtx *de_ctx, Signature *s, const int l
 
             } else if (DetectEngineBufferTypeSupportsMultiInstanceGetById(de_ctx, list)) {
                 // fall through
+            } else if (b->only_tc && (s->init_data->init_flags & SIG_FLAG_INIT_BIDIR_TOSERVER)) {
+                // fall through
+            } else if (b->only_ts && (s->init_data->init_flags & SIG_FLAG_INIT_BIDIR_TOCLIENT)) {
+                // fall through
             } else {
+                // we create a new buffer for the same id but forced different direction
                 SCLogWarning("duplicate instance for %s in '%s'",
                         DetectEngineBufferTypeGetNameById(de_ctx, list), s->sig_str);
                 s->init_data->curbuf = b;
@@ -1441,6 +1455,10 @@ int DetectBufferSetActiveList(DetectEngineCtx *de_ctx, Signature *s, const int l
     s->init_data->curbuf->tail = NULL;
     s->init_data->curbuf->multi_capable =
             DetectEngineBufferTypeSupportsMultiInstanceGetById(de_ctx, list);
+    // TODO try to be smart and only set these if the keyword is ambiguous
+    s->init_data->curbuf->only_tc = s->init_data->init_flags & SIG_FLAG_INIT_BIDIR_TOCLIENT;
+    s->init_data->curbuf->only_ts = s->init_data->init_flags & SIG_FLAG_INIT_BIDIR_TOSERVER;
+
     SCLogDebug("new: idx %u list %d set up curbuf %p", s->init_data->buffer_index - 1, list,
             s->init_data->curbuf);
 

src/detect-flow.c

@@ -79,6 +79,48 @@ void DetectFlowRegister (void)
     DetectSetupParseRegexes(PARSE_REGEX, &parse_regex);
 }
 
+static int DetectBidirToClientSetup(DetectEngineCtx *de_ctx, Signature *s, const char *flowstr)
+{
+    if (!(s->flags & SIG_FLAG_BOTHDIR)) {
+        SCLogError("Cannot have bidir keyword in a non bidirectional signature");
+        return -1;
+    }
+    s->init_data->init_flags |= SIG_FLAG_INIT_BIDIR_TOCLIENT;
+    s->init_data->init_flags &= ~SIG_FLAG_INIT_BIDIR_TOSERVER;
+    return 0;
+}
+
+static int DetectBidirToServerSetup(DetectEngineCtx *de_ctx, Signature *s, const char *flowstr)
+{
+    if (!(s->flags & SIG_FLAG_BOTHDIR)) {
+        SCLogError("Cannot have bidir keyword in a non bidirectional signature");
+        return -1;
+    }
+    s->init_data->init_flags |= SIG_FLAG_INIT_BIDIR_TOSERVER;
+    s->init_data->init_flags &= ~SIG_FLAG_INIT_BIDIR_TOCLIENT;
+    return 0;
+}
+
+/**
+ * \brief Registration function for flow: keyword
+ */
+void DetectBidirRegister(void)
+{
+    sigmatch_table[DETECT_BIDIR_TOCLIENT].name = "bidir.toclient";
+    sigmatch_table[DETECT_BIDIR_TOCLIENT].desc =
+            "match next keywords only toclient side for bidirectional rules";
+    sigmatch_table[DETECT_BIDIR_TOCLIENT].url = "/rules/flow-keywords.html#TODO";
+    sigmatch_table[DETECT_BIDIR_TOCLIENT].Setup = DetectBidirToClientSetup;
+    sigmatch_table[DETECT_BIDIR_TOCLIENT].flags |= SIGMATCH_NOOPT;
+
+    sigmatch_table[DETECT_BIDIR_TOSERVER].name = "bidir.toserver";
+    sigmatch_table[DETECT_BIDIR_TOSERVER].desc =
+            "match next keywords only toclient side for bidirectional rules";
+    sigmatch_table[DETECT_BIDIR_TOSERVER].url = "/rules/flow-keywords.html#TODO";
+    sigmatch_table[DETECT_BIDIR_TOSERVER].Setup = DetectBidirToServerSetup;
+    sigmatch_table[DETECT_BIDIR_TOSERVER].flags |= SIGMATCH_NOOPT;
+}
+
 /**
  * \param pflags packet flags (p->flags)
  * \param pflowflags packet flow flags (p->flowflags)

src/detect-flow.h

@@ -44,4 +44,6 @@ int DetectFlowSetupImplicit(Signature *s, uint32_t flags);
 /* prototypes */
 void DetectFlowRegister (void);
 
+void DetectBidirRegister(void);
+
 #endif /* SURICATA_DETECT_FLOW_H */

src/detect-parse.c

@@ -1951,6 +1951,9 @@ static int SigValidate(DetectEngineCtx *de_ctx, Signature *s)
     } bufdir[nlists + 1];
     memset(&bufdir, 0, (nlists + 1) * sizeof(struct BufferVsDir));
 
+    int ts_excl = 0;
+    int tc_excl = 0;
+
     for (uint32_t x = 0; x < s->init_data->buffer_index; x++) {
         SignatureInitDataBuffer *b = &s->init_data->buffers[x];
         const DetectBufferType *bt = DetectEngineBufferTypeGetById(de_ctx, b->id);
@@ -1988,8 +1991,16 @@ static int SigValidate(DetectEngineCtx *de_ctx, Signature *s)
                         DetectEngineBufferTypeGetNameById(de_ctx, app->sm_list), app->dir,
                         app->alproto);
                 SCLogDebug("b->id %d nlists %d", b->id, nlists);
-                bufdir[b->id].ts += (app->dir == 0);
-                bufdir[b->id].tc += (app->dir == 1);
+                if (b->only_tc) {
+                    if (app->dir == 1)
+                        tc_excl++;
+                } else if (b->only_ts) {
+                    if (app->dir == 0)
+                        ts_excl++;
+                } else {
+                    bufdir[b->id].ts += (app->dir == 0);
+                    bufdir[b->id].tc += (app->dir == 1);
+                }
             }
         }
 
@@ -2002,8 +2013,6 @@ static int SigValidate(DetectEngineCtx *de_ctx, Signature *s)
         }
     }
 
-    int ts_excl = 0;
-    int tc_excl = 0;
     int dir_amb = 0;
     for (int x = 0; x < nlists; x++) {
         if (bufdir[x].ts == 0 && bufdir[x].tc == 0)

src/detect.c

@@ -1175,18 +1175,15 @@ static bool DetectRunTxInspectRule(ThreadVars *tv,
             break;
         } else if (!(inspect_flags & BIT_U32(engine->id)) && s->flags & SIG_FLAG_BOTHDIR &&
                    direction != engine->dir) {
-            const bool skip_engine = (engine->alproto != 0 && engine->alproto != f->alproto);
-            /* special case: 'alert http' will have engines
-             * for both HTTP1 and HTTP2. */
-            if (unlikely(skip_engine)) {
-                engine = engine->next;
-                continue;
+            // for bidirectional rules, the engines on the opposite direction
+            // are ordered by progress on the different side
+            // so we have a two mixed-up lists, and we skip the elements
+            if (direction == 0 && engine->next == NULL) {
+                // do not match yet on request only
+                break;
             }
-            // for bidirectional rules, for engines on the opposite direction
-            // as the current packet, check if we have enough progress in the tx.
-            // That means, do not return a full match, if we have only the request
-            // as there are response engines to inspect later.
-            break;
+            engine = engine->next;
+            continue;
         }
 
         engine = engine->next;

src/detect.h

@@ -292,6 +292,8 @@ typedef struct DetectPort_ {
     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_JA3                   BIT_U32(10) /**< signature has ja3 keyword */
+#define SIG_FLAG_INIT_BIDIR_TOCLIENT        BIT_U32(11) /**< signature now takes keywords toclient */
+#define SIG_FLAG_INIT_BIDIR_TOSERVER        BIT_U32(12) /**< signature now takes keywords toserver */
 
 /* signature mask flags */
 /** \note: additions should be added to the rule analyzer as well */
@@ -524,6 +526,8 @@ typedef struct SignatureInitDataBuffer_ {
                      set up. */
     bool multi_capable; /**< true if we can have multiple instances of this buffer, so e.g. for
                            http.uri. */
+    bool only_tc;       /**< true if we can only used toclient. */
+    bool only_ts;       /**< true if we can only used toserver. */
     /* sig match list */
     SigMatch *head;
     SigMatch *tail;