Skip to content

Commit

Permalink
detect: avoid race condition on app layer used
Browse files Browse the repository at this point in the history 
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=<optimized out>) at detect.c:1589
 #4  0x000000000045c9f3 in Detect (data=<optimized out>, p=<optimized out>, tv=<optimized out>, pq=<optimized out>, postpq=<optimized out>) at detect.c:1744
 #5  Detect (tv=<optimized out>, p=<optimized out>, data=<optimized out>, pq=<optimized out>, postpq=<optimized out>) 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=<optimized out>, data=0x13b029bd0, slot=<optimized out>) 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 ?? ()
  • Loading branch information
@regit
regit committed Nov 17, 2014
1 parent a1ff46d commit 568f478
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 28 deletions.
34 changes: 20 additions & 14 deletions src/detect-engine-state.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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);

Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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);
Expand Down
7 changes: 4 additions & 3 deletions src/detect-engine-state.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
30 changes: 19 additions & 11 deletions src/detect.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -794,16 +794,22 @@ 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");

/* 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);
Expand All @@ -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++) {
Expand Down Expand Up @@ -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++) {
Expand Down Expand Up @@ -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++) {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down
20 changes: 20 additions & 0 deletions src/flow.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 4 additions & 0 deletions src/flow.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);



Expand Down

0 comments on commit 568f478

Please sign in to comment.