Skip to content
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

RFC - WIP - userguide: explain rule types and categorization - v1 #12089

Closed

Conversation

jufajardini
Copy link
Contributor

Add documentation about the rule types introduced by commit 2696fda.

Compiled docs: https://suri-rtd-test.readthedocs.io/en/doc-sigtypes-et-properties-v1/rules/intro.html#rule-s-types-and-categorization

This is a WIP, the table needs reviewing and expanding:

TODO:

  • more examples
  • reviewing whether some of the explanations are actually correct
  • feedback on where's the best place for this section to go is also welcome

There are some workflows that I would like to add to the documentation, too, somehow, but must find a good solution for not simply bringing images, but ideally something that can be generated from code description.

The purpose of this documentation is both for better informing our users, and also work as foundational work for the upcoming SuriCon presentation about the same topic.

Examples:
APP_Layer, Packet, TX, STREAM-v20241029 drawio(1)

OverallAlgoHorizontal-20241029-BiggerFontFriendlyLabels drawio

Add documentation about the rule types introduced by 2696fda.
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.25%. Comparing base (dd71ef0) to head (d8cacdc).
Report is 19 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12089      +/-   ##
==========================================
- Coverage   83.25%   83.25%   -0.01%     
==========================================
  Files         910      910              
  Lines      257571   257571              
==========================================
- Hits       214450   214437      -13     
- Misses      43121    43134      +13     
Flag Coverage Δ
fuzzcorpus 61.22% <ø> (+0.05%) ⬆️
livemode 19.41% <ø> (-0.01%) ⬇️
pcap 44.43% <ø> (-0.04%) ⬇️
suricata-verify 62.79% <ø> (+0.01%) ⬆️
unittests 59.34% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! :)
Only did a brief correctness of the content review. Left some questions and suggestions inline.

* - Decode Events Only
- Packet
- Per-packet basis
- Packets that are broken on an IP address level
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/IP address/IP

- Packet
- Per-packet basis
- Packets that are broken on an IP address level
- ---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keyword: decode-event

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks :D

- Packet
- Per-packet basis
- Packet-level info (e.g.: header info, ttl...)
- ---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move (e.g. ttl) to "Example" column?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea :D

Comment on lines +393 to +397
* - IP Only (negated address)
- Flow
- Once per direction
- On the flow, on IP address level (negated addressed)
- ---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding and opinion is that negated address will be resolved internally and have the same properties as above so we can merge these and mention that we could have negated addresses.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is special handling for sigs that could be ip-only, but contain address negation. The radix based implementation doesn't support this, so there is a special "like ip-only" logic to make this sigs behave more or less the same as non-negated ip-only

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Need to study the code to understand this properly.
ok so, questions:

  1. does this information concern the user?
  2. does it make sense to mention it separately? or rather, can we explain why is it separately mentioned w/o going into the internals somehow?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is at least visible to the use in the engine analysis

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would rather do something like 2. I want to offer a good insight on how Suricata actually handles things, because sometimes the low level details will impact difference in behaviors that might otherwise not make sense. (Not sure if my words here make enough sense xD )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do :)
Let's do that then!

* - IP Only
- Flow
- Once per direction
- On the flow, on IP address level
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure about the language "on IP address level"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okie. Do you have a suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "Matches on the IP address on the flow"?

* - Protocol Detection Only
- Flow
- Once per direction, when protocol detection is done
- app-layer-protocol keyword
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Matches protocol detected for the flow
Feel free to change the words but this seemed redundant as the keyword is mentioned in the examples

Comment on lines +411 to +412
- Match on payload
- Match against the reassembled stream. If stream unavailable, match per-packet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: When do we match against a not reassembled stream? IPS mode?

Comment: These two statements are confusing. Match on payload but also stream?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing: "Stream" seems to be used on a stream only condition so no pkt match. Someone should verify if this is correct..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rules with scope 'flow, if stateful', will apply to the stream if the reassembled stream is available - which may stop being the case if we reach stream-depth, for instance. In that case, the rules will be applied on a packet-level. The difficulty is explaining this on a higher level...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: When do we match against a not reassembled stream? IPS mode?

I guess... when we match against payload keywords, or header keywords, or, say, just the protocol...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two statements are confusing. Match on payload but also stream?

I can see, even based on my previous message, how it is indeed too confusing >__<

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the protocol...?

Just the protocol, yes indeed. Payload keywords and header keywords not sure as they could be fragmented I suppose.. 🤔

- Match against the reassembled stream. If stream unavailable, match per-packet
- Flow, if stateful, per-packet if not
- 'content' with 'starts with' or 'depth'
* - Stream
Copy link
Member

@inashivb inashivb Nov 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example: flow keywords?

Edit: Nope

@jufajardini
Copy link
Contributor Author

Followed by: #12105

@jufajardini jufajardini closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants