Dns over http2 5773 v1 #9965

catenacyber · 2023-12-05T13:15:16Z

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/5773

Describe changes:

analyze DNS over HTTP2

SV_BRANCH=pr/1509

OISF/suricata-verify#1509

Draft to get feedback about approach...
Leaving comments on the code for specific questions

TODO :

Commit history to clean
free dns state in HTTP2 state

Generic approach :

recognize DNS over HTTP2
Enqueue a pseudo-packet with DNS payload on same flow
Dequeue and analyze packet as DNS
restore HTTP2 state of flow

Ticket: 5773

catenacyber · 2023-12-05T13:16:26Z

rust/src/applayer.rs

@@ -477,6 +477,7 @@ pub const APP_LAYER_PARSER_EOF_TS : u16 = BIT_U16!(5);
 pub const APP_LAYER_PARSER_EOF_TC : u16 = BIT_U16!(6);
 pub const APP_LAYER_PARSER_TRUNC_TS : u16 = BIT_U16!(7);
 pub const APP_LAYER_PARSER_TRUNC_TC : u16 = BIT_U16!(8);
+pub const APP_LAYER_PARSER_LAYERED_PACKET : u16 = BIT_U16!(11);


please suggest me a better name :-p

Stacked? Encapped? Encapsulated? They'll kinda mean the same thing.

catenacyber · 2023-12-05T13:16:48Z

rust/src/http2/http2.rs

@@ -25,6 +25,9 @@ use crate::conf::conf_get;
 use crate::core::*;
 use crate::filecontainer::*;
 use crate::filetracker::*;
+
+use crate::dns::dns::rs_dns_state_new;


TODO use rs_dns_state_free

catenacyber · 2023-12-05T13:17:13Z

rust/src/http2/http2.rs

@@ -144,6 +147,8 @@ pub struct HTTP2Transaction {
    pub escaped: Vec<Vec<u8>>,
    pub req_line: Vec<u8>,
    pub resp_line: Vec<u8>,
+
+    pub doh: Vec<u8>,


TODO: add comment to explain this field

I feel like it needs a longer name, I suppose a comment would help.

catenacyber · 2023-12-05T13:17:42Z

rust/src/http2/http2.rs

        let mut host = None;
        for block in blocks {
            if block.name == b"content-encoding" {
                self.decoder.http2_encoding_fromvec(&block.value, dir);
+            } else if block.name == b"accept" {
+                //TODO? faster pattern matching


Victor, is this ok to match on the different header names ?

catenacyber · 2023-12-05T13:18:06Z

rust/src/http2/http2.rs

+                }
+            } else if block.name == b"content-type" {
+                if block.value == b"application/dns-message" {
+                    // push original 2-bytes DNS/TCP header


TODO: have a longer comment

catenacyber · 2023-12-05T13:18:22Z

rust/src/http2/http2.rs

@@ -299,13 +327,17 @@ impl HTTP2Transaction {
                &xid,
            );
        };
+        if !self.doh.is_empty() {
+            self.doh.extend_from_slice(decompressed);


TODO comment

catenacyber · 2023-12-05T13:19:01Z

rust/src/http2/http2.rs

@@ -441,6 +474,9 @@ pub struct HTTP2State {
    dynamic_headers_tc: HTTP2DynTable,
    transactions: VecDeque<HTTP2Transaction>,
    progress: HTTP2ConnectionState,
+    // layered packets contents for DNS over HTTP2
+    layered: Vec<Vec<u8>>,
+    dns_state: Option<*mut std::os::raw::c_void>,


TODO comment

I am not sure if this should be generic or DNS-specific...

Is DoH a little special in that it doesn't follow the normal PROTO-over-HTTP formats like something like GRPC might do? Where they just use the bodies for their payload?

Where they just use the bodies for their payload?

Indeed DoH is not exactly like that : it has DNS message in the URI for requests (a base64-encoded parameter), and in bodies for responses.

Why does it matter ?

Where they just use the bodies for their payload?

Indeed DoH is not exactly like that : it has DNS message in the URI for requests (a base64-encoded parameter), and in bodies for responses.

Why does it matter ?

Say we were to support gRPC over HTTP, and Avro over HTTP, and Thrift over HTTP. Those would also make sense to have something generic. As DoH is different, could it be put within the same generic container or does it always need some special handling due to be in the URI. I guess not. But that's the path of thought I was on.

Indeed I was hesitating for naming dns_state or something more generic...

I'm OK without over thinking making it more generic until we have the need to make it more generic.

catenacyber · 2023-12-05T13:19:23Z

rust/src/http2/http2.rs

-                                        flow).is_err() {
-                                        self.set_event(HTTP2Event::FailedDecompression);
+                                        flow) {
+                                    Ok(_) => {


TODO helper function for less indent

catenacyber · 2023-12-05T13:20:58Z

rust/src/http2/http2.rs

+                            unsafe {
+                                self.dns_state = Some(rs_dns_state_new(std::ptr::null_mut(), ALPROTO_HTTP2));
+                            }
+                        }


⚠️ This is a key first step :
we

set a flag in pstate (so that AppLayerParserParse knows that it should deque packets)

push the to-be pseudo-packet payload in HTTP2 state

create a DNS state if there is not one yet

catenacyber · 2023-12-05T13:22:34Z

src/app-layer-parser.c

@@ -1438,6 +1441,32 @@ int AppLayerParserParse(ThreadVars *tv, AppLayerParserThreadCtx *alp_tctx, Flow
        }
    }

+    if (pstate->flags & APP_LAYER_PARSER_LAYERED_PACKET) {
+        // hardcoded logic


TODO: instead of hardcoded logic, protocols (such as HTTP2) should have a generic function that returns the pseudo-packets payloads

catenacyber · 2023-12-05T13:22:54Z

src/app-layer-parser.c

+                    continue;
+                }
+                PKT_SET_SRC(np, PKT_SRC_APP_LAYER_LAYERED);
+                // TODO np->flags |= PKT_PSEUDO_DETECTLOG_FLUSH;


TODO: remove until we need special flagging

catenacyber · 2023-12-05T13:25:05Z

src/app-layer-parser.c

+                np->payload = SCMalloc(b_len);
+                memcpy(np->payload, b, b_len);
+                np->payload_len = b_len;
+                PacketEnqueueNoLock(&tv->decode_pq, np);


⚠️ this is the second key step :

@victorjulien is &tv->decode_pq the right place to ensue such pseudo packets ?
I fear this will not work if I encapsulate my DNS over HTTP2 on GRE or other tunnels...

(&fw->pq is not already accessible here)

catenacyber · 2023-12-05T13:25:57Z

src/flow-worker.c

+        // switch flow to new protocol
+        void *alstate_orig = p->flow->alstate;
+        AppProto alproto_orig = p->flow->alproto;
+        // hardcoded logic


TODO: have a not hardcoded logic for this

catenacyber · 2023-12-05T13:26:59Z

src/flow-worker.c

+        SCFree(x->payload);
+        FlowDeReference(&x->flow);
+        TmqhOutputPacketpool(tv, x);
+    }


⚠️ this is the third key step

We deque and process the packets, switching the protocol and state...

catenacyber · 2023-12-05T13:27:30Z

src/flow-worker.c

+        FramesPrune(x->flow, x);
+
+        // switch back to real protocol
+        p->flow->alstate = alstate_orig;


Is flow locked ? Can a race condition happen between changing alstate and alproto ?

catenacyber · 2023-12-05T13:28:23Z

src/app-layer-parser.c

+                // TODO np->flags |= PKT_PSEUDO_DETECTLOG_FLUSH;
+                np->payload = SCMalloc(b_len);
+                memcpy(np->payload, b, b_len);
+                np->payload_len = b_len;


Should I put a timestamp for this packet ? Where can I get it ?

catenacyber · 2023-12-05T13:29:02Z

src/stream-tcp.h

@@ -205,5 +205,8 @@ uint64_t StreamDataRightEdge(const TcpStream *stream, const bool eof);
 void StreamTcpThreadCacheEnable(void);
 void StreamTcpThreadCacheCleanup(void);

+// move elsewhere


Ideas where this function should belong ?

catenacyber · 2023-12-05T13:29:24Z

src/stream-tcp.c

+    return np;
+error:
+    FlowDeReference(&np->flow);
+    PacketPoolReturnPacket(np);


This PacketPoolReturnPacket was missing originally, correct ?

catenacyber · 2023-12-05T13:32:18Z

src/flow-worker.c

+            FLOWWORKER_PROFILING_END(x, PROFILE_FLOWWORKER_DETECT);
+        }
+        OutputLoggerLog(tv, x, fw->output_thread);
+        FramesPrune(x->flow, x);


I should test frames I guess

victorjulien · 2023-12-05T13:38:43Z

Before diving into the details I would like to read what the high level design is around the "layered" logic and pseudo packet(s), and how it would hook into detect and logging.

catenacyber · 2023-12-05T13:41:49Z

Before diving into the details I would like to read what the high level design is around the "layered" logic and pseudo packet(s), and how it would hook into detect and logging.

I wrote this :

recognize DNS over HTTP2
Enqueue a pseudo-packet with DNS payload on same flow
Dequeue and analyze packet as DNS
restore HTTP2 state of flow

What do you expect different for a high-level design ?

catenacyber · 2023-12-05T13:44:28Z

There is no change to the json schema.

flow events remain HTTP2
for this flow, we have both http and dns events (and alerts for both as well)

victorjulien · 2023-12-05T13:53:14Z

Before diving into the details I would like to read what the high level design is around the "layered" logic and pseudo packet(s), and how it would hook into detect and logging.

I wrote this :
* recognize DNS over HTTP2

* Enqueue a pseudo-packet with DNS payload on same flow

* Dequeue and analyze packet as DNS

* restore HTTP2 state of flow
What do you expect different for a high-level design ?

An actual explanation of the flow of packets and logic? Just a few half sentences isn't an explanation.

catenacyber · 2023-12-05T21:04:22Z

This PR is a POC for DNS over HTTP2.
This is HTTP2 traffic where the HTTP2 payload can be interpreted as DNS.

As such, it is an example of a flow having multiple app-layer protocols. (DCERPC over SMB is the example in the current code).

high level explanation of the design

Functionnaly, in terms of output :

The goal, in terms of output, is to keep the same flow event (and so the same Flow structure in Suricata) which will have http2 as its app_proto, but have both http and dns events related to this flow (thanks to the flow_id). Signatures and alerts that work for dns, will work out of the bow for DNS over HTTP2. DNS over HTTP2 logs will be the same as normal DNS logs.
Another approach could be to make flow having a list of app_protos. (Looks too much a breaking change)
Or another approach could be to create a new app_proto dns_over_http2 (kind of like http1 and http2 share keywords under the http app_proto). But nothing prevents the flow to have both DoH and plain HTTP2 requests...

How does layering work

Memory management :
Into the code, memory ownership and structures, the DNS stuff must go somewhere related to the Flow. As Flow structure does not change, we use one of its fields, the alstate which is a pointer to one of the app-protocols state. So, the HTTP2State may own a DNSState. (Actually, I am not sure that we need a DNSState, maybe we can do only with DNSTransactions, but the current code uses state)
Data processing steps :
To have logging and detection for DNS, the hack is to switch the Flow's app_proto + alstate back and forth between HTTP2 and DNS. The normal state is to have HTTP2, log it and detect it.
If there is a DNS message in the HTTP2 payload,
we parse, log and detect on the HTTP2 layer,
we somehow extract the bytes of the DNS message out of the HTTP2 payload
we create a pseudo-packet with the DNS payload. Pseudo-packets already exist in Suricata to be able to run logging/detection on events such as flow timeouts for instance (or protocol change, like HTTP1->HTTP2 upgrade where one packet has the first bytes being HTTP1 and the last one being HTTP2). (Actually I am not sure we need a full packet instead of just a payload + an app_proto but Detect and OutputLoggerLog take a packet as an argument). Note that one HTTP2 packet can have multiple frames/streams and thus multiple DNS messages.
we switch app_proto + alstate to DNS, parse log and detect the DNS layer (watching out for type confusion)
then switch back app_proto + alstate to HTTP2 to be ready to process next frames
Code API
Generically for layered protocols, and for the app-proto API, this means a callback function that will give the payload (and the top app_proto like dns in this case) for pseudo-packets, and a function that will return the top app_proto state (the DNSState here) out of the base one (the HTTP2State here)

The current implementation also adds one bit flag for the u16 app-layer-parser flags (like APP_LAYER_PARSER_TRUNC_TC)

How is the implementation dealing with the current constraints ?

Which constraints ?

What are (expected) performance impacts ?

For time, I would roughly expect that a DoH packet takes twice as much time as a HTTP2 or DNS packet as we parse/log/detect both twice.

For space, we use a bit more memory for HTTP2 flows (not for other flows) and transactions, adding one or two fields that are pointers, unused if it is not DoH, and that are roughly twice the size of a HTTP2 flow when it is DoH (as it is both a DNS and a HTTP2 flow)

This is for regular processing.
For worst-case scenarios, the current POC has certainly its specific issues (like we do not enforce a U16_MAX limit for DNS messages as HTTP2 can transport more than 65k bytes)

suricata-qa · 2023-12-06T13:35:03Z

Information: QA ran without warnings.

Pipeline 16892

victorjulien · 2023-12-07T13:13:30Z

Thanks Philippe, this is a very helpful explanation. Lets discuss the overall design before doing more code work on the current PR, as I'm not happy with the double packet logic. I would like the engine itself to understand layered protocols, instead of working around the current lack of support for this use case.

Some questions on what the design should support:

Conceptually, would we want to offer the user a way to match on both http and dns at the same time? e.g.

alert doh ... http.header; content:":authority: dns.google"; dns.query; content:"malware.com"; ..

Would we want EVE to log both in a single record? e.g.

{
  "event_type":"doh"
  "dns": {
  }
  "http2": {
  }
}

victorjulien · 2023-12-07T13:16:44Z

In my thinking we could do 2 models:

flow->alstate(http2)->alstate(dns) - where the engine is updated to inspect/manage/log both states more or less independently
flow->alstate(http2)->tx(http2)->tx(dns) - here there would be a 1:1 mapping between http2 and DNS tx's (kind of like files are in a tx) and the engine could inspect and log them in a single pass, making it possible to support the examples above

catenacyber · 2023-12-07T13:28:01Z

Conceptually, would we want to offer the user a way to match on both http and dns at the same time?

Looks nice as it brings more features.

This would be possible with the ALPROTO_DOH solution

This looks kind of a specific solution for DoH, does not seem so generic for layered protocols. I mean will there be a combinatorial explosion for ALPROTO with such a solution ? Or not because there are not so many existing combinations

flow->alstate(http2)->tx(http2)->tx(dns) - here there would be a 1:1 mapping between http2 and DNS tx's (kind of like files are in a tx) and the engine could inspect and log them in a single pass, making it possible to support the examples above

Are not DNS transactions unidirectional (and HTTP2 are bidirectional) ?

victorjulien · 2023-12-07T13:56:58Z

Conceptually, would we want to offer the user a way to match on both http and dns at the same time?

Looks nice as it brings more features.

This would be possible with the ALPROTO_DOH solution

This looks kind of a specific solution for DoH, does not seem so generic for layered protocols. I mean will there be a combinatorial explosion for ALPROTO with such a solution ? Or not because there are not so many existing combinations

SMB/DCERPC seem quite similar. I think there the (hardcoded) solution is closer to having 2 states and have the detect/log etc code iterate them where needed.

flow->alstate(http2)->tx(http2)->tx(dns) - here there would be a 1:1 mapping between http2 and DNS tx's (kind of like files are in a tx) and the engine could inspect and log them in a single pass, making it possible to support the examples above

Are not DNS transactions unidirectional (and HTTP2 are bidirectional) ?

Yeah, so perhaps in that case you'd need tx(http2)->tx(dns)[2] or something, where the direction determines which dns tx to use

catenacyber · 2023-12-07T15:43:19Z

So, I will try the ALPROTO_DOH2 next

zoomequipd · 2023-12-11T15:45:08Z

Conceptually, would we want to offer the user a way to match on both http and dns at the same time? e.g.
alert doh ... http.header; content:":authority: dns.google"; dns.query; content:"malware.com"; ..

The more I thought about this, the more I liked this idea. As we start seeing some DOH Providers offering "specialized" dns services and and being used to facilitate Alternated DNS Roots, the ability to match the :authority/host header with the DNS query/domain could be used to provide high fidelity detection.

So, given a single query for DOH, the following signatures would fire.
Assuming that all these other

alert dns ... dns.query; content:"malware.com"; ... sid:1; ...
alert http ... http.uri; content:"<base64 encoded dns query for malware.com>"; ... sid:2; ...
 ^^^^ (would likely need 3 rules for the various base64 offsets)
alert doh ... http.header; content:":authority: dns.google"; dns.query; content:"malware.com"; ... sid:3;...

sid:1 allows us to write typical DNS formatted rules, where I don't care what protocol is used to make the query
sid:2 allows us to write signature to detect protocol layer issues and other detections on dns traffic
sid:3 allows us to write signatures base specifically on DOH traffic and can combined elements of the HTTP layer and DNS buffers

Question:

As I don't understand the full context of this PR, how does this impact the additional DNS buffers efforts that @jasonish is working on? Will alert doh and alert dns both benefit from the additional buffers?
with alert doh can the to_server/to_client be used to enforce matching on request vs response?

catenacyber · 2023-12-12T13:38:39Z

Note : The ALPROTO_DOH2 implementation will not be able to use DNS frames. (because frames are meant to reference a UDP payload or TCP stream, not a base64-decoded temporary buffer, right ?)

catenacyber · 2023-12-12T13:40:54Z

with alert doh can the to_server/to_client be used to enforce matching on request vs response?

I do not see any difficulties for this

Will alert doh and alert dns both benefit from the additional buffers?

doh means to use all dns buffers

victorjulien · 2023-12-12T13:47:02Z

Note : The ALPROTO_DOH2 implementation will not be able to use DNS frames. (because frames are meant to reference a UDP payload or TCP stream, not a base64-decoded temporary buffer, right ?)

Correct.

catenacyber · 2023-12-12T22:03:32Z

New version in #10040

catenacyber added 11 commits December 5, 2023 10:16

http2: extract dns request

0c908a4

Ticket: 5773

doh: pstate flag for layered packet to come

db356b1

doh: store layered packet in HTTP2 state

08f4f0a

doh: get layered on C side, ready to queue

1f79632

doh: enque pseudo packet

746cf64

fixup! doh: enque pseudo packet

2c350df

doh: switch proto and state back and forth

e7c66e3

doh: try if dns is parsed

f911442

fixup! doh: try if dns is parsed

123de0a

doh: it works also for responses

4ca50c9

fixup format

083cc8b

catenacyber commented Dec 5, 2023

View reviewed changes

catenacyber closed this Dec 12, 2023

Dns over http2 5773 v1 #9965

Dns over http2 5773 v1 #9965

Conversation

catenacyber commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorjulien commented Dec 5, 2023

catenacyber commented Dec 5, 2023

catenacyber commented Dec 5, 2023

victorjulien commented Dec 5, 2023 • edited

catenacyber commented Dec 5, 2023

suricata-qa commented Dec 6, 2023

victorjulien commented Dec 7, 2023

victorjulien commented Dec 7, 2023 • edited

catenacyber commented Dec 7, 2023

victorjulien commented Dec 7, 2023

catenacyber commented Dec 7, 2023

zoomequipd commented Dec 11, 2023

catenacyber commented Dec 12, 2023

catenacyber commented Dec 12, 2023

victorjulien commented Dec 12, 2023

catenacyber commented Dec 12, 2023

victorjulien commented Dec 5, 2023 •

edited

victorjulien commented Dec 7, 2023 •

edited