Project

General

Profile

Actions

Bug #2881

open

http.protocol parsing inaccuracy : accept spaces in URI

Added by chris lujan about 5 years ago. Updated 6 months ago.

Status:
In Review
Priority:
Normal
Target version:
Affected Versions:
Effort:
Difficulty:
low
Label:

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

Related issues 3 (0 open3 closed)

Related to Suricata - Task #3479: libhtp 0.5.33 (4.1.x)ClosedPhilippe AntoineActions
Related to Suricata - Task #3922: libhtp 0.5.35ClosedPhilippe AntoineActions
Related to Suricata - Task #4667: libhtp 0.5.39ClosedVictor JulienActions
Actions #1

Updated by chris lujan about 5 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.

Actions #2

Updated by Victor Julien about 5 years ago

  • Status changed from New to Assigned
  • Assignee set to Philippe Antoine
  • Target version set to TBD
Actions #3

Updated by Victor Julien about 5 years ago

I think uri's are not supposed to have spaces, but I think it would be good to address this anyway.

Actions #4

Updated by Philippe Antoine about 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.

Actions #5

Updated by Philippe Antoine about 4 years ago

  • Related to Task #3479: libhtp 0.5.33 (4.1.x) added
Actions #6

Updated by Philippe Antoine about 4 years ago

  • Status changed from Assigned to In Review
Actions #7

Updated by Philippe Antoine about 4 years ago

  • Target version changed from TBD to 6.0.0beta1
Actions #8

Updated by Philippe Antoine almost 4 years ago

Actions #9

Updated by Victor Julien over 3 years ago

  • Target version changed from 6.0.0beta1 to 6.0.0rc1
Actions #10

Updated by Victor Julien over 3 years ago

  • Target version changed from 6.0.0rc1 to 7.0.0-beta1
Actions #11

Updated by Victor Julien over 3 years ago

Actions #12

Updated by Philippe Antoine over 3 years ago

Actions #13

Updated by Philippe Antoine over 3 years ago

  • Target version changed from 7.0.0-beta1 to 6.0.1
Actions #14

Updated by Philippe Antoine over 3 years ago

Actions #15

Updated by Victor Julien over 3 years ago

  • Target version changed from 6.0.1 to 7.0.0-beta1
Actions #16

Updated by Philippe Antoine over 3 years ago

Actions #17

Updated by Philippe Antoine over 3 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.

Actions #18

Updated by Philippe Antoine over 3 years ago

https://github.com/OISF/suricata/pull/5614 merged for 6.0.1

Still work to do for 7

Actions #19

Updated by Philippe Antoine over 2 years ago

Actions #21

Updated by Philippe Antoine over 1 year ago

  • Priority changed from Normal to High
Actions #22

Updated by Victor Julien over 1 year ago

  • Target version changed from 7.0.0-beta1 to 7.0.0-rc1
Actions #23

Updated by Philippe Antoine over 1 year ago

  • Status changed from In Review to Closed
Actions #24

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

Actions #25

Updated by Philippe Antoine about 1 year ago

  • Target version changed from 8.0.0-beta1 to 7.0.0-rc2
Actions #27

Updated by Philippe Antoine 11 months ago

  • Target version changed from 7.0.0-rc2 to 8.0.0-beta1
Actions #28

Updated by Brandon Murphy 10 months ago

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;)
Actions #29

Updated by Philippe Antoine 10 months 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)

Actions #30

Updated by Brandon Murphy 10 months 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

Actions #31

Updated by Philippe Antoine 10 months 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 ?

Actions #32

Updated by Philippe Antoine 7 months ago

  • Subject changed from http.protocol parsing inaccuracy to http.protocol parsing inaccuracy : accept spaces in URI
Actions #33

Updated by Brandon Murphy 6 months 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?

Actions #35

Updated by Philippe Antoine 6 months ago

So, SCHTPGenerateNormalizedUri needs to add this normalization step (stripping spaces on the right), is it correct @Brandon Murphy ?

Actions #36

Updated by Brandon Murphy 6 months 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.

Actions

Also available in: Atom PDF