Project

General

Profile

Actions

Feature #6426

closed

HTTP/2 - app-layer-event and normalization when userinfo is in the :authority pseudo header for the http.host header

Added by Brandon Murphy 4 months ago. Updated 2 months ago.

Status:
Closed
Priority:
Normal
Target version:
Effort:
Difficulty:
Label:

Description

as per RFC 9113

":authority" MUST NOT include the deprecated userinfo subcomponent for "http" or "https" schemed URIs.

I'm wondering if we can get an app-layer-event for when this occurs and for the information to be normalized out when populating the http.host buffer but leaving it in the http.host.raw buffer.

Attached is a pcap of this occurring.


Files

http2_userinfo_in_authority_1.pcap (1.05 KB) http2_userinfo_in_authority_1.pcap Brandon Murphy, 10/27/2023 08:58 PM

Subtasks 2 (0 open2 closed)

Feature #6430: HTTP/2 - app-layer-event and normalization when userinfo is in the :authority pseudo header for the http.host header (6.0.x backport)ClosedPhilippe AntoineActions
Feature #6507: HTTP/2 - app-layer-event and normalization when userinfo is in the :authority pseudo header for the http.host header (7.0.x backport)ClosedPhilippe AntoineActions
Actions #1

Updated by Brandon Murphy 4 months ago

Addtiional Reference: RFC 9110 - https://www.rfc-editor.org/rfc/rfc9110#name-deprecation-of-userinfo-in-

A sender MUST NOT generate the userinfo subcomponent (and its "@" delimiter) when an "http" or "https" URI reference is generated within a message as a target URI or field value.

Before making use of an "http" or "https" URI reference received from an untrusted source, a recipient SHOULD parse for userinfo and treat its presence as an error; it is likely being used to obscure the authority for the sake of phishing attacks.

Actions #2

Updated by Victor Julien 4 months ago

  • Status changed from New to Assigned
  • Assignee changed from OISF Dev to Philippe Antoine
  • Target version changed from TBD to 7.0.3
  • Label Needs backport to 6.0 added
Actions #3

Updated by OISF Ticketbot 4 months ago

  • Subtask #6430 added
Actions #4

Updated by OISF Ticketbot 4 months ago

  • Label deleted (Needs backport to 6.0)
Actions #5

Updated by Philippe Antoine 4 months ago

  • Status changed from Assigned to Resolved

Why an event when this can be a rule such as

alert http2 any any -> any any (msg:"SURICATA HTTP2 user info in authority"; flow:established,to_server; http.request_header; content:":authority: "; startswith; content: "@"; pcre: /:authority: ([^:]*:[^@]*)@/,flow:http2_userinfo; sid:1; rev:1;)

What do you think @Brandon Murphy ?

Actions #6

Updated by Brandon Murphy 4 months ago

I'm not sure why this issue shows as "resolved".

I would prefer an event as we can then use that app-layer-event in other rules. Additionally, the RFC states "treat its presence as an error" - which seems like a good match for an app-layer-event.

And normalizing it out of the uri lets us do things like this where we don't want to inspect the values of the user info, but can still use it's presence within the rule logic via the app-layer-event.

alert http2 any any -> any any (msg:"Bad thing that has user info"; flow:established,to_server; app-layer-event:http2.userinfo_in_uri; http.method; content:"POST"; http.uri; bsize:8; content:"/foo.php"; sid:1; rev:1;)

of course, if I wanted to inspect the values of the user info, I can use `http.request_line`.

Heck, given that user info can be in the uri for HTTP/1, maybe there should be a `http.user_info` buffer and it this info gets thrown into it.

Actions #7

Updated by Brandon Murphy 4 months ago

idk if i'm reading this right but it looks like maybe the user info is already getting normalized out of the uri?

https://github.com/OISF/suricata/blob/c8a7204b159553d338a6294218e696a72efdb4db/src/app-layer-htp-libhtp.c#L78-L90

Actions #8

Updated by Philippe Antoine 4 months ago

I would prefer an event as we can then use that app-layer-event in other rules. Additionally, the RFC states "treat its presence as an error" - which seems like a good match for an app-layer-event.

You can use

http.request_header; content:":authority: "; startswith; content: "@"; pcre: /:authority: ([^:]*:[^@]*)@/,flow:http2_userinfo;

and it would do the same thing as app-layer-event:http2.userinfo_in_uri; right ?

Actions #9

Updated by Brandon Murphy 4 months ago

Um...yeah? but that would also happen at the rule level, which I can't imagine is very optimal compared to putting it in C or Rust (though you'd be the one to know). It would also only benefit users with the specific ruleset containing the rule, so in my case only ET users. Whereas as an event would benefit all suricata users.

Additionally, we'd have to include that logic in every rule that we wanted to test for user info, which, i'm guessing negatively impacts performance as more rules contain that logic? (reminds me of the base64 pcre, or the pcre for an ip address that is used in hundreds of rules)

Actions #10

Updated by Philippe Antoine 4 months ago

Um...yeah? but that would also happen at the rule level, which I can't imagine is very optimal compared to putting it in C or Rust (though you'd be the one to know).

One problem is that the app-layer-event gets computed every time, even if you do not have rules...

I get the point that this expression is not the simplest :-)

Actions #11

Updated by Brandon Murphy 4 months ago

Philippe Antoine wrote in #note-10:

One problem is that the app-layer-event gets computed every time, even if you do not have rules...

But if libhtp is already parsing uri->username and uri->password, can't this be leveraged without introducing much new overhead?

https://github.com/OISF/libhtp/blob/0.5.x/htp/htp_util.c#L787-L808

I get the point that this expression is not the simplest :-)

I guess so, my apologizes because I hadn't expected it to be a complex task.

Ok, so can we focus on the how, with respect to HTTP/2, userinfo currently ends up within http.host? Do you want a different ticket for that?

Actions #12

Updated by Philippe Antoine 4 months ago

  • Status changed from Resolved to In Review
Actions #13

Updated by Philippe Antoine 4 months ago

But if libhtp is already parsing uri->username and uri->password, can't this be leveraged without introducing much new overhead?

libhtp does HTTP1, not HTTP2

I guess so, my apologizes because I hadn't expected it to be a complex task.

The feature is simple as you can see in the PR.
I am not confident into assessing the runtime cost vs other options...

Ok, so can we focus on the how, with respect to HTTP/2, userinfo currently ends up within http.host? Do you want a different ticket for that?

For me this ticket is about this.

If you want a `http.user_info` buffer, this could be a new ticket

Actions #14

Updated by Brandon Murphy 4 months ago

Philippe Antoine wrote in #note-13:

But if libhtp is already parsing uri->username and uri->password, can't this be leveraged without introducing much new overhead?

libhtp does HTTP1, not HTTP2

see, this is why i can't be trusted to look at code :-)

If you want a `http.user_info` buffer, this could be a new ticket

k. IMO, this seems like a good choice to have for HTTP/1. I'll try to get one started.

Actions #15

Updated by Brandon Murphy 4 months ago

So, I tested the normalized http.host field when userinfo is included in the :authority header. based on the attached pcap, only the username field makes it into the http.host buffer.

Sids 2 and 3 fire on the attached pcap.

alert http any any -> any any (msg:"user info in http.host"; flow:established,to_server; http.host; content:"username|3a|password"; sid:1;)
alert http any any -> any any (msg:"user info in http.host"; flow:established,to_server; http.host; content:"username"; sid:2;)
alert http any any -> any any (msg:"user info in http.host"; flow:established,to_server; http.host; bsize:8; content:"username"; sid:3;)

I shouldn't be trusted to read this, but I think this section of code is extracting anything "up to" the first colon in the authority?

https://github.com/OISF/suricata/blob/c8a7204b159553d338a6294218e696a72efdb4db/rust/src/http2/detect.rs#L617-L624

thus resulting in the "username" value making it to http.host;

Actions #16

Updated by Philippe Antoine 4 months ago

Looks like a bug deserving its own ticket, right ?

Actions #17

Updated by Brandon Murphy 4 months ago

I guess if you think so.

IMO it fell under the for the information to be normalized out when populating the http.host buffer part of the request because IMO, none of the userinfo should show up in http.host at all.

i'll go ahead and create one.

Actions #18

Updated by Brandon Murphy 4 months ago

Brandon Murphy wrote in #note-17:

i'll go ahead and create one.

https://redmine.openinfosecfoundation.org/issues/6479

Actions #19

Updated by Victor Julien 3 months ago

  • Target version changed from 7.0.3 to 8.0.0-beta1
  • Label Needs backport to 7.0 added
Actions #20

Updated by OISF Ticketbot 3 months ago

  • Subtask #6507 added
Actions #21

Updated by OISF Ticketbot 3 months ago

  • Label deleted (Needs backport to 7.0)
Actions #22

Updated by Philippe Antoine 3 months ago

  • Status changed from In Review to Resolved
Actions #24

Updated by Philippe Antoine 2 months ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF