From f3b8dece70cadeb988cb3dfced07f5d2145705ef Mon Sep 17 00:00:00 2001 From: samuel40791765 Date: Thu, 7 Nov 2024 01:40:50 +0000 Subject: [PATCH] Relax check instead of version bump --- ssl/ssl_test.cc | 22 +++++++++++++--------- ssl/ssl_transfer_asn1.cc | 33 +++++++++------------------------ 2 files changed, 22 insertions(+), 33 deletions(-) diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index bcf0cc4641..70d958dc19 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -7844,7 +7844,7 @@ struct EncodeDecodeKATTestParam { }; static const EncodeDecodeKATTestParam kEncodeDecodeKATs[] = { - // V1 input round-trips as V3 output + // V1 input round-trips as V2 output {"308201173082011302010102020303020240003081fa0201010408000000000000000104" "0800000000000000010420000004d29e62f41ded4bb33d0faa6ffada380e2c489dfbfb44" "4f574e475244010420cf3926d1ec5a562a642935a8050222b0aed93ffd9d1cac682274d9" @@ -7853,7 +7853,7 @@ static const EncodeDecodeKATTestParam kEncodeDecodeKATs[] = { "a80400043085668dcf9f0921094ebd7f91bf2a8c60d276e4c279fd85a989402f67868232" "4fd8098dc19d900b856d0a77e048e3ced2a104020204d2a20402021c20a4020400b10301" "01ffb20302011da206040474657374a7030101ff020108020100a0030101ff", - "308201803082017c02010102020303020240003082016202010304080000000000000001" + "308201803082017c02010102020303020240003082016202010204080000000000000001" "040800000000000000010420000004d29e62f41ded4bb33d0faa6ffada380e2c489dfbfb" "444f574e475244010420cf3926d1ec5a562a642935a8050222b0aed93ffd9d1cac682274" "d942e99e42a6040200000201000201030440b9b409f5129440622f87f84402010c040c1f" @@ -7867,7 +7867,9 @@ static const EncodeDecodeKATTestParam kEncodeDecodeKATs[] = { // In runner.go, the test case "Basic-Server-TLS-Sync-SSL_Transfer" is used // to generate below bytes by adding print statement on the output of // |SSL_to_bytes| in bssl_shim.cc. - // V2 input round-trips as V3 output. + // We've bumped the buffer size in the |previous_client/server_finished| + // fields. This verifies that the original size is parsable and reencoded + // with the new size. {"308201173082011302010102020303020240003081fa0201020408000000000000000104" "0800000000000000010420000004d29e62f41ded4bb33d0faa6ffada380e2c489dfbfb44" "4f574e475244010420cf3926d1ec5a562a642935a8050222b0aed93ffd9d1cac682274d9" @@ -7876,7 +7878,7 @@ static const EncodeDecodeKATTestParam kEncodeDecodeKATs[] = { "a80400043085668dcf9f0921094ebd7f91bf2a8c60d276e4c279fd85a989402f67868232" "4fd8098dc19d900b856d0a77e048e3ced2a104020204d2a20402021c20a4020400b10301" "01ffb20302011da206040474657374a7030101ff020108020100a0030101ff", - "308201803082017c02010102020303020240003082016202010304080000000000000001" + "308201803082017c02010102020303020240003082016202010204080000000000000001" "040800000000000000010420000004d29e62f41ded4bb33d0faa6ffada380e2c489dfbfb" "444f574e475244010420cf3926d1ec5a562a642935a8050222b0aed93ffd9d1cac682274" "d942e99e42a6040200000201000201030440b9b409f5129440622f87f84402010c040c1f" @@ -7891,8 +7893,10 @@ static const EncodeDecodeKATTestParam kEncodeDecodeKATs[] = { // "TLS-TLS13-AES_128_GCM_SHA256-server-SSL_Transfer" is used to generate // below bytes by adding print statement on the output of |SSL_to_bytes| in // bssl_shim.cc. - // V2 input round-trips as V3 output. - {"308203883082038402010102020304020240003082036a020102040800000000000000000" + // We've bumped the buffer size in the |previous_client/server_finished| + // fields. This verifies that the original size is parsable and reencoded + // with the new size. + {"308203883082038402010102020304020240003082036a020102040800000000000000000" "408000000000000000004206beca5c14aff6b92757545948b883c6c175327814bedcf38a6" "b2e4c43bc02d180420a32aee5b7705a19e4bb2b47f4918199c76cee7245f1311bc4ba3888" "3d33f236a04020000020100020101040c000000000000000000000000020100040c000000" @@ -7917,7 +7921,7 @@ static const EncodeDecodeKATTestParam kEncodeDecodeKATs[] = { "41be58ecbd60d18128dfa28f4b1a00042a2a0000ba2330210201010204030013013016020" "101020117040e300c0201010201000201000101ffbb233021020101020403001301301602" "0101020117040e300c0201010201000201000101ff020108020100a0030101ff", - "308203f0308203ec0201010202030402024000308203d202010304080000000000000000" + "308203f0308203ec0201010202030402024000308203d202010204080000000000000000" "0408000000000000000004206beca5c14aff6b92757545948b883c6c175327814bedcf38" "a6b2e4c43bc02d180420a32aee5b7705a19e4bb2b47f4918199c76cee7245f1311bc4ba3" "8883d33f236a040200000201000201010440000000000000000000000000020100040c00" @@ -7950,7 +7954,7 @@ static const EncodeDecodeKATTestParam kEncodeDecodeKATs[] = { // "TLS-ECH-Server-Cipher-HKDF-SHA256-AES-256-GCM-SSL_Transfer" is used // to generate below bytes by adding print statement on the output of // |SSL_to_bytes| in bssl_shim.cc. - {"308203e3308203df0201010202030402024000308203c502010304080000000000000000" + {"308203e3308203df0201010202030402024000308203c502010204080000000000000000" "04080000000000000000042028431b914ffdb44ea92ca53d5734976c6a16f141d44f180b" "0816a5cb2b8e79030420bdaf544fa82d833d58c92213e44e850cc0b8147699b0b410d4aa" "2a277030f3220402000002010002010104409e155007d04cd03cf4d8a95ce244dc978a87" @@ -8013,7 +8017,7 @@ TEST_P(EncodeDecodeKATTest, RoundTrips) { encoded_ptr.reset(encoded); // Check the encoded bytes are the same as the test input. ASSERT_EQ(output_bytes.size(), encoded_len); - ASSERT_EQ(memcmp(output_bytes.data(), encoded, encoded_len), 0); + ASSERT_EQ(OPENSSL_memcmp(output_bytes.data(), encoded, encoded_len), 0); } TEST(SSLTest, ZeroSizedWriteFlushesHandshakeMessages) { diff --git a/ssl/ssl_transfer_asn1.cc b/ssl/ssl_transfer_asn1.cc index e2f1640741..c8be3b9c9a 100644 --- a/ssl/ssl_transfer_asn1.cc +++ b/ssl/ssl_transfer_asn1.cc @@ -115,8 +115,7 @@ static bool SSL3_STATE_get_optional_octet_string(CBS *cbs, void *dst, enum SSL3_STATE_SERDE_VERSION { SSL3_STATE_SERDE_VERSION_ONE = 1, - SSL3_STATE_SERDE_VERSION_TWO = 2, - SSL3_STATE_SERDE_VERSION_THREE = 3 + SSL3_STATE_SERDE_VERSION_TWO = 2 }; static const unsigned kS3EstablishedSessionTag = @@ -192,7 +191,7 @@ static int SSL3_STATE_to_bytes(SSL3_STATE *in, uint16_t protocol_version, CBB s3, child, child2; if (!CBB_add_asn1(cbb, &s3, CBS_ASN1_SEQUENCE) || - !CBB_add_asn1_uint64(&s3, SSL3_STATE_SERDE_VERSION_THREE) || + !CBB_add_asn1_uint64(&s3, SSL3_STATE_SERDE_VERSION_TWO) || !CBB_add_asn1_octet_string(&s3, in->read_sequence, TLS_SEQ_NUM_SIZE) || !CBB_add_asn1_octet_string(&s3, in->write_sequence, TLS_SEQ_NUM_SIZE) || !CBB_add_asn1_octet_string(&s3, in->server_random, SSL3_RANDOM_SIZE) || @@ -445,8 +444,6 @@ static int SSL3_STATE_parse_session(CBS *cbs, UniquePtr *out, } } -#define PRE_V3_PREV_FINISHED_MAX_SIZE 12 - // SSL3_STATE_from_bytes recovers SSL3_STATE from |cbs|. // |ssl| is used because |tls1_configure_aead| is used to recover // |aead_read_ctx| and |aead_write_ctx|. @@ -472,15 +469,9 @@ static int SSL3_STATE_from_bytes(SSL *ssl, CBS *cbs, const SSL_CTX *ctx) { int pending_app_data_present, read_buffer_present; if (!CBS_get_asn1(cbs, &s3, CBS_ASN1_SEQUENCE) || !CBS_get_asn1_uint64(&s3, &serde_version) || - serde_version > SSL3_STATE_SERDE_VERSION_THREE || - (is_tls13 && serde_version < SSL3_STATE_SERDE_VERSION_TWO)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_SERIALIZATION_INVALID_SSL3_STATE); - return 0; - } - - bool is_pre_v3 = (serde_version < SSL3_STATE_SERDE_VERSION_THREE); - - if (!CBS_get_asn1(&s3, &read_seq, CBS_ASN1_OCTETSTRING) || + serde_version > SSL3_STATE_SERDE_VERSION_TWO || + (is_tls13 && serde_version < SSL3_STATE_SERDE_VERSION_TWO) || + !CBS_get_asn1(&s3, &read_seq, CBS_ASN1_OCTETSTRING) || CBS_len(&read_seq) != TLS_SEQ_NUM_SIZE || !CBS_get_asn1(&s3, &write_seq, CBS_ASN1_OCTETSTRING) || CBS_len(&write_seq) != TLS_SEQ_NUM_SIZE || @@ -494,17 +485,11 @@ static int SSL3_STATE_from_bytes(SSL *ssl, CBS *cbs, const SSL_CTX *ctx) { !CBS_get_asn1_uint64(&s3, &early_data_reason) || early_data_reason > ssl_early_data_reason_max_value || !CBS_get_asn1(&s3, &previous_client_finished, CBS_ASN1_OCTETSTRING) || - (is_pre_v3 && - CBS_len(&previous_client_finished) != PRE_V3_PREV_FINISHED_MAX_SIZE) || - (!is_pre_v3 && - CBS_len(&previous_client_finished) != PREV_FINISHED_MAX_SIZE) || + CBS_len(&previous_client_finished) > PREV_FINISHED_MAX_SIZE || !CBS_get_asn1_uint64(&s3, &previous_client_finished_len) || previous_client_finished_len > PREV_FINISHED_MAX_SIZE || !CBS_get_asn1(&s3, &previous_server_finished, CBS_ASN1_OCTETSTRING) || - (is_pre_v3 && - CBS_len(&previous_server_finished) != PRE_V3_PREV_FINISHED_MAX_SIZE) || - (!is_pre_v3 && - CBS_len(&previous_server_finished) != PREV_FINISHED_MAX_SIZE) || + CBS_len(&previous_server_finished) > PREV_FINISHED_MAX_SIZE || !CBS_get_asn1_uint64(&s3, &previous_server_finished_len) || previous_server_finished_len > PREV_FINISHED_MAX_SIZE || !CBS_get_asn1_uint64(&s3, &empty_record_count) || @@ -536,11 +521,11 @@ static int SSL3_STATE_from_bytes(SSL *ssl, CBS *cbs, const SSL_CTX *ctx) { return 0; } - bool is_post_v2 = (serde_version >= SSL3_STATE_SERDE_VERSION_TWO); + bool is_v2 = (serde_version == SSL3_STATE_SERDE_VERSION_TWO); // We should have no more data at this point if we are deserializing v1 // encoding. - if (!is_post_v2 && CBS_len(&s3) > 0) { + if (!is_v2 && CBS_len(&s3) > 0) { OPENSSL_PUT_ERROR(SSL, SSL_R_SERIALIZATION_INVALID_SSL3_STATE); return 0; }