From 19988310d1dfe941be7fd9231a64c98aba7391d2 Mon Sep 17 00:00:00 2001 From: Victor Julien Date: Thu, 8 Mar 2018 08:35:16 +0100 Subject: [PATCH] detect: fix tx iterator logic in detect The 'tx_id' variable was used to be passed into the IterFunc as a minumum tx to return. The IterFunc could then return either the tx for that id, or a later one if that turned out to be the first available tx. The tx_id however, was still used for some things as if it was the current tx id. Most importantly for setting the tx id for alert ammending. So this could lead to alerts with missing or wrong applayer records. --- src/detect.c | 45 ++++++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 21 deletions(-) diff --git a/src/detect.c b/src/detect.c index 30009ecc06..0f9a1822fa 100644 --- a/src/detect.c +++ b/src/detect.c @@ -1359,7 +1359,7 @@ static void DetectRunTx(ThreadVars *tv, const AppProto alproto = f->alproto; const uint64_t total_txs = AppLayerParserGetTxCnt(f, alstate); - uint64_t tx_id = AppLayerParserGetTransactionInspectId(f->alparser, flow_flags); + uint64_t tx_id_min = AppLayerParserGetTransactionInspectId(f->alparser, flow_flags); const int tx_end_state = AppLayerParserGetStateProgressCompletionStatus(alproto, flow_flags); AppLayerGetTxIteratorFunc IterFunc = AppLayerGetTxIterator(ipproto, alproto); @@ -1367,7 +1367,7 @@ static void DetectRunTx(ThreadVars *tv, memset(&state, 0, sizeof(state)); while (1) { - AppLayerGetTxIterTuple ires = IterFunc(ipproto, alproto, alstate, tx_id, total_txs, &state); + AppLayerGetTxIterTuple ires = IterFunc(ipproto, alproto, alstate, tx_id_min, total_txs, &state); if (ires.tx_ptr == NULL) break; @@ -1375,9 +1375,12 @@ static void DetectRunTx(ThreadVars *tv, alstate, ires.tx_id, ires.tx_ptr, tx_end_state, flow_flags); if (tx.tx_ptr == NULL) { SCLogDebug("%p/%"PRIu64" no transaction to inspect", - tx.tx_ptr, tx_id); + tx.tx_ptr, tx_id_min); + + tx_id_min++; // next (if any) run look for +1 goto next; } + tx_id_min = tx.tx_id + 1; // next look for cur + 1 uint32_t array_idx = 0; uint32_t total_rules = det_ctx->match_array_cnt; @@ -1390,7 +1393,7 @@ static void DetectRunTx(ThreadVars *tv, alstate, &tx); PACKET_PROFILING_DETECT_END(p, PROF_DETECT_PF_TX); SCLogDebug("%p/%"PRIu64" rules added from prefilter: %u candidates", - tx.tx_ptr, tx_id, det_ctx->pmq.rule_id_array_cnt); + tx.tx_ptr, tx.tx_id, det_ctx->pmq.rule_id_array_cnt); total_rules += det_ctx->pmq.rule_id_array_cnt; if (!(RuleMatchCandidateTxArrayHasSpace(det_ctx, total_rules))) { @@ -1425,11 +1428,11 @@ static void DetectRunTx(ThreadVars *tv, array_idx++; SCLogDebug("%p/%"PRIu64" rule %u (%u) added from 'match' list", - tx.tx_ptr, tx_id, s->id, id); + tx.tx_ptr, tx.tx_id, s->id, id); } } SCLogDebug("%p/%"PRIu64" rules added from 'match' list: %u", - tx.tx_ptr, tx_id, array_idx - x); (void)x; + tx.tx_ptr, tx.tx_id, array_idx - x); (void)x; /* merge stored state into results */ if (tx.de_state != NULL) { @@ -1440,7 +1443,7 @@ static void DetectRunTx(ThreadVars *tv, const bool have_new_file = (tx.de_state->flags & DETECT_ENGINE_STATE_FLAG_FILE_NEW); if (have_new_file) { SCLogDebug("%p/%"PRIu64" destate: need to consider new file", - tx.tx_ptr, tx_id); + tx.tx_ptr, tx.tx_id); tx.de_state->flags &= ~DETECT_ENGINE_STATE_FLAG_FILE_NEW; } @@ -1474,11 +1477,11 @@ static void DetectRunTx(ThreadVars *tv, DetectRunTxSortHelper); SCLogDebug("%p/%"PRIu64" rules added from 'continue' list: %u", - tx.tx_ptr, tx_id, array_idx - old); + tx.tx_ptr, tx.tx_id, array_idx - old); } } - det_ctx->tx_id = tx_id; + det_ctx->tx_id = tx.tx_id; det_ctx->tx_id_set = 1; det_ctx->p = p; @@ -1497,37 +1500,37 @@ static void DetectRunTx(ThreadVars *tv, if (det_ctx->tx_candidates[i].flags != NULL) { i++; SCLogDebug("%p/%"PRIu64" inspecting SKIP NEXT: sid %u (%u), flags %08x", - tx.tx_ptr, tx_id, s->id, s->num, inspect_flags ? *inspect_flags : 0); + tx.tx_ptr, tx.tx_id, s->id, s->num, inspect_flags ? *inspect_flags : 0); } else if (det_ctx->tx_candidates[i+1].flags != NULL) { SCLogDebug("%p/%"PRIu64" inspecting SKIP CURRENT: sid %u (%u), flags %08x", - tx.tx_ptr, tx_id, s->id, s->num, inspect_flags ? *inspect_flags : 0); + tx.tx_ptr, tx.tx_id, s->id, s->num, inspect_flags ? *inspect_flags : 0); continue; } else { // if it's all the same, inspect the current one and skip next. i++; SCLogDebug("%p/%"PRIu64" inspecting SKIP NEXT: sid %u (%u), flags %08x", - tx.tx_ptr, tx_id, s->id, s->num, inspect_flags ? *inspect_flags : 0); + tx.tx_ptr, tx.tx_id, s->id, s->num, inspect_flags ? *inspect_flags : 0); } } } SCLogDebug("%p/%"PRIu64" inspecting: sid %u (%u), flags %08x", - tx.tx_ptr, tx_id, s->id, s->num, inspect_flags ? *inspect_flags : 0); + tx.tx_ptr, tx.tx_id, s->id, s->num, inspect_flags ? *inspect_flags : 0); if (inspect_flags) { if (*inspect_flags & (DE_STATE_FLAG_FULL_INSPECT|DE_STATE_FLAG_SIG_CANT_MATCH)) { SCLogDebug("%p/%"PRIu64" inspecting: sid %u (%u), flags %08x ALREADY COMPLETE", - tx.tx_ptr, tx_id, s->id, s->num, *inspect_flags); + tx.tx_ptr, tx.tx_id, s->id, s->num, *inspect_flags); continue; } } if (inspect_flags) { /* continue previous inspection */ - SCLogDebug("%p/%"PRIu64" Continueing sid %u", tx.tx_ptr, tx_id, s->id); + SCLogDebug("%p/%"PRIu64" Continueing sid %u", tx.tx_ptr, tx.tx_id, s->id); } else { /* start new inspection */ - SCLogDebug("%p/%"PRIu64" Start sid %u", tx.tx_ptr, tx_id, s->id); + SCLogDebug("%p/%"PRIu64" Start sid %u", tx.tx_ptr, tx.tx_id, s->id); } /* call individual rule inspection */ @@ -1542,9 +1545,9 @@ static void DetectRunTx(ThreadVars *tv, if (s->action & ACTION_DROP) alert_flags |= PACKET_ALERT_FLAG_DROP_FLOW; - SCLogDebug("%p/%"PRIu64" sig %u (%u) matched", tx.tx_ptr, tx_id, s->id, s->num); + SCLogDebug("%p/%"PRIu64" sig %u (%u) matched", tx.tx_ptr, tx.tx_id, s->id, s->num); if (!(s->flags & SIG_FLAG_NOALERT)) { - PacketAlertAppend(det_ctx, s, p, tx_id, alert_flags); + PacketAlertAppend(det_ctx, s, p, tx.tx_id, alert_flags); } else { DetectSignatureApplyActions(p, s, alert_flags); } @@ -1564,7 +1567,7 @@ static void DetectRunTx(ThreadVars *tv, if (tx.tx_progress >= tx.tx_end_state) { new_detect_flags |= APP_LAYER_TX_INSPECTED_FLAG; SCLogDebug("%p/%"PRIu64" tx is done for direction %s. Flag %016"PRIx64, - tx.tx_ptr, tx_id, + tx.tx_ptr, tx.tx_id, flow_flags & STREAM_TOSERVER ? "toserver" : "toclient", new_detect_flags); } @@ -1572,7 +1575,7 @@ static void DetectRunTx(ThreadVars *tv, new_detect_flags |= tx.prefilter_flags; SCLogDebug("%p/%"PRIu64" updated prefilter flags %016"PRIx64" " "(was: %016"PRIx64") for direction %s. Flag %016"PRIx64, - tx.tx_ptr, tx_id, tx.prefilter_flags, tx.prefilter_flags_orig, + tx.tx_ptr, tx.tx_id, tx.prefilter_flags, tx.prefilter_flags_orig, flow_flags & STREAM_TOSERVER ? "toserver" : "toclient", new_detect_flags); } @@ -1581,7 +1584,7 @@ static void DetectRunTx(ThreadVars *tv, { new_detect_flags |= tx.detect_flags; SCLogDebug("%p/%"PRIu64" Storing new flags %016"PRIx64" (was %016"PRIx64")", - tx.tx_ptr, tx_id, new_detect_flags, tx.detect_flags); + tx.tx_ptr, tx.tx_id, new_detect_flags, tx.detect_flags); AppLayerParserSetTxDetectFlags(ipproto, alproto, tx.tx_ptr, flow_flags, new_detect_flags); }