From 4cd736fcc9d0e0218ec32037100212a8db158f1b Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Tue, 16 Apr 2013 21:47:42 +0200 Subject: [PATCH] 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. --- src/detect-flowvar.c | 121 +++++++++++++++++++++++++++++++++++++++++++ src/detect-flowvar.h | 4 ++ src/detect-pcre.c | 13 ++++- src/detect.c | 1 + src/detect.h | 13 +++++ 5 files changed, 150 insertions(+), 2 deletions(-) diff --git a/src/detect-flowvar.c b/src/detect-flowvar.c index 9379a64a4a..0ec0c04675 100644 --- 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 1f57be21d1..96d0f5aa64 100644 --- 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 26f576755e..b7a1ba58cc 100644 --- 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 532a2c4bf1..7e8efbcaaf 100644 --- 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 30798581ac..187b7520d4 100644 --- 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,