wbrawner/openssl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Don't change the state of the ETM flags until CCS processing

Browse source 
In 1.1.0 changing the ciphersuite during a renegotiation can result in
a crash leading to a DoS attack. In master this does not occur with TLS
(instead you get an internal error, which is still wrong but not a security
issue) - but the problem still exists in the DTLS code.

The problem is caused by changing the flag indicating whether to use ETM
or not immediately on negotiation of ETM, rather than at CCS. Therefore,
during a renegotiation, if the ETM state is changing (usually due to a
change of ciphersuite), then an error/crash will occur.

Due to the fact that there are separate CCS messages for read and write
we actually now need two flags to determine whether to use ETM or not.

CVE-2017-3733

Reviewed-by: Richard Levitte <levitte@openssl.org>
This commit is contained in:
Matt Caswell 2017-02-03 14:06:20 +00:00
parent cc22cd546b
commit 28a31a0a10
11 changed files with 42 additions and 26 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
include/openssl/ssl3.h
View file

@ -265,11 +265,14 @@ extern "C" {
# define TLS1_FLAGS_SKIP_CERT_VERIFY 0x0010
/* Set if we encrypt then mac instead of usual mac then encrypt */
# define TLS1_FLAGS_ENCRYPT_THEN_MAC 0x0100
# define TLS1_FLAGS_ENCRYPT_THEN_MAC_READ 0x0100
# define TLS1_FLAGS_ENCRYPT_THEN_MAC TLS1_FLAGS_ENCRYPT_THEN_MAC_READ
/* Set if extended master secret extension received from peer */
# define TLS1_FLAGS_RECEIVED_EXTMS 0x0200
# define TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE 0x0400
# define SSL3_MT_HELLO_REQUEST 0
# define SSL3_MT_CLIENT_HELLO 1
# define SSL3_MT_SERVER_HELLO 2

2
ssl/d1_lib.c
View file

@ -937,7 +937,7 @@ size_t DTLS_get_data_mtu(const SSL *s)
&blocksize, &ext_overhead))
return 0;
if (SSL_USE_ETM(s))
if (SSL_READ_ETM(s))
ext_overhead += mac_overhead;
else
int_overhead += mac_overhead;

4
ssl/record/rec_layer_d1.c
View file

@ -1029,7 +1029,7 @@ int do_dtls1_write(SSL *s, int type, const unsigned char *buf,
* wb->buf
*/
if (!SSL_USE_ETM(s) && mac_size != 0) {
if (!SSL_WRITE_ETM(s) && mac_size != 0) {
if (!s->method->ssl3_enc->mac(s, &wr,
&(p[SSL3_RECORD_get_length(&wr) + eivlen]),
1))
@ -1047,7 +1047,7 @@ int do_dtls1_write(SSL *s, int type, const unsigned char *buf,
if (s->method->ssl3_enc->enc(s, &wr, 1, 1) < 1)
goto err;
if (SSL_USE_ETM(s) && mac_size != 0) {
if (SSL_WRITE_ETM(s) && mac_size != 0) {
if (!s->method->ssl3_enc->mac(s, &wr,
&(p[SSL3_RECORD_get_length(&wr)]), 1))
goto err;

6
ssl/record/rec_layer_s3.c
View file

@ -401,7 +401,7 @@ int ssl3_write_bytes(SSL *s, int type, const void *buf_, size_t len,
if (type == SSL3_RT_APPLICATION_DATA &&
len >= 4 * (max_send_fragment = s->max_send_fragment) &&
s->compress == NULL && s->msg_callback == NULL &&
!SSL_USE_ETM(s) && SSL_USE_EXPLICIT_IV(s) &&
!SSL_WRITE_ETM(s) && SSL_USE_EXPLICIT_IV(s) &&
EVP_CIPHER_flags(EVP_CIPHER_CTX_cipher(s->enc_write_ctx)) &
EVP_CIPH_FLAG_TLS1_1_MULTIBLOCK) {
unsigned char aad[13];
@ -870,7 +870,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
* in the wb->buf
*/
if (!SSL_USE_ETM(s) && mac_size != 0) {
if (!SSL_WRITE_ETM(s) && mac_size != 0) {
unsigned char *mac;
if (!WPACKET_allocate_bytes(thispkt, mac_size, &mac)
@ -923,7 +923,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
SSLerr(SSL_F_DO_SSL3_WRITE, ERR_R_INTERNAL_ERROR);
goto err;
}
if (SSL_USE_ETM(s) && mac_size != 0) {
if (SSL_WRITE_ETM(s) && mac_size != 0) {
unsigned char *mac;
if (!WPACKET_allocate_bytes(thispkt, mac_size, &mac)

14
ssl/record/ssl3_record.c
View file

@ -383,7 +383,7 @@ int ssl3_get_record(SSL *s)
* If in encrypt-then-mac mode calculate mac from encrypted record. All
* the details below are public so no timing details can leak.
*/
if (SSL_USE_ETM(s) && s->read_hash) {
if (SSL_READ_ETM(s) && s->read_hash) {
unsigned char *mac;
/* TODO(size_t): convert this to do size_t properly */
imac_size = EVP_MD_CTX_size(s->read_hash);
@ -440,7 +440,7 @@ int ssl3_get_record(SSL *s)
/* r->length is now the compressed data plus mac */
if ((sess != NULL) &&
(s->enc_read_ctx != NULL) &&
(EVP_MD_CTX_md(s->read_hash) != NULL) && !SSL_USE_ETM(s)) {
(!SSL_READ_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL)) {
/* s->read_hash != NULL => mac_size != -1 */
unsigned char *mac = NULL;
unsigned char mac_tmp[EVP_MAX_MD_SIZE];
@ -915,7 +915,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, size_t n_recs, int send)
}
ret = 1;
if (!SSL_USE_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL) {
if (!SSL_READ_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL) {
imac_size = EVP_MD_CTX_size(s->read_hash);
if (imac_size < 0)
return -1;
@ -1092,7 +1092,7 @@ int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int send)
header[11] = (unsigned char)(rec->length >> 8);
header[12] = (unsigned char)(rec->length & 0xff);
if (!send && !SSL_USE_ETM(ssl) &&
if (!send && !SSL_READ_ETM(ssl) &&
EVP_CIPHER_CTX_mode(ssl->enc_read_ctx) == EVP_CIPH_CBC_MODE &&
ssl3_cbc_record_digest_supported(mac_ctx)) {
/*
@ -1118,7 +1118,7 @@ int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int send)
EVP_MD_CTX_free(hmac);
return 0;
}
if (!send && !SSL_USE_ETM(ssl) && FIPS_mode())
if (!send && !SSL_READ_ETM(ssl) && FIPS_mode())
if (!tls_fips_digest_extra(ssl->enc_read_ctx,
mac_ctx, rec->input,
rec->length, rec->orig_len)) {
@ -1408,7 +1408,7 @@ int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap)
rr->data = rr->input;
rr->orig_len = rr->length;
if (SSL_USE_ETM(s) && s->read_hash) {
if (SSL_READ_ETM(s) && s->read_hash) {
unsigned char *mac;
mac_size = EVP_MD_CTX_size(s->read_hash);
OPENSSL_assert(mac_size <= EVP_MAX_MD_SIZE);
@ -1452,7 +1452,7 @@ int dtls1_process_record(SSL *s, DTLS1_BITMAP *bitmap)
#endif
/* r->length is now the compressed data plus mac */
if ((sess != NULL) && !SSL_USE_ETM(s) &&
if ((sess != NULL) && !SSL_READ_ETM(s) &&
(s->enc_read_ctx != NULL) && (EVP_MD_CTX_md(s->read_hash) != NULL)) {
/* s->read_hash != NULL => mac_size != -1 */
unsigned char *mac = NULL;

6
ssl/ssl_locl.h
View file

@ -395,7 +395,8 @@
# define SSL_CLIENT_USE_SIGALGS(s) \
SSL_CLIENT_USE_TLS1_2_CIPHERS(s)
# define SSL_USE_ETM(s) (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC)
# define SSL_READ_ETM(s) (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC_READ)
# define SSL_WRITE_ETM(s) (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE)
/* Mostly for SSLv3 */
# define SSL_PKEY_RSA 0
@ -1132,6 +1133,9 @@ struct ssl_st {
/* The available PSK key exchange modes */
int psk_kex_mode;
/* Set to one if we have negotiated ETM */
int use_etm;
} ext;
/*-

4
ssl/statem/extensions.c
View file

@ -207,7 +207,7 @@ static const EXTENSION_DEFINITION ext_defs[] = {
#endif
{
TLSEXT_TYPE_encrypt_then_mac,
EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_2_AND_BELOW_ONLY,
EXT_CLIENT_HELLO | EXT_TLS1_2_SERVER_HELLO | EXT_TLS1_2_AND_BELOW_ONLY | EXT_SSL3_ALLOWED,
init_etm, tls_parse_ctos_etm, tls_parse_stoc_etm,
tls_construct_stoc_etm, tls_construct_ctos_etm, NULL
},
@ -912,7 +912,7 @@ static int init_srp(SSL *s, unsigned int context)
static int init_etm(SSL *s, unsigned int context)
{
s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
s->ext.use_etm = 0;
return 1;
}

2
ssl/statem/extensions_clnt.c
View file

@ -1172,7 +1172,7 @@ int tls_parse_stoc_etm(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
if (!(s->options & SSL_OP_NO_ENCRYPT_THEN_MAC)
&& s->s3->tmp.new_cipher->algorithm_mac != SSL_AEAD
&& s->s3->tmp.new_cipher->algorithm_enc != SSL_RC4)
s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC;
s->ext.use_etm = 1;
return 1;
}

6
ssl/statem/extensions_srvr.c
View file

@ -451,7 +451,7 @@ int tls_parse_ctos_etm(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
size_t chainidx, int *al)
{
if (!(s->options & SSL_OP_NO_ENCRYPT_THEN_MAC))
s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC;
s->ext.use_etm = 1;
return 1;
}
@ -953,7 +953,7 @@ int tls_construct_stoc_use_srtp(SSL *s, WPACKET *pkt, unsigned int context,
int tls_construct_stoc_etm(SSL *s, WPACKET *pkt, unsigned int context, X509 *x,
size_t chainidx, int *al)
{
if ((s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC) == 0)
if (!s->ext.use_etm)
return 1;
/*
@ -964,7 +964,7 @@ int tls_construct_stoc_etm(SSL *s, WPACKET *pkt, unsigned int context, X509 *x,
|| s->s3->tmp.new_cipher->algorithm_enc == SSL_RC4
|| s->s3->tmp.new_cipher->algorithm_enc == SSL_eGOST2814789CNT
|| s->s3->tmp.new_cipher->algorithm_enc == SSL_eGOST2814789CNT12) {
s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
s->ext.use_etm = 0;
return 1;
}

15
ssl/t1_enc.c
View file

@ -129,6 +129,11 @@ int tls1_change_cipher_state(SSL *s, int which)
#endif
if (which & SSL3_CC_READ) {
if (s->ext.use_etm)
s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC_READ;
else
s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC_READ;
if (s->s3->tmp.new_cipher->algorithm2 & TLS1_STREAM_MAC)
s->mac_flags |= SSL_MAC_FLAG_READ_MAC_STREAM;
else
@ -167,6 +172,11 @@ int tls1_change_cipher_state(SSL *s, int which)
mac_secret = &(s->s3->read_mac_secret[0]);
mac_secret_size = &(s->s3->read_mac_secret_size);
} else {
if (s->ext.use_etm)
s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE;
else
s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE;
if (s->s3->tmp.new_cipher->algorithm2 & TLS1_STREAM_MAC)
s->mac_flags |= SSL_MAC_FLAG_WRITE_MAC_STREAM;
else
@ -369,9 +379,8 @@ int tls1_setup_key_block(SSL *s)
if (s->s3->tmp.key_block_length != 0)
return (1);
if (!ssl_cipher_get_evp
(s->session, &c, &hash, &mac_type, &mac_secret_size, &comp,
SSL_USE_ETM(s))) {
if (!ssl_cipher_get_evp(s->session, &c, &hash, &mac_type, &mac_secret_size,
&comp, s->ext.use_etm)) {
SSLerr(SSL_F_TLS1_SETUP_KEY_BLOCK, SSL_R_CIPHER_OR_HASH_UNAVAILABLE);
return (0);
}

4
test/dtls_mtu_test.c
View file

@ -16,7 +16,7 @@
#include "ssltestlib.h"
/* for SSL_USE_ETM() */
/* for SSL_READ_ETM() */
#include "../ssl/ssl_locl.h"
static int debug = 0;
@ -133,7 +133,7 @@ static int mtu_test(SSL_CTX *ctx, const char *cs, int no_etm)
}
}
rv = 1;
if (SSL_USE_ETM(clnt_ssl))
if (SSL_READ_ETM(clnt_ssl))
rv = 2;
out:
SSL_free(clnt_ssl);