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

Quic ietf 4967 v5 #7106

Closed
wants to merge 13 commits into from
Closed

Conversation

catenacyber
Copy link
Contributor

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

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 #7101 with

  • Q039 support
  • move earlier the check against header.length
  • use Suricata's own quic_tls_extension_name (more complete including especially quic_transport_parameters

Still to do more generally about quic : see https://redmine.openinfosecfoundation.org/issues/4966 (frame support)

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
@catenacyber catenacyber mentioned this pull request Mar 4, 2022
@victorjulien
Copy link
Member

Looking much better

 126536 "1"
  58347 "ff00001d"
  21606 "faceb002"
   2189 "Q043"
    180 "Q046"

No other versions than these.

@victorjulien
Copy link
Member

ja3 situation remains unchanged. A single IP in my network has 993 unique values, which seems suspicious. If it is correct, then I seriously question if we should bother with ja3 at all for quic.

@catenacyber
Copy link
Contributor Author

Oops, forgot to update S-V test with new extensions naming

let extv = quic_get_tls_extensions(ch.ext, &mut ja3, true);
return Ok((rest, Frame::Crypto(Crypto { ciphers, extv, ja3 })));
}
ServerHello(sh) => {
Copy link
Member

Choose a reason for hiding this comment

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

In TLS/tcp we only have ja3 for client hello iirc, for the server side there is ja3s. How is that here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used only one ja3.
I do not understand why there should be a different naming.

The client ja3 has EllipticCurve and EllipticCurvePointFormat data while the server does not have them (as the server is not supposed to send such extensions) cf quic_tls_ja3_client_extends

Does that answer your question ?

Copy link
Member

Choose a reason for hiding this comment

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

at least for tls ja3 is for clients, ja3s for servers

Copy link
Member

Choose a reason for hiding this comment

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

this is not our invention, just part of the official "standard" https://github.com/salesforce/ja3#ja3s

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, you want two signature keywords ja3 and ja3s instead of using toclient or toserver ? Or ?

Copy link
Member

Choose a reason for hiding this comment

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

I want to it match how we do it for tls, unless there is a good reason not to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #7130

I find it a bit strange as a code duplication, but I do not think this reason is good enough

@catenacyber
Copy link
Contributor Author

A single IP in my network has 993 unique values, which seems suspicious. If it is correct, then I seriously question if we should bother with ja3 at all for quic.

I think it is correct.
@victorjulien You can check what Wireshark outputs like tshark -Tfields -e tls.handshake.ja3_full -r /Users/catena/Downloads/udp443.pcap | grep -v "^$"

@catenacyber
Copy link
Contributor Author

Needs OISF/suricata-verify#780

@suricata-qa
Copy link

WARNING:

field test baseline %
tlpr1_stats_chk
.app_layer.flow.quic 33523 39556 84.75%
.app_layer.tx.quic 106109 4198 2527.61%
.app_layer.error.quic.parser 54065 43951 123.01%

Pipeline 6470

@victorjulien victorjulien marked this pull request as draft March 5, 2022 09:45
@catenacyber catenacyber mentioned this pull request Mar 6, 2022
@catenacyber
Copy link
Contributor Author

Replaced by #7118

@catenacyber catenacyber closed this Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants