Bug #2881
closedhttp.protocol parsing inaccuracy : accept spaces in URI
Added by chris lujan almost 6 years ago. Updated 7 months ago.
Description
Request:
GET /uid=0(root) gid=0(root) groups=0(root)asdf HTTP/1.1
User-Agent: curl/7.29.0
Accept: */*
eve.json output:"http":{"protocol":"gid=0(root) groups=0(root)asdf HTTP\/1.1"}
It appears that the http.protocol is matching too greedily with the space character and could use something like /\S+$/m
instead.
Files
b8ee56effed96ba.pcap (467 Bytes) b8ee56effed96ba.pcap | Brandon Murphy, 06/15/2023 04:53 PM |
Updated by chris lujan almost 6 years ago
Conversely, the http.url field is only matching up until the first space resulting in something like:
"http":{"url":"/uid=0(root)"}
which leads me to believe those fields are created by splitting the line by spaces.
Updated by Victor Julien almost 6 years ago
- Status changed from New to Assigned
- Assignee set to Philippe Antoine
- Target version set to TBD
Updated by Victor Julien almost 6 years ago
I think uri's are not supposed to have spaces, but I think it would be good to address this anyway.
Updated by Philippe Antoine over 5 years ago
Thanks Chris.
Indeed, Uris are not supposed to have spaces, but the protocol field is even less supposed to have spaces.
So I think we can take the last space in the request line as the uri end, instead of the second one.
Updated by Philippe Antoine almost 5 years ago
- Related to Task #3479: libhtp 0.5.33 (4.1.x) added
Updated by Philippe Antoine almost 5 years ago
- Status changed from Assigned to In Review
Updated by Philippe Antoine over 4 years ago
- Target version changed from TBD to 6.0.0beta1
Updated by Philippe Antoine over 4 years ago
- Blocks Task #3824: libhtp 0.5.34 added
Updated by Victor Julien over 4 years ago
- Target version changed from 6.0.0beta1 to 6.0.0rc1
Updated by Victor Julien over 4 years ago
- Target version changed from 6.0.0rc1 to 7.0.0-beta1
Updated by Victor Julien over 4 years ago
- Blocks deleted (Task #3824: libhtp 0.5.34)
Updated by Philippe Antoine over 4 years ago
- Related to Task #3922: libhtp 0.5.35 added
Updated by Philippe Antoine about 4 years ago
- Target version changed from 7.0.0-beta1 to 6.0.1
Updated by Philippe Antoine about 4 years ago
- Related to Task #4180: libhtp 0.5.36 added
Updated by Victor Julien about 4 years ago
- Target version changed from 6.0.1 to 7.0.0-beta1
Updated by Philippe Antoine about 4 years ago
- Related to deleted (Task #4180: libhtp 0.5.36)
Updated by Philippe Antoine about 4 years ago
https://github.com/OISF/suricata/pull/5599 for 6.0.1
For 7 :
changing the handling in 7 would be good, but I'm not sure it should be optional.
Updated by Philippe Antoine about 4 years ago
https://github.com/OISF/suricata/pull/5614 merged for 6.0.1
Still work to do for 7
Updated by Philippe Antoine over 3 years ago
- Related to Task #4667: libhtp 0.5.39 added
Updated by Philippe Antoine over 2 years ago
https://github.com/OISF/suricata/pull/6884 is latest PR to review
Updated by Philippe Antoine over 2 years ago
- Priority changed from Normal to High
Updated by Victor Julien about 2 years ago
- Target version changed from 7.0.0-beta1 to 7.0.0-rc1
Updated by Philippe Antoine about 2 years ago
- Status changed from In Review to Closed
Updated by Victor Julien almost 2 years ago
- Status changed from Closed to In Review
- Priority changed from High to Normal
- Target version changed from 7.0.0-rc1 to 8.0.0-beta1
Was accidentally closed. Postponing once more to give rule writers more time to update things on their end.
Updated by Philippe Antoine over 1 year ago
- Target version changed from 8.0.0-beta1 to 7.0.0-rc2
Updated by Philippe Antoine over 1 year ago
Updated by Philippe Antoine over 1 year ago
- Target version changed from 7.0.0-rc2 to 8.0.0-beta1
Updated by Brandon Murphy over 1 year ago
- File b8ee56effed96ba.pcap b8ee56effed96ba.pcap added
I was testing the v16 fork of this and found a difference between 6.0.9 and v16. I was able to confirm the same behavior in v17 fork.
Current Behavior: When the v17 fork is presented with HTTP/1 Request which contains a double space after the URI and before the Protocol, the extra space is added to the end of the URI.
Expected Behavior: I expected the URI would be normalized and remove any trailing spaces, while the http.uri.raw buffer would contain the space.
alert http $HOME_NET any -> any any (msg:"Test Double Space after URI - alerts on 6.0.9"; flow:established,to_server; http.method; content:"POST"; http.uri; content:"|2e|php"; endswith; sid:1;) alert http $HOME_NET any -> any any (msg:"Test Double Space after URI - alerts on v17"; flow:established,to_server; http.method; content:"POST"; http.uri; content:"|2e|php|20|"; endswith; sid:2;)
Updated by Philippe Antoine over 1 year ago
Thanks @Brandon Murphy for this report.
Would not the solution rather be to consider the URI before the last block of spaces ? (even the raw one)
Otherwise, SCHTPGenerateNormalizedUri
needs to add this normalization step (stripping spaces on the right)
Updated by Brandon Murphy over 1 year ago
Would not the solution rather be to consider the URI before the last block of spaces ? (even the raw one)
When inconstancies or typos like a double space occur in malicious network traffic, we often use them when creating a rule. The ability to write signatures which can make use of these typos, such as the use of the double space, can provide for good fast_patterns in a rule, along with a low FP rate and the ability to very confidently attribute the traffic to a specific malware family/actor, etc.
I'm not sure where/how the best place to allow that to occur is (if not http.uri.raw
). Perhaps http.start
is the answer here? Based on https://github.com/OISF/suricata/pull/8869 it appears to have not been selected for "overloading" so that limits our use of http.start
Updated by Philippe Antoine over 1 year ago
When inconstancies or typos like a double space occur in malicious network traffic, we often use them when creating a rule.
Indeed
Could http.request_line
be used in this case ?
Updated by Philippe Antoine over 1 year ago
- Subject changed from http.protocol parsing inaccuracy to http.protocol parsing inaccuracy : accept spaces in URI
Updated by Brandon Murphy about 1 year ago
Sorry, it looks like I dropped the ball on this one. I've "watched" this ticket to make sure that doesn't happen again.
Could http.request_line be used in this case ?
I guess if this is the only solution, then ok. The issue is that it's uncommon usage of the buffer and lacks normalization of the uri, etc. It would pretty much make http.request_line the new http.uri.raw in cases where a double space is present, which will be an edge case often forgotten about and hard to teach people new to the engine. At the very least, if this is selected as the desired option, would it be possible to add a section to the documents on this behavior?
I feel like anyone looking at network traffic can clearly see that the URI has a space in it and that the protocol is at the end.
Would not the solution rather be to consider the URI before the last block of spaces ? (even the raw one)
RFC says this
A request-line begins with a method token, followed by a single space (SP), the request-target, another single space (SP), the protocol version, and ends with CRLF. request-line = method SP request-target SP HTTP-version CRLF
What do you think about considering the URI before the last space? (not the last "block of spaces"). This conforms with format of the request-line as per the RFC.
given a request line of GET /foo.php HTTP/1.1
http.uri = /foo.php
(no trailing space, gets normalized out)
http.uri.raw = /foo.php
(trailing space)
http.request_line = GET /foo.php HTTP/1.1
(contains the double space)
More or less the method would be anything to the "left" of the first space, the version anything "right" of the last space and anything in between those two is the URI?
Updated by Brandon Murphy about 1 year ago
https://forum.suricata.io/t/http-protocol-error-field-parsing/4150/2
Issue mentioned here
Updated by Philippe Antoine about 1 year ago
So, SCHTPGenerateNormalizedUri
needs to add this normalization step (stripping spaces on the right), is it correct @Brandon Murphy ?
Updated by Brandon Murphy about 1 year ago
I won't pretend to be a developer and tell you what function needs updating, I defer to you on that! :-)
Given the behavior mentioned in #note-28, I'm honestly surprised trailing spaces were not already normalized out, but I sure don't know enough to read through libhtp and suricata code to figure it out.
Updated by Philippe Antoine 7 months ago
- Status changed from In Review to Closed