From 7001a7fce6abe7fa5fc5b9ef52d74851c0db6e24 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sun, 26 Oct 2014 00:03:08 -0400 Subject: [PATCH] Don't bother accepting key_arg when parsing SSL_SESSION. Doing some archeaology, since the initial OpenSSL commit, key_arg has been omitted from the serialization if key_arg_length was 0. Since this is an SSLv2-only field and resuming an SSLv2 session with SSLv3+ is not possible, there is no need to support parsing those sessions. Interestingly, it is actually not the case that key_arg_length was only ever set in SSLv2, historically. In the initial commit of OpenSSL, SSLeay 0.8.1b, key_arg was used to store what appears to be the IV. That was then removed in the next commit, an import of SSLeay 0.9.0b, at which point key_arg was only ever set in SSLv3. That is old enough that there is certainly no need to parse pre-SSLeay-0.9.0b sessions... Change-Id: Ia768a2d97ddbe60309be20e2efe488640c4776d9 Reviewed-on: https://boringssl-review.googlesource.com/2050 Reviewed-by: Adam Langley --- ssl/ssl_asn1.c | 7 ++----- ssl/ssl_test.c | 42 +++++++++++------------------------------- 2 files changed, 13 insertions(+), 36 deletions(-) diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c index 53dc9962..ee6eee51 100644 --- a/ssl/ssl_asn1.c +++ b/ssl/ssl_asn1.c @@ -98,8 +98,6 @@ * cipher OCTET STRING, -- two bytes long * sessionID OCTET STRING, * masterKey OCTET STRING, - * keyArg [0] IMPLICIT OCTET STRING OPTIONAL, - * -- ignored: legacy SSLv2-only field. * time [1] INTEGER OPTIONAL, -- seconds since UNIX epoch * timeout [2] INTEGER OPTIONAL, -- in seconds * peer [3] Certificate OPTIONAL, @@ -410,8 +408,8 @@ static int d2i_SSL_SESSION_get_octet_string(CBS *cbs, uint8_t **out_ptr, SSL_SESSION *d2i_SSL_SESSION(SSL_SESSION **a, const uint8_t **pp, long length) { SSL_SESSION *ret = NULL; CBS cbs, session, cipher, session_id, master_key; - CBS key_arg, peer, sid_ctx, peer_sha256, original_handshake_hash; - int has_key_arg, has_peer, has_peer_sha256, extended_master_secret; + CBS peer, sid_ctx, peer_sha256, original_handshake_hash; + int has_peer, has_peer_sha256, extended_master_secret; uint64_t version, ssl_version; uint64_t session_time, timeout, verify_result, ticket_lifetime_hint; @@ -431,7 +429,6 @@ SSL_SESSION *d2i_SSL_SESSION(SSL_SESSION **a, const uint8_t **pp, long length) { !CBS_get_asn1(&session, &cipher, CBS_ASN1_OCTETSTRING) || !CBS_get_asn1(&session, &session_id, CBS_ASN1_OCTETSTRING) || !CBS_get_asn1(&session, &master_key, CBS_ASN1_OCTETSTRING) || - !CBS_get_optional_asn1(&session, &key_arg, &has_key_arg, kKeyArgTag) || !CBS_get_optional_asn1_uint64(&session, &session_time, kTimeTag, time(NULL)) || !CBS_get_optional_asn1_uint64(&session, &timeout, kTimeoutTag, 3) || diff --git a/ssl/ssl_test.c b/ssl/ssl_test.c index a2022732..edf509ec 100644 --- a/ssl/ssl_test.c +++ b/ssl/ssl_test.c @@ -307,20 +307,6 @@ static const char kOpenSSLSession[] = * filling in missing fields from |kOpenSSLSession|. This includes * providing |peer_sha256|, so |peer| is not serialized. */ static const char kCustomSession[] = - "MIIBggIBAQICAwMEAsAvBCAG5Q1ndq4Yfmbeo1zwLkNRKmCXGdNgWvGT3cskV0yQ" - "kAQwJlrlzkAWBOWiLj/jJ76D7l+UXoizP2KI2C7I2FccqMmIfFmmkUy32nIJ0mZH" - "IWoJgAEBoQYCBFRDO46iBAICASykAwQBAqUDAgEUphAEDnd3dy5nb29nbGUuY29t" - "pwcEBWhlbGxvqAcEBXdvcmxkqQUCAwGJwKqBpwSBpBwUQvoeOk0Kg36SYTcLEkXq" - "KwOBfF9vE4KX0NxeLwjcDTpsuh3qXEaZ992r1N38VDcyS6P7I6HBYN9BsNHM362z" - "ZnY27GpTw+Kwd751CLoXFPoaMOe57dbBpXoro6Pd3BTbf/Tzr88K06yEOTDKPNj3" - "+inbMaVigtK4PLyPq+Topyzvx9USFgRvyuoxn0Hgb+R0A3j6SLRuyOdAi4gv7Y5o" - "liynrSIEIAYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGBgYGrgMEAQevAwQB" - "BLADBAEF"; - -/* kCustomSession2 is kCustomSession with the old SSLv2-only key_arg - * field removed. Encoding the decoded version of kCustomSession - * should not preserve key_arg. */ -static const char kCustomSession2[] = "MIIBfwIBAQICAwMEAsAvBCAG5Q1ndq4Yfmbeo1zwLkNRKmCXGdNgWvGT3cskV0yQ" "kAQwJlrlzkAWBOWiLj/jJ76D7l+UXoizP2KI2C7I2FccqMmIfFmmkUy32nIJ0mZH" "IWoJoQYCBFRDO46iBAICASykAwQBAqUDAgEUphAEDnd3dy5nb29nbGUuY29tpwcE" @@ -355,18 +341,16 @@ static int decode_base64(uint8_t **out, size_t *out_len, const char *in) { return 1; } -static int test_ssl_session_asn1(const char *input_b64, - const char *expected_b64) { +static int test_ssl_session_asn1(const char *input_b64) { int ret = 0, len; - size_t input_len, expected_len; - uint8_t *input = NULL, *expected = NULL, *encoded = NULL; + size_t input_len; + uint8_t *input = NULL, *encoded = NULL; const uint8_t *cptr; uint8_t *ptr; SSL_SESSION *session = NULL; /* Decode the input. */ - if (!decode_base64(&input, &input_len, input_b64) || - !decode_base64(&expected, &expected_len, expected_b64)) { + if (!decode_base64(&input, &input_len, input_b64)) { goto done; } @@ -380,27 +364,27 @@ static int test_ssl_session_asn1(const char *input_b64, /* Verify the SSL_SESSION encoding round-trips. */ len = i2d_SSL_SESSION(session, NULL); - if (len < 0 || (size_t)len != expected_len) { + if (len < 0 || (size_t)len != input_len) { fprintf(stderr, "i2d_SSL_SESSION(NULL) returned invalid length\n"); goto done; } - encoded = OPENSSL_malloc(expected_len); + encoded = OPENSSL_malloc(input_len); if (encoded == NULL) { fprintf(stderr, "malloc failed\n"); goto done; } ptr = encoded; len = i2d_SSL_SESSION(session, &ptr); - if (len < 0 || (size_t)len != expected_len) { + if (len < 0 || (size_t)len != input_len) { fprintf(stderr, "i2d_SSL_SESSION returned invalid length\n"); goto done; } - if (ptr != encoded + expected_len) { + if (ptr != encoded + input_len) { fprintf(stderr, "i2d_SSL_SESSION did not advance ptr correctly\n"); goto done; } - if (memcmp(expected, encoded, expected_len) != 0) { + if (memcmp(input, encoded, input_len) != 0) { fprintf(stderr, "i2d_SSL_SESSION did not round-trip\n"); goto done; } @@ -418,9 +402,6 @@ static int test_ssl_session_asn1(const char *input_b64, if (input) { OPENSSL_free(input); } - if (expected) { - OPENSSL_free(expected); - } if (encoded) { OPENSSL_free(encoded); } @@ -431,9 +412,8 @@ int main(void) { SSL_library_init(); if (!test_cipher_rules() || - !test_ssl_session_asn1(kOpenSSLSession, kOpenSSLSession) || - !test_ssl_session_asn1(kCustomSession, kCustomSession2) || - !test_ssl_session_asn1(kCustomSession2, kCustomSession2)) { + !test_ssl_session_asn1(kOpenSSLSession) || + !test_ssl_session_asn1(kCustomSession)) { return 1; }