Quic ietf 4967 v3 #7095

catenacyber · 2022-03-02T21:58:00Z

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4967
https://redmine.openinfosecfoundation.org/issues/5143

Describe changes:

Parses standardized QUIC IETF v1
bump up rust carte tls_parser already used by rep, so that it can be used by quic
Rustfmt previous code

suricata-verify-pr: 775
OISF/suricata-verify#775

Replaces #7082 with

default features false for added crate rustls
detection for sni
detection for ja3 string

Still to do :

see https://redmine.openinfosecfoundation.org/issues/4966

Ticket: 4967 The format of initial packet for quic ietf, ie quic v1, is described in rfc 9000, section 17.2.2

so that we can use new functions in quic parser

and logs interesting extensions from crypto frame

As it can be 4, but it can also be 1, based on the first decrypted byte

The way to determine if the payload is encrypted is by storing in the state if we have seen a crypto frame in both directions...

So as to keep parse not too big

for detection

Ticket: 5143

codecov · 2022-03-02T22:23:04Z

Codecov Report

Merging #7095 (065f1ae) into master (ec01a94) will decrease coverage by 0.14%.
The diff coverage is 90.69%.

@@            Coverage Diff             @@
##           master    #7095      +/-   ##
==========================================
- Coverage   75.54%   75.39%   -0.15%     
==========================================
  Files         628      629       +1     
  Lines      185325   185363      +38     
==========================================
- Hits       139996   139761     -235     
- Misses      45329    45602     +273

Flag	Coverage Δ
fuzzcorpus	`54.38% <38.46%> (-0.34%)`	⬇️
suricata-verify	`54.58% <85.71%> (-0.03%)`	⬇️
unittests	`63.17% <69.04%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2022-03-03T03:40:05Z

WARNING:

field	test	baseline	%
	tlpw1_stats_chk
.app_layer.tx.quic	2592	1140	227.37%
.app_layer.error.quic.parser	3768	1810	208.18%
	tlpr1_stats_chk
.app_layer.tx.quic	13818	4198	329.16%
.app_layer.error.quic.parser	1416606	43951	3223.15%

Pipeline 6451

victorjulien · 2022-03-03T10:59:45Z

rust/src/quic/frames.rs

+pub(crate) struct Crypto {
+    pub ciphers: Vec<TlsCipherSuiteID>,
+    pub extv: Vec<QuicTlsExtension>,
+    //does not work, because of lifetime due to references to the slice used for parsing


can this comment move to extv just above and explain why it is like this instead of using Vec<TlsExtension>? Comments about why code that is in use are like they are, are more useful than comments explaining commented out things, I think.

victorjulien

Some comments, mostly code tidiness suggestions. Will also give this a run on my own quic traffic.

victorjulien · 2022-03-03T11:03:53Z

rust/src/quic/frames.rs

+                    _ => {}
+                }
+                let mut values = Vec::new();
+                match e {


can this be broken out into a util func?

@jasonish is there a better rustier way to build a string and write integer into it ?

You could do

let mut s = String::new(); s.push_str(&my_u16.to_string());

Thank you Jason, looks better indeed

victorjulien · 2022-03-03T11:04:55Z

rust/src/quic/frames.rs

+}
+
+// get interesting stuff out of parsed tls extensions
+fn quic_get_tls_extensions(


could this func be done in a more compact way?

more compact with Jason advice

victorjulien · 2022-03-03T11:05:33Z

rust/src/quic/frames.rs

+                }
+                extv.push(QuicTlsExtension { etype, values })
+            }
+            if client {


perhaps move this into a client specific util func

victorjulien · 2022-03-03T11:07:04Z

rust/src/quic/frames.rs

+                    let mut dash = false;
+                    for c in &ch.ciphers {
+                        if dash {
+                            match write!(&mut ja3, "-") {


these kinds of statements are taking 3 lines each for no good reason. Can these be made more compact?

let _ = write...

(noted above already)

push_str seems nice

victorjulien · 2022-03-03T11:14:51Z

rust/src/quic/quic.rs

+            if framebuf.len() < 4 + hkey.sample_len() {
+                return false;
+            }
+            let mut h2 = Vec::with_capacity(hlen + header.length);


what is the max size we might alloc here?

Not more than the UDP payload size...
Should it be more obvious or debug tested ?

yeah, I was trying to figure out where it is set and the logic is quite hard to follow but ultimately seems to come from quic_var_uint, so it might be higher if not properly checked?

If it shouldn't be > UDP payload size, could it be u16?

It comes indeed from quic_var_uint
But then, we have if header.length > rest.len() {
before `let (mut framebuf, next_buf) = rest.split_at(header.length);

Do you want the check to happen earlier ?

victorjulien · 2022-03-03T11:16:15Z

rust/src/quic/quic.rs

+                        framebuf = &output;
+                    }
+                    buf = next_buf;
+                    match QuicData::from_bytes(framebuf) {


can this func be broken up, for example handling the following in a until func? Not only is the func large, the indent levels are also getting hard to track

victorjulien · 2022-03-03T13:17:05Z

Should the ja3 string here end in a comma?

{
  "version": "1",
  "sni": "mask.icloud.com",
  "ja3": "771,51914-4865-4866-4867,64250-0-10-16-5-13-18-51-45-43-57-27-20-64250,6682-29-23-24-25,",
  "extensions": [
    {
      "type": "Grease"
    },
    {
      "type": "ServerName",
      "values": [
        "mask.icloud.com"
      ]
    },
    {
      "type": "SupportedGroups"
    },
    {
      "type": "ApplicationLayerProtocolNegotiation",
      "values": [
        "h3"
      ]
    },
    {
      "type": "StatusRequest"
    },
    {
      "type": "SignatureAlgorithms"
    },
    {
      "type": "SignedCertificateTimestamp"
    },
    {
      "type": "KeyShare"
    },
    {
      "type": "PskExchangeModes"
    },
    {
      "type": "SupportedVersions"
    },
    {
      "type": "TlsExtensionType(57 / 0x39)"
    },
    {
      "type": "TlsExtensionType(27 / 0x1b)"
    },
    {
      "type": "ServerCertificateType"
    },
    {
      "type": "Grease"
    }
  ]
}

victorjulien · 2022-03-03T13:44:02Z

@jasonish any thoughts on the logging here (see example above). I find the extentions rather verbose, but can't immediately come up with an alternative.

Additionally I wonder how to log the unknown extensions. If we log them this way and then later add support for them its in a way a breaking output change? :)

catenacyber · 2022-03-03T15:04:57Z

Should the ja3 string here end in a comma?

Yes it is
I used https://infosecwriteups.com/demystifying-ja3-one-handshake-at-a-time-c80b04ccb393 to learn about JA3
It uses extension EllipticCurvePointFormat which got obsolete in TLS1.3

My understanding is that every ja3 should contain 4 commas to distinguish the 5 different fields

jasonish · 2022-03-03T17:10:37Z

rust/src/quic/frames.rs

+                match write!(ja3, "{}", u16::from(etype)) {
+                    _ => {}
+                }


The idiomatic way to avoid the result warning is:

let _ = write!(...

jasonish · 2022-03-03T17:14:38Z

rust/src/quic/quic.rs

+    if state.parse(buf, false) {
+        return AppLayerResult::ok();
+    }
+    return AppLayerResult::err();


Really tiny nit.. Idiomatic Rust would be:

if state.parse(buf, false) { AppLayerResult::ok() } else { AppLayerResult::err() }

Ok. I did not know Rust was in the explicit else side

jasonish · 2022-03-03T17:25:33Z

@jasonish any thoughts on the logging here (see example above). I find the extentions rather verbose, but can't immediately come up with an alternative.

I think it looks fine as I can't really see another way. Would we want to log all the extensions by default? Or should we have an extended mode that logs extensions which is off by default.

Additionally I wonder how to log the unknown extensions. If we log them this way and then later add support for them its in a way a breaking output change? :)

Maybe something like:

"extensions": [
    {
      "type": 0,
      "name": "ServerName",
      "values": [
        "mask.icloud.com"
      ]
    },
    {
      "type": 99
    },

type is the only required field.

I also noticed that we're using names like ServerName, while Wireshark uses server_name. Whats the most common here?

catenacyber · 2022-03-03T20:21:14Z

I also noticed that we're using names like ServerName, while Wireshark uses server_name. Whats the most common here?

In the RFC https://datatracker.ietf.org/doc/html/rfc3546
server_name is the enum value and ServerName the name of the structure...

catenacyber · 2022-03-03T20:29:59Z

Replaced by #7101

jasonish · 2022-03-03T20:57:56Z

In the RFC https://datatracker.ietf.org/doc/html/rfc3546
server_name is the enum value and ServerName the name of the structure...

I think using a name of server_name is more consistent with the RFC.

victorjulien · 2022-03-04T06:45:10Z

In the RFC https://datatracker.ietf.org/doc/html/rfc3546
server_name is the enum value and ServerName the name of the structure...

I think using a name of server_name is more consistent with the RFC.

Wonder if we should also make things more compact, e.g. instead of ApplicationLayerProtocolNegotiation use ALPN

catenacyber · 2022-03-04T07:42:38Z

These wordings come from crate tls-parser...
Should Suricata define its own wording ?

jasonish · 2022-03-04T07:49:06Z

Should Suricata define its own wording ?

I'm not opposed to it. An upstream library could change its naming which would affect us resulting in a breaking change.

catenacyber added 9 commits February 28, 2022 14:11

quic: rustfmt

e2fa104

quic: complete parsing of initial for non gquic

be45f19

Ticket: 4967 The format of initial packet for quic ietf, ie quic v1, is described in rfc 9000, section 17.2.2

rdp: bump up tls-parser crate version

b1a67ff

so that we can use new functions in quic parser

quic: parse more frames

f96956f

and logs interesting extensions from crypto frame

quic: use packet num length when decrypting

a24304b

As it can be 4, but it can also be 1, based on the first decrypted byte

quic: do not try to parse encrypted data

0c9d9e1

The way to determine if the payload is encrypted is by storing in the state if we have seen a crypto frame in both directions...

quic: decrypt in its own function

d5ba17e

So as to keep parse not too big

quic: use sni from crypto frame with tls

f9a89f5

for detection

quic: ja3 computation and logging and detection

065f1ae

Ticket: 5143

catenacyber requested review from victorjulien, jasonish and a team as code owners March 2, 2022 21:58

catenacyber mentioned this pull request Mar 2, 2022

Quic ietf 4967 v2 #7082

Closed

victorjulien reviewed Mar 3, 2022

View reviewed changes

victorjulien requested changes Mar 3, 2022

View reviewed changes

jasonish reviewed Mar 3, 2022

View reviewed changes

catenacyber mentioned this pull request Mar 3, 2022

Quic ietf 4967 v4 #7101

Closed

catenacyber closed this Mar 3, 2022

Quic ietf 4967 v3 #7095

Quic ietf 4967 v3 #7095

Conversation

catenacyber commented Mar 2, 2022

codecov bot commented Mar 2, 2022

Codecov Report

suricata-qa commented Mar 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

victorjulien left a 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 Mar 3, 2022

victorjulien commented Mar 3, 2022

catenacyber commented Mar 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonish commented Mar 3, 2022

catenacyber commented Mar 3, 2022

catenacyber commented Mar 3, 2022

jasonish commented Mar 3, 2022

victorjulien commented Mar 4, 2022

catenacyber commented Mar 4, 2022

jasonish commented Mar 4, 2022