google / gonids

gonids is a library to parse IDS rules, with a focus primarily on Suricata rule compatibility. There is a discussion forum available that you can join on Google Groups: https://groups.google.com/forum/#!topic/gonids/
Apache License 2.0
179 stars 48 forks source link

Avoids over recursion in ParseRule #144

Closed catenacyber closed 4 years ago

catenacyber commented 4 years ago

Found by oss-fuzz : https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=21920

We no not need to parse a comment from a commented

Hope this is right this time :-)

duanehoward commented 4 years ago

I wonder if, instead of changing the function signatures and such, does it make sense to just set a bool or a global counter? something like:

maxCommentDepth := 3
commentDepth := 0
func ParseRule(rule string) (*Rule, error) {

  case itemComment:
    if commentDepth  > maxCommentDepth {
      return r, fmt.Errorf("maximum comment depth exceeded, %s", rule)
    }
    if r.Action {
      ...
    }
}
catenacyber commented 4 years ago

Yes, it makes sense.

But using a global counter, you lose the ability to call ParseRule in parallel (I think the right word is thread-safety)

So, I think it is better to use local data. But we may find other solutions, such as lex the comment character alone and then keep a state in ParseRule wether we are in a comment

duanehoward commented 4 years ago

@catenacyber thanks. I'm unclear as to whether you're still seeking approval for this PR, or if you're experimenting with one of the other ideas noted in comment the previous comment https://github.com/google/gonids/pull/144#issuecomment-622312526

catenacyber commented 4 years ago

Duane, I am still seeking approval for this PR.

But if you wish this problem to be solved another way, I can give it a go.

duanehoward commented 4 years ago

Not critical or urgent, but in a future PR would you mind adding a few tests for parseRuleAux specifically?