mod_ssl: ClientHello variable collection #483

csmutz · 2024-09-25T02:46:49Z

This patch implements collection of variables from the ClientHello which are later made available in the same manner as the other environment variables of mod_ssl. A new directive, SSLClientHelloVars, enables collection of the raw variables during the clienthello callback which are then formated and provided as environment variables for all the requests in that connection, subject to the standard StdEnvVars functionality. If SSLClientHelloVars is not enabled or if openssl prior to 1.1.1 is used, this option should have little impact--no clienthello information is collected. The environment variables are populated with null if openssl < 1.1.1 is used or SSLClientHelloVars is not set to on.

The ClientHello variables are provided as hex encoded data the same as the raw network protocol/what is returned from openssl.

This patch has been tested on ubuntu 24.10 apache 2.4.62/openssl 3.3.1 verifying correct operation of the config directive when not set (default to off), set to off, and set to on. The variables have been tested in the context of both environment variables for cgi scripts and CustomLog entries. The variables have been used to succesfully generate the correct ja3 and ja4 fingerprints for a small number of common clients (as validated by comparison to zeek implementations of ja3/ja4).

This patch complements my prior pull request, #477.

notroj · 2024-09-25T07:28:37Z

Very interesting indeed - thanks for another PR.

Higher-level comment: does this need to be a new directive? I think it would be better as another SSLOptions option, it's quite similar to SSLOptions +ExportCertData . Secondly, if the only point of exporting these is to create the JA3/JA4 fingerprints, would it be better to do that within mod_ssl? If the algorithm is simple enough and stable, it would not be inappropriate feature I guess.

notroj

Also would be easier to review if you squashed it :)

notroj · 2024-09-25T07:29:46Z

modules/ssl/ssl_engine_vars.c

+
+    if (strEQ(var, "VERSION")) {
+        return apr_psprintf(p, "%04x", (uint16_t) clienthello_vars->version);
+    } else if (strEQ(var, "CIPHERS") && (clienthello_vars->ciphers_len > 0)) {


Code style nit: newline before else if

notroj · 2024-09-25T07:29:58Z

modules/ssl/ssl_engine_vars.c

+        value = apr_palloc(p, clienthello_vars->extids_len * 4 + 1);
+        for (i = 0; i < clienthello_vars->extids_len; i++)
+        {
+            snprintf(value + i * 4, 5, "%04x", (uint16_t) clienthello_vars->extids_data[i]);


Should use apr_snprintf here

fixed, using apr_snprintf

notroj · 2024-09-25T07:30:12Z

modules/ssl/ssl_engine_vars.c

+    } else if (strEQ(var, "EXTENSIONS") && (clienthello_vars->extids_len > 0)) {
+        value = apr_palloc(p, clienthello_vars->extids_len * 4 + 1);
+        for (i = 0; i < clienthello_vars->extids_len; i++)
+        {


style nit: no newline before {

notroj · 2024-09-25T07:49:07Z

I pushed your fork to get a CI run and there are a couple of compiler warnings too https://github.com/notroj/httpd/actions/runs/11028425131

ylavic · 2024-09-25T11:30:26Z

modules/ssl/ssl_engine_kernel.c

+    sslcon->clienthello_vars = apr_palloc(c->pool, sizeof(*clienthello_vars));
+    clienthello_vars = sslcon->clienthello_vars;
+
+    if (clienthello_vars) {


apr_palloc() cannot return NULL in httpd, no need for this conditional.

fixed, removed unnecessary conditional

ylavic · 2024-09-25T11:30:29Z

modules/ssl/ssl_engine_kernel.c

+        SSL_client_hello_get0_ext(ssl, 0x10, &data, &clienthello_vars->alpn_len);
+        clienthello_vars->alpn_data = apr_pmemdup(c->pool, data, clienthello_vars->alpn_len);
+        SSL_client_hello_get0_ext(ssl, 0x2b, &data, &clienthello_vars->versions_len);
+        clienthello_vars->versions_data = apr_pmemdup(c->pool, data, clienthello_vars->versions_len);


Don't these magic 0x.. extensions have a #define somewhere?

fixed, using extension definitions from openssl (consistent with definition used in clienthello callback to get SNI).

ylavic · 2024-09-25T11:30:42Z

modules/ssl/ssl_engine_kernel.c

@@ -2520,6 +2566,12 @@ int ssl_callback_ClientHello(SSL *ssl, int *al, void *arg)

 give_up:
    init_vhost(c, ssl, servername);
+
+    s = mySrvFromConn(c);
+    sc = mySrvConfig(s);


Using mySrvConfigFromConn(c)->clienthello_vars directly could do probably.

fixed, using much more concise code as recommended

ylavic · 2024-09-25T11:30:58Z

modules/ssl/ssl_private.h

+    apr_size_t ciphers_len;
+    const unsigned char *ciphers_data;
+    apr_size_t extids_len;
+    int *extids_data;


const int *?

fixed, changed to const int *

csmutz · 2024-09-25T14:19:41Z

These are the critical questions in implementation approach here--I'm willing to change.

Higher-level comment: does this need to be a new directive? I think it would be better as another SSLOptions option, it's quite similar to SSLOptions +ExportCertData .

I used a directive to help clarify that the SSLClientHelloVars directive only applies at the server/virtual host level, not at directory level, and because this directive has preformance impacts for every connection where it is enabled, regardless if StdEnvVars is on or not. This is because the clienthello variables have to be collected at the start of the connection (you can't retrieve them later if they aren't stored) and the default needs to be to not collect these (most users don't want the additional overhead, even if small). I viewed this as different from the other SSLOptions which are focused on ENV variable creation/clutter only (the certs are available, ExportCertData just switches additional parsing, env vars).

If you want me to convert this to an SSLOptions, I will do so. Then we would just document that this specific option only applies at the server, virtualhost level (is ignored elsewhere) and this option impacts more than env var clutter, it has a performance and memory impact on every connection for the life of the connection?

Secondly, if the only point of exporting these is to create the JA3/JA4 fingerprints, would it be better to do that within mod_ssl? If the algorithm is simple enough and stable, it would not be inappropriate feature I guess.

The fact that we have both ja3 and ja4 speak to the longevity of these. I don't know how long it will be until there is yet another TLS fingerprint method. For my purposes, both ja3 and ja4 don't meet all my needs and I would like to be able to use the raw values in both logs and cgi scripts. I think there are others who will want to do so. Ex. to answer questions like what percentage of clients still advertise TLS 1.0 or TLS 1.1 in supported versions? I think exposing the raw values is best to cover all possible use cases. At the very least, this patch makes implementing ja3 and ja4 easier as the source values are readily available. Regarding implementation difficulty, ja3 is pretty simple. ja4 has a lot of moving parts and edge cases (I've seen not quite right ja4 implementations). If you want to make acceptance of PR contingent upon implementation of ja3 and ja4 in addition to exposing the raw data I would probably do that, but removing access to the raw variables would be a mistake IMO.

I'll work on the fixes requested. Can you give me a pointer to methodology to compile/test against trunk so I can replicate your test pipeline? I've been testing on 2.4 release and simply manually splicing diffs in trunk. This worked in #477 but this is much bigger patch.

notroj · 2024-09-25T14:58:23Z

Thanks a lot for the detailed response @csmutz. Makes sense to me - keep it as a new directive, and keep it exporting everything.

Regarding dev methodology, I recommend starting with git or SVN trunk. If you develop against trunk (whether from git or SVN), you can run the test suite directly. Something like this is a good starting point:

$ svn co https://svn.apache.org/repos/asf/httpd/test/framework/trunk test/framework
$ ./configure --with-test-suite=./test/framework --enable-maintainer-mode=c11 --enable-mods-shared=reallyall ...
$ make && make check

... should be documented somewhere but maybe isn't. Unfortunately because of quirks of our hybrid SVN/git config the CI doesn't run for non-committer PRs currently. I think it should run when you push to your personal fork but maybe you need GitHub Actions enabled or something.

csmutz · 2024-09-26T01:31:18Z

@notroj

In addition to all the above fixes, I have two things I'd like you to double check for me. I really don't want to introduce any subtle errors.

Mismatch in define statements for openssl version 1.1.1.

In the clienthello callback I piggyback on the existing define statement that includes a check for LIBRESSL_VERSION_NUMBER:

#if OPENSSL_VERSION_NUMBER >= 0x10101000L && !defined(LIBRESSL_VERSION_NUMBER)

everywhere else the define statements I use only check openssl version:

#if OPENSSL_VERSION_NUMBER >= 0x10101000L

Is there a potential to cause a problem (failure to compile) with this mismatch? I don't understand why both openssl and libressl versions would be defined at the same time but if that never occurs, why would this condition exist? I could easily duplicate the libressl check but I don't want to duplicate it if I don't understand it.

Reliance on calloc in initialization of sslconn to test for allocation and population of clienthello_vars struct.

At variable formatting time (per request) I check to see if the clienthello variables were populated previously by testing the pointer to the clienthello_vars ssl_engine_vars.c line 991 struct which should fail if clienthello_vars weren't populated because of the calloc used to create sslconn mod_ssl.c line 482. I've convinced myself the logic here is sound (and testing doesn't result in crashes) and I think this follows conventions I see in other places, but could you double check that you agree there's no way an uninitialized pointer could be used here?

Thanks very much for the help!

csmutz · 2024-09-26T04:30:54Z

I've fixed all the requested changes and have fixed the compiler warnings.

ylavic · 2024-09-26T11:08:26Z

modules/ssl/ssl_engine_kernel.c

+    clienthello_vars->ciphers_len = SSL_client_hello_get0_ciphers(ssl, &data);
+    clienthello_vars->ciphers_data = apr_pmemdup(c->pool, data, clienthello_vars->ciphers_len);
+    if (SSL_client_hello_get1_extensions_present(ssl, &ids, &clienthello_vars->extids_len) == 1) {
+        clienthello_vars->extids_data = apr_pmemdup(c->pool, ids, clienthello_vars->extids_len * sizeof(int));


Does this work if ->extids_len == 0 can be returned succesfully?

Also, to avoid a possible dangling ->extids_data I would either:

else { clienthello_vars->extids_data = NULL; clienthello_vars->extids_len = 0; }

or use apr_pcalloc() above, the additional/implicit memset() would be in the noise in this path anyway (I think), possibly more future extensions proof too.

good catch. We have a big issue if extids_len is never set.

There were a couple serious bugs and other issues here. Much of this code assumed that the openssl functions would set the associated lengths correctly even if the functions returned failure which is not right.

To fix these issues, I've rewritten this whole code block doing the following:

using calloc for struct (strictly shouldn't be necessary but agreed this is not expensive)

correctly check return codes from various SSL_client_hello_* functions, explicitly set len vars to 0 on failure

conditions to check len > 0 before memdup, explicitly set pointers to null if len == 0

I also have tested with diversity of clients including some lacking some collected extensions that prior to fixes would cause crash and other undefined behavior. These cases were all handled correctly after fixes.

ylavic · 2024-09-26T11:08:32Z

modules/ssl/ssl_engine_vars.c

+#if OPENSSL_VERSION_NUMBER >= 0x10101000L
+    char *value;
+    modssl_clienthello_vars *clienthello_vars;
+    int i;


apr_size_t i to match the type of ->extids_len

ylavic · 2024-09-26T11:14:44Z

1. Mismatch in define statements for openssl version 1.1.1.

The problem here is that libressl always defines OPENSSL_VERSION_NUMBER to 0x20000000L (i.e. 2.0 has been appropriated while openssl was still in 1.x), that's why openssl went to 3.x directly after 1.x too.
Anyway, we have the situation where OPENSSL_VERSION_NUMBER >= 0x10101000L is always true for libressl, although all the new openssl-1.x APIs have not made it to libressl..
So using OPENSSL_VERSION_NUMBER >= 0x10101000L with or without checking LIBRESSL_VERSION_NUMBER depends on whether libressl actually has the enclosed feature.
If you don't (want to) know, require !defined(LIBRESSL_VERSION_NUMBER) and someone who care about libressl will either remove it or change to the exact LIBRESSL_VERSION_NUMBER where implemented.

2. Reliance on calloc in initialization of sslconn to test for allocation and population of clienthello_vars struct.

Looks fine here for the "sslconn->clienthello_vars" pointer itself.
Maybe zero the pointed to data here too (see new comment there).

csmutz · 2024-09-26T14:09:31Z

The problem here is that libressl always defines OPENSSL_VERSION_NUMBER to 0x20000000L (i.e. 2.0 has been appropriated while openssl was still in 1.x), that's why openssl went to 3.x directly after 1.x too.

Oh, that's ugly.

Since the clienthello callback was already wrapped in the !libressl check, I'll replicate this same check across all the other places. As it is, this won't compile for libressl.

csmutz · 2024-09-27T02:43:26Z

Thanks for your patience and help.

I've made the preprocessor checks for openssl version consistent (libressl won't have this functionality) and addressed the requested items above.

notroj · 2024-09-27T09:21:54Z

modules/ssl/ssl_engine_kernel.c

+    if (clienthello_vars->ciphers_len > 0) {
+        clienthello_vars->ciphers_data = apr_pmemdup(c->pool, data, clienthello_vars->ciphers_len);
+    } else {
+        clienthello_vars->ciphers_data = NULL;


Since you're now using apr_pcalloc the explicit setting of NULL / 0 through here is not necessary.

Done. I've removed the explicit/redundant assignments of _len to 0 and _data to NULL which should be fine as long as failed calls to the openssl client hello functions never modify _len. I've left the checks for _len >0 before memdup.

I've tested on conditions that triggered bugs due to uninitialized or 0 _len before by testing connections where some of the collected client hello extensions is not present and these cases functioned correctly.

ylavic

+1, thanks for this PR!

notroj · 2024-10-01T16:07:12Z

Seconded, thanks again.

csmutz added 12 commits September 21, 2024 23:42

Update mod_ssl.xml

80df5a6

Update mod_ssl.xml

60fcb59

Update mod_ssl.xml

c026f19

Update mod_ssl.xml

e1613e8

Merge branch 'apache:trunk' into modssl-clienthello-vars

0919ba8

Update mod_ssl.xml

4ae3e26

Merge branch 'apache:trunk' into modssl-clienthello-vars

eba1aa3

Update ssl_engine_kernel.c

ae23a1c

Update ssl_engine_vars.c

99e9583

Update ssl_private.h

443a20b

Update mod_ssl.c

b8032ad

Update ssl_engine_config.c

0c927f3

notroj suggested changes Sep 25, 2024

View reviewed changes

ylavic reviewed Sep 25, 2024

View reviewed changes

csmutz added 5 commits September 25, 2024 21:49

Merge branch 'apache:trunk' into modssl-clienthello-vars

0851666

Update ssl_private.h

dfb0365

Update ssl_engine_vars.c

5ac67aa

Update ssl_engine_kernel.c

c39d16a

Update ssl_engine_vars.c

90af9c6

ylavic reviewed Sep 26, 2024

View reviewed changes

csmutz added 4 commits September 26, 2024 20:09

Update ssl_engine_kernel.c

4bf820c

Update ssl_engine_vars.c

3b23641

Update ssl_private.h

7f40fd6

Update ssl_engine_kernel.c

a7d1316

notroj reviewed Sep 27, 2024

View reviewed changes

remove redundant assignments to NULL/0

8b87913

ylavic approved these changes Sep 27, 2024

View reviewed changes

asfgit closed this in e9915b2 Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mod_ssl: ClientHello variable collection #483

mod_ssl: ClientHello variable collection #483

csmutz commented Sep 25, 2024

notroj commented Sep 25, 2024

notroj left a comment

notroj Sep 25, 2024

csmutz Sep 26, 2024 •

edited

Loading

notroj Sep 25, 2024

csmutz Sep 26, 2024 •

edited

Loading

notroj Sep 25, 2024

csmutz Sep 26, 2024 •

edited

Loading

notroj commented Sep 25, 2024

ylavic Sep 25, 2024

csmutz Sep 26, 2024 •

edited

Loading

ylavic Sep 25, 2024

csmutz Sep 26, 2024 •

edited

Loading

ylavic Sep 25, 2024

csmutz Sep 26, 2024 •

edited

Loading

ylavic Sep 25, 2024

csmutz Sep 26, 2024 •

edited

Loading

csmutz commented Sep 25, 2024

notroj commented Sep 25, 2024

csmutz commented Sep 26, 2024 •

edited

Loading

csmutz commented Sep 26, 2024 •

edited

Loading

ylavic Sep 26, 2024

csmutz Sep 26, 2024

csmutz Sep 27, 2024

ylavic Sep 26, 2024

csmutz Sep 27, 2024

ylavic commented Sep 26, 2024

csmutz commented Sep 26, 2024

csmutz commented Sep 27, 2024

notroj Sep 27, 2024

csmutz Sep 27, 2024

ylavic left a comment

notroj commented Oct 1, 2024

mod_ssl: ClientHello variable collection #483

mod_ssl: ClientHello variable collection #483

Conversation

csmutz commented Sep 25, 2024

notroj commented Sep 25, 2024

notroj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csmutz Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csmutz Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csmutz Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

notroj commented Sep 25, 2024

Choose a reason for hiding this comment

csmutz Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csmutz Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csmutz Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csmutz Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

csmutz commented Sep 25, 2024

notroj commented Sep 25, 2024

csmutz commented Sep 26, 2024 • edited Loading

csmutz commented Sep 26, 2024 • edited Loading

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

ylavic commented Sep 26, 2024

csmutz commented Sep 26, 2024

csmutz commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ylavic left a comment

Choose a reason for hiding this comment

notroj commented Oct 1, 2024

csmutz Sep 26, 2024 •

edited

Loading

csmutz Sep 26, 2024 •

edited

Loading

csmutz Sep 26, 2024 •

edited

Loading

csmutz Sep 26, 2024 •

edited

Loading

csmutz Sep 26, 2024 •

edited

Loading

csmutz Sep 26, 2024 •

edited

Loading

csmutz Sep 26, 2024 •

edited

Loading

csmutz commented Sep 26, 2024 •

edited

Loading

csmutz commented Sep 26, 2024 •

edited

Loading