Support EVP_AEAD in DTLS. This CL removes the last of the EVP_CIPHER codepath in ssl/. The dead code is intentionally not pruned for ease of review, except in DTLS-only code where adding new logic to support both, only to remove half, would be cumbersome. Fixes made: - dtls1_retransmit_state is taught to retain aead_write_ctx rather than enc_write_ctx. - d1_pkt.c reserves space for the variable-length nonce when echoed into the packet. - dtls1_do_write sizes the MTU based on EVP_AEAD max overhead. - tls1_change_cipher_state_cipher should not free AEAD write contexts in DTLS. This matches the (rather confused) ownership for the EVP_CIPHER contexts. I've added a TODO to resolve this craziness. A follow-up CL will remove all the resultant dead code. Change-Id: I644557f4db53bbfb182950823ab96d5e4c908866 Reviewed-on: https://boringssl-review.googlesource.com/2699 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/include/openssl/dtls1.h b/include/openssl/dtls1.h index 57337f6..0fc3ae6 100644 --- a/include/openssl/dtls1.h +++ b/include/openssl/dtls1.h
@@ -111,8 +111,7 @@ struct dtls1_retransmit_state { - EVP_CIPHER_CTX *enc_write_ctx; /* cryptographic state */ - EVP_MD_CTX *write_hash; /* used for mac generation */ + SSL_AEAD_CTX *aead_write_ctx; SSL_SESSION *session; unsigned short epoch; };
diff --git a/ssl/d1_both.c b/ssl/d1_both.c index 9081e4b..8d1dbf9 100644 --- a/ssl/d1_both.c +++ b/ssl/d1_both.c
@@ -214,8 +214,14 @@ void dtls1_hm_fragment_free(hm_fragment *frag) { if (frag->msg_header.is_ccs) { - EVP_CIPHER_CTX_free(frag->msg_header.saved_retransmit_state.enc_write_ctx); - EVP_MD_CTX_destroy(frag->msg_header.saved_retransmit_state.write_hash); + /* TODO(davidben): Simplify aead_write_ctx ownership, probably by just + * forbidding DTLS renego. */ + SSL_AEAD_CTX *aead_write_ctx = + frag->msg_header.saved_retransmit_state.aead_write_ctx; + if (aead_write_ctx) { + EVP_AEAD_CTX_cleanup(&aead_write_ctx->ctx); + OPENSSL_free(aead_write_ctx); + } } if (frag->fragment) { OPENSSL_free(frag->fragment); @@ -231,7 +237,8 @@ int dtls1_do_write(SSL *s, int type) { int ret; int curr_mtu; - unsigned int len, frag_off, mac_size = 0, blocksize = 0; + unsigned int len, frag_off; + size_t max_overhead = 0; /* AHA! Figure out the MTU, and stick to the right size */ if (s->d1->mtu < dtls1_min_mtu() && @@ -255,21 +262,18 @@ (int)s->d1->w_msg_hdr.msg_len + DTLS1_HM_HEADER_LENGTH); } - if (s->write_hash && - (s->enc_write_ctx == NULL || - EVP_CIPHER_CTX_mode(s->enc_write_ctx) != EVP_CIPH_GCM_MODE)) { - mac_size = EVP_MD_CTX_size(s->write_hash); - } - - if (s->enc_write_ctx && - (EVP_CIPHER_CTX_mode(s->enc_write_ctx) == EVP_CIPH_CBC_MODE)) { - blocksize = 2 * EVP_CIPHER_block_size(s->enc_write_ctx->cipher); + /* Determine the maximum overhead of the current cipher. */ + if (s->aead_write_ctx != NULL) { + max_overhead = EVP_AEAD_max_overhead(s->aead_write_ctx->ctx.aead); + if (s->aead_write_ctx->variable_nonce_included_in_record) { + max_overhead += s->aead_write_ctx->variable_nonce_len; + } } frag_off = 0; while (s->init_num) { curr_mtu = s->d1->mtu - BIO_wpending(SSL_get_wbio(s)) - - DTLS1_RT_HEADER_LENGTH - mac_size - blocksize; + DTLS1_RT_HEADER_LENGTH - max_overhead; if (curr_mtu <= DTLS1_HM_HEADER_LENGTH) { /* grr.. we could get an error if MTU picked was wrong */ @@ -277,7 +281,7 @@ if (ret <= 0) { return ret; } - curr_mtu = s->d1->mtu - DTLS1_RT_HEADER_LENGTH - mac_size - blocksize; + curr_mtu = s->d1->mtu - DTLS1_RT_HEADER_LENGTH - max_overhead; } if (s->init_num > curr_mtu) { @@ -978,8 +982,7 @@ frag->msg_header.is_ccs = is_ccs; /* save current state*/ - frag->msg_header.saved_retransmit_state.enc_write_ctx = s->enc_write_ctx; - frag->msg_header.saved_retransmit_state.write_hash = s->write_hash; + frag->msg_header.saved_retransmit_state.aead_write_ctx = s->aead_write_ctx; frag->msg_header.saved_retransmit_state.session = s->session; frag->msg_header.saved_retransmit_state.epoch = s->d1->w_epoch; @@ -1044,14 +1047,12 @@ 0, frag->msg_header.frag_len); /* save current state */ - saved_state.enc_write_ctx = s->enc_write_ctx; - saved_state.write_hash = s->write_hash; + saved_state.aead_write_ctx = s->aead_write_ctx; saved_state.session = s->session; saved_state.epoch = s->d1->w_epoch; /* restore state in which the message was originally sent */ - s->enc_write_ctx = frag->msg_header.saved_retransmit_state.enc_write_ctx; - s->write_hash = frag->msg_header.saved_retransmit_state.write_hash; + s->aead_write_ctx = frag->msg_header.saved_retransmit_state.aead_write_ctx; s->session = frag->msg_header.saved_retransmit_state.session; s->d1->w_epoch = frag->msg_header.saved_retransmit_state.epoch; @@ -1066,8 +1067,7 @@ : SSL3_RT_HANDSHAKE); /* restore current state */ - s->enc_write_ctx = saved_state.enc_write_ctx; - s->write_hash = saved_state.write_hash; + s->aead_write_ctx = saved_state.aead_write_ctx; s->session = saved_state.session; s->d1->w_epoch = saved_state.epoch;
diff --git a/ssl/d1_lib.c b/ssl/d1_lib.c index 7e76905..e4928a5 100644 --- a/ssl/d1_lib.c +++ b/ssl/d1_lib.c
@@ -256,22 +256,11 @@ return ret; } -/* As it's impossible to use stream ciphers in "datagram" mode, this - * simple filter is designed to disengage them in DTLS. Unfortunately - * there is no universal way to identify stream SSL_CIPHER, so we have - * to explicitly list their SSL_* codes. Currently RC4 is the only one - * available, but if new ones emerge, they will have to be added... */ const SSL_CIPHER *dtls1_get_cipher(unsigned int u) { const SSL_CIPHER *ciph = ssl3_get_cipher(u); - - if (ciph != NULL) { - if (ciph->algorithm_enc == SSL_RC4) { - return NULL; - } - /* TODO(davidben): EVP_AEAD does not work in DTLS yet. */ - if (ciph->algorithm2 & SSL_CIPHER_ALGORITHM2_AEAD) { - return NULL; - } + /* DTLS does not support stream ciphers. */ + if (ciph == NULL || ciph->algorithm_enc == SSL_RC4) { + return NULL; } return ciph;
diff --git a/ssl/d1_pkt.c b/ssl/d1_pkt.c index 67b06a5..85bd305 100644 --- a/ssl/d1_pkt.c +++ b/ssl/d1_pkt.c
@@ -1213,17 +1213,15 @@ p += 10; /* Explicit IV length, block ciphers appropriate version flag */ - if (s->enc_write_ctx) { - int mode = EVP_CIPHER_CTX_mode(s->enc_write_ctx); - if (mode == EVP_CIPH_CBC_MODE) { - eivlen = EVP_CIPHER_CTX_iv_length(s->enc_write_ctx); - if (eivlen <= 1) { - eivlen = 0; - } - } else if (mode == EVP_CIPH_GCM_MODE) { - /* Need explicit part of IV for GCM mode */ - eivlen = EVP_GCM_TLS_EXPLICIT_IV_LEN; + if (s->enc_write_ctx && SSL_USE_EXPLICIT_IV(s) && + EVP_CIPHER_CTX_mode(s->enc_write_ctx) == EVP_CIPH_CBC_MODE) { + eivlen = EVP_CIPHER_CTX_iv_length(s->enc_write_ctx); + if (eivlen <= 1) { + eivlen = 0; } + } else if (s->aead_write_ctx != NULL && + s->aead_write_ctx->variable_nonce_included_in_record) { + eivlen = s->aead_write_ctx->variable_nonce_len; } /* lets setup the record stuff. */
diff --git a/ssl/d1_srvr.c b/ssl/d1_srvr.c index 6f2e844..e4c57c3 100644 --- a/ssl/d1_srvr.c +++ b/ssl/d1_srvr.c
@@ -294,9 +294,7 @@ case SSL3_ST_SW_CERT_A: case SSL3_ST_SW_CERT_B: - /* Check if it is anon DH or normal PSK */ - if (!(s->s3->tmp.new_cipher->algorithm_auth & SSL_aNULL) && - !(s->s3->tmp.new_cipher->algorithm_mkey & SSL_kPSK)) { + if (ssl_cipher_has_server_public_key(s->s3->tmp.new_cipher)) { dtls1_start_timer(s); ret = ssl3_send_server_certificate(s); if (ret <= 0) {
diff --git a/ssl/t1_enc.c b/ssl/t1_enc.c index 48079ff..e4e9907 100644 --- a/ssl/t1_enc.c +++ b/ssl/t1_enc.c
@@ -362,6 +362,15 @@ } aead_ctx = s->aead_read_ctx; } else { + /* When updating the cipher state for DTLS, we do not wish to overwrite the + * old ones because DTLS stores pointers to them in order to implement + * retransmission. See dtls1_hm_fragment_free. + * + * TODO(davidben): Simplify aead_write_ctx ownership, probably by just + * forbidding DTLS renego. */ + if (SSL_IS_DTLS(s)) { + s->aead_write_ctx = NULL; + } if (!tls1_aead_ctx_init(&s->aead_write_ctx)) { return 0; } @@ -440,7 +449,17 @@ if (is_read) { tls1_cleanup_aead_ctx(&s->aead_read_ctx); } else { - tls1_cleanup_aead_ctx(&s->aead_write_ctx); + /* When updating the cipher state for DTLS, we do not wish to free the old + * ones because DTLS stores pointers to them in order to implement + * retransmission. See dtls1_hm_fragment_free. + * + * TODO(davidben): Simplify aead_write_ctx ownership, probably by just + * forbidding DTLS renego. */ + if (!SSL_IS_DTLS(s)) { + tls1_cleanup_aead_ctx(&s->aead_write_ctx); + } else { + s->aead_write_ctx = NULL; + } } if (is_read) { @@ -622,8 +641,8 @@ goto cipher_unavailable_err; } - /* TODO(davidben): Make DTLS record-layer code EVP_AEAD-aware. */ - if (!SSL_IS_DTLS(s)) { + /* TODO(davidben): Prune away dead code. To be done in follow-up commit. */ + if (1) { if (!ssl_cipher_get_evp_aead(&aead, &mac_secret_len, &fixed_iv_len, s->session->cipher, ssl3_version_from_wire(s, s->version))) {
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go index d76bdf7..2342682 100644 --- a/ssl/test/runner/runner.go +++ b/ssl/test/runner/runner.go
@@ -998,10 +998,7 @@ } func isDTLSCipher(suiteName string) bool { - // TODO(davidben): AES-GCM exists in DTLS 1.2 but is currently - // broken because DTLS is not EVP_AEAD-aware. - return !hasComponent(suiteName, "RC4") && - !hasComponent(suiteName, "GCM") + return !hasComponent(suiteName, "RC4") } func addCipherSuiteTests() {