Move client_hello_version from s2n_connection to s2n_client_hello #3819

lrstewart · 2023-02-10T00:37:04Z

Resolved issues:

Description of changes:

I'm moving s2n_connection.client_hello_version to s2n_client_hello.version. This will make it accessible from s2n_client_hello APIs, like the new fingerprinting APIs. It also removes the need for s2n_client_hello.sslv2.

I double-checked that we don't unexpectedly wipe s2n_client_hello anywhere, and found one location in s2n_parse_client_hello in the case of a retry. The specific test for that retry version check continued to pass after my change, but was no longer testing the right thing (it was now checking 0 vs TLS1.2 instead of SSLv2 vs TLS1.2). The check itself was deceptive / not quite right due to the behavior of SSLv2 vs other versions, so I rewrote it to be clearer and handle the wipe properly.

Testing:

Basically just a name change. Existing tests pass.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

harrisonkaiser

If I understand correctly, the SSLv2 check needs to come before we parse because of the different format format of the client hello for SSLv2.

lrstewart · 2023-02-10T17:31:38Z

If I understand correctly, the SSLv2 check needs to come before we parse because of the different format format of the client hello for SSLv2.

Yes. The SSLv2 check can/should come before we parse because of the different format / parsing of SSLv2. But we can't check for any other versions at that point.

lrstewart · 2023-02-11T01:26:18Z

tls/s2n_fingerprint.c

+    uint8_t high = (ch->version / 10);
+    uint8_t low = (ch->version % 10);
+    uint16_t version = (high << 8) | low;
+
    bool is_list = false;
-    uint16_t version = 0;
-    struct s2n_stuffer message = { 0 };
-    RESULT_GUARD_POSIX(s2n_stuffer_init_written(&message, &ch->raw_message));
-    RESULT_GUARD_POSIX(s2n_stuffer_read_uint16(&message, &version));


I'm torn on whether this is an improvement :\ Math is definitely faster, but the stuffer operations were probably more readable.

Yeah, true. The stuffers do seem more readable... Although we do this conversion in other places, so maybe it makes sense here. I could see it either way I guess.

I'm in favor of the more efficient math. But we may be able to maintain readability: If we do the conversion in other places does it make sense to turn it into a macro/function to make it more readable?

I think part of my hesitation to make this change is that it actually still wouldn't work for sslv2. In sslv2, we'd need to know that the ClientHello was an sslv2 ClientHello, but the version we'd include in the fingerprint wouldn't be sslv2. It'd be conn->client_protocol_version / the version listed in the ClientHello: basically the version we'd read from the stuffer, if s2n-tls didn't weirdly drop the first few bytes of the sslv2 ClientHello.

goatgoose · 2023-02-14T23:40:20Z

tls/s2n_fingerprint.c

+    uint8_t high = (ch->version / 10);
+    uint8_t low = (ch->version % 10);
+    uint16_t version = (high << 8) | low;
+
    bool is_list = false;
-    uint16_t version = 0;
-    struct s2n_stuffer message = { 0 };
-    RESULT_GUARD_POSIX(s2n_stuffer_init_written(&message, &ch->raw_message));
-    RESULT_GUARD_POSIX(s2n_stuffer_read_uint16(&message, &version));


Yeah, true. The stuffers do seem more readable... Although we do this conversion in other places, so maybe it makes sense here. I could see it either way I guess.

harrisonkaiser · 2023-02-15T02:56:59Z

tests/unit/s2n_fingerprint_ja3_test.c

@@ -201,7 +201,7 @@ int main(int argc, char **argv)

            struct s2n_client_hello *client_hello = s2n_connection_get_client_hello(server);
            EXPECT_NOT_NULL(client_hello);
-            EXPECT_FALSE(client_hello->sslv2);
+            EXPECT_EQUAL(client_hello->version, S2N_TLS12);


This is now a more specific check. EXPECT_NOT_EQUAL(client_hello->version, S2N_SSLv2) would be the direct translation. Just checking to make sure that is intentional.

It doesn't matter. We already expect TLS1.2 on line 212, so this test already has to do TLS1.2.

harrisonkaiser · 2023-02-15T03:19:26Z

tls/s2n_client_key_exchange.c

@@ -32,7 +32,7 @@
 #include "utils/s2n_random.h"
 #include "utils/s2n_safety.h"

-#define get_client_hello_protocol_version(conn) (conn->client_hello_version == S2N_SSLv2 ? conn->client_protocol_version : conn->client_hello_version)
+#define get_client_hello_protocol_version(conn) (conn->client_hello.version == S2N_SSLv2 ? conn->client_protocol_version : conn->client_hello.version)


I found this macro confusing. My understanding after thinking about it for a bit is as follows:

It's not possible to negotiate the SSLv2 so if the client_hello says we are using SSLv2 we need to check the client_protocol_version instead.

But I'm confused why we don't just use client_protocol_version? Is it not in sync with client_hello.version at some moment?

Perhaps this comment is relevant:

/* client_hello.version is set when parsing the record header for * SSLv2 ClientHellos, not when parsing the ClientHello itself. * Therefore, client_hello.version is meaningful ONLY for SSLv2 * before we parse the ClientHello. */

harrisonkaiser · 2023-02-15T03:22:20Z

tls/s2n_fingerprint.c

+    uint8_t high = (ch->version / 10);
+    uint8_t low = (ch->version % 10);
+    uint16_t version = (high << 8) | low;
+
    bool is_list = false;
-    uint16_t version = 0;
-    struct s2n_stuffer message = { 0 };
-    RESULT_GUARD_POSIX(s2n_stuffer_init_written(&message, &ch->raw_message));
-    RESULT_GUARD_POSIX(s2n_stuffer_read_uint16(&message, &version));


I'm in favor of the more efficient math. But we may be able to maintain readability: If we do the conversion in other places does it make sense to turn it into a macro/function to make it more readable?

harrisonkaiser · 2023-02-15T03:23:16Z

tls/s2n_fingerprint.c

@@ -235,7 +236,7 @@ static S2N_RESULT s2n_fingerprint_ja3(struct s2n_client_hello *ch,
        struct s2n_stuffer *output, uint32_t *output_size, struct s2n_hash_state *hash)
 {
    RESULT_ENSURE_REF(ch);
-    RESULT_ENSURE(!ch->sslv2, S2N_ERR_PROTOCOL_VERSION_UNSUPPORTED);
+    RESULT_ENSURE(ch->version > S2N_SSLv2, S2N_ERR_PROTOCOL_VERSION_UNSUPPORTED);


I assume we are intentionally checking more here (i.e. ch->version isn't an unknown protocol version).

A client hello without a version would be invalid and we wouldn't know how to parse it, so yes we are intentionally checking more. We have more to validate than just a flag now.

github-actions · 2023-04-25T01:55:02Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions bot added the s2n-core team label Feb 10, 2023

lrstewart changed the title ~~Ja3 ch~~ Move client_hello_version from s2n_connection to s2n_client_hello Feb 10, 2023

lrstewart mentioned this pull request Feb 10, 2023

Add JA3 fingerprinting #3817

Merged

lrstewart marked this pull request as ready for review February 10, 2023 01:26

lrstewart requested review from goatgoose and harrisonkaiser February 10, 2023 01:26

harrisonkaiser approved these changes Feb 10, 2023

View reviewed changes

lrstewart added 3 commits February 10, 2023 17:15

Move client_hello_version from s2n_connection to s2n_client_hello

bd7b225

Try to make sslv2 checking more accurate / clear

862829a

Update s2n_fingerprint.c

5ee0677

lrstewart force-pushed the ja3_ch branch from 65c0f04 to 5ee0677 Compare February 11, 2023 01:25

lrstewart commented Feb 11, 2023

View reviewed changes

goatgoose approved these changes Feb 14, 2023

View reviewed changes

harrisonkaiser reviewed Feb 15, 2023

View reviewed changes

harrisonkaiser approved these changes Feb 15, 2023

View reviewed changes

Merge branch 'main' into ja3_ch

67ee1a5

github-actions bot added the status/stale label Apr 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move client_hello_version from s2n_connection to s2n_client_hello #3819

Move client_hello_version from s2n_connection to s2n_client_hello #3819

lrstewart commented Feb 10, 2023

harrisonkaiser left a comment

lrstewart commented Feb 10, 2023

lrstewart Feb 11, 2023

goatgoose Feb 14, 2023

harrisonkaiser Feb 15, 2023

lrstewart Feb 23, 2023 •

edited

Loading

goatgoose Feb 14, 2023

harrisonkaiser Feb 15, 2023

lrstewart Feb 15, 2023

harrisonkaiser Feb 15, 2023

harrisonkaiser Feb 15, 2023

harrisonkaiser Feb 15, 2023

lrstewart Feb 15, 2023

github-actions bot commented Apr 25, 2023

Move client_hello_version from s2n_connection to s2n_client_hello #3819

Are you sure you want to change the base?

Move client_hello_version from s2n_connection to s2n_client_hello #3819

Conversation

lrstewart commented Feb 10, 2023

Resolved issues:

Description of changes:

Testing:

harrisonkaiser left a comment

Choose a reason for hiding this comment

lrstewart commented Feb 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrstewart Feb 23, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 25, 2023

lrstewart Feb 23, 2023 •

edited

Loading