From 162dafbdf21d3a4750cc134bfd9815fa7f503bc3 Mon Sep 17 00:00:00 2001 From: Eric Leblond Date: Wed, 30 Oct 2024 00:07:56 +0100 Subject: [PATCH] detect/filestore: fix options handling and impact The filestore keyword had an influence on the signature matching when it should not. For example, if Suricata is analysing a traffic with a GET http request to uri /example and have the 2 following signatures loaded: alert http any any -> any any (msg:"ex"; http.uri; content:"/example"; sid:1; rev:1;) alert http any any -> any any (msg:"ex"; http.uri; content:"/example"; filestore; sid:2; rev:1;) then the first signature will match and the second one will not. Also the options of filestore were not honored correctly. A signature like: alert http any any -> any any (msg:"ex"; http.uri; content:"/example"; filestore:to_client,tx; sid:2; rev:1;) was not storing the file in the answer to the request. This patch updates the logic in filestore keyword handling to fix the problems. The patch first makes sure that a signature with filestore will hit even if there is no file in the current application layer context. Then the patch makes sure that postmatch handles the different options correctly. As filestore keyword is not anymore preventing a match, we need to update some unit tests that were using this "feature". Tickets: 7356 7357 --- src/app-layer-htp.c | 4 +- src/detect-engine-file.c | 17 ++++++++- src/detect-filestore.c | 80 ++++++++++++++++++++++++++++++++-------- 3 files changed, 83 insertions(+), 18 deletions(-) diff --git a/src/app-layer-htp.c b/src/app-layer-htp.c index 328a10b72cb8..de756ec59dd4 100644 --- a/src/app-layer-htp.c +++ b/src/app-layer-htp.c @@ -5809,12 +5809,12 @@ libhtp:\n\ /* do detect */ SigMatchSignatures(&th_v, de_ctx, det_ctx, p1); - FAIL_IF((PacketAlertCheck(p1, 1))); + FAIL_IF((PacketAlertCheck(p1, 2))); /* do detect */ SigMatchSignatures(&th_v, de_ctx, det_ctx, p1); - FAIL_IF((PacketAlertCheck(p1, 1))); + FAIL_IF((PacketAlertCheck(p1, 2))); r = AppLayerParserParse( &th_v, alp_tctx, &f, ALPROTO_HTTP1, STREAM_TOCLIENT, httpbuf2, httplen2); diff --git a/src/detect-engine-file.c b/src/detect-engine-file.c index 26601ce8a96b..97bc42eedb6f 100644 --- a/src/detect-engine-file.c +++ b/src/detect-engine-file.c @@ -194,7 +194,22 @@ uint8_t DetectFileInspectGeneric(DetectEngineCtx *de_ctx, DetectEngineThreadCtx if (ffc == NULL) { SCReturnInt(DETECT_ENGINE_INSPECT_SIG_CANT_MATCH_FILES); } else if (ffc->head == NULL) { - SCReturnInt(DETECT_ENGINE_INSPECT_SIG_NO_MATCH); + if (s->flags & SIG_FLAG_FILESTORE) { + /* If the signature has filestore, we need to check if we are in + a scope of capture where we need to prepare the capture for + an upcoming file. */ + if (s->filestore_ctx && (s->filestore_ctx->scope == FILESTORE_SCOPE_TX)) { + /* In scope TX, we need to prepare file storage for file that could + appear on the transaction so we store the transaction */ + det_ctx->filestore[det_ctx->filestore_cnt].file_id = 0; + det_ctx->filestore[det_ctx->filestore_cnt].tx_id = det_ctx->tx_id; + det_ctx->filestore_cnt++; + } + /* Other scopes than TX are going to be handled in post match without + any setup needed here so we can just return a match for them. */ + SCReturnInt(DETECT_ENGINE_INSPECT_SIG_MATCH); + } else + SCReturnInt(DETECT_ENGINE_INSPECT_SIG_NO_MATCH); } uint8_t r = DETECT_ENGINE_INSPECT_SIG_NO_MATCH; diff --git a/src/detect-filestore.c b/src/detect-filestore.c index e93dcceabd9c..4c5b5da10004 100644 --- a/src/detect-filestore.c +++ b/src/detect-filestore.c @@ -98,6 +98,47 @@ void DetectFilestoreRegister(void) g_file_match_list_id = DetectBufferTypeRegister("files"); } +static void FilestoreTriggerFlowStorage( + Flow *f, int toserver_dir, int toclient_dir, uint64_t init_tx_id) +{ + SCLogDebug("init tx_id is: %ld", init_tx_id); + /* set flags in Flow and AppLayerStateData */ + AppLayerStateData *sd = AppLayerParserGetStateData(f->proto, f->alproto, f->alstate); + if (toclient_dir) { + f->file_flags |= FLOWFILE_STORE_TC; + if (sd != NULL) { + sd->file_flags |= FLOWFILE_STORE_TC; + } + } + if (toserver_dir) { + f->file_flags |= FLOWFILE_STORE_TS; + if (sd != NULL) { + sd->file_flags |= FLOWFILE_STORE_TS; + } + } + /* Start storage for all existing files attached to the flow in correct direction */ + void *alstate = FlowGetAppState(f); + if (alstate != NULL) { + uint64_t total_txs = AppLayerParserGetTxCnt(f, alstate); + for (uint64_t tx_id = init_tx_id; tx_id < total_txs; tx_id++) { + void *txv = AppLayerParserGetTx(f->proto, f->alproto, alstate, tx_id); + DEBUG_VALIDATE_BUG_ON(txv == NULL); + if (txv != NULL) { + AppLayerTxData *txd = AppLayerParserGetTxData(f->proto, f->alproto, txv); + DEBUG_VALIDATE_BUG_ON(txd == NULL); + if (txd != NULL) { + if (toclient_dir) { + txd->file_flags |= FLOWFILE_STORE_TC; + } + if (toserver_dir) { + txd->file_flags |= FLOWFILE_STORE_TS; + } + } + } + } + } +} + /** * \brief apply the post match filestore with options */ @@ -170,20 +211,7 @@ static int FilestorePostMatchWithOptions(Packet *p, Flow *f, const DetectFilesto } } } else if (this_flow) { - /* set in flow and AppLayerStateData */ - AppLayerStateData *sd = AppLayerParserGetStateData(f->proto, f->alproto, f->alstate); - if (toclient_dir) { - f->file_flags |= FLOWFILE_STORE_TC; - if (sd != NULL) { - sd->file_flags |= FLOWFILE_STORE_TC; - } - } - if (toserver_dir) { - f->file_flags |= FLOWFILE_STORE_TS; - if (sd != NULL) { - sd->file_flags |= FLOWFILE_STORE_TS; - } - } + FilestoreTriggerFlowStorage(f, toserver_dir, toclient_dir, tx_id); } else { FileStoreFileById(fc, file_id); } @@ -207,11 +235,33 @@ static int DetectFilestorePostMatch(DetectEngineThreadCtx *det_ctx, { SCEnter(); + DEBUG_VALIDATE_BUG_ON(p->flow == NULL); + if (det_ctx->filestore_cnt == 0) { + /* here we have no file but the signature is fully matched and + filestore option indicate we need to extract for file for the session + so we trigger flow storage. */ + if (s->filestore_ctx && (s->filestore_ctx->scope == FILESTORE_SCOPE_SSN)) { + int toserver_dir = 0; + int toclient_dir = 0; + switch (s->filestore_ctx->direction) { + case FILESTORE_DIR_BOTH: + toserver_dir = 1; + toclient_dir = 1; + break; + case FILESTORE_DIR_TOSERVER: + toserver_dir = 1; + break; + case FILESTORE_DIR_TOCLIENT: + toclient_dir = 1; + break; + } + FilestoreTriggerFlowStorage(p->flow, toserver_dir, toclient_dir, det_ctx->tx_id); + } SCReturnInt(0); } - if ((s->filestore_ctx == NULL && !(s->flags & SIG_FLAG_FILESTORE)) || p->flow == NULL) { + if (s->filestore_ctx == NULL && !(s->flags & SIG_FLAG_FILESTORE)) { #ifndef DEBUG SCReturnInt(0); #else