Add DataPosition for PCRE

elmeyer commented 3 years ago

Previously, a rule such as: alert tcp $EXTERNAL_NET any -> $HTTP_SERVERS $HTTP_PORTS (msg:"SERVER-IIS Malformed Hit-Highlighting Argument File Access Attempt"; flow:to_server,established; http.uri; content:"CiWebHitsFile="; nocase; pkt_data; pcre:"/CiWebHitsFile=\/?([^\r\n\x3b\&]*\.\.\/)?/i"; http.uri; content:"CiRestriction=none"; nocase; fast_pattern; content:"ciHiliteType=Full"; nocase; metadata:ruleset community, service http; classtype:web-application-attack; reference:bugtraq,950; reference:cve,2000-0097; reference:url,technet.microsoft.com/en-us/security/bulletin/ms00-006; reference:url,www.securityfocus.com/archive/1/43762; sid:1019; rev:30;) would not be parsed completely, resulting in a loss of information about which buffer the pcre expression should be matched against.

google-cla[bot] commented 3 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

google-cla[bot] commented 3 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

elmeyer commented 3 years ago

@googlebot I signed it!

duanehoward commented 3 years ago

Previously, a rule such as: alert tcp $EXTERNAL_NET any -> $HTTP_SERVERS $HTTP_PORTS (msg:"SERVER-IIS Malformed Hit-Highlighting Argument File Access Attempt"; flow:to_server,established; http.uri; content:"CiWebHitsFile="; nocase; pkt_data; pcre:"/CiWebHitsFile=\/?([^\r\n\x3b\&]*\.\.\/)?/i"; http.uri; content:"CiRestriction=none"; nocase; fast_pattern; content:"ciHiliteType=Full"; nocase; metadata:ruleset community, service http; classtype:web-application-attack; reference:bugtraq,950; reference:cve,2000-0097; reference:url,technet.microsoft.com/en-us/security/bulletin/ms00-006; reference:url,www.securityfocus.com/archive/1/43762; sid:1019; rev:30;) would not be parsed completely, resulting in a loss of information about which buffer the pcre expression should be matched against.

is the rule as specified here correct? My previous understanding was that PCRE buffers were specified with the parameters at the end of the PCRE. Which, appears to be the case per the documentation

elmeyer commented 3 years ago

First, apologies for my delay in reviewing this. The code looks good, Before merging in I'd like to see unit tests that exercise the new functionality. At minimum:
1. A test in parser_test.go that covers parsing a rule where PCRE leverages a dataposition value

2. A test for the (r Rule) String() function that also exercises the new output expectations.

Sorry, should have added these tests right away.

is the rule as specified here correct? My previous understanding was that PCRE buffers were specified with the parameters at the end of the PCRE. Which, appears to be the case per the documentation

This may stem from a misunderstanding of this repo's goals on my part. Specifying a sticky buffer before a pcre keyword is allowed in the newer Snort 3 syntax (see e.g. the Rules Authors Introduction to Writing Snort 3 Rules). As a consequence, the buffer options at the end of a pcre pattern are obsolete in Snort 3. If parsing Snort 3 syntax is out of scope for this repo, you can of course ignore this PR.

duanehoward commented 3 years ago

First, apologies for my delay in reviewing this. The code looks good, Before merging in I'd like to see unit tests that exercise the new functionality. At minimum:
1. A test in parser_test.go that covers parsing a rule where PCRE leverages a dataposition value

2. A test for the (r Rule) String() function that also exercises the new output expectations.
Sorry, should have added these tests right away.

is the rule as specified here correct? My previous understanding was that PCRE buffers were specified with the parameters at the end of the PCRE. Which, appears to be the case per the documentation

This may stem from a misunderstanding of this repo's goals on my part. Specifying a sticky buffer before a pcre keyword is allowed in the newer Snort 3 syntax (see e.g. the Rules Authors Introduction to Writing Snort 3 Rules). As a consequence, the buffer options at the end of a pcre pattern are obsolete in Snort 3. If parsing Snort 3 syntax is out of scope for this repo, you can of course ignore this PR.

At this time, I'd consider Snort 3 syntax out of scope, but that's largely due to my lack of knowledge around what's changed. I do think that as Suricata (our primary supported syntax) and Snort 3 syntax diverges in order to support them we may need distinct rule structures or parsing logic that can identify one from the other (with corresponding stringers, etc.)

Proper Snort 3 support isn't something that I will have time to support in full in the near to medium term. I have a chat thread going with the Suricata developers clarifying whether sticky buffer support for PCRE is planned or already functional, if it is we can include this. I'll report back.

duanehoward commented 3 years ago

First, apologies for my delay in reviewing this. The code looks good, Before merging in I'd like to see unit tests that exercise the new functionality. At minimum:
1. A test in parser_test.go that covers parsing a rule where PCRE leverages a dataposition value

2. A test for the (r Rule) String() function that also exercises the new output expectations.
Sorry, should have added these tests right away.

is the rule as specified here correct? My previous understanding was that PCRE buffers were specified with the parameters at the end of the PCRE. Which, appears to be the case per the documentation

This may stem from a misunderstanding of this repo's goals on my part. Specifying a sticky buffer before a pcre keyword is allowed in the newer Snort 3 syntax (see e.g. the Rules Authors Introduction to Writing Snort 3 Rules). As a consequence, the buffer options at the end of a pcre pattern are obsolete in Snort 3. If parsing Snort 3 syntax is out of scope for this repo, you can of course ignore this PR.
At this time, I'd consider Snort 3 syntax out of scope, but that's largely due to my lack of knowledge around what's changed. I do think that as Suricata (our primary supported syntax) and Snort 3 syntax diverges in order to support them we may need distinct rule structures or parsing logic that can identify one from the other (with corresponding stringers, etc.)

Proper Snort 3 support isn't something that I will have time to support in full in the near to medium term. I have a chat thread going with the Suricata developers clarifying whether sticky buffer support for PCRE is planned or already functional, if it is we can include this. I'll report back.

I've confirmed with the devs that supporting sticky buffers in this way is actually valid for newer versions of Suricata as well. We may want to add some additional parsing logic for redundant patterns or conflicting ones such as: http.uri; ... pcre:"/xyz/U"; and http.header; ... pcre:"/xyz/U"; but that can be done later. Please proceed with the requested unit tests and I'll review.

elmeyer commented 3 years ago

I've added the requested tests. I hope it's ok that I simply appended the parser test to the section labelled "Begin Suricata 5.0 features", since I don't know which version of Suricata first supported sticky buffers in this way.

google / gonids