From b3d9ef4ba1230cbff2d2f2ab596ccd010323e8ed Mon Sep 17 00:00:00 2001 From: jsing Date: Sun, 2 May 2021 17:46:58 +0000 Subject: [PATCH] Clean up tls1_change_cipher_state(). Replace flag gymnastics at call sites with separate read and write, functions which call the common code. Condition on s->server instead of using SSL_ST_ACCEPT, for consistency and more readable code. ok inoguchi@ tb@ --- lib/libssl/ssl_clnt.c | 8 +++----- lib/libssl/ssl_locl.h | 5 +++-- lib/libssl/ssl_pkt.c | 11 ++--------- lib/libssl/ssl_srvr.c | 8 +++----- lib/libssl/t1_enc.c | 39 ++++++++++++++++++--------------------- 5 files changed, 29 insertions(+), 42 deletions(-) diff --git a/lib/libssl/ssl_clnt.c b/lib/libssl/ssl_clnt.c index 022efd8b3b1..1874d22b946 100644 --- a/lib/libssl/ssl_clnt.c +++ b/lib/libssl/ssl_clnt.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_clnt.c,v 1.95 2021/05/02 17:18:10 jsing Exp $ */ +/* $OpenBSD: ssl_clnt.c,v 1.96 2021/05/02 17:46:58 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -474,15 +474,13 @@ ssl3_connect(SSL *s) S3I(s)->hs.state = SSL3_ST_CW_FINISHED_A; s->internal->init_num = 0; - s->session->cipher = S3I(s)->hs.cipher; + if (!tls1_setup_key_block(s)) { ret = -1; goto end; } - - if (!tls1_change_cipher_state(s, - SSL3_CHANGE_CIPHER_CLIENT_WRITE)) { + if (!tls1_change_write_cipher_state(s)) { ret = -1; goto end; } diff --git a/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h index 38b68384647..9dfa1243c94 100644 --- a/lib/libssl/ssl_locl.h +++ b/lib/libssl/ssl_locl.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_locl.h,v 1.340 2021/05/02 17:18:10 jsing Exp $ */ +/* $OpenBSD: ssl_locl.h,v 1.341 2021/05/02 17:46:58 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1378,7 +1378,8 @@ int tls1_PRF(SSL *s, const unsigned char *secret, size_t secret_len, const void *seed5, size_t seed5_len, unsigned char *out, size_t out_len); void tls1_cleanup_key_block(SSL *s); -int tls1_change_cipher_state(SSL *s, int which); +int tls1_change_read_cipher_state(SSL *s); +int tls1_change_write_cipher_state(SSL *s); int tls1_setup_key_block(SSL *s); int tls1_export_keying_material(SSL *s, unsigned char *out, size_t olen, const char *label, size_t llen, const unsigned char *p, size_t plen, diff --git a/lib/libssl/ssl_pkt.c b/lib/libssl/ssl_pkt.c index 6e0cfe21029..ba59aa32371 100644 --- a/lib/libssl/ssl_pkt.c +++ b/lib/libssl/ssl_pkt.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_pkt.c,v 1.41 2021/04/25 13:15:22 jsing Exp $ */ +/* $OpenBSD: ssl_pkt.c,v 1.42 2021/05/02 17:46:58 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1154,8 +1154,6 @@ ssl3_read_bytes(SSL *s, int type, unsigned char *buf, int len, int peek) int ssl3_do_change_cipher_spec(SSL *s) { - int i; - if (S3I(s)->hs.tls12.key_block == NULL) { if (s->session == NULL || s->session->master_key_length == 0) { /* might happen if dtls1_read_bytes() calls this */ @@ -1168,12 +1166,7 @@ ssl3_do_change_cipher_spec(SSL *s) return (0); } - if (S3I(s)->hs.state & SSL_ST_ACCEPT) - i = SSL3_CHANGE_CIPHER_SERVER_READ; - else - i = SSL3_CHANGE_CIPHER_CLIENT_READ; - - if (!tls1_change_cipher_state(s, i)) + if (!tls1_change_read_cipher_state(s)) return (0); /* diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c index 8e6a1859ebd..f884ea316f9 100644 --- a/lib/libssl/ssl_srvr.c +++ b/lib/libssl/ssl_srvr.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_srvr.c,v 1.107 2021/05/02 17:28:33 jsing Exp $ */ +/* $OpenBSD: ssl_srvr.c,v 1.108 2021/05/02 17:46:58 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -647,15 +647,13 @@ ssl3_accept(SSL *s) goto end; S3I(s)->hs.state = SSL3_ST_SW_FINISHED_A; s->internal->init_num = 0; - s->session->cipher = S3I(s)->hs.cipher; + if (!tls1_setup_key_block(s)) { ret = -1; goto end; } - - if (!tls1_change_cipher_state(s, - SSL3_CHANGE_CIPHER_SERVER_WRITE)) { + if (!tls1_change_write_cipher_state(s)) { ret = -1; goto end; } diff --git a/lib/libssl/t1_enc.c b/lib/libssl/t1_enc.c index 6cdae0caedb..e3cdcc134b2 100644 --- a/lib/libssl/t1_enc.c +++ b/lib/libssl/t1_enc.c @@ -1,4 +1,4 @@ -/* $OpenBSD: t1_enc.c,v 1.141 2021/05/02 17:18:10 jsing Exp $ */ +/* $OpenBSD: t1_enc.c,v 1.142 2021/05/02 17:46:58 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -294,8 +294,8 @@ tls1_generate_key_block(SSL *s, uint8_t *key_block, size_t key_block_len) NULL, 0, NULL, 0, key_block, key_block_len); } -int -tls1_change_cipher_state(SSL *s, int which) +static int +tls1_change_cipher_state(SSL *s, int is_write) { const unsigned char *client_write_mac_secret, *server_write_mac_secret; const unsigned char *client_write_key, *server_write_key; @@ -305,26 +305,10 @@ tls1_change_cipher_state(SSL *s, int which) unsigned char *key_block; const EVP_CIPHER *cipher; const EVP_AEAD *aead; - char is_read, use_client_keys; aead = tls12_record_layer_aead(s->internal->rl); cipher = tls12_record_layer_cipher(s->internal->rl); - /* - * is_read is true if we have just read a ChangeCipherSpec message, - * that is we need to update the read cipherspec. Otherwise we have - * just written one. - */ - is_read = (which & SSL3_CC_READ) != 0; - - /* - * use_client_keys is true if we wish to use the keys for the "client - * write" direction. This is the case if we're a client sending a - * ChangeCipherSpec, or a server reading a client's ChangeCipherSpec. - */ - use_client_keys = ((which == SSL3_CHANGE_CIPHER_CLIENT_WRITE) || - (which == SSL3_CHANGE_CIPHER_SERVER_READ)); - if (aead != NULL) { key_len = EVP_AEAD_key_length(aead); iv_len = SSL_CIPHER_AEAD_FIXED_NONCE_LEN(S3I(s)->hs.cipher); @@ -349,7 +333,8 @@ tls1_change_cipher_state(SSL *s, int which) server_write_iv = key_block; key_block += iv_len; - if (use_client_keys) { + /* Use client write keys on client write and server read. */ + if ((!s->server && is_write) || (s->server && !is_write)) { mac_secret = client_write_mac_secret; key = client_write_key; iv = client_write_iv; @@ -365,7 +350,7 @@ tls1_change_cipher_state(SSL *s, int which) goto err; } - if (is_read) { + if (!is_write) { if (!tls12_record_layer_change_read_cipher_state(s->internal->rl, mac_secret, mac_secret_size, key, key_len, iv, iv_len)) goto err; @@ -386,6 +371,18 @@ tls1_change_cipher_state(SSL *s, int which) return (0); } +int +tls1_change_read_cipher_state(SSL *s) +{ + return tls1_change_cipher_state(s, 0); +} + +int +tls1_change_write_cipher_state(SSL *s) +{ + return tls1_change_cipher_state(s, 1); +} + int tls1_setup_key_block(SSL *s) { -- 2.20.1