From 124037fdc0571b5bd9022412348e9979a1726a31 Mon Sep 17 00:00:00 2001 From: "Dr. Stephen Henson" Date: Tue, 16 Jun 2015 14:44:29 +0100 Subject: [PATCH] Tidy up ssl3_digest_cached_records logic. Rewrite ssl3_digest_cached_records handling. Only digest cached records if digest array is NULL: this means it is safe to call ssl3_digest_cached_records multiple times (subsequent calls are no op). Remove flag TLS1_FLAGS_KEEP_HANDSHAKE instead only update handshake buffer if digest array is NULL. Add additional "keep" parameter to ssl3_digest_cached_records to indicate if the handshake buffer should be retained after digesting cached records (needed for TLS 1.2 client authentication). Reviewed-by: Matt Caswell --- include/openssl/ssl3.h | 1 - ssl/d1_srvr.c | 9 ++---- ssl/s3_clnt.c | 21 ++++---------- ssl/s3_enc.c | 63 ++++++++++++++++++++---------------------- ssl/s3_srvr.c | 32 ++++++++------------- ssl/ssl_locl.h | 2 +- ssl/t1_enc.c | 28 ++++++++----------- 7 files changed, 63 insertions(+), 93 deletions(-) diff --git a/include/openssl/ssl3.h b/include/openssl/ssl3.h index 138b80c81a..d56105e178 100644 --- a/include/openssl/ssl3.h +++ b/include/openssl/ssl3.h @@ -365,7 +365,6 @@ extern "C" { /* Removed from OpenSSL 1.1.0 */ # define TLS1_FLAGS_TLS_PADDING_BUG 0x0 # define TLS1_FLAGS_SKIP_CERT_VERIFY 0x0010 -# define TLS1_FLAGS_KEEP_HANDSHAKE 0x0020 /* * Set when the handshake is ready to process peer's ChangeCipherSpec message. * Cleared after the message has been processed. diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c index dfdc573d64..7a40d66a14 100644 --- a/ssl/d1_srvr.c +++ b/ssl/d1_srvr.c @@ -640,12 +640,9 @@ int dtls1_accept(SSL *s) * For sigalgs freeze the handshake buffer. If we support * extms we've done this already. */ - if (!(s->s3->flags & SSL_SESS_FLAG_EXTMS)) { - s->s3->flags |= TLS1_FLAGS_KEEP_HANDSHAKE; - if (!ssl3_digest_cached_records(s)) { - s->state = SSL_ST_ERR; - return -1; - } + if (!ssl3_digest_cached_records(s, 1)) { + s->state = SSL_ST_ERR; + return -1; } } else { s->state = SSL3_ST_SR_CERT_VRFY_A; diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 6b4c860350..a911bb154a 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -1168,7 +1168,7 @@ int ssl3_get_server_hello(SSL *s) * Don't digest cached records if no sigalgs: we may need them for client * authentication. */ - if (!SSL_USE_SIGALGS(s) && !ssl3_digest_cached_records(s)) + if (!SSL_USE_SIGALGS(s) && !ssl3_digest_cached_records(s, 0)) goto f_err; /* lets get the compression algorithm */ /* COMPRESSION */ @@ -2030,10 +2030,8 @@ int ssl3_get_certificate_request(SSL *s) * If we get here we don't need any cached handshake records as we * wont be doing client auth. */ - if (s->s3->handshake_buffer) { - if (!ssl3_digest_cached_records(s)) - goto err; - } + if (!ssl3_digest_cached_records(s, 0)) + goto err; return (1); } @@ -3026,15 +3024,8 @@ int ssl3_send_client_verify(SSL *s) } s2n(u, p); n = u + 4; - /* - * For extended master secret we've already digested cached - * records. - */ - if (s->session->flags & SSL_SESS_FLAG_EXTMS) { - BIO_free(s->s3->handshake_buffer); - s->s3->handshake_buffer = NULL; - s->s3->flags &= ~TLS1_FLAGS_KEEP_HANDSHAKE; - } else if (!ssl3_digest_cached_records(s)) + /* Digest cached records and discard handshake buffer */ + if (!ssl3_digest_cached_records(s, 0)) goto err; } else #ifndef OPENSSL_NO_RSA @@ -3216,7 +3207,7 @@ int ssl3_send_client_certificate(SSL *s) return (1); } else { s->s3->tmp.cert_req = 2; - if (s->s3->handshake_buffer && !ssl3_digest_cached_records(s)) { + if (!ssl3_digest_cached_records(s, 0)) { ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_INTERNAL_ERROR); s->state = SSL_ST_ERR; return 0; diff --git a/ssl/s3_enc.c b/ssl/s3_enc.c index bde19e1fb8..5e52af827b 100644 --- a/ssl/s3_enc.c +++ b/ssl/s3_enc.c @@ -497,8 +497,7 @@ void ssl3_free_digest_list(SSL *s) void ssl3_finish_mac(SSL *s, const unsigned char *buf, int len) { - if (s->s3->handshake_buffer - && !(s->s3->flags & TLS1_FLAGS_KEEP_HANDSHAKE)) { + if (s->s3->handshake_dgst == NULL) { BIO_write(s->s3->handshake_buffer, (void *)buf, len); } else { int i; @@ -509,7 +508,7 @@ void ssl3_finish_mac(SSL *s, const unsigned char *buf, int len) } } -int ssl3_digest_cached_records(SSL *s) +int ssl3_digest_cached_records(SSL *s, int keep) { int i; long mask; @@ -517,38 +516,37 @@ int ssl3_digest_cached_records(SSL *s) long hdatalen; void *hdata; - /* Allocate handshake_dgst array */ - ssl3_free_digest_list(s); - s->s3->handshake_dgst = - OPENSSL_malloc(sizeof(*s->s3->handshake_dgst) * SSL_MAX_DIGEST); if (s->s3->handshake_dgst == NULL) { - SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS, ERR_R_MALLOC_FAILURE); - return 0; - } - memset(s->s3->handshake_dgst, 0, - sizeof(*s->s3->handshake_dgst) * SSL_MAX_DIGEST); - hdatalen = BIO_get_mem_data(s->s3->handshake_buffer, &hdata); - if (hdatalen <= 0) { - SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS, SSL_R_BAD_HANDSHAKE_LENGTH); - return 0; - } + /* Allocate handshake_dgst array */ + s->s3->handshake_dgst = + OPENSSL_malloc(sizeof(*s->s3->handshake_dgst) * SSL_MAX_DIGEST); + if (s->s3->handshake_dgst == NULL) { + SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS, ERR_R_MALLOC_FAILURE); + return 0; + } + hdatalen = BIO_get_mem_data(s->s3->handshake_buffer, &hdata); + if (hdatalen <= 0) { + SSLerr(SSL_F_SSL3_DIGEST_CACHED_RECORDS, SSL_R_BAD_HANDSHAKE_LENGTH); + return 0; + } - /* Loop through bitso of algorithm2 field and create MD_CTX-es */ - for (i = 0; ssl_get_handshake_digest(i, &mask, &md); i++) { - if ((mask & ssl_get_algorithm2(s)) && md) { - s->s3->handshake_dgst[i] = EVP_MD_CTX_create(); - if (EVP_MD_nid(md) == NID_md5) { - EVP_MD_CTX_set_flags(s->s3->handshake_dgst[i], - EVP_MD_CTX_FLAG_NON_FIPS_ALLOW); + /* Loop through bits of algorithm2 field and create MD_CTX-es */ + for (i = 0; ssl_get_handshake_digest(i, &mask, &md); i++) { + if ((mask & ssl_get_algorithm2(s)) && md) { + s->s3->handshake_dgst[i] = EVP_MD_CTX_create(); + if (EVP_MD_nid(md) == NID_md5) { + EVP_MD_CTX_set_flags(s->s3->handshake_dgst[i], + EVP_MD_CTX_FLAG_NON_FIPS_ALLOW); + } + EVP_DigestInit_ex(s->s3->handshake_dgst[i], md, NULL); + EVP_DigestUpdate(s->s3->handshake_dgst[i], hdata, hdatalen); + } else { + s->s3->handshake_dgst[i] = NULL; } - EVP_DigestInit_ex(s->s3->handshake_dgst[i], md, NULL); - EVP_DigestUpdate(s->s3->handshake_dgst[i], hdata, hdatalen); - } else { - s->s3->handshake_dgst[i] = NULL; } + } - if (!(s->s3->flags & TLS1_FLAGS_KEEP_HANDSHAKE)) { - /* Free handshake_buffer BIO */ + if (keep == 0) { BIO_free(s->s3->handshake_buffer); s->s3->handshake_buffer = NULL; } @@ -588,9 +586,8 @@ static int ssl3_handshake_mac(SSL *s, int md_nid, unsigned char md_buf[EVP_MAX_MD_SIZE]; EVP_MD_CTX ctx, *d = NULL; - if (s->s3->handshake_buffer) - if (!ssl3_digest_cached_records(s)) - return 0; + if (!ssl3_digest_cached_records(s, 0)) + return 0; /* * Search for digest of specified type in the handshake_dgst array diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index b98beacf48..203e894f8e 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -507,11 +507,9 @@ int ssl3_accept(SSL *s) skip = 1; s->s3->tmp.cert_request = 0; s->state = SSL3_ST_SW_SRVR_DONE_A; - if (s->s3->handshake_buffer) { - if (!ssl3_digest_cached_records(s)) { - s->state = SSL_ST_ERR; - return -1; - } + if (!ssl3_digest_cached_records(s, 0)) { + s->state = SSL_ST_ERR; + return -1; } } else { s->s3->tmp.cert_request = 1; @@ -598,14 +596,11 @@ int ssl3_accept(SSL *s) } /* * For sigalgs freeze the handshake buffer. If we support - * extms we've done this already. + * extms we've done this already so this is a no-op */ - if (!(s->s3->flags & SSL_SESS_FLAG_EXTMS)) { - s->s3->flags |= TLS1_FLAGS_KEEP_HANDSHAKE; - if (!ssl3_digest_cached_records(s)) { - s->state = SSL_ST_ERR; - return -1; - } + if (!ssl3_digest_cached_records(s, 1)) { + s->state = SSL_ST_ERR; + return -1; } } else { int offset = 0; @@ -620,11 +615,9 @@ int ssl3_accept(SSL *s) * CertificateVerify should be generalized. But it is next * step */ - if (s->s3->handshake_buffer) { - if (!ssl3_digest_cached_records(s)) { - s->state = SSL_ST_ERR; - return -1; - } + if (!ssl3_digest_cached_records(s, 0)) { + s->state = SSL_ST_ERR; + return -1; } for (dgst_num = 0; dgst_num < SSL_MAX_DIGEST; dgst_num++) if (s->s3->handshake_dgst[dgst_num]) { @@ -1538,7 +1531,7 @@ int ssl3_get_client_hello(SSL *s) } if (!SSL_USE_SIGALGS(s) || !(s->verify_mode & SSL_VERIFY_PEER)) { - if (!ssl3_digest_cached_records(s)) + if (!ssl3_digest_cached_records(s, 0)) goto f_err; } @@ -3055,7 +3048,6 @@ int ssl3_get_cert_verify(SSL *s) end: BIO_free(s->s3->handshake_buffer); s->s3->handshake_buffer = NULL; - s->s3->flags &= ~TLS1_FLAGS_KEEP_HANDSHAKE; EVP_MD_CTX_cleanup(&mctx); EVP_PKEY_free(pkey); return (ret); @@ -3163,7 +3155,7 @@ int ssl3_get_client_certificate(SSL *s) goto f_err; } /* No client certificate so digest cached records */ - if (s->s3->handshake_buffer && !ssl3_digest_cached_records(s)) { + if (s->s3->handshake_buffer && !ssl3_digest_cached_records(s, 0)) { al = SSL_AD_INTERNAL_ERROR; goto f_err; } diff --git a/ssl/ssl_locl.h b/ssl/ssl_locl.h index f1046c5ffe..8f8d99700b 100644 --- a/ssl/ssl_locl.h +++ b/ssl/ssl_locl.h @@ -1922,7 +1922,7 @@ void ssl3_free_digest_list(SSL *s); __owur unsigned long ssl3_output_cert_chain(SSL *s, CERT_PKEY *cpk); __owur SSL_CIPHER *ssl3_choose_cipher(SSL *ssl, STACK_OF(SSL_CIPHER) *clnt, STACK_OF(SSL_CIPHER) *srvr); -__owur int ssl3_digest_cached_records(SSL *s); +__owur int ssl3_digest_cached_records(SSL *s, int keep); __owur int ssl3_new(SSL *s); void ssl3_free(SSL *s); __owur int ssl3_accept(SSL *s); diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c index e8653412aa..9942bb433f 100644 --- a/ssl/t1_enc.c +++ b/ssl/t1_enc.c @@ -679,9 +679,8 @@ int tls1_cert_verify_mac(SSL *s, int md_nid, unsigned char *out) EVP_MD_CTX ctx, *d = NULL; int i; - if (s->s3->handshake_buffer) - if (!ssl3_digest_cached_records(s)) - return 0; + if (!ssl3_digest_cached_records(s, 0)) + return 0; for (i = 0; i < SSL_MAX_DIGEST; i++) { if (s->s3->handshake_dgst[i] @@ -709,9 +708,8 @@ int tls1_final_finish_mac(SSL *s, const char *str, int slen, unsigned char hash[2 * EVP_MAX_MD_SIZE]; unsigned char buf2[12]; - if (s->s3->handshake_buffer) - if (!ssl3_digest_cached_records(s)) - return 0; + if (!ssl3_digest_cached_records(s, 0)) + return 0; hashlen = ssl_handshake_hash(s, hash, sizeof(hash)); @@ -736,17 +734,13 @@ int tls1_generate_master_secret(SSL *s, unsigned char *out, unsigned char *p, if (s->session->flags & SSL_SESS_FLAG_EXTMS) { unsigned char hash[EVP_MAX_MD_SIZE * 2]; int hashlen; - /* If we don't have any digests cache records */ - if (s->s3->handshake_buffer) { - /* - * keep record buffer: this wont affect client auth because we're - * freezing the buffer at the same point (after client key - * exchange and before certificate verify) - */ - s->s3->flags |= TLS1_FLAGS_KEEP_HANDSHAKE; - if (!ssl3_digest_cached_records(s)) - return -1; - } + /* Digest cached records keeping record buffer (if present): + * this wont affect client auth because we're freezing the buffer + * at the same point (after client key exchange and before certificate + * verify) + */ + if (!ssl3_digest_cached_records(s, 1)) + return -1; hashlen = ssl_handshake_hash(s, hash, sizeof(hash)); #ifdef SSL_DEBUG fprintf(stderr, "Handshake hashes:\n"); -- 2.25.1