Smb raw inspection/v4#15729
Open
inashivb wants to merge 2 commits into
Open
Conversation
Suricata internally classifies the rules into certain categories based on what it sees in a rule. Widely, any rule containing a content keyword immediately becomes a stream type rule calling for inspection on stream payload unless there are keywords that require matching on packet as well. In that case, the rule becomes a pkt-stream rule. This type of rule would cause the inspection to be triggered per stream payload and if that didn't result in success, it'll go for the packet payload. This category is a bit ambiguous and may result in surplus alerts in certain cases. One example is: rule keywords to be matched: content: "suricata"; stream_size:<,100; 1. pkt 1 (toserver): TCP/APP_PROTO/yabadabadoooooosuricataoooooo 2. pkt 2 (toclient): TCP/APP_PROTO/okyaba 3. pkt 3 (toserver): TCP/APP_PROTO/ok At (1), last ACK'd stream window is inspected, no match is found. Since it's a pkt-stream rule, inspection is carried on on pkt 1's payload -- it's a MATCH! After parsing pkt 1, APP_PROTO parser demands an immediate inspection of the raw stream data. So, when pkt 3 arrives, there's stream data ready for inspection and because a pkt-stream rule is in question, the stream payload match must be done -- it's a MATCH! This patch aims to separate the logic of inspection by breaking down the wide "pkt" category into two distinct categories: 1. The keyword needs to look inside the packet data. 2. The keyword needs to look only at the packet dimensions. This is the new category. Rule keywords falling in the new category would skip packet payload inspection entirely simply because they should. Keywords falling in this category are: - stream_size - flow.pkts STODO review all keywords requiring packet. STODO this won't work for http yet -- no raw inspection calls so it could be quite delayed.
Internals --------- Suricata's stream engine returns data for inspection to the detection engine from the stream when the chunk size is reached. Bug --- Inspection triggered only in the specified chunk sizes may be too late when it comes to inspection of smaller protocol specific data which could result in delayed inspection, incorrect data logged with a transaction and logs misindicating the pkt that triggered an alert. Fix --- Fix this by making an explicit call from all respective applayer parsers to trigger raw stream inspection which shall make the data available for inspection in the following call of the stream engine. This needs to happen per direction on the completion of an entity like a request or a response. Important notes --------------- 1. The above mentioned behavior with and without this patch is affected internally by the following conditions. - inspection depth - stream depth In these special cases, the inspection window will be affected and Suricata may not consider all the data that could be expected to be inspected. 2. This only applies to applayer protocols running over TCP. 3. The inspection window is only considered up to the ACK'd data. 4. This entire issue is about IDS mode only. SMB parser creates a transaction per request-response pair. Appropriate calls to trigger raw stream inspection have been added on succesful parsing of each request and response. Task 7863 Bug 7004
inashivb
force-pushed
the
smb-raw-inspection/v4
branch
from
94f3676 to
57936f1
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #15729 +/- ##
==========================================
- Coverage 82.96% 82.96% -0.01%
==========================================
Files 1003 1003
Lines 275031 275250 +219
==========================================
+ Hits 228192 228349 +157
- Misses 46839 46901 +62
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
WARNING:
Pipeline = 32261 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previous PR: #15637
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7863
SV_BRANCH=OISF/suricata-verify#3164
Changes since v3:
Note: An internal test failure is evaluated and explained.
If this is acceptable, I'll clear the TODOs and submit a clean PR.