Skip to content

Commit 4ad9361

Browse files
committed
Don't change the state of the ETM flags until CCS processing
Changing the ciphersuite during a renegotiation can result in a crash leading to a DoS attack. ETM has not been implemented in 1.1.0 for DTLS so this is TLS only. 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>
1 parent 9c5a691 commit 4ad9361

File tree

6 files changed

+36
-19
lines changed

6 files changed

+36
-19
lines changed

Diff for: include/openssl/ssl3.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -264,11 +264,14 @@ extern "C" {
264264
# define TLS1_FLAGS_SKIP_CERT_VERIFY 0x0010
265265

266266
/* Set if we encrypt then mac instead of usual mac then encrypt */
267-
# define TLS1_FLAGS_ENCRYPT_THEN_MAC 0x0100
267+
# define TLS1_FLAGS_ENCRYPT_THEN_MAC_READ 0x0100
268+
# define TLS1_FLAGS_ENCRYPT_THEN_MAC TLS1_FLAGS_ENCRYPT_THEN_MAC_READ
268269

269270
/* Set if extended master secret extension received from peer */
270271
# define TLS1_FLAGS_RECEIVED_EXTMS 0x0200
271272

273+
# define TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE 0x0400
274+
272275
# define SSL3_MT_HELLO_REQUEST 0
273276
# define SSL3_MT_CLIENT_HELLO 1
274277
# define SSL3_MT_SERVER_HELLO 2

Diff for: ssl/record/rec_layer_s3.c

+3-3
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,7 @@ int ssl3_write_bytes(SSL *s, int type, const void *buf_, int len)
395395
if (type == SSL3_RT_APPLICATION_DATA &&
396396
u_len >= 4 * (max_send_fragment = s->max_send_fragment) &&
397397
s->compress == NULL && s->msg_callback == NULL &&
398-
!SSL_USE_ETM(s) && SSL_USE_EXPLICIT_IV(s) &&
398+
!SSL_WRITE_ETM(s) && SSL_USE_EXPLICIT_IV(s) &&
399399
EVP_CIPHER_flags(EVP_CIPHER_CTX_cipher(s->enc_write_ctx)) &
400400
EVP_CIPH_FLAG_TLS1_1_MULTIBLOCK) {
401401
unsigned char aad[13];
@@ -791,7 +791,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
791791
* wb->buf
792792
*/
793793

794-
if (!SSL_USE_ETM(s) && mac_size != 0) {
794+
if (!SSL_WRITE_ETM(s) && mac_size != 0) {
795795
if (s->method->ssl3_enc->mac(s, &wr[j],
796796
&(outbuf[j][wr[j].length + eivlen]),
797797
1) < 0)
@@ -814,7 +814,7 @@ int do_ssl3_write(SSL *s, int type, const unsigned char *buf,
814814
goto err;
815815

816816
for (j = 0; j < numpipes; j++) {
817-
if (SSL_USE_ETM(s) && mac_size != 0) {
817+
if (SSL_WRITE_ETM(s) && mac_size != 0) {
818818
if (s->method->ssl3_enc->mac(s, &wr[j],
819819
outbuf[j] + wr[j].length, 1) < 0)
820820
goto err;

Diff for: ssl/record/ssl3_record.c

+5-5
Original file line numberDiff line numberDiff line change
@@ -346,7 +346,7 @@ int ssl3_get_record(SSL *s)
346346
* If in encrypt-then-mac mode calculate mac from encrypted record. All
347347
* the details below are public so no timing details can leak.
348348
*/
349-
if (SSL_USE_ETM(s) && s->read_hash) {
349+
if (SSL_READ_ETM(s) && s->read_hash) {
350350
unsigned char *mac;
351351
mac_size = EVP_MD_CTX_size(s->read_hash);
352352
OPENSSL_assert(mac_size <= EVP_MAX_MD_SIZE);
@@ -393,7 +393,7 @@ int ssl3_get_record(SSL *s)
393393
/* r->length is now the compressed data plus mac */
394394
if ((sess != NULL) &&
395395
(s->enc_read_ctx != NULL) &&
396-
(EVP_MD_CTX_md(s->read_hash) != NULL) && !SSL_USE_ETM(s)) {
396+
(!SSL_READ_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL)) {
397397
/* s->read_hash != NULL => mac_size != -1 */
398398
unsigned char *mac = NULL;
399399
unsigned char mac_tmp[EVP_MAX_MD_SIZE];
@@ -823,7 +823,7 @@ int tls1_enc(SSL *s, SSL3_RECORD *recs, unsigned int n_recs, int send)
823823
}
824824

825825
ret = 1;
826-
if (!SSL_USE_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL)
826+
if (!SSL_READ_ETM(s) && EVP_MD_CTX_md(s->read_hash) != NULL)
827827
mac_size = EVP_MD_CTX_size(s->read_hash);
828828
if ((bs != 1) && !send) {
829829
int tmpret;
@@ -997,7 +997,7 @@ int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int send)
997997
header[11] = (rec->length) >> 8;
998998
header[12] = (rec->length) & 0xff;
999999

1000-
if (!send && !SSL_USE_ETM(ssl) &&
1000+
if (!send && !SSL_READ_ETM(ssl) &&
10011001
EVP_CIPHER_CTX_mode(ssl->enc_read_ctx) == EVP_CIPH_CBC_MODE &&
10021002
ssl3_cbc_record_digest_supported(mac_ctx)) {
10031003
/*
@@ -1022,7 +1022,7 @@ int tls1_mac(SSL *ssl, SSL3_RECORD *rec, unsigned char *md, int send)
10221022
EVP_MD_CTX_free(hmac);
10231023
return -1;
10241024
}
1025-
if (!send && !SSL_USE_ETM(ssl) && FIPS_mode())
1025+
if (!send && !SSL_READ_ETM(ssl) && FIPS_mode())
10261026
if (!tls_fips_digest_extra(ssl->enc_read_ctx,
10271027
mac_ctx, rec->input,
10281028
rec->length, rec->orig_len)) {

Diff for: ssl/ssl_locl.h

+6-1
Original file line numberDiff line numberDiff line change
@@ -378,7 +378,8 @@
378378
# define SSL_CLIENT_USE_SIGALGS(s) \
379379
SSL_CLIENT_USE_TLS1_2_CIPHERS(s)
380380

381-
# define SSL_USE_ETM(s) (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC)
381+
# define SSL_READ_ETM(s) (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC_READ)
382+
# define SSL_WRITE_ETM(s) (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE)
382383

383384
/* Mostly for SSLv3 */
384385
# define SSL_PKEY_RSA_ENC 0
@@ -1110,6 +1111,10 @@ struct ssl_st {
11101111
*/
11111112
unsigned char *alpn_client_proto_list;
11121113
unsigned alpn_client_proto_list_len;
1114+
1115+
/* Set to one if we have negotiated ETM */
1116+
int tlsext_use_etm;
1117+
11131118
/*-
11141119
* 1 if we are renegotiating.
11151120
* 2 if we are a server and are inside a handshake

Diff for: ssl/t1_enc.c

+12-3
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,11 @@ int tls1_change_cipher_state(SSL *s, int which)
130130
#endif
131131

132132
if (which & SSL3_CC_READ) {
133+
if (s->tlsext_use_etm)
134+
s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC_READ;
135+
else
136+
s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC_READ;
137+
133138
if (s->s3->tmp.new_cipher->algorithm2 & TLS1_STREAM_MAC)
134139
s->mac_flags |= SSL_MAC_FLAG_READ_MAC_STREAM;
135140
else
@@ -168,6 +173,11 @@ int tls1_change_cipher_state(SSL *s, int which)
168173
mac_secret = &(s->s3->read_mac_secret[0]);
169174
mac_secret_size = &(s->s3->read_mac_secret_size);
170175
} else {
176+
if (s->tlsext_use_etm)
177+
s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE;
178+
else
179+
s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC_WRITE;
180+
171181
if (s->s3->tmp.new_cipher->algorithm2 & TLS1_STREAM_MAC)
172182
s->mac_flags |= SSL_MAC_FLAG_WRITE_MAC_STREAM;
173183
else
@@ -367,9 +377,8 @@ int tls1_setup_key_block(SSL *s)
367377
if (s->s3->tmp.key_block_length != 0)
368378
return (1);
369379

370-
if (!ssl_cipher_get_evp
371-
(s->session, &c, &hash, &mac_type, &mac_secret_size, &comp,
372-
SSL_USE_ETM(s))) {
380+
if (!ssl_cipher_get_evp(s->session, &c, &hash, &mac_type, &mac_secret_size,
381+
&comp, s->tlsext_use_etm)) {
373382
SSLerr(SSL_F_TLS1_SETUP_KEY_BLOCK, SSL_R_CIPHER_OR_HASH_UNAVAILABLE);
374383
return (0);
375384
}

Diff for: ssl/t1_lib.c

+6-6
Original file line numberDiff line numberDiff line change
@@ -1674,7 +1674,7 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
16741674
#endif
16751675
if (!custom_ext_add(s, 1, &ret, limit, al))
16761676
return NULL;
1677-
if (s->s3->flags & TLS1_FLAGS_ENCRYPT_THEN_MAC) {
1677+
if (s->tlsext_use_etm) {
16781678
/*
16791679
* Don't use encrypt_then_mac if AEAD or RC4 might want to disable
16801680
* for other cases too.
@@ -1683,7 +1683,7 @@ unsigned char *ssl_add_serverhello_tlsext(SSL *s, unsigned char *buf,
16831683
|| s->s3->tmp.new_cipher->algorithm_enc == SSL_RC4
16841684
|| s->s3->tmp.new_cipher->algorithm_enc == SSL_eGOST2814789CNT
16851685
|| s->s3->tmp.new_cipher->algorithm_enc == SSL_eGOST2814789CNT12)
1686-
s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
1686+
s->tlsext_use_etm = 0;
16871687
else {
16881688
/*-
16891689
* check for enough space.
@@ -1916,7 +1916,7 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
19161916
/* Clear any signature algorithms extension received */
19171917
OPENSSL_free(s->s3->tmp.peer_sigalgs);
19181918
s->s3->tmp.peer_sigalgs = NULL;
1919-
s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
1919+
s->tlsext_use_etm = 0;
19201920

19211921
#ifndef OPENSSL_NO_SRP
19221922
OPENSSL_free(s->srp_ctx.login);
@@ -2264,7 +2264,7 @@ static int ssl_scan_clienthello_tlsext(SSL *s, PACKET *pkt, int *al)
22642264
}
22652265
#endif
22662266
else if (type == TLSEXT_TYPE_encrypt_then_mac)
2267-
s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC;
2267+
s->tlsext_use_etm = 1;
22682268
/*
22692269
* Note: extended master secret extension handled in
22702270
* tls_check_serverhello_tlsext_early()
@@ -2366,7 +2366,7 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al)
23662366
SSL_DTLSEXT_HB_DONT_SEND_REQUESTS);
23672367
#endif
23682368

2369-
s->s3->flags &= ~TLS1_FLAGS_ENCRYPT_THEN_MAC;
2369+
s->tlsext_use_etm = 0;
23702370

23712371
s->s3->flags &= ~TLS1_FLAGS_RECEIVED_EXTMS;
23722372

@@ -2585,7 +2585,7 @@ static int ssl_scan_serverhello_tlsext(SSL *s, PACKET *pkt, int *al)
25852585
/* Ignore if inappropriate ciphersuite */
25862586
if (s->s3->tmp.new_cipher->algorithm_mac != SSL_AEAD
25872587
&& s->s3->tmp.new_cipher->algorithm_enc != SSL_RC4)
2588-
s->s3->flags |= TLS1_FLAGS_ENCRYPT_THEN_MAC;
2588+
s->tlsext_use_etm = 1;
25892589
} else if (type == TLSEXT_TYPE_extended_master_secret) {
25902590
s->s3->flags |= TLS1_FLAGS_RECEIVED_EXTMS;
25912591
if (!s->hit)

0 commit comments

Comments
 (0)