Project

General

Profile

Actions

Bug #4786

open

xbits: no error on invalid 'expire' values

Added by Brandon Murphy 11 months ago. Updated 2 months ago.

Status:
In Progress
Priority:
Normal
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

alert http any any -> any any (msg:"TEST - No Error")"; flow:established,to_server; http.method; content:"GET"; xbits:set,ET.2020_8260.1,track ip_src,expire 10,noalert; sid:1;)
alert http any any -> any any (msg:"TEST - Error")"; flow:established,to_server; http.method; content:"GET"; xbits:set,ET.2020_8260.1,noalert,track ip_src,expire 10; sid:2;)
alert http any any -> any any (msg:"TEST - No Error")"; flow:established,to_server; http.method; content:"GET"; xbits:set,ET.2020_8260.1,track ip_src,expire 10,asdf; sid:3;)

only sid 2 produces an error, despite that all 3 sids should be considered "invalid"

Error Produced by sid:2

[1539] 27/10/2021 -- 21:58:51 - (detect-xbits.c:208) <Error> (DetectXbitParse) -- [ERRCODE: SC_ERR_PCRE_MATCH(2)] - "set,ET.2020_8260.1,noalert,track ip_src,expire 10" is not a valid setting for xbits.

I'd also add that the documentation is a bit vague on the proper use of the noalert keyword in relation to xbits. It currently reads

To not alert, use noalert;

I suggest adding a bit of context which indicates it should be a standalone keyword in the rule and not an "option" to the xbits keyword.


Related issues 3 (1 open2 closed)

Related to Optimization #5207: Common Rust parser for *bitsIn ProgressShivani BhardwajActions
Copied to Bug #4820: xbits: no error on invalid 'expire' valuesClosedShivani BhardwajActions
Copied to Bug #4821: xbits: no error on invalid 'expire' valuesRejectedJeff LucovskyActions
Actions #1

Updated by Victor Julien 11 months ago

  • Subject changed from Invalid options in xbits do no produce an error if palced after expire to xbits: no error on invalid 'expire' values
  • Status changed from New to Assigned
  • Assignee set to Shivani Bhardwaj
  • Target version set to 7.0rc1
  • Label Needs backport to 5.0, Needs backport to 6.0 added
Actions #2

Updated by Shivani Bhardwaj 11 months ago

  • Priority changed from Normal to High
Actions #3

Updated by Jeff Lucovsky 11 months ago

  • Copied to Bug #4820: xbits: no error on invalid 'expire' values added
Actions #4

Updated by Jeff Lucovsky 11 months ago

  • Copied to Bug #4821: xbits: no error on invalid 'expire' values added
Actions #5

Updated by Shivani Bhardwaj 9 months ago

  • Status changed from Assigned to In Progress
Actions #6

Updated by Shivani Bhardwaj 8 months ago

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! :)

Actions #7

Updated by Brandon Murphy 8 months ago

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

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.
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>

Actions #8

Updated by Shivani Bhardwaj 8 months ago

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! :)

Actions #9

Updated by Brandon Murphy 8 months ago

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.

Actions #10

Updated by Shivani Bhardwaj 6 months ago

Actions #11

Updated by Victor Julien 6 months ago

  • Priority changed from High to Normal
Actions #12

Updated by Victor Julien 2 months ago

  • Label deleted (Needs backport to 5.0, Needs backport to 6.0)
Actions

Also available in: Atom PDF