detect/filestore: fix options handling and impact #12312

regit · 2024-12-20T07:40:34Z

Update of #12176

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

Contribution style:

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

I have updated the User Guide (in doc/userguide/) to reflect the changes made
I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
(including schema descriptions)
I have created a ticket at
https://redmine.openinfosecfoundation.org/projects/suricata/issues

Link to ticket: https://redmine.openinfosecfoundation.org/issues/7357

Describe changes:

rebase code
don't filestore before tx triggering the alert
put everything in one commit
rework the commit message

SV_BRANCH=OISF/suricata-verify#2202

victorjulien · 2024-12-20T08:05:51Z

In the commit, use

Ticket: 7356
Ticket: 7357

so our tracking can work (and the leaderboard counts them)

suricata-qa · 2024-12-20T10:05:05Z

Information: QA ran without warnings.

Pipeline 24034

victorjulien · 2024-12-20T10:06:39Z

SV_REPO=OISF/suricata-verify#2202

Should be using SV_BRANCH

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". Ticket: 7356 Ticket: 7357

regit · 2024-12-20T15:11:39Z

In the commit, use
Ticket: 7356
Ticket: 7357
so our tracking can work (and the leaderboard counts them)

Done, thanks for the check.

suricata-qa · 2024-12-20T17:35:03Z

Information: QA ran without warnings.

Pipeline 24041

regit · 2025-01-20T20:22:19Z

@victorjulien any update on this MR ?

catenacyber · 2025-01-21T20:15:13Z

I think we need to discuss the functionality/expected behavior before the implementation : see https://redmine.openinfosecfoundation.org/issues/7356#note-4

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.

As you said (in redmine), this is because there is no files in the to_server direction

We have no file in the direction of http.uri but from documentation filestore should not prevent the match.

I do not find such documentation.
And for me, the current behavior makes sense.

I understand the need is to store the file answered by the server if the request matches some URI...

As tried in Ticket 7357
alert http any any -> any any (msg:"exe"; http.uri; content:"exe"; filestore:both,flow; sid:2; rev:1;)
This seems to make sense to me that it should match indeed

And bidirectional signatures should also be a cleaner way to support it in 8 cf #12405

catenacyber · 2025-01-21T20:28:16Z

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)));


What does this change mean ? Can there be a comment ?

catenacyber · 2025-01-21T20:29:56Z

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);


When do we get ffc != NULL && ffc->head == NULL ?

This still looks fishy to me as a signature with file.data that does not match today because of this SCReturnInt(DETECT_ENGINE_INSPECT_SIG_NO_MATCH), will cause FP with this PR...

Talking about fishy, I think it is a good catch ;) I'm going to add a suricata-verify test so we are sure this case is handled.

Ok, but do you know, in plain English, when we have the cases :

ffc == NULL (protocol or direction not supporting files ?)

ffc != NULL && ffc->head == NULL protocol supporting files but tx does not have a file in this direction ? (that is what I expected but this seems a wrong expectation)

catenacyber · 2025-01-21T20:32:05Z

src/detect-engine-file.c

+                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


But postmatch does not make signature fail to match, does it ?

I see (void)sigmatch_table[smd->type].Match(det_ctx, p, s, smd->ctx); return code ignored in DetectRunPostMatch

Yes, before we were not going to postmatch as the filestore without file was the cases where ffc or ffc->head were null and as a result it was a NO_MATCH.

I understand a bit more.
Why not handle FILESTORE_SCOPE_TX in postmatch as well ?

Is it not a problem if the signature matched so far but will not fully match in the end ?

catenacyber · 2025-01-21T20:34:16Z

src/detect-filestore.c

+                DEBUG_VALIDATE_BUG_ON(txd == NULL);
+                if (txd != NULL) {
+                    if (toclient_dir) {
+                        txd->file_flags |= FLOWFILE_STORE_TC;


I do not think we need to set AppLayerTxData file_flags here as we already set AppLayerStateData one, and in the end we use a logical or of both

regit · 2025-01-21T20:38:45Z

I think we need to discuss the functionality/expected behavior before the implementation : see https://redmine.openinfosecfoundation.org/issues/7356#note-4
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.

As you said (in redmine), this is because there is no files in the to_server direction

We have no file in the direction of http.uri but from documentation filestore should not prevent the match.

I do not find such documentation. And for me, the current behavior makes sense.

I'm quoting the documentation here. First sentence of https://docs.suricata.io/en/latest/rules/file-keywords.html#filestore:

Stores files to disk if the signature matched.

Matched is past tense so signature is a full match. And Suricata is just going to take action on it.

I understand the need is to store the file answered by the server if the request matches some URI...

As tried in Ticket 7357 alert http any any -> any any (msg:"exe"; http.uri; content:"exe"; filestore:both,flow; sid:2; rev:1;) This seems to make sense to me that it should match indeed

And bidirectional signatures should also be a cleaner way to support it in 8 cf #12405

The documentation specifies the scope of extraction is a parameter. What i'm just doing here is to implement it. Only "improvisation" is the usage of first sentence of the documentation (which is clear to me) to change behavior.

catenacyber · 2025-01-21T20:39:52Z

src/detect-filestore.c

@@ -207,11 +235,33 @@ static int DetectFilestorePostMatch(DetectEngineThreadCtx *det_ctx,
 {
    SCEnter();

+    DEBUG_VALIDATE_BUG_ON(p->flow == NULL);
+
    if (det_ctx->filestore_cnt == 0) {


By the way, det_ctx->filestore_cnt looks fishy to me cf OISF/suricata-verify#1999

Its scope is one packet when a signature can take multiple packets to match...

catenacyber · 2025-01-21T20:53:50Z

I'm quoting the documentation here. First sentence of https://docs.suricata.io/en/latest/rules/file-keywords.html#filestore:
Stores files to disk if the signature matched.
Matched is past tense so signature is a full match. And Suricata is just going to take action on it.

Ok, I see, my English may not be so good.

That means that alert ip any any -> any any ( filestore; sid: 1;) should match every packet, right ?

regit · 2025-01-21T21:18:58Z

I'm quoting the documentation here. First sentence of https://docs.suricata.io/en/latest/rules/file-keywords.html#filestore:
Stores files to disk if the signature matched.
Matched is past tense so signature is a full match. And Suricata is just going to take action on it.
Ok, I see, my English may not be so good.

At least what is sure is that your accent is better than mine ;)

That means that alert ip any any -> any any ( filestore; sid: 1;) should match every packet, right ?

Yes, I understand it like that too.

suricata-qa · 2025-01-22T11:00:03Z

Information: QA ran without warnings.

Pipeline 24317

catenacyber

Thanks for the work :-)

This requires a decision on the behavior of alert http any any -> any any (msg:"ex"; http.uri; content:"/example"; filestore; sid:2; rev:1;) matching for a GET and alert ip any any -> any any ( filestore; sid: 1;) matching on SSH packets

CI : ⚠️ to investigate
Code : some remarks
Commits segmentation : to check again when it is no longer a draft
Commit messages : ⚠️ to improve
Git ID set : looks fine for me
CLA : you already contributed
Doc update : ⚠️ I think it deserves a doc update to clarify and explicit because we understand 2 different things from the current doc.
Redmine ticket : I think we should merge the tickets into one...
Rustfmt : no rust
Tests : left some remarks there, should have more tests that we do not do FPs
Dependencies added: none

victorjulien · 2025-01-22T13:30:16Z

We have 3 scopes for filestore:

file, this is the default if the option is omitted. It means "this file".
tx, all the files in the transaction.
flow/ssn, all the files in the flow
Additionally, the direction can be specified I think.

The basic problem is that "this file" only makes sense in the context of actually inspecting a file. A rule that works on the IP layer, doesn't match a file and it has no knowledge about files.

Similarly, "this tx" only makes sense if the rule actually inspects a tx. This is similar to the tx metadata logging issue for non-tx rules. I think we should reject rules that try to specify a mismatching scope.

That leaves only the flow/ssn scope. That I think does make sense to have for rules w/o a tx or file.

Scopes:
file - require file inspection (file.name, file.data, etc)
tx - require tx inspection (basically a rule of type app_tx)
flow/ssn - support all

regit · 2025-01-22T17:04:22Z

We have 3 scopes for filestore:

file, this is the default if the option is omitted. It means "this file".

tx, all the files in the transaction.

flow/ssn, all the files in the flow
Additionally, the direction can be specified I think.

The basic problem is that "this file" only makes sense in the context of actually inspecting a file. A rule that works on the IP layer, doesn't match a file and it has no knowledge about files.

Similarly, "this tx" only makes sense if the rule actually inspects a tx. This is similar to the tx metadata logging issue for non-tx rules. I think we should reject rules that try to specify a mismatching scope.

That leaves only the flow/ssn scope. That I think does make sense to have for rules w/o a tx or file.

Scopes: file - require file inspection (file.name, file.data, etc) tx - require tx inspection (basically a rule of type app_tx) flow/ssn - support all

Victor, what is your conclusion here ? Not sure I understand what you try to say.

catenacyber · 2025-01-22T19:27:24Z

My understand of Victor's comment is that filestore should affect signature matching, based on its scope :

file : only matches if there is a file (cf FILE_SIG_NEED_FILE ? ) , so alert http any any -> any any (msg:"ex"; http.uri; content:"/example"; filestore; sid:2; rev:1;) does not match
tx : use SIG_FLAG_APPLAYER so alert http any any -> any any (msg:"exe"; http.uri; content:"exe"; filestore:both,tx; sid:2; rev:1;) should match (I think it does not now and your PR fixes it) and also alert ip any any -> any any ( filestore:both,tx; sid: 1;) does not match on TCP SYN packet
ssn/flow : use SIG_MASK_REQUIRE_FLOW so alert http any any -> any any (msg:"exe"; http.uri; content:"exe"; filestore:both,flow; sid:2; rev:1;) should match (I think it does not now and your PR fixes it) and also alert ip any any -> any any ( filestore:both,flow; sid: 1;) does not match on ARP packets

So yeah, we should set the right signature flags based on the scope...

victorjulien · 2025-01-22T20:53:38Z

My understand of Victor's comment is that filestore should affect signature matching, based on its scope :

* file : only matches if there is a file (cf `FILE_SIG_NEED_FILE` ? ) , so `alert http any any -> any any (msg:"ex"; http.uri; content:"/example"; filestore; sid:2; rev:1;)` does not match

* tx : use `SIG_FLAG_APPLAYER` so `alert http any any -> any any (msg:"exe"; http.uri; content:"exe"; filestore:both,tx; sid:2; rev:1;)` should match (I think it does not now and your PR fixes it) and also `alert ip any any -> any any ( filestore:both,tx; sid: 1;)` does not match on TCP SYN packet

* ssn/flow : use `SIG_MASK_REQUIRE_FLOW` so `alert http any any -> any any (msg:"exe"; http.uri; content:"exe"; filestore:both,flow; sid:2; rev:1;)` should match (I think it does not now and your PR fixes it) and also `alert ip any any -> any any ( filestore:both,flow; sid: 1;)` does not match on ARP packets

So yeah, we should set the right signature flags based on the scope...

My point is we should outright fail to load the mismatches in scope. Lets make explicit which ones can be mixed, reject all other sigs.

catenacyber · 2025-01-23T15:13:15Z

I think the signature alert http any any -> any any (msg:"ex"; http.uri; content:"/example"; filestore; sid:2; rev:1;) is ok as it does not have mismatched scopes.
The question is about its behavior : should it match if there is no file in the http.uri direction ?

victorjulien · 2025-01-23T15:15:28Z

I think the signature alert http any any -> any any (msg:"ex"; http.uri; content:"/example"; filestore; sid:2; rev:1;) is ok as it does not have mismatched scopes. The question is about its behavior : should it match if there is no file in the http.uri direction ?

It does have mismatched scopes, I think. There is no file inspection. There can be multiple files in a single http tx with multipart.

catenacyber · 2025-01-23T15:24:04Z

Oh, I see, thanks :-)

regit requested a review from victorjulien as a code owner December 20, 2024 07:40

regit force-pushed the 7357-filestore-keyword-v3.0 branch from 162dafb to e1a4441 Compare December 20, 2024 15:10

catenacyber reviewed Jan 21, 2025

View reviewed changes

regit added 3 commits January 22, 2025 07:43

file/store: WIP fix on filestore_cnt

d9c0844

formatting

e2734d6

remove useless else

6173435

catenacyber requested changes Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

detect/filestore: fix options handling and impact #12312

detect/filestore: fix options handling and impact #12312

regit commented Dec 20, 2024 •

edited

Loading

victorjulien commented Dec 20, 2024

suricata-qa commented Dec 20, 2024

victorjulien commented Dec 20, 2024

regit commented Dec 20, 2024

suricata-qa commented Dec 20, 2024

regit commented Jan 20, 2025

catenacyber commented Jan 21, 2025

catenacyber Jan 21, 2025

catenacyber Jan 21, 2025

regit Jan 21, 2025

catenacyber Jan 22, 2025

catenacyber Jan 21, 2025

regit Jan 21, 2025

catenacyber Jan 22, 2025

catenacyber Jan 21, 2025

regit commented Jan 21, 2025

catenacyber Jan 21, 2025

catenacyber commented Jan 21, 2025

regit commented Jan 21, 2025

suricata-qa commented Jan 22, 2025

catenacyber left a comment

victorjulien commented Jan 22, 2025

regit commented Jan 22, 2025

catenacyber commented Jan 22, 2025

victorjulien commented Jan 22, 2025

catenacyber commented Jan 23, 2025

victorjulien commented Jan 23, 2025

catenacyber commented Jan 23, 2025

detect/filestore: fix options handling and impact #12312

Are you sure you want to change the base?

detect/filestore: fix options handling and impact #12312

Conversation

regit commented Dec 20, 2024 • edited Loading

Contribution style:

Our Contribution agreements:

Changes (if applicable):

victorjulien commented Dec 20, 2024

suricata-qa commented Dec 20, 2024

victorjulien commented Dec 20, 2024

regit commented Dec 20, 2024

suricata-qa commented Dec 20, 2024

regit commented Jan 20, 2025

catenacyber commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

regit commented Jan 21, 2025

Choose a reason for hiding this comment

catenacyber commented Jan 21, 2025

regit commented Jan 21, 2025

suricata-qa commented Jan 22, 2025

catenacyber left a comment

Choose a reason for hiding this comment

victorjulien commented Jan 22, 2025

regit commented Jan 22, 2025

catenacyber commented Jan 22, 2025

victorjulien commented Jan 22, 2025

catenacyber commented Jan 23, 2025

victorjulien commented Jan 23, 2025

catenacyber commented Jan 23, 2025

regit commented Dec 20, 2024 •

edited

Loading