Project

General

Profile

Actions

Bug #317

closed

Invalid Rules

Added by Peter Manev about 11 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Affected Versions:
Effort:
medium
Difficulty:
low
Label:

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

InvalidRules.tar.bz2 (14.9 KB) InvalidRules.tar.bz2 Peter Manev, 08/19/2011 06:26 AM
InvalidRules.xls (13 KB) InvalidRules.xls Peter Manev, 08/19/2011 06:26 AM
InvalidRulesUpdated.xls (14 KB) InvalidRulesUpdated.xls Peter Manev, 10/31/2011 09:27 AM
InvalidRulesUpdatedwithCategories.xls (18.5 KB) InvalidRulesUpdatedwithCategories.xls Eileen Donlon, 11/04/2011 11:53 AM

Related issues 1 (1 open0 closed)

Related to Bug #2982: invalid dsize distance rule being loaded by suricataIn ReviewJeff LucovskyActions
Actions #1

Updated by Victor Julien almost 11 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?

Actions #2

Updated by Peter Manev almost 11 years ago

Sure. Will update.

Actions #3

Updated by Peter Manev almost 11 years ago

Updated file attached
Updates include current git master and Snort 2.9.1.2

Actions #4

Updated by Eileen Donlon almost 11 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.

Actions #5

Updated by Peter Manev almost 11 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?

Actions #6

Updated by Victor Julien about 10 years ago

  • Target version set to TBD
Actions #7

Updated by Andreas Herz almost 7 years ago

  • Assignee changed from Eileen Donlon to Andreas Herz
Actions #8

Updated by Andreas Herz over 6 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.

Actions #9

Updated by Andreas Herz over 6 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?

Actions #10

Updated by Peter Manev over 6 years ago

Ideally - the checks should be there.

Actions #11

Updated by Victor Julien over 6 years ago

Agreed. If a rule can't match due to internal inconsistencies then we should reject it.

Actions #12

Updated by Peter Manev over 6 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#.

Actions #13

Updated by Victor Julien about 4 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.

Actions #14

Updated by Andreas Herz over 3 years ago

  • Assignee set to Community Ticket
Actions #15

Updated by Jason Taylor over 3 years ago

  • Status changed from New to Assigned
  • Assignee changed from Community Ticket to Jason Taylor
Actions #16

Updated by Jason Taylor over 3 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

Actions #17

Updated by Jason Taylor over 3 years ago

  • Related to Bug #2982: invalid dsize distance rule being loaded by suricata added
Actions #18

Updated by Andreas Herz about 3 years ago

Jason Taylor are you also interested in working on the parsing?

Actions #19

Updated by Jason Taylor about 3 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?

Actions #20

Updated by Andreas Herz about 3 years ago

yep, when it's merged we can close it. Thanks!

Actions #21

Updated by Jason Taylor about 3 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 :)

Actions #22

Updated by Andreas Herz about 3 years ago

That's an ongoing issue for quite some time, so we're happy for every contribution :)

Actions #23

Updated by Philippe Antoine over 1 year ago

  • Status changed from Assigned to Closed

Remaning work is tracked in #2982

Actions

Also available in: Atom PDF