Output/TLS: Allow logging of client/server handshake parameters - V4 #12650
Conversation
Add new custom log fields: "client_handshake" which logs the following: 1. TLS version used during handshake 2. TLS extensions, excluding GREASE, SNI and ALPN 3. All cipher suites, excluding GREASE 4. All signature algorithms, excluding GREASE The use-case is for logging TLS handshake parameters in order to survey them, and so that JA4 hashe can be computed offline (in the case that they're not already computed for the purposes of rule matching).
"server_handshake" which logs the following: 1. TLS version used during handshake 2. The chosen cipher suite, excluding GREASE 3. TLS extensions, excluding GREASE
The JA4 object can now 'track' the data from each TLS conversation without the user having to explicitly enabling ja4-fingerprint. This is to allow for other fields to be output without the JA4, for example client_handshake.
rmcconnell-r7
requested review from
jasonish,
jufajardini,
victorjulien and
a team
as code owners
|
Replaces: #12526 |
|
Ci failures for rust clippy are expected currently. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12650 +/- ##
==========================================
+ Coverage 80.76% 80.78% +0.01%
==========================================
Files 932 931 -1
Lines 259381 259448 +67
==========================================
+ Hits 209484 209589 +105
+ Misses 49897 49859 -38
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Thanks for this work
CI : not fully green, but a rebase should do it
Code : looking
Commits segmentation : ok
Commit messages :
Also sv- and cl- could be srv and cli for more clarity : sv is suricata-verify for us ;-)
typo JA4 hashe missing s
Git ID set : looks fine for me
CLA : you already contributed
Doc update : ok but
Redmine ticket : ok
Rustfmt : nit: could you add a commit that does rustfmt rust/src/ja4.rs
Tests : ok thanks :-)
Dependencies added: none
| jb_open_object(js, "client_handshake"); | ||
|
|
||
| const uint16_t vers = SCJA4GetVersion(ssl_state->client_connp.ja4); | ||
| JsonTlsLogVersion(js, vers); |
There was a problem hiding this comment.
Why do we use a different version than ssl_state->server_connp.version ? Can it have different values ?
There was a problem hiding this comment.
{"timestamp":"2024-05-08T15:10:21.211207+0100","flow_id":1452419569489363,"pcap_cnt":6764,"event_type":"tls","src_ip":"172.17.56.188","src_port":61461,"dest_ip":"172.17.0.130","dest_port":8080,"proto":"TCP","pkt_src":"wire/pcap","tls":{"sni":"collector.azure.microsoft.scloud","version":"UNDETERMINED","ja3":{"hash":"66eaff235d22f1390d518dda0a0adb10","string":"771,49196-49195-49188-49187-49162-49161-49192-49191-49172-49171-157-156-61-60-53-47-10,0-5-10-11-13-35-23-65281,25-24-23,0"},"ja4":"t12d170800_699f1e34060c_7af1ed941c26","client_handshake":{"version":"TLS 1.2","ciphers":[49196,49195,49188,49187,49162,49161,49192,49191,49172,49171,157,156,61,60,53,47,10],"exts":[0,5,10,11,13,35,23,65281],"sig_algs":[2052,2053,2054,1025,1281,513,1027,1283,515,514,1537,1539]},"from_proto":"http"}}
If you see a snippet here, you will see the version from the output is UNDETERMINED whereas the version in the client_handshake, is: TLS 1.2, which is used to generate the JA4.
The version in this case is the server-version, as you've suggested, so if there hasn't been any server-side communication, this won't be set whilst the JA4 can be generated (as this is client-side). So the idea was to expose the 'raw fields' that are used to generate the hash.
There was a problem hiding this comment.
Thanks, deserves a comment in the code ;-)
There was a problem hiding this comment.
Good point, will be done in the next iteration :)
| #ifndef SURICATA_UTIL_JA4_H | ||
| #define SURICATA_UTIL_JA4_H | ||
|
|
||
| #define JA4_HEX_LEN 36 |
| @@ -1553,8 +1553,7 @@ static int TLSDecodeHandshakeHello(SSLState *ssl_state, | |||
| /* Ensure that we have a JA4 state defined by now, we are in a client/server hello | |||
| and we don't have such a state yet (to avoid leaking memory in case this function | |||
| is entered more than once). */ | |||
| if (SC_ATOMIC_GET(ssl_config.enable_ja4) && | |||
| ssl_state->current_flags & | |||
| if (ssl_state->current_flags & | |||
There was a problem hiding this comment.
@jasonish what do you think about this commit ?
There was a problem hiding this comment.
Seems the if is still relevant, as we're testing for it as part of the if statement?
There was a problem hiding this comment.
I mean it removes the config check ssl_config.enable_ja4 for ja4 computation, and moves it just before logging it...
Does that look ok ?
There was a problem hiding this comment.
I think the original intention was to avoid computing it when disabled, not simply avoid logging it. Something else that I'm realizing is just how much JA4 is still in the code with the --disable-ja4 compile time option. I wonder how this plays with that.
There was a problem hiding this comment.
Ok, its just stubs. But the original intention stands.
There was a problem hiding this comment.
Would it make sense to disassociate the fields that go into computing JA4, from JA4 itself. That way they could be tracked and logged even if ja4 is disabled at compile time? Thoughts @victorjulien @catenacyber ?
There was a problem hiding this comment.
I think it would be cool indeed
There was a problem hiding this comment.
I find that a little confusing, sorry! So the fields that are used to make up a JA4 are to be unrelated to a JA4? Or are you meaning for it to be a handshake object that can produce a JA4? With this change, it really is just that, only it's called JA4. Maybe calling it Handshake would be more suited? So they are tracked without JA$ enablement but we only produce the hash when required? Really, that is what I have produced. So I'm a little uncertain of what you're meaning..
There was a problem hiding this comment.
Yeah, what I'm getting at is that the fields used to compute JA4 not be tied to JA4 code, and tracked independent of JA4 being enabled or not, compiled in or not. Then JA4 is just a user of these fields, if that makes sense.
What I'm not positive about is if that makes sense for our reasoning of making JA4 a compile time option, which is a clear separation from patents (even though JA4 has an exception).
There was a problem hiding this comment.
Could that be a big change to the design of many components within the system? It's hard for me to be sure if my changes will break something else. With this design, it was aiming to allow for extraction of the handshake data as raw fields so that the JA4 can be computed offline. The addition of this part i.e. making it work without having to ja4-fingerprint=yes was something discussed in a previous PR and I thought I'd provide the code for proposal. But now, it seems much more effort for the output we're getting. Maybe it's best to remove that, and force the user to enable JA4/fingerprint in order to get the handshake data, too? (so as-is in the current release) If/when this is finalised we can see about introducing the above proposal?
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/6695
Describe changes:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_REPO=
SV_BRANCH=OISF/suricata-verify#2313
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=