Bug #317
closedInvalid Rules
Description
Please find attached some tests with invalid rule keywords combinations (bad "grammar") that should not be loaded by the engine, nevertheless they are getting loaded.
modifiers and rule keywords - distance,within, depth, offset...
Snort corrected some of the issues they had - ""Improved error checking for invalid combinations of "depth", "offset", "distance", and "within" modifiers in rules. Rules that mix relative and non-relative options on the same content will now cause errors."" - http://blog.snort.org/2010/12/snort-2903-is-coming-soon.html
dated back in Dec 2010, some of them are still not addressed, I believe.
Please find a comparison of invalid rules and if they load or not. I have tested all the bad rules with Sur 1.0.4/1.0.5/git master, Snort 2.8.5.1/2.9.0.5/current beta, the results are in the spreadsheet attached.
Thanks
Files
Updated by Victor Julien about 13 years ago
- Assignee set to Eileen Donlon
@Peter Pan, can you update this to include our 1.1beta3 release and the 2.9.1.2 Snort release?
Updated by Peter Manev about 13 years ago
- File InvalidRulesUpdated.xls InvalidRulesUpdated.xls added
Updated file attached
Updates include current git master and Snort 2.9.1.2
Updated by Eileen Donlon about 13 years ago
I've added a sheet to Peter's workbook which lists categories of invalid or bad rules, and color-coded the rules in Peter's list to show the category. The point of listing the categories is that it makes it easier to develop a comprehensive set of rules for testing. Let me know if you have done this already! A lot of these checks have already been implemented but I don't know if they've all been tested.
The "bad" rule checks would be part of the rule analyzer.
Updated by Peter Manev about 13 years ago
I like the idea.
I think it would be good if we can number the categorizations as well i.e.
01 - Semantics Err
02 - Syntax Err
...
We could rewrite the "msg" in the alerts to reflect the categorization.
Then it shouldn’t be difficult to write a script that would test all the rules and reflect the results in one go for every git update/release update.
Other opinions?
Updated by Andreas Herz about 9 years ago
- Assignee changed from Eileen Donlon to Andreas Herz
Updated by Andreas Herz almost 9 years ago
Quite old :) But tested the rules again against 3.0 and the only rules left are Content-Greater-Than-Dsize.rule and the other Dsiz-*.rule files.
Updated by Andreas Herz over 8 years ago
To resolve all the examples we would need to include several checks when parsing dsize rules since the checks depend on several other keywords. Do we want to add this or are we just fine with loading those rules since they shouldn't hurt but also not match?
Updated by Victor Julien over 8 years ago
Agreed. If a rule can't match due to internal inconsistencies then we should reject it.
Updated by Peter Manev over 8 years ago
As an addition: this rule below -
alert tcp any any -> any any (msg: "la di da"; content: "0a:23:bf hu-ha"; )
loads with 3.1dev (rev d39e575) without err but it shouldn't load since it is missing a sid#.
Updated by Victor Julien over 6 years ago
- Assignee changed from Andreas Herz to Anonymous
- Effort set to medium
- Difficulty set to low
Think it would be nice to refresh this and open tickets for individual issues.
Updated by Jason Taylor over 5 years ago
- Status changed from New to Assigned
- Assignee changed from Community Ticket to Jason Taylor
Updated by Jason Taylor over 5 years ago
suricata-verify tests have been submitted for the invalid rules.
https://github.com/OISF/suricata-verify/pull/57
there is still one rule that is successfully loading in current suricata master branch:
alert udp any any -> any any (msg:"TEST SUCCESFULL - dsize/distance INVALID combination "; dsize:10; content:"boom"; content:"loom"; distance:10; sid:6666663; rev:1;)
#2982 has been created to track the invalid rule loading
Updated by Jason Taylor over 5 years ago
- Related to Bug #2982: invalid dsize distance rule being loaded by suricata added
Updated by Andreas Herz over 5 years ago
Jason Taylor are you also interested in working on the parsing?
Updated by Jason Taylor over 5 years ago
Sure, I can certainly take a look, which parsing items are you referring to?
As a side note I am working on https://redmine.openinfosecfoundation.org/issues/2982 which I think is the only outstanding issue related to this ticket?
Updated by Andreas Herz over 5 years ago
yep, when it's merged we can close it. Thanks!
Updated by Jason Taylor over 5 years ago
Ideally I will have some time to work on this in the near future, if it's something time sensitive then by all means someone else can take it. This is all educational so I am stumbling my way through learning :)
Updated by Andreas Herz over 5 years ago
That's an ongoing issue for quite some time, so we're happy for every contribution :)
Updated by Philippe Antoine almost 4 years ago
- Status changed from Assigned to Closed
Remaning work is tracked in #2982