From fc559ce227d4254840b5f1241d5408c52879ce8c Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Tue, 15 Apr 2014 12:39:22 +0200 Subject: [PATCH] detect: fix alstate handling Previously, the alstate use in the main detect loop was unsafe. The alstate pointer would be set duing a lock, but it would again be used after one or more lock/unlock cycles. If the data pointed to would disappear, a dangling pointer would be the result. Due to they way flows are cleaned up using reference counting and such, changes of this happening were very small. However, at least one path can lead to this situation. So it had to be fixed. --- src/detect-engine-state.c | 7 ++++- src/detect.c | 60 ++++++++++++++++++++++++++------------- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/src/detect-engine-state.c b/src/detect-engine-state.c index ad54705d66..702f6b2c3d 100644 --- a/src/detect-engine-state.c +++ b/src/detect-engine-state.c @@ -761,10 +761,15 @@ end: return; } +/** \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) { FLOWLOCK_WRLOCK(f); - AppLayerParserSetTransactionInspectId(f->alparser, f->proto, f->alproto, f->alstate, direction); + if (f->alparser && f->alstate) { + AppLayerParserSetTransactionInspectId(f->alparser, f->proto, f->alproto, f->alstate, direction); + } FLOWLOCK_UNLOCK(f); return; diff --git a/src/detect.c b/src/detect.c index 55671aa691..d614c0e9a7 100644 --- a/src/detect.c +++ b/src/detect.c @@ -206,7 +206,7 @@ void DetectExitPrintStats(ThreadVars *tv, void *data); void DbgPrintSigs(DetectEngineCtx *, SigGroupHead *); void DbgPrintSigs2(DetectEngineCtx *, SigGroupHead *); -static void PacketCreateMask(Packet *, SignatureMask *, uint16_t, void *, StreamMsg *, int); +static void PacketCreateMask(Packet *, SignatureMask *, uint16_t, int, StreamMsg *, int); /* tm module api functions */ TmEcode Detect(ThreadVars *, Packet *, void *, PacketQueue *, PacketQueue *); @@ -785,20 +785,26 @@ end: * \param p Packet. * \param flags Flags. * \param alproto Flow alproto. - * \param alstate Flow alstate. + * \param has_state Bool indicating we have (al)state * \param sms_runflags Used to store state by detection engine. */ static inline void DetectMpmPrefilter(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx, StreamMsg *smsg, Packet *p, - uint8_t flags, AppProto alproto, void *alstate, uint8_t *sms_runflags) + uint8_t flags, AppProto alproto, int has_state, uint8_t *sms_runflags) { /* have a look at the reassembled stream (if any) */ if (p->flowflags & FLOW_PKT_ESTABLISHED) { SCLogDebug("p->flowflags & FLOW_PKT_ESTABLISHED"); /* all http based mpms */ - if (alstate != NULL && alproto == ALPROTO_HTTP) { + if (has_state && alproto == ALPROTO_HTTP) { FLOWLOCK_WRLOCK(p->flow); + void *alstate = FlowGetAppState(p->flow); + if (alstate == NULL) { + SCLogDebug("no alstate"); + FLOWLOCK_UNLOCK(p->flow); + return; + } HtpState *htp_state = (HtpState *)alstate; if (htp_state->connp == NULL) { @@ -922,10 +928,16 @@ static inline void DetectMpmPrefilter(DetectEngineCtx *de_ctx, FLOWLOCK_UNLOCK(p->flow); } /* all dns based mpms */ - else if (alproto == ALPROTO_DNS && alstate != NULL) { + else if (alproto == ALPROTO_DNS && has_state) { 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); + if (alstate == NULL) { + SCLogDebug("no alstate"); + FLOWLOCK_UNLOCK(p->flow); + return; + } uint64_t idx = AppLayerParserGetTransactionInspectId(p->flow->alparser, flags); uint64_t total_txs = AppLayerParserGetTxCnt(p->flow->proto, alproto, alstate); @@ -977,11 +989,18 @@ static inline void DetectMpmPrefilter(DetectEngineCtx *de_ctx, } /* UDP DNS inspection is independent of est or not */ - if (alproto == ALPROTO_DNS && alstate != NULL) { + if (alproto == ALPROTO_DNS && has_state) { if (p->flowflags & FLOW_PKT_TOSERVER) { SCLogDebug("mpm inspection"); if (det_ctx->sgh->flags & SIG_GROUP_HEAD_MPM_DNSQUERY) { FLOWLOCK_RDLOCK(p->flow); + void *alstate = FlowGetAppState(p->flow); + if (alstate == NULL) { + SCLogDebug("no alstate"); + FLOWLOCK_UNLOCK(p->flow); + return; + } + uint64_t idx = AppLayerParserGetTransactionInspectId(p->flow->alparser, flags); uint64_t total_txs = AppLayerParserGetTxCnt(p->flow->proto, alproto, alstate); for (; idx < total_txs; idx++) { @@ -1092,7 +1111,6 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh #endif uint32_t idx; uint8_t flags = 0; /* flow/state flags */ - void *alstate = NULL; StreamMsg *smsg = NULL; Signature *s = NULL; SigMatch *sm = NULL; @@ -1101,6 +1119,7 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh int state_alert = 0; int alerts = 0; int app_decoder_events = 0; + int has_state = 0; /* do we have an alstate to work with? */ SCEnter(); @@ -1182,10 +1201,10 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh (p->proto == IPPROTO_UDP) || (p->proto == IPPROTO_SCTP && (p->flowflags & FLOW_PKT_ESTABLISHED))) { - alstate = FlowGetAppState(pflow); + has_state = (FlowGetAppState(pflow) != NULL); alproto = FlowGetAppProtocol(pflow); alversion = AppLayerParserGetStateVersion(pflow->alparser); - SCLogDebug("alstate %p, alproto %u", alstate, alproto); + SCLogDebug("alstate %s, alproto %u", has_state ? "true" : "false", alproto); } else { SCLogDebug("packet doesn't have established flag set (proto %d)", p->proto); } @@ -1278,26 +1297,27 @@ 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) && alstate != NULL) { + 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_state = DeStateFlowHasInspectableState(pflow, alproto, alversion, flags); - if (has_state == 1) { + int has_inspectable_state = DeStateFlowHasInspectableState(pflow, alproto, alversion, flags); + if (has_inspectable_state == 1) { DeStateDetectContinueDetection(th_v, de_ctx, det_ctx, p, pflow, flags, alproto, alversion); - } else if (has_state == 2) { - alstate = NULL; + } else if (has_inspectable_state == 2) { + /* no inspectable state, so pretend we don't have a state at all */ + has_state = 0; } } PACKET_PROFILING_DETECT_END(p, PROF_DETECT_STATEFUL); /* create our prefilter mask */ SignatureMask mask = 0; - PacketCreateMask(p, &mask, alproto, alstate, smsg, app_decoder_events); + PacketCreateMask(p, &mask, alproto, has_state, smsg, app_decoder_events); /* run the mpm for each type */ PACKET_PROFILING_DETECT_START(p, PROF_DETECT_MPM); - DetectMpmPrefilter(de_ctx, det_ctx, smsg, p, flags, alproto, alstate, &sms_runflags); + DetectMpmPrefilter(de_ctx, det_ctx, smsg, p, flags, alproto, has_state, &sms_runflags); PACKET_PROFILING_DETECT_END(p, PROF_DETECT_MPM); PACKET_PROFILING_DETECT_START(p, PROF_DETECT_PREFILTER); @@ -1498,7 +1518,7 @@ int SigMatchSignatures(ThreadVars *th_v, DetectEngineCtx *de_ctx, DetectEngineTh /* consider stateful sig matches */ if (s->flags & SIG_FLAG_STATE_MATCH) { - if (alstate == NULL) { + if (has_state == 0) { SCLogDebug("state matches but no state, we can't match"); goto next; } @@ -1551,7 +1571,7 @@ next: end: /* see if we need to increment the inspect_id and reset the de_state */ - if (alstate != NULL && AppLayerParserProtocolSupportsTxs(p->proto, alproto)) { + if (has_state && AppLayerParserProtocolSupportsTxs(p->proto, alproto)) { PACKET_PROFILING_DETECT_START(p, PROF_DETECT_STATEFUL); DeStateUpdateInspectTransactionId(pflow, flags); PACKET_PROFILING_DETECT_END(p, PROF_DETECT_STATEFUL); @@ -2046,7 +2066,7 @@ deonly: * SIG_MASK_REQUIRE_HTTP_STATE, SIG_MASK_REQUIRE_DCE_STATE */ static void -PacketCreateMask(Packet *p, SignatureMask *mask, AppProto alproto, void *alstate, StreamMsg *smsg, +PacketCreateMask(Packet *p, SignatureMask *mask, AppProto alproto, int has_state, StreamMsg *smsg, int app_decoder_events) { /* no payload inspect flag doesn't apply to smsg */ @@ -2076,7 +2096,7 @@ PacketCreateMask(Packet *p, SignatureMask *mask, AppProto alproto, void *alstate SCLogDebug("packet has flow"); (*mask) |= SIG_MASK_REQUIRE_FLOW; - if (alstate != NULL) { + if (has_state) { switch(alproto) { case ALPROTO_HTTP: SCLogDebug("packet/flow has http state");