Output/TLS: Allow logging of client/server handshake parameters - V2 #12501
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 - "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 use-case is for logging TLS handshake parameters in order to survey them, and so that JA4(S) hashes can be computed offline (in the case that they're not already computed for the purposes of rule matching).
rmcconnell-r7
requested review from
catenacyber,
jasonish,
jufajardini,
victorjulien and
a team
as code owners
|
NOTE: This PR may contain new authors. |
|
Replaces: #12071 |
|
Please specify the SV branch as SV_BRANCH=OISF/suricata-verify#2265 This way the CI here will use it. |
|
Done. Thanks, tbh I guessed that may be needed haha.. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12501 +/- ##
==========================================
+ Coverage 80.56% 80.59% +0.02%
==========================================
Files 925 925
Lines 259292 259399 +107
==========================================
+ Hits 208906 209051 +145
+ Misses 50386 50348 -38
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -6792,6 +6840,9 @@ | |||
| }, | |||
| "ja4": { | |||
| "type": "string" | |||
| }, | |||
| "ja4s": { | |||
| "type": "string" | |||
There was a problem hiding this comment.
Is there not some license question around ja4s ?
There was a problem hiding this comment.
And this needs at least to be in its own commit
There was a problem hiding this comment.
Adding the label [decision-required] about this point
| out[0..36].copy_from_slice(hash.as_bytes()); | ||
| } | ||
|
|
||
| #[no_mangle] | ||
| pub unsafe extern "C" fn SCJA4SGetHash(j: &mut JA4, out: &mut [u8; 25]) { |
There was a problem hiding this comment.
Can we have something like #define JA4_LEN 25 ?
There was a problem hiding this comment.
Meaning it should be defined in rust and used from rust.h thanks to cbindgen ;-)
| @@ -695,7 +695,8 @@ static inline int TLSDecodeHSHelloVersion(SSLState *ssl_state, | |||
| ssl_state->curr_connp->version = version; | |||
|
|
|||
| if (ssl_state->curr_connp->ja4 != NULL && | |||
| ssl_state->current_flags & SSL_AL_FLAG_STATE_CLIENT_HELLO) { | |||
| ssl_state->current_flags & | |||
| (SSL_AL_FLAG_STATE_CLIENT_HELLO | SSL_AL_FLAG_STATE_SERVER_HELLO)) { | |||
| SCJA4SetTLSVersion(ssl_state->curr_connp->ja4, version); | |||
There was a problem hiding this comment.
This looks like changing the behavior of ja4...
| { | ||
| char ssl_version[SSL_VERSION_MAX_STRLEN]; | ||
| SSLVersionToString(ssl_state->server_connp.version, ssl_version); |
It was rather from #12071 (comment)
Does it work now if JA4 is disabled ? SV PR tests require JA4... |
Add new custom log fields:
The use-case is for logging TLS handshake parameters in order to survey them, and so that JA4(S) hashes can be computed offline (in the case that they're not already computed for the purposes of rule matching).
As part of this update suricata now creates a JA4 object when the message type is
CLIENT_HELLO | SERVER_HELLOto allow for custom fields without the need to explicitly enable JA4 viaja4-fingerprint. That enables us to produce the handshake parameters, as well as the other JA4 related fields, without the fingerprint(s). Is that ok? I think that was the intention from the previous PR discussion?Link to my SV tests PR: OISF/suricata-verify#2265
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#2265
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=