From: Victor Julien Date: Tue, 24 Mar 2015 14:36:39 +0000 (+0100) Subject: detect-state: handle duplicate inspect/match X-Git-Tag: suricata-3.0RC1~434 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=37f0bd57b68f888689e1fb664d6dd05054cc7d9f;p=thirdparty%2Fsuricata.git detect-state: handle duplicate inspect/match If for a packet we have a TX N that has detect state and a TX N+1 that has no detect state, but does have 'progress', we have a corner case in stateful detection. ContinueDetection inspects TX N, but cannot flag the rule in the de_state_sig_array as the next (TX N+1) has already started and needs to be inspected. 'StartDetection' however, is then unaware of the fact that ContinueDetection already inspected the rule. It uses the per session 'inspect_id' that is only moved forward at the end of the detection run. This patch adds a workaround. It uses the DetectEngineThreadCtx:: de_state_sig_array to store an offset between the 'base' inspect_id and the inspect_id that StartDetection should use. The data type is limited, so if the offset would be too big, a search based fall back is implemented as well. --- diff --git a/src/detect-engine-state.c b/src/detect-engine-state.c index 8cbc5b8540..110d236f84 100644 --- a/src/detect-engine-state.c +++ b/src/detect-engine-state.c @@ -81,6 +81,18 @@ /** convert enum to string */ #define CASE_CODE(E) case E: return #E +/** The DetectEngineThreadCtx::de_state_sig_array contains 2 separate values: + * 1. the first bit tells the prefilter engine to bypass the rule (or not) + * 2. the other bits allow 'ContinueDetect' to specify an offset again the + * base tx id. This offset will then be used by 'StartDetect' to not + * inspect transactions again for the same signature. + * + * The offset in (2) has a max value due to the limited data type. If it is + * set to max the code will fall back to a slower path that validates that + * we're not adding duplicate rules to the detection state. + */ +#define MAX_STORED_TXID_OFFSET 127 + /******** static internal helpers *********/ static inline int StateIsValid(uint16_t alproto, void *alstate) @@ -124,7 +136,6 @@ static DeStateStoreFlowRules *DeStateStoreFlowRulesAlloc(void) return d; } -#ifdef DEBUG_VALIDATION static int DeStateSearchState(DetectEngineState *state, uint8_t direction, SigIntId num) { DetectEngineStateDirection *dir_state = &state->dir_state[direction & STREAM_TOSERVER ? 0 : 1]; @@ -140,7 +151,7 @@ static int DeStateSearchState(DetectEngineState *state, uint8_t direction, SigIn { DeStateStoreItem *item = &tx_store->store[store_cnt]; if (item->sid == num) { - SCLogDebug("BUG! sid %u already in state: %p %p %p %u %u, direction %s", + SCLogDebug("sid %u already in state: %p %p %p %u %u, direction %s", num, state, dir_state, tx_store, state_cnt, store_cnt, direction & STREAM_TOSERVER ? "toserver" : "toclient"); return 1; @@ -149,7 +160,6 @@ static int DeStateSearchState(DetectEngineState *state, uint8_t direction, SigIn } return 0; } -#endif static void DeStateSignatureAppend(DetectEngineState *state, Signature *s, uint32_t inspect_flags, uint8_t direction) { @@ -424,11 +434,14 @@ static void StoreStateTxFileOnly(DetectEngineThreadCtx *det_ctx, } } +/** + * \param check_before_add check for duplicates before adding the sig + */ static void StoreStateTx(DetectEngineThreadCtx *det_ctx, Flow *f, const uint8_t flags, const uint16_t alversion, const uint64_t tx_id, void *tx, Signature *s, SigMatch *sm, - const uint32_t inspect_flags, const uint16_t file_no_match) + const uint32_t inspect_flags, const uint16_t file_no_match, int check_before_add) { if (AppLayerParserSupportsTxDetectState(f->proto, f->alproto)) { DetectEngineState *destate = AppLayerParserGetTxDetectState(f->proto, f->alproto, tx); @@ -444,7 +457,8 @@ static void StoreStateTx(DetectEngineThreadCtx *det_ctx, SCLogDebug("destate created for %"PRIu64, tx_id); } - DeStateSignatureAppend(destate, s, inspect_flags, flags); + if (check_before_add == 0 || DeStateSearchState(destate, flags, s->num) == 0) + DeStateSignatureAppend(destate, s, inspect_flags, flags); DeStateStoreStateVersion(f, alversion, flags); StoreStateTxHandleFiles(det_ctx, f, destate, flags, tx_id, file_no_match); @@ -462,6 +476,7 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, uint32_t inspect_flags = 0; uint8_t direction = (flags & STREAM_TOSERVER) ? 0 : 1; int alert_cnt = 0; + int check_before_add = 0; FLOWLOCK_WRLOCK(f); /* TX based matches (inspect engines) */ @@ -474,7 +489,18 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, goto end; } + /* if continue detection already inspected this rule for this tx, + * continue with the first not-inspected tx */ + uint8_t offset = det_ctx->de_state_sig_array[s->num] & 0xef; tx_id = AppLayerParserGetTransactionInspectId(f->alparser, flags); + if (offset > 0) { + SCLogDebug("using stored_tx_id %u instead of %u", (uint)tx_id+offset, (uint)tx_id); + tx_id += offset; + } + if (offset == MAX_STORED_TXID_OFFSET) { + check_before_add = 1; + } + total_txs = AppLayerParserGetTxCnt(f->proto, alproto, alstate); SCLogDebug("total_txs %"PRIu64, total_txs); @@ -557,7 +583,7 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, /* store */ StoreStateTx(det_ctx, f, flags, alversion, tx_id, tx, - s, sm, inspect_flags, file_no_match); + s, sm, inspect_flags, file_no_match, check_before_add); } else { StoreStateTxFileOnly(det_ctx, f, flags, tx_id, tx, file_no_match); } @@ -704,6 +730,16 @@ static int DoInspectItem(ThreadVars *tv, SCLogDebug("skip and bypass: tx %u packet %u", (uint)inspect_tx_id, (uint)p->pcap_cnt); } else { SCLogDebug("just skip: tx %u packet %u", (uint)inspect_tx_id, (uint)p->pcap_cnt); + + /* make sure that if we reinspect this right now from + * start detection, we skip this tx we just matched on */ + uint64_t base_tx_id = AppLayerParserGetTransactionInspectId(f->alparser, flags); + uint64_t offset = (inspect_tx_id + 1) - base_tx_id; + if (offset > MAX_STORED_TXID_OFFSET) + offset = MAX_STORED_TXID_OFFSET; + det_ctx->de_state_sig_array[item->sid] = (uint8_t)offset; + BUG_ON(det_ctx->de_state_sig_array[item->sid] & DE_STATE_MATCH_NO_NEW_STATE); // check that we don't set the bit + SCLogDebug("storing tx_id %u for this sid", (uint)inspect_tx_id + 1); } return 0; } @@ -730,6 +766,16 @@ static int DoInspectItem(ThreadVars *tv, SCLogDebug("skip and bypass: tx %u packet %u", (uint)inspect_tx_id, (uint)p->pcap_cnt); } else { SCLogDebug("just skip: tx %u packet %u", (uint)inspect_tx_id, (uint)p->pcap_cnt); + + /* make sure that if we reinspect this right now from + * start detection, we skip this tx we just matched on */ + uint64_t base_tx_id = AppLayerParserGetTransactionInspectId(f->alparser, flags); + uint64_t offset = (inspect_tx_id + 1) - base_tx_id; + if (offset > MAX_STORED_TXID_OFFSET) + offset = MAX_STORED_TXID_OFFSET; + det_ctx->de_state_sig_array[item->sid] = (uint8_t)offset; + BUG_ON(det_ctx->de_state_sig_array[item->sid] & DE_STATE_MATCH_NO_NEW_STATE); // check that we don't set the bit + SCLogDebug("storing tx_id %u for this sid", (uint)inspect_tx_id + 1); } return 0; } @@ -794,6 +840,17 @@ static int DoInspectItem(ThreadVars *tv, if (TxIsLast(inspect_tx_id, total_txs) || inprogress || next_tx_no_progress) { det_ctx->de_state_sig_array[item->sid] = DE_STATE_MATCH_NO_NEW_STATE; SCLogDebug("inspected, now bypass: tx %u packet %u", (uint)inspect_tx_id, (uint)p->pcap_cnt); + } else { + /* make sure that if we reinspect this right now from + * start detection, we skip this tx we just matched on */ + uint64_t base_tx_id = AppLayerParserGetTransactionInspectId(f->alparser, flags); + uint64_t offset = (inspect_tx_id + 1) - base_tx_id; + if (offset > MAX_STORED_TXID_OFFSET) + offset = MAX_STORED_TXID_OFFSET; + det_ctx->de_state_sig_array[item->sid] = (uint8_t)offset; + BUG_ON(det_ctx->de_state_sig_array[item->sid] & DE_STATE_MATCH_NO_NEW_STATE); // check that we don't set the bit + + SCLogDebug("storing tx_id %u for this sid", (uint)inspect_tx_id + 1); } RULE_PROFILING_END(det_ctx, s, (alert == 1), p); @@ -1094,17 +1151,6 @@ void DetectEngineStateResetTxs(Flow *f) } } -/** \brief get string for match enum */ -const char *DeStateMatchResultToString(DeStateMatchResult res) -{ - switch (res) { - CASE_CODE (DE_STATE_MATCH_NO_NEW_STATE); - CASE_CODE (DE_STATE_MATCH_HAS_NEW_STATE); - } - - return NULL; -} - /*********Unittests*********/ #ifdef UNITTESTS diff --git a/src/detect-engine-state.h b/src/detect-engine-state.h index ab30ccffed..7ef4df2c45 100644 --- a/src/detect-engine-state.h +++ b/src/detect-engine-state.h @@ -100,10 +100,8 @@ * the HAS_NEW_STATE flag, while if we don't have a new tx, we set * NO_NEW_STATE, to avoid getting the sig reinspected for the already * inspected tx. */ -typedef enum { - DE_STATE_MATCH_HAS_NEW_STATE = 0, - DE_STATE_MATCH_NO_NEW_STATE, -} DeStateMatchResult; +#define DE_STATE_MATCH_HAS_NEW_STATE 0x00 +#define DE_STATE_MATCH_NO_NEW_STATE 0x80 /* TX BASED (inspect engines) */ diff --git a/src/detect.c b/src/detect.c index e446d37459..8ff4066805 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1374,10 +1374,10 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh PACKET_PROFILING_DETECT_START(p, PROF_DETECT_STATEFUL); /* stateful app layer detection */ if ((p->flags & PKT_HAS_FLOW) && has_state) { - /* initialize to 0(DE_STATE_MATCH_HAS_NEW_STATE) */ memset(det_ctx->de_state_sig_array, 0x00, det_ctx->de_state_sig_array_len); int has_inspectable_state = DeStateFlowHasInspectableState(pflow, alproto, alversion, flags); if (has_inspectable_state == 1) { + /* initialize to 0(DE_STATE_MATCH_HAS_NEW_STATE) */ DeStateDetectContinueDetection(th_v, de_ctx, det_ctx, p, pflow, flags, alproto, alversion); } else if (has_inspectable_state == 2) { @@ -1500,7 +1500,7 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh } } if (sflags & SIG_FLAG_STATE_MATCH) { - if (det_ctx->de_state_sig_array[s->num] == DE_STATE_MATCH_NO_NEW_STATE) + if (det_ctx->de_state_sig_array[s->num] & DE_STATE_MATCH_NO_NEW_STATE) goto next; }