From 4a03a80c1d989658282db830c8b35b5351c280aa Mon Sep 17 00:00:00 2001 From: Benjamin Kaduk Date: Wed, 22 Apr 2020 09:12:36 -0700 Subject: [PATCH] Some cleanup for the main QUIC changes Try to reduce unneeded whitespace changes and wrap new code to 80 columns. Reword documentation to attempt to improve clarity. Add some more sanity checks and clarifying comments to the code. Update referenced I-D versions. --- doc/man3/SSL_CTX_set_quic_method.pod | 43 +++++++------- include/openssl/ssl.h | 4 +- include/openssl/sslerr.h | 4 +- include/openssl/tls1.h | 2 +- ssl/build.info | 7 ++- ssl/ssl_ciph.c | 2 + ssl/ssl_lib.c | 2 +- ssl/ssl_local.h | 3 +- ssl/ssl_quic.c | 45 +++++++-------- ssl/statem/extensions_clnt.c | 5 +- ssl/statem/extensions_srvr.c | 8 ++- ssl/statem/statem.c | 2 +- ssl/statem/statem_lib.c | 27 +++++---- ssl/statem/statem_local.h | 2 + ssl/statem/statem_quic.c | 23 ++++---- ssl/tls13_enc.c | 85 +++++++++++++++++++--------- test/sslapitest.c | 11 ++-- util/libssl.num | 2 +- 18 files changed, 166 insertions(+), 111 deletions(-) diff --git a/doc/man3/SSL_CTX_set_quic_method.pod b/doc/man3/SSL_CTX_set_quic_method.pod index 60bf704944b2e..3d7bf7e682137 100644 --- a/doc/man3/SSL_CTX_set_quic_method.pod +++ b/doc/man3/SSL_CTX_set_quic_method.pod @@ -63,22 +63,25 @@ SSL_quic_max_handshake_flight_len() returns the maximum number of bytes that may be received at the given encryption level. This function should be used to limit buffering in the QUIC implementation. -See https://tools.ietf.org/html/draft-ietf-quic-transport-16#section-4.4. +See https://tools.ietf.org/html/draft-ietf-quic-transport-27#section-4. SSL_quic_read_level() returns the current read encryption level. SSL_quic_write_level() returns the current write encryption level. -SSL_provide_quic_data() provides data from QUIC at a particular encryption -level B. It is an error to call this function outside of the handshake -or with an encryption level other than the current read level. It returns one -on success and zero on error. +SSL_provide_quic_data() is used to provide data from QUIC CRYPTO frames to the +state machine, at a particular encryption level B. It is an error to +call this function outside of the handshake or with an encryption level other +than the current read level. The application must buffer and consolidate any +frames with less than four bytes of content. It returns one on success and +zero on error. SSL_process_quic_post_handshake() processes any data that QUIC has provided after the handshake has completed. This includes NewSessionTicket messages sent by the server. -SSL_is_quic() indicates whether a connection uses QUIC. +SSL_is_quic() indicates whether a connection uses QUIC. A given B +or B can only be used with QUIC or TLS, but not both. =head1 NOTES @@ -89,11 +92,11 @@ functions allow a QUIC implementation to serve as the underlying transport as described in draft-ietf-quic-tls. When configured for QUIC, SSL_do_handshake() will drive the handshake as -before, but it will not use the configured B. It will call functions on -B to configure secrets and send data. If data is needed from -the peer, it will return B. When received, the caller -should call SSL_provide_quic_data() and then SSL_do_handshake() to continue -the handshake. After the handshake is complete, the caller should call +before, but it will not use the configured B. It will call functions from +the configured B to configure secrets and send data. If data +is needed from the peer, it will return B. When received, +the caller should call SSL_provide_quic_data() and then SSL_do_handshake() to +continue the handshake. After the handshake is complete, the caller should call SSL_provide_quic_data() for any post-handshake data, followed by SSL_process_quic_post_handshake() to process it. It is an error to call SSL_read()/SSL_read_ex() and SSL_write()/SSL_write_ex() in QUIC. @@ -105,7 +108,7 @@ pass the active write level to add_handshake_data() when writing data. Callers can use SSL_quic_write_level() to query the active write level when generating their own errors. -See https://tools.ietf.org/html/draft-ietf-quic-tls-15#section-4.1 for more +See https://tools.ietf.org/html/draft-ietf-quic-tls-27#section-4.1 for more details. To avoid DoS attacks, the QUIC implementation must limit the amount of data @@ -113,11 +116,12 @@ being queued up. The implementation can call SSL_quic_max_handshake_flight_len() to get the maximum buffer length at each encryption level. -draft-ietf-quic-tls defines a new TLS extension quic_transport_parameters +draft-ietf-quic-tls defines a new TLS extension "quic_transport_parameters" used by QUIC for each endpoint to unilaterally declare its supported -transport parameters. draft-ietf-quic-transport (section 7.4) defines the -contents of that extension (a TransportParameters struct) and describes how -to handle it and its semantic meaning. +transport parameters. The contents of the extension are specified in +https://tools.ietf.org/html/draft-ietf-quic-transport-27#section-18 (as +a sequence of tag/length/value parameters) along with the interpretation of the +various parameters and the rules for their processing. OpenSSL handles this extension as an opaque byte string. The caller is responsible for serializing and parsing it. @@ -205,10 +209,11 @@ SSL_process_quic_post_handshake() return 1 on success, and 0 on error. SSL_quic_read_level() and SSL_quic_write_level() return the current -encryption level as B (B). +encryption level as an B +(B). -SSL_quic_max_handshake_flight_len() returns the maximum length of a flight -for a given encryption level. +SSL_quic_max_handshake_flight_len() returns the maximum length in bytes of a +flight for a given encryption level. SSL_is_quic() returns 1 if QUIC is being used, 0 if not. diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 8d9b9fb0a36a6..08d4c74b237cc 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2473,10 +2473,10 @@ __owur int SSL_process_quic_post_handshake(SSL *ssl); __owur int SSL_is_quic(SSL *ssl); -# endif - int SSL_CIPHER_get_prf_nid(const SSL_CIPHER *c); +# endif + # ifdef __cplusplus } # endif diff --git a/include/openssl/sslerr.h b/include/openssl/sslerr.h index e3915c0a55918..80cc5f379a661 100644 --- a/include/openssl/sslerr.h +++ b/include/openssl/sslerr.h @@ -11,7 +11,9 @@ #ifndef HEADER_SSLERR_H # define HEADER_SSLERR_H -# include +# ifndef HEADER_SYMHACKS_H +# include +# endif # ifdef __cplusplus extern "C" diff --git a/include/openssl/tls1.h b/include/openssl/tls1.h index 6e16c97316ddc..c11ca1efa30c9 100644 --- a/include/openssl/tls1.h +++ b/include/openssl/tls1.h @@ -148,7 +148,7 @@ extern "C" { /* Temporary extension type */ # define TLSEXT_TYPE_renegotiate 0xff01 -/* ExtensionType value from draft-ietf-quic-tls-13 */ +/* ExtensionType value from draft-ietf-quic-tls-27 */ # define TLSEXT_TYPE_quic_transport_parameters 0xffa5 # ifndef OPENSSL_NO_NEXTPROTONEG diff --git a/ssl/build.info b/ssl/build.info index eec0d14f2c563..497d943900d64 100644 --- a/ssl/build.info +++ b/ssl/build.info @@ -12,5 +12,8 @@ SOURCE[../libssl]=\ ssl_asn1.c ssl_txt.c ssl_init.c ssl_conf.c ssl_mcnf.c \ bio_ssl.c ssl_err.c tls_srp.c t1_trce.c ssl_utst.c \ record/ssl3_buffer.c record/ssl3_record.c record/dtls1_bitmap.c \ - statem/statem.c record/ssl3_record_tls13.c \ - ssl_quic.c statem/statem_quic.c + statem/statem.c record/ssl3_record_tls13.c + +IF[{- !$disabled{quic} -}] + SOURCE[../libssl]=ssl_quic.c statem/statem_quic.c +ENDIF diff --git a/ssl/ssl_ciph.c b/ssl/ssl_ciph.c index a3fe97597b592..67514f40b8dd3 100644 --- a/ssl/ssl_ciph.c +++ b/ssl/ssl_ciph.c @@ -2163,6 +2163,7 @@ int ssl_cert_is_disabled(size_t idx) return 0; } +#ifndef OPENSSL_NO_QUIC int SSL_CIPHER_get_prf_nid(const SSL_CIPHER *c) { switch (c->algorithm2 & (0xFF << TLS1_PRF_DGST_SHIFT)) { @@ -2194,3 +2195,4 @@ int SSL_CIPHER_get_prf_nid(const SSL_CIPHER *c) } return NID_undef; } +#endif diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c index da898d193cc25..8d23a72d300eb 100644 --- a/ssl/ssl_lib.c +++ b/ssl/ssl_lib.c @@ -3972,7 +3972,7 @@ EVP_PKEY *SSL_CTX_get0_privatekey(const SSL_CTX *ctx) const SSL_CIPHER *SSL_get_current_cipher(const SSL *s) { - if (s->session != NULL) + if ((s->session != NULL) && (s->session->cipher != NULL)) return s->session->cipher; return NULL; } diff --git a/ssl/ssl_local.h b/ssl/ssl_local.h index 016b2538587a8..fa61ec838c10c 100644 --- a/ssl/ssl_local.h +++ b/ssl/ssl_local.h @@ -1086,6 +1086,7 @@ struct quic_data_st { OSSL_ENCRYPTION_LEVEL level; size_t offset; size_t length; + /* char data[]; should be here but C90 VLAs not allowed here */ }; typedef struct quic_data_st QUIC_DATA; int quic_set_encryption_secrets(SSL *ssl, OSSL_ENCRYPTION_LEVEL level); @@ -1561,8 +1562,6 @@ typedef struct tls_group_info_st { # define TLS_CURVE_CHAR2 0x1 # define TLS_CURVE_CUSTOM 0x2 -typedef struct cert_pkey_st CERT_PKEY; - /* * Structure containing table entry of certificate info corresponding to * CERT_PKEY entries diff --git a/ssl/ssl_quic.c b/ssl/ssl_quic.c index 2d8accbdd18c8..e0ee8b0824b33 100644 --- a/ssl/ssl_quic.c +++ b/ssl/ssl_quic.c @@ -11,10 +11,6 @@ #include "internal/cryptlib.h" #include "internal/refcount.h" -#ifdef OPENSSL_NO_QUIC -NON_EMPTY_TRANSLATION_UNIT -#else - int SSL_set_quic_transport_params(SSL *ssl, const uint8_t *params, size_t params_len) { @@ -109,10 +105,10 @@ int SSL_provide_quic_data(SSL *ssl, OSSL_ENCRYPTION_LEVEL level, return 0; } - /* Split the QUIC messages up, if necessary */ + /* Split on handshake message boundaries, if necessary */ while (len > 0) { QUIC_DATA *qd; - const uint8_t *p = data + 1; + const uint8_t *p; /* Check for an incomplete block */ qd = ssl->quic_input_data_tail; @@ -130,6 +126,12 @@ int SSL_provide_quic_data(SSL *ssl, OSSL_ENCRYPTION_LEVEL level, } } + if (len < SSL3_HM_HEADER_LENGTH) { + SSLerr(SSL_F_SSL_PROVIDE_QUIC_DATA, SSL_R_BAD_LENGTH); + return 0; + } + /* TLS Handshake message header has 1-byte type and 3-byte length */ + p = data + 1; n2l3(p, l); l += SSL3_HM_HEADER_LENGTH; @@ -163,15 +165,8 @@ int SSL_provide_quic_data(SSL *ssl, OSSL_ENCRYPTION_LEVEL level, int SSL_CTX_set_quic_method(SSL_CTX *ctx, const SSL_QUIC_METHOD *quic_method) { - switch (ctx->method->version) { - case DTLS1_VERSION: - case DTLS1_2_VERSION: - case DTLS_ANY_VERSION: - case DTLS1_BAD_VER: + if (ctx->method->version != TLS_ANY_VERSION) return 0; - default: - break; - } ctx->quic_method = quic_method; ctx->options &= ~SSL_OP_ENABLE_MIDDLEBOX_COMPAT; return 1; @@ -179,15 +174,8 @@ int SSL_CTX_set_quic_method(SSL_CTX *ctx, const SSL_QUIC_METHOD *quic_method) int SSL_set_quic_method(SSL *ssl, const SSL_QUIC_METHOD *quic_method) { - switch (ssl->method->version) { - case DTLS1_VERSION: - case DTLS1_2_VERSION: - case DTLS_ANY_VERSION: - case DTLS1_BAD_VER: + if (ssl->method->version != TLS_ANY_VERSION) return 0; - default: - break; - } ssl->quic_method = quic_method; ssl->options &= ~SSL_OP_ENABLE_MIDDLEBOX_COMPAT; return 1; @@ -225,6 +213,12 @@ int quic_set_encryption_secrets(SSL *ssl, OSSL_ENCRYPTION_LEVEL level) /* May not have selected cipher, yet */ const SSL_CIPHER *c = NULL; + /* + * It probably doesn't make sense to use an (external) PSK session, + * but in theory some kinds of external session caches could be + * implemented using it, so allow psksession to be used as well as + * the regular session. + */ if (ssl->session != NULL) c = SSL_SESSION_get0_cipher(ssl->session); else if (ssl->psksession != NULL) @@ -268,6 +262,11 @@ int SSL_process_quic_post_handshake(SSL *ssl) return 0; } + /* + * This is always safe (we are sure to be at a record boundary) because + * SSL_read()/SSL_write() are never used for QUIC connections -- the + * application data is handled at the QUIC layer instead. + */ ossl_statem_set_in_init(ssl, 1); ret = ssl->handshake_func(ssl); ossl_statem_set_in_init(ssl, 0); @@ -281,5 +280,3 @@ int SSL_is_quic(SSL* ssl) { return SSL_IS_QUIC(ssl); } - -#endif diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index a9f73f07dcae1..a2992b9602275 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -1936,7 +1936,7 @@ int tls_parse_stoc_early_data(SSL *s, PACKET *pkt, unsigned int context, #ifndef OPENSSL_NO_QUIC /* * QUIC server must send 0xFFFFFFFF or it's a PROTOCOL_VIOLATION - * per draft-ietf-quic-tls-24 S4.5 + * per draft-ietf-quic-tls-27 S4.5 */ if (s->quic_method != NULL && max_early_data != 0xFFFFFFFF) { SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_F_TLS_PARSE_STOC_EARLY_DATA, @@ -2045,7 +2045,8 @@ int tls_parse_stoc_quic_transport_params(SSL *s, PACKET *pkt, unsigned int conte &s->ext.peer_quic_transport_params, &s->ext.peer_quic_transport_params_len)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, - SSL_F_TLS_PARSE_STOC_QUIC_TRANSPORT_PARAMS, ERR_R_INTERNAL_ERROR); + SSL_F_TLS_PARSE_STOC_QUIC_TRANSPORT_PARAMS, + ERR_R_INTERNAL_ERROR); return 0; } return 1; diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 313d6ea87fe94..6ff2232e467ba 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -1316,7 +1316,8 @@ int tls_parse_ctos_quic_transport_params(SSL *s, PACKET *pkt, unsigned int conte &s->ext.peer_quic_transport_params, &s->ext.peer_quic_transport_params_len)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, - SSL_F_TLS_PARSE_CTOS_QUIC_TRANSPORT_PARAMS, ERR_R_INTERNAL_ERROR); + SSL_F_TLS_PARSE_CTOS_QUIC_TRANSPORT_PARAMS, + ERR_R_INTERNAL_ERROR); return 0; } return 1; @@ -1952,7 +1953,7 @@ EXT_RETURN tls_construct_stoc_early_data(SSL *s, WPACKET *pkt, return EXT_RETURN_NOT_SENT; #ifndef OPENSSL_NO_QUIC - /* QUIC server must always send 0xFFFFFFFF, per draft-ietf-quic-tls-24 S4.5 */ + /* QUIC server must always send 0xFFFFFFFF, per draft-ietf-quic-tls-27 S4.5 */ if (s->quic_method != NULL) max_early_data = 0xFFFFFFFF; #endif @@ -2016,7 +2017,8 @@ EXT_RETURN tls_construct_stoc_quic_transport_params(SSL *s, WPACKET *pkt, || !WPACKET_sub_memcpy_u16(pkt, s->ext.quic_transport_params, s->ext.quic_transport_params_len)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, - SSL_F_TLS_CONSTRUCT_STOC_QUIC_TRANSPORT_PARAMS, ERR_R_INTERNAL_ERROR); + SSL_F_TLS_CONSTRUCT_STOC_QUIC_TRANSPORT_PARAMS, + ERR_R_INTERNAL_ERROR); return EXT_RETURN_FAIL; } diff --git a/ssl/statem/statem.c b/ssl/statem/statem.c index 0a8acedebfefe..d76924da85c48 100644 --- a/ssl/statem/statem.c +++ b/ssl/statem/statem.c @@ -577,6 +577,7 @@ static SUB_STATE_RETURN read_state_machine(SSL *s) ret = dtls_get_message(s, &mt, &len); #ifndef OPENSSL_NO_QUIC } else if (SSL_IS_QUIC(s)) { + /* QUIC behaves like DTLS -- all in one go. */ ret = quic_get_message(s, &mt, &len); #endif } else { @@ -907,7 +908,6 @@ int statem_flush(SSL *s) #ifndef OPENSSL_NO_QUIC if (SSL_IS_QUIC(s)) { if (!s->quic_method->flush_flight(s)) { - /* NOTE: BIO_flush() does not generate an error */ SSLerr(SSL_F_STATEM_FLUSH, ERR_R_INTERNAL_ERROR); return 0; } diff --git a/ssl/statem/statem_lib.c b/ssl/statem/statem_lib.c index b22b7f5e93567..ac79fa1281048 100644 --- a/ssl/statem/statem_lib.c +++ b/ssl/statem/statem_lib.c @@ -42,23 +42,29 @@ int ssl3_do_write(SSL *s, int type) { int ret; size_t written = 0; + #ifndef OPENSSL_NO_QUIC - if (SSL_IS_QUIC(s) && type == SSL3_RT_HANDSHAKE) { - ret = s->quic_method->add_handshake_data(s, s->quic_write_level, - (const uint8_t*)&s->init_buf->data[s->init_off], - s->init_num); - if (!ret) { - ret = -1; - /* QUIC can't sent anything out sice the above failed */ - SSLerr(SSL_F_SSL3_DO_WRITE, SSL_R_INTERNAL_ERROR); + if (SSL_IS_QUIC(s)) { + if (type == SSL3_RT_HANDSHAKE) { + ret = s->quic_method->add_handshake_data(s, s->quic_write_level, + (const uint8_t*)&s->init_buf->data[s->init_off], + s->init_num); + if (!ret) { + ret = -1; + /* QUIC can't sent anything out sice the above failed */ + SSLerr(SSL_F_SSL3_DO_WRITE, SSL_R_INTERNAL_ERROR); + } else { + written = s->init_num; + } } else { - written = s->init_num; + /* QUIC doesn't use ChangeCipherSpec */ + ret = -1; + SSLerr(SSL_F_SSL3_DO_WRITE, ERR_R_SHOULD_NOT_HAVE_BEEN_CALLED); } } else #endif ret = ssl3_write_bytes(s, type, &s->init_buf->data[s->init_off], s->init_num, &written); - if (ret < 0) return -1; if (type == SSL3_RT_HANDSHAKE) @@ -1171,7 +1177,6 @@ int tls_get_message_header(SSL *s, int *mt) do { while (s->init_num < SSL3_HM_HEADER_LENGTH) { - /* QUIC: either create a special ssl_read_bytes... or if/else this */ i = s->method->ssl_read_bytes(s, SSL3_RT_HANDSHAKE, &recvd_type, &p[s->init_num], SSL3_HM_HEADER_LENGTH - s->init_num, diff --git a/ssl/statem/statem_local.h b/ssl/statem/statem_local.h index 1551dac952785..6a7a3e0c5d38e 100644 --- a/ssl/statem/statem_local.h +++ b/ssl/statem/statem_local.h @@ -93,7 +93,9 @@ WORK_STATE ossl_statem_server_post_process_message(SSL *s, WORK_STATE wst); __owur int tls_get_message_header(SSL *s, int *mt); __owur int tls_get_message_body(SSL *s, size_t *len); __owur int dtls_get_message(SSL *s, int *mt, size_t *len); +#ifndef OPENSSL_NO_QUIC __owur int quic_get_message(SSL *s, int *mt, size_t *len); +#endif /* Message construction and processing functions */ __owur int tls_process_initial_server_flight(SSL *s); diff --git a/ssl/statem/statem_quic.c b/ssl/statem/statem_quic.c index eb1a76ec9d814..a2ba29337c049 100644 --- a/ssl/statem/statem_quic.c +++ b/ssl/statem/statem_quic.c @@ -11,10 +11,6 @@ #include "statem_local.h" #include "internal/cryptlib.h" -#ifdef OPENSSL_NO_QUIC -NON_EMPTY_TRANSLATION_UNIT -#else - int quic_get_message(SSL *s, int *mt, size_t *len) { size_t l; @@ -23,7 +19,14 @@ int quic_get_message(SSL *s, int *mt, size_t *len) if (qd == NULL || (qd->length - qd->offset) != 0) { s->rwstate = SSL_READING; - *len = 0; + *mt = *len = 0; + return 0; + } + + if (!ossl_assert(qd->length >= SSL3_HM_HEADER_LENGTH)) { + SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_QUIC_GET_MESSAGE, + SSL_R_BAD_LENGTH); + *mt = *len = 0; return 0; } @@ -31,14 +34,14 @@ int quic_get_message(SSL *s, int *mt, size_t *len) if (qd->level != s->quic_read_level) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_QUIC_GET_MESSAGE, SSL_R_WRONG_ENCRYPTION_LEVEL_RECEIVED); - *len = 0; + *mt = *len = 0; return 0; } if (!BUF_MEM_grow_clean(s->init_buf, (int)qd->length)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_QUIC_GET_MESSAGE, ERR_R_BUF_LIB); - *len = 0; + *mt = *len = 0; return 0; } @@ -83,8 +86,8 @@ int quic_get_message(SSL *s, int *mt, size_t *len) */ #define SERVER_HELLO_RANDOM_OFFSET (SSL3_HM_HEADER_LENGTH + 2) /* KeyUpdate and NewSessionTicket do not need to be added */ - if (!SSL_IS_TLS13(s) || (s->s3->tmp.message_type != SSL3_MT_NEWSESSION_TICKET - && s->s3->tmp.message_type != SSL3_MT_KEY_UPDATE)) { + if (s->s3->tmp.message_type != SSL3_MT_NEWSESSION_TICKET + && s->s3->tmp.message_type != SSL3_MT_KEY_UPDATE) { if (s->s3->tmp.message_type != SSL3_MT_SERVER_HELLO || s->init_num < SERVER_HELLO_RANDOM_OFFSET + SSL3_RANDOM_SIZE || memcmp(hrrrandom, @@ -105,5 +108,3 @@ int quic_get_message(SSL *s, int *mt, size_t *len) return 1; } - -#endif diff --git a/ssl/tls13_enc.c b/ssl/tls13_enc.c index 89678d6c22167..f73b97fbe496b 100644 --- a/ssl/tls13_enc.c +++ b/ssl/tls13_enc.c @@ -453,6 +453,7 @@ static const unsigned char exporter_master_secret[] = "exp master"; static const unsigned char resumption_master_secret[] = "res master"; static const unsigned char early_exporter_master_secret[] = "e exp master"; #endif + #ifndef OPENSSL_NO_QUIC static int quic_change_cipher_state(SSL *s, int which) { @@ -461,7 +462,7 @@ static int quic_change_cipher_state(SSL *s, int which) int hashleni; int ret = 0; const EVP_MD *md = NULL; - OSSL_ENCRYPTION_LEVEL level = ssl_encryption_initial; + OSSL_ENCRYPTION_LEVEL level; int is_handshake = ((which & SSL3_CC_HANDSHAKE) == SSL3_CC_HANDSHAKE); int is_client_read = ((which & SSL3_CHANGE_CIPHER_CLIENT_READ) == SSL3_CHANGE_CIPHER_CLIENT_READ); int is_server_write = ((which & SSL3_CHANGE_CIPHER_SERVER_WRITE) == SSL3_CHANGE_CIPHER_SERVER_WRITE); @@ -485,34 +486,62 @@ static int quic_change_cipher_state(SSL *s, int which) if (is_client_read || is_server_write) { if (is_handshake) { + /* + * This looks a bit weird, since the condition is basically "the + * server is writing" but we set both the server *and* client + * handshake traffic keys here. That's because there's only a fixed + * number of change-cipher-state events in the TLS 1.3 handshake, + * and in particular there's not an event in between when the server + * writes encrypted handshake messages and when the client writes + * encrypted handshake messages, so we generate both here. + */ level = ssl_encryption_handshake; - if (!tls13_hkdf_expand(s, md, s->handshake_secret, client_handshake_traffic, - sizeof(client_handshake_traffic)-1, hash, hashlen, - s->client_hand_traffic_secret, hashlen, 1) - || !ssl_log_secret(s, CLIENT_HANDSHAKE_LABEL, s->client_hand_traffic_secret, hashlen) - || !tls13_derive_finishedkey(s, md, s->client_hand_traffic_secret, + if (!tls13_hkdf_expand(s, md, s->handshake_secret, + client_handshake_traffic, + sizeof(client_handshake_traffic)-1, hash, + hashlen, s->client_hand_traffic_secret, + hashlen, 1) + || !ssl_log_secret(s, CLIENT_HANDSHAKE_LABEL, + s->client_hand_traffic_secret, hashlen) + || !tls13_derive_finishedkey(s, md, + s->client_hand_traffic_secret, s->client_finished_secret, hashlen) - || !tls13_hkdf_expand(s, md, s->handshake_secret, server_handshake_traffic, - sizeof(server_handshake_traffic)-1, hash, hashlen, - s->server_hand_traffic_secret, hashlen, 1) - || !ssl_log_secret(s, SERVER_HANDSHAKE_LABEL, s->server_hand_traffic_secret, hashlen) - || !tls13_derive_finishedkey(s, md, s->server_hand_traffic_secret, - s->server_finished_secret, hashlen)) { + || !tls13_hkdf_expand(s, md, s->handshake_secret, + server_handshake_traffic, + sizeof(server_handshake_traffic)-1, hash, + hashlen, s->server_hand_traffic_secret, + hashlen, 1) + || !ssl_log_secret(s, SERVER_HANDSHAKE_LABEL, + s->server_hand_traffic_secret, hashlen) + || !tls13_derive_finishedkey(s, md, + s->server_hand_traffic_secret, + s->server_finished_secret, + hashlen)) { /* SSLfatal() already called */ goto err; } } else { + /* + * As above, we generate both sets of application traffic keys at + * the same time. + */ level = ssl_encryption_application; - if (!tls13_hkdf_expand(s, md, s->master_secret, client_application_traffic, - sizeof(client_application_traffic)-1, hash, hashlen, - s->client_app_traffic_secret, hashlen, 1) - || !ssl_log_secret(s, CLIENT_APPLICATION_LABEL, s->client_app_traffic_secret, hashlen) - || !tls13_hkdf_expand(s, md, s->master_secret, server_application_traffic, - sizeof(server_application_traffic)-1, hash, hashlen, + if (!tls13_hkdf_expand(s, md, s->master_secret, + client_application_traffic, + sizeof(client_application_traffic)-1, hash, + hashlen, s->client_app_traffic_secret, + hashlen, 1) + || !ssl_log_secret(s, CLIENT_APPLICATION_LABEL, + s->client_app_traffic_secret, hashlen) + || !tls13_hkdf_expand(s, md, s->master_secret, + server_application_traffic, + sizeof(server_application_traffic)-1, + hash, hashlen, s->server_app_traffic_secret, hashlen, 1) - || !ssl_log_secret(s, SERVER_APPLICATION_LABEL, s->server_app_traffic_secret, hashlen)) { + || !ssl_log_secret(s, SERVER_APPLICATION_LABEL, + s->server_app_traffic_secret, hashlen)) { /* SSLfatal() already called */ goto err; } @@ -532,9 +561,11 @@ static int quic_change_cipher_state(SSL *s, int which) level = ssl_encryption_early_data; if (!tls13_hkdf_expand(s, md, s->early_secret, client_early_traffic, - sizeof(client_early_traffic)-1, hash, hashlen, - s->client_early_traffic_secret, hashlen, 1) - || !ssl_log_secret(s, CLIENT_EARLY_LABEL, s->client_early_traffic_secret, hashlen) + sizeof(client_early_traffic)-1, hash, + hashlen, s->client_early_traffic_secret, + hashlen, 1) + || !ssl_log_secret(s, CLIENT_EARLY_LABEL, + s->client_early_traffic_secret, hashlen) || !quic_set_encryption_secrets(s, level)) { /* SSLfatal() already called */ goto err; @@ -547,9 +578,11 @@ static int quic_change_cipher_state(SSL *s, int which) * We also create the resumption master secret, but this time use the * hash for the whole handshake including the Client Finished */ - if (!tls13_hkdf_expand(s, md, s->master_secret, resumption_master_secret, - sizeof(resumption_master_secret)-1, hash, hashlen, - s->resumption_master_secret, hashlen, 1)) { + if (!tls13_hkdf_expand(s, md, s->master_secret, + resumption_master_secret, + sizeof(resumption_master_secret)-1, hash, + hashlen, s->resumption_master_secret, + hashlen, 1)) { /* SSLfatal() already called */ goto err; } @@ -566,6 +599,7 @@ static int quic_change_cipher_state(SSL *s, int which) return ret; } #endif /* OPENSSL_NO_QUIC */ + int tls13_change_cipher_state(SSL *s, int which) { unsigned char *iv; @@ -832,7 +866,6 @@ int tls13_change_cipher_state(SSL *s, int which) s->statem.enc_write_state = ENC_WRITE_STATE_WRITE_PLAIN_ALERTS; else s->statem.enc_write_state = ENC_WRITE_STATE_VALID; - ret = 1; err: OPENSSL_cleanse(secret, sizeof(secret)); diff --git a/test/sslapitest.c b/test/sslapitest.c index 784a6a911facf..5bebcf2d254e1 100644 --- a/test/sslapitest.c +++ b/test/sslapitest.c @@ -6660,9 +6660,11 @@ static int test_ssl_dup(void) #ifndef OPENSSL_NO_QUIC -static int test_quic_set_encryption_secrets(SSL *ssl, OSSL_ENCRYPTION_LEVEL level, +static int test_quic_set_encryption_secrets(SSL *ssl, + OSSL_ENCRYPTION_LEVEL level, const uint8_t *read_secret, - const uint8_t *write_secret, size_t secret_len) + const uint8_t *write_secret, + size_t secret_len) { test_printf_stderr("quic_set_encryption_secrets() %s, lvl=%d, len=%zd\n", ssl->server ? "server" : "client", level, secret_len); @@ -6673,11 +6675,12 @@ static int test_quic_add_handshake_data(SSL *ssl, OSSL_ENCRYPTION_LEVEL level, { SSL *peer = (SSL*)SSL_get_app_data(ssl); - test_printf_stderr("quic_add_handshake_data() %s, lvl=%d, *data=0x%02X, len=%zd\n", - ssl->server ? "server" : "client", level, (int)*data, len); + TEST_info("quic_add_handshake_data() %s, lvl=%d, *data=0x%02X, len=%zd\n", + ssl->server ? "server" : "client", level, (int)*data, len); if (!TEST_ptr(peer)) return 0; + /* We're called with what is locally written; this gives it to the peer */ if (!TEST_true(SSL_provide_quic_data(peer, level, data, len))) { ERR_print_errors_fp(stderr); return 0; diff --git a/util/libssl.num b/util/libssl.num index 15785fe10d018..0452d9cb785ad 100644 --- a/util/libssl.num +++ b/util/libssl.num @@ -500,7 +500,7 @@ SSL_CTX_set_post_handshake_auth 500 1_1_1 EXIST::FUNCTION: SSL_get_signature_type_nid 501 1_1_1a EXIST::FUNCTION: SSL_quic_read_level 10094 1_1_1d EXIST::FUNCTION:QUIC SSL_set_quic_transport_params 10095 1_1_1d EXIST::FUNCTION:QUIC -SSL_CIPHER_get_prf_nid 10096 1_1_1d EXIST::FUNCTION: +SSL_CIPHER_get_prf_nid 10096 1_1_1d EXIST::FUNCTION:QUIC SSL_is_quic 10097 1_1_1d EXIST::FUNCTION:QUIC SSL_get_peer_quic_transport_params 10098 1_1_1d EXIST::FUNCTION:QUIC SSL_quic_write_level 10099 1_1_1d EXIST::FUNCTION:QUIC