Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dns over http2 5773 v12 #11292

Closed
wants to merge 7 commits into from

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • analyze DNS over HTTP2

SV_BRANCH=OISF/suricata-verify#1734

Draft to get feedback about approach...

#11242 with needed rebase

TODO :

Functionnaly, in terms of output :

  • The flow will have doh2 as app_proto (and http2 as app_proto_orig)
  • There are doh2 events that have both http2 and dns fields. dns logging is done like alerts, not like dns events...
  • DNS and HTTP and HTTP2 signatures work on DOH2
  • Signatures can be DOH2 specific. These signatures can combine http, http2 and dns keywords.
  • DNS Frames do not work on DoH2

Memory management

  • a HTTP2 tx can own 2 DNS tx (one to client, one to server)
  • a HTTP2 tx stores the streamed DNS response content until it is complete before parsing it (limiting to U16_MAX bytes)

API

  • There is a new DOH2 app-layer protocol which resorts to HTTP2 for most of the things
  • HTTP2 parsing discovering DOH2 changes the app_proto to DOH2, doh2 can be enabled or disabled in suricata.yaml config
  • There are more alproto "comparison" functions
  • Every DNS keyword needs to check the flow alproto to change the transaction
  • Every DNS/HTTP keyword is automatically registered for DOH2 (hacking DetectAppLayerMpmRegister2...

by making tx parsing and creation more easily available,
without needing a dns state.

Dns event NotResponse is now set on the right tx, and not the one
before.

Also debug log for Z-flag on request says "request" instead of
"response"

Also rustfmt dns.rs
Now a flow alproto can be changed by a call to AppLayerParserParse
when HTTP2 forces the flow to turn into DOH2.
Ticket: 5773

Handles both directions the same way for data if content type is
application/dns-message
@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 21055

@@ -146,6 +149,12 @@ pub struct HTTP2Transaction {
pub escaped: Vec<Vec<u8>>,
pub req_line: Vec<u8>,
pub resp_line: Vec<u8>,

is_doh_response: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this all be wrapped in an Option? It seems like a large expansion of the http/2 tx

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good idea, will try to improve on it

js.open_array("query")?;
for i in 0..0xFFFF {
let mut jsa = JsonBuilder::try_new_object()?;
if !SCDnsLogJsonQuery(dtx, i, 0xFFFFFFFFFFFFFFFF, &mut jsa) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are these 0xFFFFFFFFFFFFFFFF uses, looks not very nice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed not very nice, this PR is still a draft waiting for the dns log overhaul in https://redmine.openinfosecfoundation.org/issues/6281 to do the same thing...

@@ -56,6 +56,12 @@ static InspectionBuffer *GetBuffer(DetectEngineThreadCtx *det_ctx,
return buffer;
}

if (f->alproto == ALPROTO_DOH2) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we hide this behind this a single (inline?) call in all places?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will try to make it nicer...

@victorjulien
Copy link
Member

looks good overall. Some comments inline.

@catenacyber
Copy link
Contributor Author

Continues in #11369

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants