flowvar: fix deadlock with http buffers

Bug #802

Flowvars are set from pcre, and lock the flow when being set. However
when HTTP buffers were inspected, flow was already locked: deadlock.

This patch introduces a post-match list in the detection engine thread
ctx, where store candidates are kept. Then a post-match function is used
to finalize the storing if the rule matches.

Solves the deadlock and brings the handling of flowvars more in line
with flowbits and flowints.

diff --git a/src/detect-flowvar.c b/src/detect-flowvar.c
index 9379a64a4a4aec15587cf5034f63a961fa99766a..0ec0c046752dff433a80dacfc1a8a030497d9ff2 100644 (file)
--- a/src/detect-flowvar.c
+++ b/src/detect-flowvar.c
@@ -44,6 +44,7 @@ static pcre_extra *parse_regex_study;
 
 int DetectFlowvarMatch (ThreadVars *, DetectEngineThreadCtx *, Packet *, Signature *, SigMatch *);
 static int DetectFlowvarSetup (DetectEngineCtx *, Signature *, char *);
+static int DetectFlowvarPostMatch(ThreadVars *tv, DetectEngineThreadCtx *det_ctx, Packet *p, Signature *s, SigMatch *sm);
 
 void DetectFlowvarRegister (void) {
     sigmatch_table[DETECT_FLOWVAR].name = "flowvar";
@@ -52,6 +53,13 @@ void DetectFlowvarRegister (void) {
     sigmatch_table[DETECT_FLOWVAR].Free  = NULL;
     sigmatch_table[DETECT_FLOWVAR].RegisterTests  = NULL;
 
+    /* post-match for flowvar storage */
+    sigmatch_table[DETECT_FLOWVAR_POSTMATCH].name = "__flowvar__postmatch__";
+    sigmatch_table[DETECT_FLOWVAR_POSTMATCH].Match = DetectFlowvarPostMatch;
+    sigmatch_table[DETECT_FLOWVAR_POSTMATCH].Setup = NULL;
+    sigmatch_table[DETECT_FLOWVAR_POSTMATCH].Free  = NULL;
+    sigmatch_table[DETECT_FLOWVAR_POSTMATCH].RegisterTests  = NULL;
+
     const char *eb;
     int eo;
     int opts = 0;
@@ -249,3 +257,116 @@ error:
 }
 
 
+/** \brief Store flowvar in det_ctx so we can exec it post-match */
+int DetectFlowvarStoreMatch(DetectEngineThreadCtx *det_ctx, uint16_t idx, uint8_t *buffer, uint16_t len) {
+    DetectFlowvarList *fs = det_ctx->flowvarlist;
+
+    /* first check if we have had a previous match for this idx */
+    for ( ; fs != NULL; fs = fs->next) {
+        if (fs->idx == idx) {
+            /* we're replacing the older store */
+            SCFree(fs->buffer);
+            fs->buffer = NULL;
+            break;
+        }
+    }
+
+    if (fs == NULL) {
+        fs = SCMalloc(sizeof(*fs));
+        if (unlikely(fs == NULL))
+            return -1;
+
+        fs->idx = idx;
+
+        fs->next = det_ctx->flowvarlist;
+        det_ctx->flowvarlist = fs;
+    }
+
+    fs->len = len;
+    fs->buffer = buffer;
+    return 0;
+}
+
+/** \brief Setup a post-match for flowvar storage
+ *  We're piggyback riding the DetectFlowvarData struct
+ */
+int DetectFlowvarPostMatchSetup(Signature *s, uint16_t idx) {
+    SigMatch *sm = NULL;
+    DetectFlowvarData *fv = NULL;
+
+    fv = SCMalloc(sizeof(DetectFlowvarData));
+    if (unlikely(fv == NULL))
+        goto error;
+    memset(fv, 0x00, sizeof(*fv));
+
+    /* we only need the idx */
+    fv->idx = idx;
+
+    sm = SigMatchAlloc();
+    if (sm == NULL)
+        goto error;
+
+    sm->type = DETECT_FLOWVAR_POSTMATCH;
+    sm->ctx = (void *)fv;
+
+    SigMatchAppendSMToList(s, sm, DETECT_SM_LIST_POSTMATCH);
+    return 0;
+error:
+    return -1;
+}
+
+/** \internal
+ *  \brief post-match func to store flowvars in the flow
+ *  \param sm sigmatch containing the idx to store
+ *  \retval 1 or -1 in case of error
+ */
+static int DetectFlowvarPostMatch(ThreadVars *tv, DetectEngineThreadCtx *det_ctx, Packet *p, Signature *s, SigMatch *sm) {
+    DetectFlowvarList *fs, *prev;
+    DetectFlowvarData *fd;
+
+    if (det_ctx->flowvarlist == NULL || p->flow == NULL)
+        return 1;
+
+    fd = (DetectFlowvarData *)sm->ctx;
+
+    prev = NULL;
+    fs = det_ctx->flowvarlist;
+    while (fs != NULL) {
+        if (fd->idx == fs->idx) {
+            FlowVarAddStr(p->flow, fs->idx, fs->buffer, fs->len);
+            /* memory at fs->buffer is now the responsibility of
+             * the flowvar code. */
+
+            if (fs == det_ctx->flowvarlist) {
+                det_ctx->flowvarlist = fs->next;
+                SCFree(fs);
+                fs = det_ctx->flowvarlist;
+            } else {
+                prev->next = fs->next;
+                SCFree(fs);
+                fs = prev->next;
+            }
+        } else {
+            prev = fs;
+            fs = fs->next;
+        }
+    }
+    return 1;
+}
+
+/** \brief Clean flowvar candidate list in det_ctx */
+void DetectFlowvarCleanupList(DetectEngineThreadCtx *det_ctx) {
+    DetectFlowvarList *fs, *next;
+    if (det_ctx->flowvarlist != NULL) {
+        fs = det_ctx->flowvarlist;
+        while (fs != NULL) {
+            next = fs->next;
+            SCFree(fs->buffer);
+            SCFree(fs);
+            fs = next;
+        }
+
+        det_ctx->flowvarlist = NULL;
+    }
+}
+

diff --git a/src/detect-flowvar.h b/src/detect-flowvar.h
index 1f57be21d1a332d48afd2f6b685172f52291ecd4..96d0f5aa646ff67c2daa656f610580938dd42250 100644 (file)
--- a/src/detect-flowvar.h
+++ b/src/detect-flowvar.h
@@ -35,5 +35,9 @@ typedef struct DetectFlowvarData_ {
 /* prototypes */
 void DetectFlowvarRegister (void);
 
+int DetectFlowvarPostMatchSetup(Signature *s, uint16_t idx);
+int DetectFlowvarStoreMatch(DetectEngineThreadCtx *, uint16_t, uint8_t *, uint16_t);
+void DetectFlowvarCleanupList(DetectEngineThreadCtx *det_ctx);
+
 #endif /* __DETECT_FLOWVAR_H__ */
 

diff --git a/src/detect-pcre.c b/src/detect-pcre.c
index 26f576755e98b1e54eb3e7e92784f52da40ec44d..b7a1ba58cc8204d22d70272536089e5d761dc585 100644 (file)
--- a/src/detect-pcre.c
+++ b/src/detect-pcre.c
@@ -34,6 +34,7 @@
 #include "flow-util.h"
 
 #include "detect-pcre.h"
+#include "detect-flowvar.h"
 
 #include "detect-parse.h"
 #include "detect-engine.h"
@@ -186,6 +187,7 @@ int DetectPcrePayloadMatch(DetectEngineThreadCtx *det_ctx, Signature *s,
     int ov[MAX_SUBSTRINGS];
     uint8_t *ptr = NULL;
     uint16_t len = 0;
+    uint16_t capture_len = 0;
 
     DetectPcreData *pe = (DetectPcreData *)sm->ctx;
 
@@ -234,8 +236,10 @@ int DetectPcrePayloadMatch(DetectEngineThreadCtx *det_ctx, Signature *s,
                         }
                     } else if (pe->flags & DETECT_PCRE_CAPTURE_FLOW) {
                         if (f != NULL) {
-                            /* flow will be locked be FlowVarAddStr */
-                            FlowVarAddStr(f, pe->capidx, (uint8_t *)str_ptr, ret);
+                            /* store max 64k. Errors are ignored */
+                            capture_len = (ret < 0xffff) ? (uint16_t)ret : 0xffff;
+                            (void)DetectFlowvarStoreMatch(det_ctx, pe->capidx,
+                                    (uint8_t *)str_ptr, capture_len);
                         }
                     }
                 }
@@ -781,6 +785,11 @@ static int DetectPcreSetup (DetectEngineCtx *de_ctx, Signature *s, char *regexst
         pd->flags |= DETECT_PCRE_RELATIVE_NEXT;
     }
 
+    if (pd->capidx != 0) {
+        if (DetectFlowvarPostMatchSetup(s, pd->capidx) < 0)
+            goto error;
+    }
+
  okay:
     ret = 0;
     SCReturnInt(ret);

diff --git a/src/detect.c b/src/detect.c
index 532a2c4bf1d5853ad4c56f26af771de8579eb80e..7e8efbcaafc26c1f74857a92116dd648157a3c1b 100644 (file)
--- a/src/detect.c
+++ b/src/detect.c
@@ -1611,6 +1611,7 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh
             p->action |= s->action;
         }
 next:
+        DetectFlowvarCleanupList(det_ctx);
         DetectReplaceFree(det_ctx->replist);
         det_ctx->replist = NULL;
         RULE_PROFILING_END(det_ctx, s, smatch);

diff --git a/src/detect.h b/src/detect.h
index 30798581ace68f94178f880b9b587cb625957caf..187b7520d4b3c9974062d6664f7e192a69598650 100644 (file)
--- a/src/detect.h
+++ b/src/detect.h
@@ -470,6 +470,16 @@ typedef struct DetectReplaceList_ {
     struct DetectReplaceList_ *next;
 } DetectReplaceList;
 
+/** list for flowvar store candidates, to be stored from
+ *  post-match function */
+typedef struct DetectFlowvarList_ {
+    uint16_t idx;                       /**< flowvar name idx */
+    uint16_t len;                       /**< data len */
+    uint8_t *buffer;                    /**< alloc'd buffer, may be freed by
+                                             post-match, post-non-match */
+    struct DetectFlowvarList_ *next;
+} DetectFlowvarList;
+
 typedef struct DetectEngineIPOnlyThreadCtx_ {
     uint8_t *sig_match_array; /* bit array of sig nums */
     uint32_t sig_match_size;  /* size in bytes of the array */
@@ -817,6 +827,8 @@ typedef struct DetectionEngineThreadCtx_ {
 
     /* string to replace */
     DetectReplaceList *replist;
+    /* flowvars to store in post match function */
+    DetectFlowvarList *flowvarlist;
 
     /* Array in which the filestore keyword stores file id and tx id. If the
      * full signature matches, these are processed by a post-match filestore
@@ -1039,6 +1051,7 @@ enum {
     DETECT_RPC,
     DETECT_DSIZE,
     DETECT_FLOWVAR,
+    DETECT_FLOWVAR_POSTMATCH,
     DETECT_FLOWINT,
     DETECT_PKTVAR,
     DETECT_NOALERT,