1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
|
From 992b6872f3ca717282ae367749a47f006d337a87 Mon Sep 17 00:00:00 2001
From: Hanno Becker <hanno.becker@arm.com>
Date: Thu, 9 Nov 2017 18:57:39 +0000
Subject: [PATCH] Fix heap corruption in ssl_decrypt_buf
Previously, MAC validation for an incoming record proceeded as follows:
1) Make a copy of the MAC contained in the record;
2) Compute the expected MAC in place, overwriting the presented one;
3) Compare both.
This resulted in a record buffer overflow if truncated MAC was used, as in this
case the record buffer only reserved 10 bytes for the MAC, but the MAC
computation routine in 2) always wrote a full digest.
For specially crafted records, this could be used to perform a controlled write of
up to 6 bytes past the boundary of the heap buffer holding the record, thereby
corrupting the heap structures and potentially leading to a crash or remote code
execution.
This commit fixes this by making the following change:
1) Compute the expected MAC in a temporary buffer that has the size of the
underlying message digest.
2) Compare to this to the MAC contained in the record, potentially
restricting to the first 10 bytes if truncated HMAC is used.
A similar fix is applied to the encryption routine `ssl_encrypt_buf`.
---
library/ssl_tls.c | 36 +++++++++++++++++-------------------
tests/ssl-opt.sh | 20 ++++++++++----------
2 files changed, 27 insertions(+), 29 deletions(-)
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -1293,14 +1293,17 @@ static int ssl_encrypt_buf( mbedtls_ssl_
defined(MBEDTLS_SSL_PROTO_TLS1_2)
if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_1 )
{
+ unsigned char mac[MBEDTLS_SSL_MAC_ADD];
+
mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_ctr, 8 );
mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_hdr, 3 );
mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc, ssl->out_len, 2 );
mbedtls_md_hmac_update( &ssl->transform_out->md_ctx_enc,
ssl->out_msg, ssl->out_msglen );
- mbedtls_md_hmac_finish( &ssl->transform_out->md_ctx_enc,
- ssl->out_msg + ssl->out_msglen );
+ mbedtls_md_hmac_finish( &ssl->transform_out->md_ctx_enc, mac );
mbedtls_md_hmac_reset( &ssl->transform_out->md_ctx_enc );
+
+ memcpy( ssl->out_msg + ssl->out_msglen, mac, ssl->transform_out->maclen );
}
else
#endif
@@ -1562,8 +1565,6 @@ static int ssl_encrypt_buf( mbedtls_ssl_
return( 0 );
}
-#define SSL_MAX_MAC_SIZE 48
-
static int ssl_decrypt_buf( mbedtls_ssl_context *ssl )
{
size_t i;
@@ -1731,7 +1732,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_
#if defined(MBEDTLS_SSL_ENCRYPT_THEN_MAC)
if( ssl->session_in->encrypt_then_mac == MBEDTLS_SSL_ETM_ENABLED )
{
- unsigned char computed_mac[SSL_MAX_MAC_SIZE];
+ unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD];
unsigned char pseudo_hdr[13];
MBEDTLS_SSL_DEBUG_MSG( 3, ( "using encrypt then mac" ) );
@@ -1749,16 +1750,16 @@ static int ssl_decrypt_buf( mbedtls_ssl_
mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, pseudo_hdr, 13 );
mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec,
ssl->in_iv, ssl->in_msglen );
- mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, computed_mac );
+ mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect );
mbedtls_md_hmac_reset( &ssl->transform_in->md_ctx_dec );
MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", ssl->in_iv + ssl->in_msglen,
ssl->transform_in->maclen );
- MBEDTLS_SSL_DEBUG_BUF( 4, "computed mac", computed_mac,
+ MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect,
ssl->transform_in->maclen );
- if( mbedtls_ssl_safer_memcmp( ssl->in_iv + ssl->in_msglen, computed_mac,
- ssl->transform_in->maclen ) != 0 )
+ if( mbedtls_ssl_safer_memcmp( ssl->in_iv + ssl->in_msglen, mac_expect,
+ ssl->transform_in->maclen ) != 0 )
{
MBEDTLS_SSL_DEBUG_MSG( 1, ( "message mac does not match" ) );
@@ -1918,15 +1919,13 @@ static int ssl_decrypt_buf( mbedtls_ssl_
#if defined(SSL_SOME_MODES_USE_MAC)
if( auth_done == 0 )
{
- unsigned char tmp[SSL_MAX_MAC_SIZE];
+ unsigned char mac_expect[MBEDTLS_SSL_MAC_ADD];
ssl->in_msglen -= ssl->transform_in->maclen;
ssl->in_len[0] = (unsigned char)( ssl->in_msglen >> 8 );
ssl->in_len[1] = (unsigned char)( ssl->in_msglen );
- memcpy( tmp, ssl->in_msg + ssl->in_msglen, ssl->transform_in->maclen );
-
#if defined(MBEDTLS_SSL_PROTO_SSL3)
if( ssl->minor_ver == MBEDTLS_SSL_MINOR_VERSION_0 )
{
@@ -1965,8 +1964,7 @@ static int ssl_decrypt_buf( mbedtls_ssl_
mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_len, 2 );
mbedtls_md_hmac_update( &ssl->transform_in->md_ctx_dec, ssl->in_msg,
ssl->in_msglen );
- mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec,
- ssl->in_msg + ssl->in_msglen );
+ mbedtls_md_hmac_finish( &ssl->transform_in->md_ctx_dec, mac_expect );
/* Call mbedtls_md_process at least once due to cache attacks */
for( j = 0; j < extra_run + 1; j++ )
mbedtls_md_process( &ssl->transform_in->md_ctx_dec, ssl->in_msg );
@@ -1981,12 +1979,12 @@ static int ssl_decrypt_buf( mbedtls_ssl_
return( MBEDTLS_ERR_SSL_INTERNAL_ERROR );
}
- MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", tmp, ssl->transform_in->maclen );
- MBEDTLS_SSL_DEBUG_BUF( 4, "computed mac", ssl->in_msg + ssl->in_msglen,
- ssl->transform_in->maclen );
+ MBEDTLS_SSL_DEBUG_BUF( 4, "expected mac", mac_expect, ssl->transform_in->maclen );
+ MBEDTLS_SSL_DEBUG_BUF( 4, "message mac", ssl->in_msg + ssl->in_msglen,
+ ssl->transform_in->maclen );
- if( mbedtls_ssl_safer_memcmp( tmp, ssl->in_msg + ssl->in_msglen,
- ssl->transform_in->maclen ) != 0 )
+ if( mbedtls_ssl_safer_memcmp( ssl->in_msg + ssl->in_msglen, mac_expect,
+ ssl->transform_in->maclen ) != 0 )
{
#if defined(MBEDTLS_SSL_DEBUG_ALL)
MBEDTLS_SSL_DEBUG_MSG( 1, ( "message mac does not match" ) );
--- a/tests/ssl-opt.sh
+++ b/tests/ssl-opt.sh
@@ -705,40 +705,40 @@ run_test "Truncated HMAC: client defa
"$P_SRV debug_level=4" \
"$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA" \
0 \
- -s "dumping 'computed mac' (20 bytes)" \
- -S "dumping 'computed mac' (10 bytes)"
+ -s "dumping 'expected mac' (20 bytes)" \
+ -S "dumping 'expected mac' (10 bytes)"
run_test "Truncated HMAC: client disabled, server default" \
"$P_SRV debug_level=4" \
"$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \
trunc_hmac=0" \
0 \
- -s "dumping 'computed mac' (20 bytes)" \
- -S "dumping 'computed mac' (10 bytes)"
+ -s "dumping 'expected mac' (20 bytes)" \
+ -S "dumping 'expected mac' (10 bytes)"
run_test "Truncated HMAC: client enabled, server default" \
"$P_SRV debug_level=4" \
"$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \
trunc_hmac=1" \
0 \
- -s "dumping 'computed mac' (20 bytes)" \
- -S "dumping 'computed mac' (10 bytes)"
+ -s "dumping 'expected mac' (20 bytes)" \
+ -S "dumping 'expected mac' (10 bytes)"
run_test "Truncated HMAC: client enabled, server disabled" \
"$P_SRV debug_level=4 trunc_hmac=0" \
"$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \
trunc_hmac=1" \
0 \
- -s "dumping 'computed mac' (20 bytes)" \
- -S "dumping 'computed mac' (10 bytes)"
+ -s "dumping 'expected mac' (20 bytes)" \
+ -S "dumping 'expected mac' (10 bytes)"
run_test "Truncated HMAC: client enabled, server enabled" \
"$P_SRV debug_level=4 trunc_hmac=1" \
"$P_CLI force_ciphersuite=TLS-RSA-WITH-AES-128-CBC-SHA \
trunc_hmac=1" \
0 \
- -S "dumping 'computed mac' (20 bytes)" \
- -s "dumping 'computed mac' (10 bytes)"
+ -S "dumping 'expected mac' (20 bytes)" \
+ -s "dumping 'expected mac' (10 bytes)"
# Tests for Encrypt-then-MAC extension
|
