Skip to content

Commit 9bb58d4

Browse files
lrstewartgoatgoose
authored and
dougch
committed
refactor: remove conn->client_hello_version (aws#5278)
Co-authored-by: Sam Clark <[email protected]>
1 parent e154010 commit 9bb58d4

13 files changed

+88
-78
lines changed

‎tests/unit/s2n_client_hello_recv_test.c

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,8 @@ int main(int argc, char **argv)
8383
EXPECT_SUCCESS(s2n_connection_set_config(server_conn, tls13_config));
8484

8585
/* Record version and protocol version are in the header for SSLv2 */
86-
server_conn->client_hello_version = S2N_SSLv2;
87-
server_conn->client_protocol_version = S2N_TLS12;
86+
server_conn->client_hello.sslv2 = true;
87+
server_conn->client_hello.legacy_version = S2N_TLS12;
8888

8989
uint8_t sslv2_client_hello[] = {
9090
SSLv2_CLIENT_HELLO_PREFIX,
@@ -103,8 +103,10 @@ int main(int argc, char **argv)
103103

104104
EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS12);
105105
EXPECT_EQUAL(server_conn->client_protocol_version, S2N_TLS12);
106-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_SSLv2);
106+
EXPECT_EQUAL(server_conn->client_hello.sslv2, true);
107+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS12);
107108
EXPECT_EQUAL(server_conn->client_hello.callback_invoked, 1);
109+
EXPECT_EQUAL(s2n_connection_get_client_hello_version(server_conn), S2N_SSLv2);
108110

109111
s2n_connection_free(server_conn);
110112

@@ -122,7 +124,7 @@ int main(int argc, char **argv)
122124
EXPECT_SUCCESS(s2n_client_hello_send(client_conn));
123125
EXPECT_EQUAL(client_conn->actual_protocol_version, S2N_TLS12);
124126
EXPECT_EQUAL(client_conn->client_protocol_version, S2N_TLS12);
125-
EXPECT_EQUAL(client_conn->client_hello_version, S2N_TLS12);
127+
EXPECT_EQUAL(client_conn->client_hello.legacy_version, S2N_TLS12);
126128

127129
EXPECT_SUCCESS(s2n_stuffer_write(&server_conn->handshake.io, &client_conn->handshake.io.blob));
128130
EXPECT_SUCCESS(s2n_client_hello_recv(server_conn));
@@ -147,15 +149,15 @@ int main(int argc, char **argv)
147149
EXPECT_SUCCESS(s2n_client_hello_send(client_conn));
148150
EXPECT_EQUAL(client_conn->actual_protocol_version, S2N_TLS12);
149151
EXPECT_EQUAL(client_conn->client_protocol_version, S2N_TLS12);
150-
EXPECT_EQUAL(client_conn->client_hello_version, S2N_TLS12);
152+
EXPECT_EQUAL(client_conn->client_hello.legacy_version, S2N_TLS12);
151153

152154
EXPECT_SUCCESS(s2n_stuffer_write(&server_conn->handshake.io, &client_conn->handshake.io.blob));
153155
EXPECT_SUCCESS(s2n_client_hello_recv(server_conn));
154156

155157
EXPECT_EQUAL(server_conn->server_protocol_version, S2N_TLS12);
156158
EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS12);
157159
EXPECT_EQUAL(server_conn->client_protocol_version, S2N_TLS12);
158-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_TLS12);
160+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS12);
159161

160162
s2n_connection_free(server_conn);
161163
s2n_connection_free(client_conn);
@@ -177,15 +179,15 @@ int main(int argc, char **argv)
177179
client_conn->client_protocol_version = S2N_TLS11;
178180

179181
EXPECT_SUCCESS(s2n_client_hello_send(client_conn));
180-
EXPECT_EQUAL(client_conn->client_hello_version, S2N_TLS11);
182+
EXPECT_EQUAL(client_conn->client_hello.legacy_version, S2N_TLS11);
181183

182184
EXPECT_SUCCESS(s2n_stuffer_write(&server_conn->handshake.io, &client_conn->handshake.io.blob));
183185
EXPECT_SUCCESS(s2n_client_hello_recv(server_conn));
184186

185187
EXPECT_EQUAL(server_conn->server_protocol_version, S2N_TLS12);
186188
EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS11);
187189
EXPECT_EQUAL(server_conn->client_protocol_version, S2N_TLS11);
188-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_TLS11);
190+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS11);
189191

190192
s2n_connection_free(server_conn);
191193
s2n_connection_free(client_conn);
@@ -205,15 +207,15 @@ int main(int argc, char **argv)
205207

206208
EXPECT_SUCCESS(s2n_client_hello_send(client_conn));
207209
EXPECT_EQUAL(client_conn->client_protocol_version, S2N_TLS12);
208-
EXPECT_EQUAL(client_conn->client_hello_version, S2N_TLS12);
210+
EXPECT_EQUAL(client_conn->client_hello.legacy_version, S2N_TLS12);
209211

210212
EXPECT_SUCCESS(s2n_stuffer_write(&server_conn->handshake.io, &client_conn->handshake.io.blob));
211213
EXPECT_SUCCESS(s2n_client_hello_recv(server_conn));
212214

213215
EXPECT_EQUAL(server_conn->server_protocol_version, S2N_TLS13);
214216
EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS12);
215217
EXPECT_EQUAL(server_conn->client_protocol_version, S2N_TLS12);
216-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_TLS12);
218+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS12);
217219

218220
s2n_connection_free(server_conn);
219221
s2n_connection_free(client_conn);
@@ -247,12 +249,12 @@ int main(int argc, char **argv)
247249

248250
EXPECT_EQUAL(client_conn->actual_protocol_version, s2n_get_highest_fully_supported_tls_version());
249251
EXPECT_EQUAL(client_conn->client_protocol_version, s2n_get_highest_fully_supported_tls_version());
250-
EXPECT_EQUAL(client_conn->client_hello_version, S2N_TLS12);
252+
EXPECT_EQUAL(client_conn->client_hello.legacy_version, S2N_TLS12);
251253

252254
EXPECT_EQUAL(server_conn->server_protocol_version, S2N_TLS13);
253255
EXPECT_EQUAL(server_conn->actual_protocol_version, s2n_get_highest_fully_supported_tls_version());
254256
EXPECT_EQUAL(server_conn->client_protocol_version, s2n_get_highest_fully_supported_tls_version());
255-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_TLS12);
257+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS13);
256258

257259
s2n_connection_free(server_conn);
258260
s2n_connection_free(client_conn);
@@ -279,7 +281,7 @@ int main(int argc, char **argv)
279281
EXPECT_EQUAL(server_conn->server_protocol_version, S2N_TLS13);
280282
EXPECT_EQUAL(server_conn->actual_protocol_version, s2n_get_highest_fully_supported_tls_version());
281283
EXPECT_EQUAL(server_conn->client_protocol_version, s2n_get_highest_fully_supported_tls_version());
282-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_TLS12);
284+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS12);
283285

284286
s2n_connection_free(server_conn);
285287
s2n_connection_free(client_conn);
@@ -313,7 +315,7 @@ int main(int argc, char **argv)
313315
EXPECT_EQUAL(server_conn->server_protocol_version, s2n_get_highest_fully_supported_tls_version());
314316
EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS12);
315317
EXPECT_EQUAL(server_conn->client_protocol_version, S2N_TLS12);
316-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_TLS12);
318+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS13);
317319

318320
s2n_connection_free(server_conn);
319321
s2n_connection_free(client_conn);
@@ -352,8 +354,8 @@ int main(int argc, char **argv)
352354
EXPECT_SUCCESS(s2n_connection_set_config(server_conn, tls12_config));
353355

354356
/* Record version and protocol version are in the header for SSLv2 */
355-
server_conn->client_hello_version = S2N_SSLv2;
356-
server_conn->client_protocol_version = S2N_TLS12;
357+
server_conn->client_hello.sslv2 = true;
358+
server_conn->client_hello.legacy_version = S2N_TLS12;
357359

358360
/* Writing a sslv2 client hello with a length 0 cipher suite list */
359361
uint8_t sslv2_client_hello[] = {
@@ -501,7 +503,7 @@ int main(int argc, char **argv)
501503
EXPECT_EQUAL(server_conn->server_protocol_version, S2N_TLS13);
502504
EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS12);
503505
EXPECT_EQUAL(server_conn->client_protocol_version, S2N_TLS12);
504-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_TLS12);
506+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS12);
505507

506508
s2n_connection_free(server_conn);
507509
EXPECT_SUCCESS(s2n_disable_tls13_in_test());
@@ -542,8 +544,9 @@ int main(int argc, char **argv)
542544
/* Successfully read the full message */
543545
EXPECT_OK(s2n_negotiate_until_message(server, &blocked, SERVER_HELLO));
544546
EXPECT_EQUAL(server->client_protocol_version, S2N_TLS12);
545-
EXPECT_EQUAL(server->client_hello_version, S2N_SSLv2);
547+
EXPECT_EQUAL(server->client_hello.legacy_version, S2N_TLS12);
546548
EXPECT_TRUE(server->client_hello.sslv2);
549+
EXPECT_EQUAL(s2n_connection_get_client_hello_version(server), S2N_SSLv2);
547550
};
548551

549552
s2n_config_free(tls12_config);

‎tests/unit/s2n_client_hello_test.c

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -749,7 +749,7 @@ int main(int argc, char **argv)
749749
EXPECT_SUCCESS(s2n_client_hello_send(conn));
750750
EXPECT_EQUAL(conn->actual_protocol_version, s2n_get_highest_fully_supported_tls_version());
751751
EXPECT_EQUAL(conn->client_protocol_version, s2n_get_highest_fully_supported_tls_version());
752-
EXPECT_EQUAL(conn->client_hello_version, S2N_TLS12);
752+
EXPECT_EQUAL(conn->client_hello.legacy_version, S2N_TLS12);
753753

754754
EXPECT_SUCCESS(s2n_connection_free(conn));
755755
};
@@ -768,7 +768,7 @@ int main(int argc, char **argv)
768768
EXPECT_SUCCESS(s2n_client_hello_send(conn));
769769
EXPECT_EQUAL(conn->actual_protocol_version, S2N_TLS12);
770770
EXPECT_EQUAL(conn->client_protocol_version, S2N_TLS12);
771-
EXPECT_EQUAL(conn->client_hello_version, S2N_TLS12);
771+
EXPECT_EQUAL(conn->client_hello.legacy_version, S2N_TLS12);
772772

773773
EXPECT_SUCCESS(s2n_connection_free(conn));
774774
};
@@ -789,7 +789,7 @@ int main(int argc, char **argv)
789789
EXPECT_SUCCESS(s2n_client_hello_send(client_conn));
790790
EXPECT_EQUAL(client_conn->actual_protocol_version, s2n_get_highest_fully_supported_tls_version());
791791
EXPECT_EQUAL(client_conn->client_protocol_version, s2n_get_highest_fully_supported_tls_version());
792-
EXPECT_EQUAL(client_conn->client_hello_version, S2N_TLS12);
792+
EXPECT_EQUAL(client_conn->client_hello.legacy_version, S2N_TLS12);
793793

794794
/* Server configured with TLS 1.2 negotiates TLS12 version */
795795
EXPECT_NOT_NULL(server_conn = s2n_connection_new(S2N_SERVER));
@@ -809,7 +809,7 @@ int main(int argc, char **argv)
809809
EXPECT_EQUAL(server_conn->server_protocol_version, S2N_TLS12);
810810
EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS12);
811811
EXPECT_EQUAL(server_conn->client_protocol_version, S2N_TLS12);
812-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_TLS12);
812+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS12);
813813

814814
EXPECT_SUCCESS(s2n_connection_free(server_conn));
815815
EXPECT_SUCCESS(s2n_connection_free(client_conn));
@@ -1538,12 +1538,12 @@ int main(int argc, char **argv)
15381538
EXPECT_OK(s2n_handshake_type_set_tls13_flag(server_conn, HELLO_RETRY_REQUEST));
15391539

15401540
/* Second client hello has version SSLv2 */
1541-
server_conn->client_hello_version = S2N_SSLv2;
1541+
server_conn->client_hello.sslv2 = true;
15421542

15431543
/* Mock having some data in the client hello */
15441544
EXPECT_SUCCESS(s2n_stuffer_write_uint16(&server_conn->handshake.io, 100));
15451545

1546-
EXPECT_FAILURE_WITH_ERRNO(s2n_parse_client_hello(server_conn), S2N_ERR_SAFETY);
1546+
EXPECT_FAILURE_WITH_ERRNO(s2n_parse_client_hello(server_conn), S2N_ERR_BAD_MESSAGE);
15471547
};
15481548

15491549
/* Test s2n_client_hello_parse_message

‎tests/unit/s2n_client_supported_versions_extension_test.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,7 @@ int main(int argc, char **argv)
440440
/* The server does not use the protocol version in the Client Hello to set the actual protocol version. */
441441
EXPECT_EQUAL(server_conn->actual_protocol_version, S2N_TLS13);
442442
EXPECT_EQUAL(server_conn->client_protocol_version, S2N_TLS13);
443-
EXPECT_EQUAL(server_conn->client_hello_version, S2N_TLS10);
443+
EXPECT_EQUAL(server_conn->client_hello.legacy_version, S2N_TLS10);
444444
}
445445

446446
/* Ensure that TLS 1.3 is enforced in the supported versions extension for ClientHellos sent in

‎tests/unit/s2n_config_test.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1259,8 +1259,8 @@ int main(int argc, char **argv)
12591259
server_conn->config = NULL;
12601260

12611261
/* Record version and protocol version are in the header for SSLv2 */
1262-
server_conn->client_hello_version = S2N_SSLv2;
1263-
server_conn->client_protocol_version = S2N_TLS12;
1262+
server_conn->client_hello.sslv2 = true;
1263+
server_conn->client_hello.legacy_version = S2N_TLS12;
12641264

12651265
/* S2N_ERR_CONFIG_NULL_BEFORE_CH_CALLBACK is only called in one location, just before the
12661266
* client hello callback. Therefore, we can assert that if we hit this error, we

‎tests/unit/s2n_connection_protocol_versions_test.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ static int s2n_overwrite_client_hello_cb(struct s2n_connection *conn, void *ctx)
7676
* with a different version.
7777
*/
7878
if (context->client_hello_version) {
79-
conn->client_hello_version = context->client_hello_version;
79+
conn->client_hello.legacy_version = context->client_hello_version;
8080
conn->client_protocol_version = context->client_hello_version;
8181
}
8282

‎tests/unit/s2n_fingerprint_ja3_test.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -253,8 +253,8 @@ int main(int argc, char **argv)
253253
* not when processing the actual ClientHello message.
254254
* So we need to set the versions manually.
255255
*/
256-
server->client_hello_version = S2N_SSLv2;
257-
server->client_protocol_version = S2N_TLS12;
256+
server->client_hello.sslv2 = true;
257+
server->client_hello.legacy_version = S2N_TLS12;
258258

259259
uint8_t sslv2_client_hello[] = {
260260
SSLv2_CLIENT_HELLO_PREFIX,

‎tls/s2n_client_hello.c

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,6 @@ static S2N_RESULT s2n_client_hello_verify_for_retry(struct s2n_connection *conn,
354354
}
355355

356356
S2N_RESULT s2n_client_hello_parse_raw(struct s2n_client_hello *client_hello,
357-
uint8_t client_protocol_version[S2N_TLS_PROTOCOL_VERSION_LEN],
358357
uint8_t client_random[S2N_TLS_RANDOM_DATA_LEN])
359358
{
360359
RESULT_ENSURE_REF(client_hello);
@@ -383,6 +382,7 @@ S2N_RESULT s2n_client_hello_parse_raw(struct s2n_client_hello *client_hello,
383382
**/
384383

385384
/* legacy_version */
385+
uint8_t client_protocol_version[S2N_TLS_PROTOCOL_VERSION_LEN] = { 0 };
386386
RESULT_GUARD_POSIX(s2n_stuffer_read_bytes(in, client_protocol_version, S2N_TLS_PROTOCOL_VERSION_LEN));
387387

388388
/* Encode the version as a 1 byte representation of the two protocol version bytes, with the
@@ -428,6 +428,11 @@ int s2n_parse_client_hello(struct s2n_connection *conn)
428428
{
429429
POSIX_ENSURE_REF(conn);
430430

431+
/* SSLv2 ClientHellos are not allowed during a HelloRetryRequest */
432+
if (s2n_is_hello_retry_handshake(conn)) {
433+
POSIX_ENSURE(!conn->client_hello.sslv2, S2N_ERR_BAD_MESSAGE);
434+
}
435+
431436
/* If a retry, move the old version of the client hello
432437
* somewhere safe so we can compare it to the new client hello later.
433438
*/
@@ -439,12 +444,7 @@ int s2n_parse_client_hello(struct s2n_connection *conn)
439444

440445
POSIX_GUARD(s2n_collect_client_hello(&conn->client_hello, &conn->handshake.io));
441446

442-
/* The ClientHello version must be TLS12 after a HelloRetryRequest */
443-
if (s2n_is_hello_retry_handshake(conn)) {
444-
POSIX_ENSURE_EQ(conn->client_hello_version, S2N_TLS12);
445-
}
446-
447-
if (conn->client_hello_version == S2N_SSLv2) {
447+
if (conn->client_hello.sslv2) {
448448
POSIX_GUARD(s2n_sslv2_client_hello_parse(conn));
449449
return S2N_SUCCESS;
450450
}
@@ -455,16 +455,14 @@ int s2n_parse_client_hello(struct s2n_connection *conn)
455455
S2N_TLS_RANDOM_DATA_LEN);
456456

457457
/* Parse raw, collected client hello */
458-
uint8_t client_protocol_version[S2N_TLS_PROTOCOL_VERSION_LEN] = { 0 };
459458
POSIX_GUARD_RESULT(s2n_client_hello_parse_raw(&conn->client_hello,
460-
client_protocol_version, conn->handshake_params.client_random));
459+
conn->handshake_params.client_random));
461460

462461
/* Protocol version in the ClientHello is fixed at 0x0303(TLS 1.2) for
463462
* future versions of TLS. Therefore, we will negotiate down if a client sends
464463
* an unexpected value above 0x0303.
465464
*/
466-
conn->client_protocol_version = MIN((client_protocol_version[0] * 10) + client_protocol_version[1], S2N_TLS12);
467-
conn->client_hello_version = conn->client_protocol_version;
465+
conn->client_protocol_version = MIN(conn->client_hello.legacy_version, S2N_TLS12);
468466

469467
/* Copy the session id to the connection. */
470468
conn->session_id_len = conn->client_hello.session_id.size;
@@ -502,9 +500,8 @@ static S2N_RESULT s2n_client_hello_parse_message_impl(struct s2n_client_hello **
502500
RESULT_GUARD_POSIX(s2n_collect_client_hello(client_hello, &in));
503501
RESULT_ENSURE(s2n_stuffer_data_available(&in) == 0, S2N_ERR_BAD_MESSAGE);
504502

505-
uint8_t protocol_version[S2N_TLS_PROTOCOL_VERSION_LEN] = { 0 };
506503
uint8_t random[S2N_TLS_RANDOM_DATA_LEN] = { 0 };
507-
RESULT_GUARD(s2n_client_hello_parse_raw(client_hello, protocol_version, random));
504+
RESULT_GUARD(s2n_client_hello_parse_raw(client_hello, random));
508505

509506
*result = client_hello;
510507
ZERO_TO_DISABLE_DEFER_CLEANUP(client_hello);
@@ -593,7 +590,7 @@ int s2n_process_client_hello(struct s2n_connection *conn)
593590
POSIX_CHECKED_MEMCPY(previous_cipher_suite_iana, conn->secure->cipher_suite->iana_value, S2N_TLS_CIPHER_SUITE_LEN);
594591

595592
/* Now choose the ciphers we have certs for. */
596-
if (conn->client_hello_version == S2N_SSLv2) {
593+
if (conn->client_hello.sslv2) {
597594
POSIX_GUARD(s2n_set_cipher_as_sslv2_server(conn, client_hello->cipher_suites.data,
598595
client_hello->cipher_suites.size / S2N_SSLv2_CIPHER_SUITE_LEN));
599596
} else {
@@ -725,9 +722,9 @@ int s2n_client_hello_send(struct s2n_connection *conn)
725722
uint8_t client_protocol_version[S2N_TLS_PROTOCOL_VERSION_LEN] = { 0 };
726723

727724
uint8_t reported_protocol_version = MIN(conn->client_protocol_version, S2N_TLS12);
725+
conn->client_hello.legacy_version = reported_protocol_version;
728726
client_protocol_version[0] = reported_protocol_version / 10;
729727
client_protocol_version[1] = reported_protocol_version % 10;
730-
conn->client_hello_version = reported_protocol_version;
731728
POSIX_GUARD(s2n_stuffer_write_bytes(out, client_protocol_version, S2N_TLS_PROTOCOL_VERSION_LEN));
732729

733730
struct s2n_blob client_random = { 0 };
@@ -815,8 +812,8 @@ int s2n_client_hello_send(struct s2n_connection *conn)
815812
* Clients may send SSLv2 ClientHellos advertising higher protocol versions for
816813
* backwards compatibility reasons. See https://tools.ietf.org/rfc/rfc2246 Appendix E.
817814
*
818-
* In this case, conn->client_hello_version will be SSLv2, but conn->client_protocol_version
819-
* will likely be higher.
815+
* In this case, conn->client_hello.legacy_version and conn->client_protocol_version
816+
* will be higher than SSLv2.
820817
*
821818
* See http://www-archive.mozilla.org/projects/security/pki/nss/ssl/draft02.html Section 2.5
822819
* for a description of the expected SSLv2 format.
@@ -826,7 +823,7 @@ int s2n_client_hello_send(struct s2n_connection *conn)
826823
int s2n_sslv2_client_hello_parse(struct s2n_connection *conn)
827824
{
828825
struct s2n_client_hello *client_hello = &conn->client_hello;
829-
client_hello->sslv2 = true;
826+
conn->client_protocol_version = MIN(client_hello->legacy_version, S2N_TLS12);
830827

831828
struct s2n_stuffer in_stuffer = { 0 };
832829
POSIX_GUARD(s2n_stuffer_init(&in_stuffer, &client_hello->raw_message));

0 commit comments

Comments
 (0)