Suricata

Hi Brandon!

Thank you for the report. I have been looking into the code and realized that the first rule you have posted is probably correct as xbits are supposed to handle "noalert" as a part of its options just like flowbits and hostbits. One example that I found in the ET rules itself is sid:2033750 created_at 2021_08_20 .
While there certainly is a problem with parsing the options and it needs to be stricter (for which I have done this work: https://github.com/OISF/suricata/pull/6889 [still in review]), sid:2 and sid:3 are indeed invalid from the three rules that you have shared.

This is what I could analyze but I could certainly be wrong as I am not very well versed with rule usage so please share your thoughts if you find noalert at the end of xbits options working as it should or not. Also, please explain if this behavior is not alright from your point of view.

Thank you very much! :)

The only difference between sid:1; and sid:2 is the ordering of where the noalert occurs within the xbits keyword options. Unless ordering matters, it shouldn't in my opinion, I'm not sure why sid:1 would be valid but sid:2 is invalid.

With regards to using the 'noalert' as an option to the xbits vs the noalert keyword. I think there just needs to be consistent usage between flowbits and xbits. Today, with flowbits, 'noalert' is not an option to flowbits keyword, it requires a standalone use of 'flowbits:noalert'. For example,

alert http any any -> any any (msg:"flowbits with noalert option"; flow:established,to_server; http.method; content:"POST"; flowbits:set,ET.whatever,noalert; sid:4;)

alert http any any -> any any (msg:"flowbits with noalert option"; flow:established,to_server; http.method; content:"POST"; flowbits:set,ET.whatever; flowbits:noalert; sid:5;)
alert http any any -> any any (msg:"flowbits with noalert option"; flow:established,to_server; http.method; content:"POST"; flowbits:set,ET.whatever; noalert; sid:6;)

For context - ET generally uses the `flowbits:noalert;` pattern because that is also supported by Suricata and is required by snort, so it saves us time in having to rewrite another keyword every time if we were to us use a `noalert;`

SIDENOTE¶

Actually in testing this, it looks like the same issue as sid:3 applies to flowbits as well, might be worth another issue?

<pre><code class="shell">
alert http any any -> any any (msg:"flowbits with noalert option"; flow:established,to_server; http.method; content:"POST"; flowbits:set,ET.whatever,asdfasdf; sid:7;)
</pre>

Brandon Murphy wrote in #note-7:

The only difference between sid:1; and sid:2 is the ordering of where the noalert occurs within the xbits keyword options. Unless ordering matters, it shouldn't in my opinion, I'm not sure why sid:1 would be valid but sid:2 is invalid.

Indeed. I had put a strict ordering check with the new parsing scheme. We are thinking of going forward with strict ordering for now.

With regards to using the 'noalert' as an option to the xbits vs the noalert keyword. I think there just needs to be consistent usage between flowbits and xbits. Today, with flowbits, 'noalert' is not an option to flowbits keyword, it requires a standalone use of 'flowbits:noalert'. For example,
[...]
sid:4 will still generate alerts on matching traffic, and doesn't throw an error. In order to get the noalert to actually work with flowbits, either of the following rules are required.
[...]

For context - ET generally uses the `flowbits:noalert;` pattern because that is also supported by Suricata and is required by snort, so it saves us time in having to rewrite another keyword every time if we were to us use a `noalert;`

Thank you very much for your valuable feedback! I discussed this with Victor and we decided to have an `xbits:noalert;` pattern just like `flowbits` and get rid of the currently supported `noalert` among `xbits` options.

Actually in testing this, it looks like the same issue as sid:3 applies to flowbits as well, might be worth another issue?

Indeed. Seems like it. Please create a new ticket for this. I'll assign it to myself and get all these issues fixed in the upcoming major release.
Thank you very much for putting time into assisting with this! :)

Indeed. I had put a strict ordering check with the new parsing scheme. We are thinking of going forward with strict ordering for now.

My guts says enforcing ordering of keyword options is a good idea, I think it will make for a more strict language less prone to these types of issues. However, this is not a small change. There are a ton of rules that would need to be tested against any new strict ordering enforcement and I'm sure there are many that will need corrected.

I also then question if we need option "keys" - consider the following xbits:set,whatever,count 10, track by_src, expire 60; - if strict ordering is enforced, count, track and expire would technically no longer be required.

Please consider being consistent with not only flowbits, but also other keywords. base64_decode, flow, etc. How will you work with keywords that have optional options? like byte_jump's "from_beginning", or byte_extract's "multiplier" or many options in byte_test.

If you do decide to move forward with strict ordering, please please please, document which keywords require strict ordering and those that do not.

I discussed this with Victor and we decided to have an `xbits:noalert;` pattern just like `flowbits` and get rid of the currently supported `noalert` among `xbits` options.

Has this been implemented? I was trying to look through the PRs and found this one:
https://github.com/OISF/suricata/pull/7127/files

However that appears to be the backport into 6.0 only only.

The latest docs don't include the note about a standalone xbits:noalert as the 6.0 branch does.
https://suricata.readthedocs.io/en/latest/rules/xbits.html#notes
https://suricata.readthedocs.io/en/suricata-6.0.5/rules/xbits.html#notes

Did this ever make it's way into the master branch?

Work on this has stalled a bit as we're working on more important issues. We'll get it addressed after 7 is out most likely, then see about how we can backport things in a way that is not too intrusive.