-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add test for extra tls alert #2080
Open
inashivb
wants to merge
1
commit into
OISF:master
Choose a base branch
from
inashivb:tls-extra-alert/v4
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| # Test Description | ||
|
|
||
| engine analysis complementary test for tls-extra-alert. | ||
|
|
||
| ## Related issues | ||
|
|
||
| None so far. State: Trying to establish what's the issue. |
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| alert tcp any any -> any 443 (flow: to_server; flags: S,CE; flowbits:set, tls_tracker; flowbits: noalert; sid:09901001; ) | ||
| alert tcp any any -> any 443 (flowbits:isset, tls_tracker; content: "|15 03 01 00 02 02|"; startswith; flowbits:set, tls_error; sid:09901031; rev:1; msg:"TLS 1.2 Fatal Alert (outgoing packet)"; ) | ||
| alert tcp any 443 -> any any (flowbits:isset, tls_tracker; content: "|15 03 01 00 02 02|"; startswith; flowbits:set, tls_error; sid:09901032; rev:1; msg:"TLS 1.2 Fatal Alert (incoming packet)"; ) | ||
| alert tcp any any -> any 443 (flow: to_server; flowbits:isset, tls_error; sid:09901033; rev:1; msg:"Allow TLS error handling (outgoing packet)"; ) | ||
| alert tcp any 443 -> any any (flow: to_client; flowbits:isset, tls_error; sid:09901034; rev:1; msg:"Allow TLS error handling (incoming packet)"; ) |
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,226 @@ | ||
| args: | ||
| - --simulate-ips | ||
| - --engine-analysis | ||
|
|
||
| pcap: false | ||
|
|
||
| checks: | ||
| - filter: | ||
| filename: rules.json | ||
| count: 1 | ||
| match: | ||
| flags: | ||
| - src_any | ||
| - dst_any | ||
| - sp_any | ||
| - noalert | ||
| - need_packet | ||
| - toserver | ||
| id: 9901001 | ||
| lists: | ||
| packet: | ||
| matches: | ||
| - name: tcp.flags | ||
| postmatch: | ||
| matches: | ||
| - flowbits: | ||
| cmd: set | ||
| names: | ||
| - tls_tracker | ||
| name: flowbits | ||
| pkt_engines: | ||
| - is_mpm: false | ||
| name: packet | ||
| requirements: | ||
| - tcp_flags_init_deinit | ||
| - real_pkt | ||
| type: pkt | ||
|
|
||
| - filter: | ||
| filename: rules.json | ||
| count: 1 | ||
| match: | ||
| flags: | ||
| - src_any | ||
| - dst_any | ||
| - sp_any | ||
| - need_packet | ||
| - need_stream | ||
| - need_flowvar | ||
| - toserver | ||
| - toclient | ||
| - prefilter | ||
| id: 9901031 | ||
| lists: | ||
| packet: | ||
| matches: | ||
| - flowbits: | ||
| cmd: isset | ||
| names: | ||
| - tls_tracker | ||
| name: flowbits | ||
| payload: | ||
| matches: | ||
| - content: | ||
| depth: 6 | ||
| ends_with: false | ||
| fast_pattern: false | ||
| is_mpm: true | ||
| length: 6 | ||
| negated: false | ||
| no_double_inspect: false | ||
| nocase: false | ||
| pattern: '|15 03 01 00 02 02|' | ||
| relative_next: false | ||
| starts_with: true | ||
| name: content | ||
| postmatch: | ||
| matches: | ||
| - flowbits: | ||
| cmd: set | ||
| names: | ||
| - tls_error | ||
| name: flowbits | ||
| mpm: | ||
| buffer: payload | ||
| depth: 6 | ||
| ends_with: false | ||
| fast_pattern: false | ||
| is_mpm: true | ||
| length: 6 | ||
| negated: false | ||
| no_double_inspect: false | ||
| nocase: false | ||
| pattern: '|15 03 01 00 02 02|' | ||
| relative_next: false | ||
| starts_with: true | ||
| pkt_engines: | ||
| - is_mpm: true | ||
| name: payload | ||
| - is_mpm: false | ||
| name: packet | ||
| requirements: | ||
| - payload | ||
| - flow | ||
| type: pkt_stream | ||
|
|
||
| - filter: | ||
| filename: rules.json | ||
| count: 1 | ||
| match: | ||
| flags: | ||
| - src_any | ||
| - dst_any | ||
| - dp_any | ||
| - need_packet | ||
| - need_stream | ||
| - need_flowvar | ||
| - toserver | ||
| - toclient | ||
| - prefilter | ||
| id: 9901032 | ||
| lists: | ||
| packet: | ||
| matches: | ||
| - flowbits: | ||
| cmd: isset | ||
| names: | ||
| - tls_tracker | ||
| name: flowbits | ||
| payload: | ||
| matches: | ||
| - content: | ||
| depth: 6 | ||
| ends_with: false | ||
| fast_pattern: false | ||
| is_mpm: true | ||
| length: 6 | ||
| negated: false | ||
| no_double_inspect: false | ||
| nocase: false | ||
| pattern: '|15 03 01 00 02 02|' | ||
| relative_next: false | ||
| starts_with: true | ||
| name: content | ||
| postmatch: | ||
| matches: | ||
| - flowbits: | ||
| cmd: set | ||
| names: | ||
| - tls_error | ||
| name: flowbits | ||
| mpm: | ||
| buffer: payload | ||
| depth: 6 | ||
| ends_with: false | ||
| fast_pattern: false | ||
| is_mpm: true | ||
| length: 6 | ||
| negated: false | ||
| no_double_inspect: false | ||
| nocase: false | ||
| pattern: '|15 03 01 00 02 02|' | ||
| relative_next: false | ||
| starts_with: true | ||
| pkt_engines: | ||
| - is_mpm: true | ||
| name: payload | ||
| - is_mpm: false | ||
| name: packet | ||
| requirements: | ||
| - payload | ||
| - flow | ||
| type: pkt_stream | ||
|
|
||
| # Following is the signature of interest | ||
| - filter: | ||
| filename: rules.json | ||
| count: 1 | ||
| match: | ||
| flags: | ||
| - src_any | ||
| - dst_any | ||
| - sp_any | ||
| - need_flowvar | ||
| - toserver | ||
| id: 9901033 | ||
| lists: | ||
| packet: | ||
| matches: | ||
| - flowbits: | ||
| cmd: isset | ||
| names: | ||
| - tls_error | ||
| name: flowbits | ||
| pkt_engines: | ||
| - is_mpm: false | ||
| name: packet | ||
| requirements: | ||
| - flow | ||
| type: pkt | ||
|
|
||
| - filter: | ||
| filename: rules.json | ||
| count: 1 | ||
| match: | ||
| flags: | ||
| - src_any | ||
| - dst_any | ||
| - dp_any | ||
| - need_flowvar | ||
| - toclient | ||
| id: 9901034 | ||
| lists: | ||
| packet: | ||
| matches: | ||
| - flowbits: | ||
| cmd: isset | ||
| names: | ||
| - tls_error | ||
| name: flowbits | ||
| pkt_engines: | ||
| - is_mpm: false | ||
| name: packet | ||
| requirements: | ||
| - flow | ||
| type: pkt |
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # Test Description | ||
|
|
||
| This test shows that Suricata generates an additional alert for TLS | ||
| for the given PCAP which shouldn't be there. | ||
|
|
||
| ## PCAP | ||
|
|
||
| Internal. | ||
|
|
||
| ## Related issues | ||
|
|
||
| None so far. State: Trying to establish what's the issue. | ||
Binary file not shown.
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| alert tcp any any -> any 443 (flow: to_server; flags: S,CE; flowbits:set, tls_tracker; flowbits: noalert; sid:09901001; ) | ||
| alert tcp any any -> any 443 (flowbits:isset, tls_tracker; content: "|15 03 01 00 02 02|"; startswith; flowbits:set, tls_error; sid:09901031; rev:1; msg:"TLS 1.2 Fatal Alert (outgoing packet)"; ) | ||
| alert tcp any 443 -> any any (flowbits:isset, tls_tracker; content: "|15 03 01 00 02 02|"; startswith; flowbits:set, tls_error; sid:09901032; rev:1; msg:"TLS 1.2 Fatal Alert (incoming packet)"; ) | ||
| alert tcp any any -> any 443 (flow: to_server; flowbits:isset, tls_error; sid:09901033; rev:1; msg:"Allow TLS error handling (outgoing packet)"; ) | ||
| alert tcp any 443 -> any any (flow: to_client; flowbits:isset, tls_error; sid:09901034; rev:1; msg:"Allow TLS error handling (incoming packet)"; ) |
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| args: | ||
| - -k none | ||
| - --simulate-ips | ||
|
|
||
| checks: | ||
| - filter: | ||
| count: 2 | ||
| match: | ||
| event_type: alert | ||
| alert.signature_id: 9901033 | ||
| pkt_src: wire/pcap | ||
| - filter: | ||
| count: 0 | ||
| match: | ||
| event_type: alert | ||
| not-has-key: pcap_cnt |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For which rule ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 9901033 as per test.yaml:
alert.signature_id: 9901033There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At which packet does it happen ? The pseudo flush ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or packet 8 and 9 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extra alert happens at pseudo flush. Yes. The test filter that indicates that i test.yaml is:
Note that the event generated w the pseudo pkt misses
pcap_cntkey and also haspkt_src: stream (flow timeout)(so subtest 1 in the test also helps making sure of that)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts are that it should not fire as-is. Its somewhat non-sensical, at least its being logged relative to a packet (start of flow) where it should not be. Its only because of information found in the future, and ends up in the pseudo-packet that we end up alerting. Or at least its confusing.
As we're only pushing this pseudo-packet through to wrap up transactions, my thought was to skip detection on non-transaction data, in particular packet rules per the discussion here: OISF/suricata#11862. But maybe this is expected behavior and there is no issue. I think we're still discussing the behavior and if this is correct or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How so with pkt_src: stream (flow timeout) ?
Do we time travel ?
Beware there are timeouts pseudo packets, but other one likes HTTP1->HTTP2 upgrade...
IIRC, there was one ticket where we needed the pseudo packet to match on the content of the last SSH packet before going encrypted cf https://redmine.openinfosecfoundation.org/issues/6578 (and one similar for frames)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently we do :)
In the log we have an alert at
2024-09-09T12:15:56.542526-0600, followed by an alert from2024-09-09T12:15:55.863332-0600.That second timestamp matches the first packet in the pcap, however hits on the rule:
and is logged with flowbits, even detected as tls, however we don't know any of that at
2024-09-09T12:15:55.863332-0600, unless we time travel to the future :)Kidding aside, that's why this alert looks so out of place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flow timeout also seems odd, as it didn't timeout. Its cleanly closed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, there is a bug in the timestamp of timeout packets, right ?
I think we just time out cleanly closed flows after sometime checking there is no session reuse...