From 568f478d41338629f1e8a4e19e69943efe370db0 Mon Sep 17 00:00:00 2001 From: Eric Leblond Date: Thu, 2 Oct 2014 11:16:20 +0200 Subject: [PATCH] detect: avoid race condition on app layer used Under serious load, it is possible that a app layer get changed on a flow when another packet of the Flow is still examined in Detect. The consequence is that it is possible to app layer to get updated and the rest of the detection run with the new app layer that don't have the same property (such as Tx handling function which are called on TLS session). This is mainly the case when alproto is fetched from Flow and then the Flow is unlocked and modifiable by another thread. When alstate is fetch later, we can have alstate not matching alproto and this causes crashes. To fix that this patch is using alindex to access to the original application layer during the detection. This means some function prototype have been update to use alindex instead of alproto. Also the FlowGet*AtIndex function are used with alindex param to access to the correct application layer. For reference, here's one the backtrace: (gdb) bt #0 0x0000000000000000 in ?? () #1 0x00000000004310e3 in AppLayerParserSetTransactionInspectId (pstate=0x151743b10, ipproto=ipproto@entry=6 '\006', alproto=alproto@entry=4, alstate=alstate@entry=0x14d47b790, direction=direction@entry=4 '\004') at app-layer-parser.c:536 #2 0x000000000048e6d7 in DeStateUpdateInspectTransactionId (f=0x7ffefc68ba00, direction=4 '\004') at detect-engine-state.c:785 #3 0x000000000045c034 in SigMatchSignatures (th_v=0x3420fa50, de_ctx=0x23b6f00, det_ctx=0x13fd68ea0, p=) at detect.c:1589 #4 0x000000000045c9f3 in Detect (data=, p=, tv=, pq=, postpq=) at detect.c:1744 #5 Detect (tv=, p=, data=, pq=, postpq=) at detect.c:1716 #6 0x000000000053d24d in TmThreadsSlotVarRun (tv=0x3420fa50, p=0x13fd56920, slot=0x14d47b790, slot@entry=0x13db1ecc0) at tm-threads.c:575 #7 0x00000000005186ba in TmThreadsSlotProcessPkt (p=0x13fd56920, s=0x13db1ecc0, tv=0x3420fa50) at tm-threads.h:148 #8 AFPReadFromRing (ptv=ptv@entry=0x13b029bd0) at source-af-packet.c:875 #9 0x000000000051b5fd in ReceiveAFPLoop (tv=, data=0x13b029bd0, slot=) at source-af-packet.c:1215 #10 0x00000000005408db in TmThreadsSlotPktAcqLoop (td=0x3420fa50) at tm-threads.c:722 #11 0x00007ffff6920b50 in start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0 #12 0x00007ffff51e8a7d in clone () from /lib/x86_64-linux-gnu/libc.so.6 #13 0x0000000000000000 in ?? () --- src/detect-engine-state.c | 34 ++++++++++++++++++++-------------- src/detect-engine-state.h | 7 ++++--- src/detect.c | 30 +++++++++++++++++++----------- src/flow.c | 20 ++++++++++++++++++++ src/flow.h | 4 ++++ 5 files changed, 67 insertions(+), 28 deletions(-) diff --git a/src/detect-engine-state.c b/src/detect-engine-state.c index 09815451a539..f62d1472ce3e 100644 --- a/src/detect-engine-state.c +++ b/src/detect-engine-state.c @@ -217,17 +217,19 @@ void DetectEngineStateFree(DetectEngineState *state) * \retval 1 inspectable state * \retval 2 inspectable state, but no update */ -int DeStateFlowHasInspectableState(Flow *f, AppProto alproto, uint16_t alversion, uint8_t flags) +int DeStateFlowHasInspectableState(Flow *f, uint8_t alindex, uint16_t alversion, uint8_t flags) { + AppProto alproto; int r = 0; + alproto = FlowGetAppProtocolAtIndex(f, alindex); SCMutexLock(&f->de_state_m); if (f->de_state == NULL || f->de_state->dir_state[flags & STREAM_TOSERVER ? 0 : 1].cnt == 0) { if (AppLayerParserProtocolSupportsTxs(f->proto, alproto)) { FLOWLOCK_RDLOCK(f); - if (FlowGetAppParser(f) != NULL && FlowGetAppState(f) != NULL) { - if (AppLayerParserGetTransactionInspectId(FlowGetAppParser(f), flags) >= - AppLayerParserGetTxCnt(f->proto, alproto, FlowGetAppState(f))) { + if (FlowGetAppParserAtIndex(f, alindex) != NULL && FlowGetAppStateAtIndex(f, alindex) != NULL) { + if (AppLayerParserGetTransactionInspectId(FlowGetAppParserAtIndex(f, alindex), flags) >= + AppLayerParserGetTxCnt(f->proto, alproto, FlowGetAppStateAtIndex(f, alindex))) { r = 2; } } @@ -485,7 +487,7 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx, Packet *p, Flow *f, uint8_t flags, - AppProto alproto, uint16_t alversion) + uint8_t alindex, uint16_t alversion) { SCMutexLock(&f->de_state_m); @@ -503,6 +505,8 @@ void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, int match = 0; uint8_t alert = 0; + AppProto alproto = FlowGetAppProtocolAtIndex(f, alindex); + DetectEngineStateDirection *dir_state = &f->de_state->dir_state[flags & STREAM_TOSERVER ? 0 : 1]; DeStateStore *store = dir_state->head; void *inspect_tx = NULL; @@ -519,14 +523,14 @@ void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, if (AppLayerParserProtocolSupportsTxs(f->proto, alproto)) { FLOWLOCK_RDLOCK(f); - alstate = FlowGetAppState(f); + alstate = FlowGetAppStateAtIndex(f, alindex); if (alstate == NULL) { FLOWLOCK_UNLOCK(f); SCMutexUnlock(&f->de_state_m); return; } - inspect_tx_id = AppLayerParserGetTransactionInspectId(FlowGetAppParser(f), + inspect_tx_id = AppLayerParserGetTransactionInspectId(FlowGetAppParserAtIndex(f, alindex), flags); total_txs = AppLayerParserGetTxCnt(f->proto, alproto, alstate); inspect_tx = AppLayerParserGetTx(f->proto, alproto, alstate, inspect_tx_id); @@ -610,7 +614,7 @@ void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, if (alproto_supports_txs) { FLOWLOCK_WRLOCK(f); - alstate = FlowGetAppState(f); + alstate = FlowGetAppStateAtIndex(f, alindex); if (alstate == NULL) { FLOWLOCK_UNLOCK(f); RULE_PROFILING_END(det_ctx, s, match, p); @@ -676,7 +680,7 @@ void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, /* RDLOCK would be nicer, but at least tlsstore needs * write lock currently. */ FLOWLOCK_WRLOCK(f); - alstate = FlowGetAppState(f); + alstate = FlowGetAppStateAtIndex(f, alindex); if (alstate == NULL) { FLOWLOCK_UNLOCK(f); RULE_PROFILING_END(det_ctx, s, 0 /* no match */, p); @@ -749,7 +753,9 @@ void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, DeStateStoreStateVersion(f->de_state, alversion, flags); DeStateStoreFileNoMatchCnt(f->de_state, file_no_match, flags); - if (!(dir_state->flags & DETECT_ENGINE_STATE_FLAG_FILE_STORE_DISABLED)) { + /* Don't enter here if we have a protocol change */ + if (!(dir_state->flags & DETECT_ENGINE_STATE_FLAG_FILE_STORE_DISABLED) && + (alproto == FlowGetAppProtocol(f))) { if (DeStateStoreFilestoreSigsCantMatch(det_ctx->sgh, f->de_state, flags) == 1) { SCLogDebug("disabling file storage for transaction"); @@ -778,14 +784,14 @@ void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, /** \brief update flow's inspection id's * * \note it is possible that f->alstate, f->alparser are NULL */ -void DeStateUpdateInspectTransactionId(Flow *f, uint8_t direction) +void DeStateUpdateInspectTransactionId(Flow *f, uint8_t alindex, uint8_t direction) { FLOWLOCK_WRLOCK(f); if (FlowGetAppParser(f) && FlowGetAppState(f)) { - AppLayerParserSetTransactionInspectId(FlowGetAppParser(f), + AppLayerParserSetTransactionInspectId(FlowGetAppParserAtIndex(f, alindex), f->proto, - FlowGetAppProtocol(f), - FlowGetAppState(f), + FlowGetAppProtocolAtIndex(f, alindex), + FlowGetAppStateAtIndex(f, alindex), direction); } FLOWLOCK_UNLOCK(f); diff --git a/src/detect-engine-state.h b/src/detect-engine-state.h index 9f497551185f..9135c89aee4b 100644 --- a/src/detect-engine-state.h +++ b/src/detect-engine-state.h @@ -151,7 +151,7 @@ void DetectEngineStateFree(DetectEngineState *state); * \retval 1 Has state. * \retval 0 Has no state. */ -int DeStateFlowHasInspectableState(Flow *f, AppProto alproto, uint16_t alversion, uint8_t flags); +int DeStateFlowHasInspectableState(Flow *f, uint8_t alindex, uint16_t alversion, uint8_t flags); /** * \brief Match app layer sig list against app state and store relevant match @@ -187,15 +187,16 @@ int DeStateDetectStartDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, void DeStateDetectContinueDetection(ThreadVars *tv, DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx, Packet *p, Flow *f, uint8_t flags, - AppProto alproto, uint16_t alversion); + uint8_t alindex, uint16_t alversion); /** * \brief Update the inspect id. * * \param f Flow(unlocked). + * \param alindex index of applayer to update * \param direction 0 for to server, 1 for toclient. */ -void DeStateUpdateInspectTransactionId(Flow *f, uint8_t direction); +void DeStateUpdateInspectTransactionId(Flow *f, uint8_t alindex, uint8_t direction); /** * \brief Reset a DetectEngineState state. diff --git a/src/detect.c b/src/detect.c index 794ca38ba295..925c7e02eea9 100644 --- a/src/detect.c +++ b/src/detect.c @@ -794,8 +794,14 @@ static StreamMsg *SigMatchSignaturesGetSmsg(Flow *f, Packet *p, uint8_t flags) */ static inline void DetectMpmPrefilter(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx, StreamMsg *smsg, Packet *p, - uint8_t flags, AppProto alproto, int has_state, uint8_t *sms_runflags) + uint8_t flags, uint8_t alindex, int has_state, uint8_t *sms_runflags) { + AppProto alproto = ALPROTO_UNKNOWN; + + if (p->flow) { + alproto = FlowGetAppProtocolAtIndex(p->flow, alindex); + } + /* have a look at the reassembled stream (if any) */ if (p->flowflags & FLOW_PKT_ESTABLISHED) { SCLogDebug("p->flowflags & FLOW_PKT_ESTABLISHED"); @@ -803,7 +809,7 @@ static inline void DetectMpmPrefilter(DetectEngineCtx *de_ctx, /* all http based mpms */ if (has_state && alproto == ALPROTO_HTTP) { FLOWLOCK_WRLOCK(p->flow); - void *alstate = FlowGetAppState(p->flow); + void *alstate = FlowGetAppStateAtIndex(p->flow, alindex); if (alstate == NULL) { SCLogDebug("no alstate"); FLOWLOCK_UNLOCK(p->flow); @@ -818,7 +824,7 @@ static inline void DetectMpmPrefilter(DetectEngineCtx *de_ctx, } int tx_progress = 0; - uint64_t idx = AppLayerParserGetTransactionInspectId(FlowGetAppParser(p->flow), + uint64_t idx = AppLayerParserGetTransactionInspectId(FlowGetAppParserAtIndex(p->flow, alindex), flags); uint64_t total_txs = AppLayerParserGetTxCnt(p->flow->proto, ALPROTO_HTTP, alstate); for (; idx < total_txs; idx++) { @@ -937,14 +943,14 @@ static inline void DetectMpmPrefilter(DetectEngineCtx *de_ctx, if (p->flowflags & FLOW_PKT_TOSERVER) { if (det_ctx->sgh->flags & SIG_GROUP_HEAD_MPM_DNSQUERY) { FLOWLOCK_RDLOCK(p->flow); - void *alstate = FlowGetAppState(p->flow); + void *alstate = FlowGetAppStateAtIndex(p->flow, alindex); if (alstate == NULL) { SCLogDebug("no alstate"); FLOWLOCK_UNLOCK(p->flow); return; } - uint64_t idx = AppLayerParserGetTransactionInspectId(FlowGetAppParser(p->flow), + uint64_t idx = AppLayerParserGetTransactionInspectId(FlowGetAppParserAtIndex(p->flow, alindex), flags); uint64_t total_txs = AppLayerParserGetTxCnt(p->flow->proto, alproto, alstate); for (; idx < total_txs; idx++) { @@ -1000,14 +1006,14 @@ static inline void DetectMpmPrefilter(DetectEngineCtx *de_ctx, SCLogDebug("mpm inspection"); if (det_ctx->sgh->flags & SIG_GROUP_HEAD_MPM_DNSQUERY) { FLOWLOCK_RDLOCK(p->flow); - void *alstate = FlowGetAppState(p->flow); + void *alstate = FlowGetAppStateAtIndex(p->flow, alindex); if (alstate == NULL) { SCLogDebug("no alstate"); FLOWLOCK_UNLOCK(p->flow); return; } - uint64_t idx = AppLayerParserGetTransactionInspectId(FlowGetAppParser(p->flow), + uint64_t idx = AppLayerParserGetTransactionInspectId(FlowGetAppParserAtIndex(p->flow, alindex), flags); uint64_t total_txs = AppLayerParserGetTxCnt(p->flow->proto, alproto, alstate); for (; idx < total_txs; idx++) { @@ -1113,6 +1119,7 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh uint8_t sms_runflags = 0; /* function flags */ uint8_t alert_flags = 0; AppProto alproto = ALPROTO_UNKNOWN; + uint8_t alindex = 0; #ifdef PROFILING int smatch = 0; /* signature match: 1, no match: 0 */ #endif @@ -1210,6 +1217,7 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh { has_state = (FlowGetAppState(pflow) != NULL); alproto = FlowGetAppProtocol(pflow); + alindex = FlowGetAppLayerIndex(pflow); alversion = AppLayerParserGetStateVersion(FlowGetAppParser(pflow)); SCLogDebug("alstate %s, alproto %u", has_state ? "true" : "false", alproto); } else { @@ -1307,10 +1315,10 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh 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); + int has_inspectable_state = DeStateFlowHasInspectableState(pflow, alindex, alversion, flags); if (has_inspectable_state == 1) { DeStateDetectContinueDetection(th_v, de_ctx, det_ctx, p, pflow, - flags, alproto, alversion); + flags, alindex, alversion); } else if (has_inspectable_state == 2) { /* no inspectable state, so pretend we don't have a state at all */ has_state = 0; @@ -1324,7 +1332,7 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh /* run the mpm for each type */ PACKET_PROFILING_DETECT_START(p, PROF_DETECT_MPM); - DetectMpmPrefilter(de_ctx, det_ctx, smsg, p, flags, alproto, has_state, &sms_runflags); + DetectMpmPrefilter(de_ctx, det_ctx, smsg, p, flags, alindex, has_state, &sms_runflags); PACKET_PROFILING_DETECT_END(p, PROF_DETECT_MPM); PACKET_PROFILING_DETECT_START(p, PROF_DETECT_PREFILTER); @@ -1583,7 +1591,7 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh /* see if we need to increment the inspect_id and reset the de_state */ if (has_state && AppLayerParserProtocolSupportsTxs(p->proto, alproto)) { PACKET_PROFILING_DETECT_START(p, PROF_DETECT_STATEFUL); - DeStateUpdateInspectTransactionId(pflow, flags); + DeStateUpdateInspectTransactionId(pflow, alindex, flags); PACKET_PROFILING_DETECT_END(p, PROF_DETECT_STATEFUL); } diff --git a/src/flow.c b/src/flow.c index 2abd113b9bbf..81c7d90fc5e1 100644 --- a/src/flow.c +++ b/src/flow.c @@ -835,21 +835,41 @@ int FlowSetProtoEmergencyTimeout(uint8_t proto, uint32_t emerg_new_timeout, return 1; } +uint8_t FlowGetAppLayerIndex(const Flow *f) +{ + return f->applayer_index; +} + AppProto FlowGetAppProtocol(const Flow *f) { return FLOW_GET_CURRENT_APPLAYER(f).alproto; } +AppProto FlowGetAppProtocolAtIndex(const Flow *f, uint8_t index) +{ + return FLOW_GET_APPLAYER(f, index).alproto; +} + void *FlowGetAppState(const Flow *f) { return FLOW_GET_CURRENT_APPLAYER(f).alstate; } +void *FlowGetAppStateAtIndex(const Flow *f, uint8_t index) +{ + return FLOW_GET_APPLAYER(f, index).alstate; +} + AppLayerParserState *FlowGetAppParser(const Flow *f) { return FLOW_GET_CURRENT_APPLAYER(f).alparser; } +AppLayerParserState *FlowGetAppParserAtIndex(const Flow *f, uint8_t index) +{ + return FLOW_GET_APPLAYER(f, index).alparser; +} + void FlowSetAppProtocol(Flow *f, AppProto proto) { FLOW_GET_CURRENT_APPLAYER(f).alproto = proto; diff --git a/src/flow.h b/src/flow.h index 55cd158f3d3d..d1b3b3d93526 100644 --- a/src/flow.h +++ b/src/flow.h @@ -582,6 +582,10 @@ AppLayerParserState *FlowGetAppParser(const Flow *f); void FlowSetAppProtocol(Flow *f, AppProto proto); void FlowSetAppState(Flow *f, void *state); void FlowSetAppParser(Flow *f, void *parser); +uint8_t FlowGetAppLayerIndex(const Flow *f); +AppProto FlowGetAppProtocolAtIndex(const Flow *f, uint8_t index); +void *FlowGetAppStateAtIndex(const Flow *f, uint8_t index); +AppLayerParserState *FlowGetAppParserAtIndex(const Flow *f, uint8_t index);