Skip to content

Commit f71036e

Browse files
davidbenCQ bot account: commit-bot@chromium.org
davidben
authored and
CQ bot account: [email protected]
committed
Remove ssl_hash_message_t from ssl_get_message.
Move to explicit hashing everywhere, matching TLS 1.2 with TLS 1.3. The ssl_get_message calls between all the handshake states are now all uniform so, when we're ready, we can rewire the TLS 1.2 state machine to look like the TLS 1.3 one. (ssl_get_message calls become an ssl_hs_read_message transition, reuse_message becomes an ssl_hs_ok transition.) This avoids some nuisance in processing the ServerHello at the 1.2 / 1.3 transition. The downside of explicit hashing is we may forget to hash something, but this will fail to interop with our tests and anyone else, so we should be able to catch it. BUG=128 Change-Id: I01393943b14dfaa98eec2a78f62c3a41c29b3a0e Reviewed-on: https://boringssl-review.googlesource.com/13266 Reviewed-by: David Benjamin <[email protected]> Commit-Queue: David Benjamin <[email protected]> CQ-Verified: CQ bot account: [email protected] <[email protected]>
1 parent 1a444da commit f71036e

9 files changed

+69
-70
lines changed

‎ssl/d1_both.c

+2-11
Original file line numberDiff line numberDiff line change
@@ -395,16 +395,11 @@ static int dtls1_process_handshake_record(SSL *ssl) {
395395
return 1;
396396
}
397397

398-
int dtls1_get_message(SSL *ssl, enum ssl_hash_message_t hash_message) {
398+
int dtls1_get_message(SSL *ssl) {
399399
if (ssl->s3->tmp.reuse_message) {
400-
/* A ssl_dont_hash_message call cannot be combined with reuse_message; the
401-
* ssl_dont_hash_message would have to have been applied to the previous
402-
* call. */
403-
assert(hash_message == ssl_hash_message);
400+
/* There must be a current message. */
404401
assert(ssl->init_msg != NULL);
405-
406402
ssl->s3->tmp.reuse_message = 0;
407-
hash_message = ssl_dont_hash_message;
408403
} else {
409404
dtls1_release_current_message(ssl, 0 /* don't free buffer */);
410405
}
@@ -429,10 +424,6 @@ int dtls1_get_message(SSL *ssl, enum ssl_hash_message_t hash_message) {
429424
ssl->init_msg = frag->data + DTLS1_HM_HEADER_LENGTH;
430425
ssl->init_num = frag->msg_len;
431426

432-
if (hash_message == ssl_hash_message && !ssl_hash_current_message(ssl)) {
433-
return -1;
434-
}
435-
436427
ssl_do_msg_callback(ssl, 0 /* read */, SSL3_RT_HANDSHAKE, frag->data,
437428
ssl->init_num + DTLS1_HM_HEADER_LENGTH);
438429
return 1;

‎ssl/handshake_client.c

+31-15
Original file line numberDiff line numberDiff line change
@@ -808,7 +808,7 @@ static int dtls1_get_hello_verify(SSL_HANDSHAKE *hs) {
808808
CBS hello_verify_request, cookie;
809809
uint16_t server_version;
810810

811-
int ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
811+
int ret = ssl->method->ssl_get_message(ssl);
812812
if (ret <= 0) {
813813
return ret;
814814
}
@@ -819,8 +819,10 @@ static int dtls1_get_hello_verify(SSL_HANDSHAKE *hs) {
819819
return 1;
820820
}
821821

822-
CBS_init(&hello_verify_request, ssl->init_msg, ssl->init_num);
822+
/* The handshake transcript is reset on HelloVerifyRequst, so do not bother
823+
* hashing it. */
823824

825+
CBS_init(&hello_verify_request, ssl->init_msg, ssl->init_num);
824826
if (!CBS_get_u16(&hello_verify_request, &server_version) ||
825827
!CBS_get_u8_length_prefixed(&hello_verify_request, &cookie) ||
826828
CBS_len(&hello_verify_request) != 0) {
@@ -852,7 +854,7 @@ static int ssl3_get_server_hello(SSL_HANDSHAKE *hs) {
852854
uint16_t server_wire_version, cipher_suite;
853855
uint8_t compression_method;
854856

855-
int ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
857+
int ret = ssl->method->ssl_get_message(ssl);
856858
if (ret <= 0) {
857859
uint32_t err = ERR_peek_error();
858860
if (ERR_GET_LIB(err) == ERR_LIB_SSL &&
@@ -995,8 +997,10 @@ static int ssl3_get_server_hello(SSL_HANDSHAKE *hs) {
995997
}
996998
ssl->s3->tmp.new_cipher = c;
997999

998-
/* Now that the cipher is known, initialize the handshake hash. */
999-
if (!ssl3_init_handshake_hash(ssl)) {
1000+
/* Now that the cipher is known, initialize the handshake hash and hash the
1001+
* ServerHello. */
1002+
if (!ssl3_init_handshake_hash(ssl) ||
1003+
!ssl_hash_current_message(ssl)) {
10001004
goto f_err;
10011005
}
10021006

@@ -1051,12 +1055,13 @@ static int ssl3_get_server_hello(SSL_HANDSHAKE *hs) {
10511055

10521056
static int ssl3_get_server_certificate(SSL_HANDSHAKE *hs) {
10531057
SSL *const ssl = hs->ssl;
1054-
int ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
1058+
int ret = ssl->method->ssl_get_message(ssl);
10551059
if (ret <= 0) {
10561060
return ret;
10571061
}
10581062

1059-
if (!ssl_check_message_type(ssl, SSL3_MT_CERTIFICATE)) {
1063+
if (!ssl_check_message_type(ssl, SSL3_MT_CERTIFICATE) ||
1064+
!ssl_hash_current_message(ssl)) {
10601065
return -1;
10611066
}
10621067

@@ -1098,7 +1103,7 @@ static int ssl3_get_cert_status(SSL_HANDSHAKE *hs) {
10981103
CBS certificate_status, ocsp_response;
10991104
uint8_t status_type;
11001105

1101-
int ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
1106+
int ret = ssl->method->ssl_get_message(ssl);
11021107
if (ret <= 0) {
11031108
return ret;
11041109
}
@@ -1110,6 +1115,10 @@ static int ssl3_get_cert_status(SSL_HANDSHAKE *hs) {
11101115
return 1;
11111116
}
11121117

1118+
if (!ssl_hash_current_message(ssl)) {
1119+
return -1;
1120+
}
1121+
11131122
CBS_init(&certificate_status, ssl->init_msg, ssl->init_num);
11141123
if (!CBS_get_u8(&certificate_status, &status_type) ||
11151124
status_type != TLSEXT_STATUSTYPE_ocsp ||
@@ -1151,7 +1160,7 @@ static int ssl3_get_server_key_exchange(SSL_HANDSHAKE *hs) {
11511160
EC_KEY *ecdh = NULL;
11521161
EC_POINT *srvr_ecpoint = NULL;
11531162

1154-
int ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
1163+
int ret = ssl->method->ssl_get_message(ssl);
11551164
if (ret <= 0) {
11561165
return ret;
11571166
}
@@ -1168,6 +1177,10 @@ static int ssl3_get_server_key_exchange(SSL_HANDSHAKE *hs) {
11681177
return 1;
11691178
}
11701179

1180+
if (!ssl_hash_current_message(ssl)) {
1181+
return -1;
1182+
}
1183+
11711184
/* Retain a copy of the original CBS to compute the signature over. */
11721185
CBS server_key_exchange;
11731186
CBS_init(&server_key_exchange, ssl->init_msg, ssl->init_num);
@@ -1381,7 +1394,7 @@ static int ssl3_get_server_key_exchange(SSL_HANDSHAKE *hs) {
13811394

13821395
static int ssl3_get_certificate_request(SSL_HANDSHAKE *hs) {
13831396
SSL *const ssl = hs->ssl;
1384-
int msg_ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
1397+
int msg_ret = ssl->method->ssl_get_message(ssl);
13851398
if (msg_ret <= 0) {
13861399
return msg_ret;
13871400
}
@@ -1394,7 +1407,8 @@ static int ssl3_get_certificate_request(SSL_HANDSHAKE *hs) {
13941407
return 1;
13951408
}
13961409

1397-
if (!ssl_check_message_type(ssl, SSL3_MT_CERTIFICATE_REQUEST)) {
1410+
if (!ssl_check_message_type(ssl, SSL3_MT_CERTIFICATE_REQUEST) ||
1411+
!ssl_hash_current_message(ssl)) {
13981412
return -1;
13991413
}
14001414

@@ -1447,12 +1461,13 @@ static int ssl3_get_certificate_request(SSL_HANDSHAKE *hs) {
14471461

14481462
static int ssl3_get_server_hello_done(SSL_HANDSHAKE *hs) {
14491463
SSL *const ssl = hs->ssl;
1450-
int ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
1464+
int ret = ssl->method->ssl_get_message(ssl);
14511465
if (ret <= 0) {
14521466
return ret;
14531467
}
14541468

1455-
if (!ssl_check_message_type(ssl, SSL3_MT_SERVER_HELLO_DONE)) {
1469+
if (!ssl_check_message_type(ssl, SSL3_MT_SERVER_HELLO_DONE) ||
1470+
!ssl_hash_current_message(ssl)) {
14561471
return -1;
14571472
}
14581473

@@ -1831,12 +1846,13 @@ static int ssl3_send_channel_id(SSL_HANDSHAKE *hs) {
18311846

18321847
static int ssl3_get_new_session_ticket(SSL_HANDSHAKE *hs) {
18331848
SSL *const ssl = hs->ssl;
1834-
int ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
1849+
int ret = ssl->method->ssl_get_message(ssl);
18351850
if (ret <= 0) {
18361851
return ret;
18371852
}
18381853

1839-
if (!ssl_check_message_type(ssl, SSL3_MT_NEW_SESSION_TICKET)) {
1854+
if (!ssl_check_message_type(ssl, SSL3_MT_NEW_SESSION_TICKET) ||
1855+
!ssl_hash_current_message(ssl)) {
18401856
return -1;
18411857
}
18421858

‎ssl/handshake_server.c

+18-11
Original file line numberDiff line numberDiff line change
@@ -815,7 +815,7 @@ static int ssl3_get_client_hello(SSL_HANDSHAKE *hs) {
815815

816816
if (hs->state == SSL3_ST_SR_CLNT_HELLO_A) {
817817
/* The first time around, read the ClientHello. */
818-
int msg_ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
818+
int msg_ret = ssl->method->ssl_get_message(ssl);
819819
if (msg_ret <= 0) {
820820
return msg_ret;
821821
}
@@ -1026,8 +1026,10 @@ static int ssl3_get_client_hello(SSL_HANDSHAKE *hs) {
10261026
goto f_err;
10271027
}
10281028

1029-
/* Now that all parameters are known, initialize the handshake hash. */
1030-
if (!ssl3_init_handshake_hash(ssl)) {
1029+
/* Now that all parameters are known, initialize the handshake hash and hash
1030+
* the ClientHello. */
1031+
if (!ssl3_init_handshake_hash(ssl) ||
1032+
!ssl_hash_current_message(ssl)) {
10311033
goto f_err;
10321034
}
10331035

@@ -1402,7 +1404,7 @@ static int ssl3_get_client_certificate(SSL_HANDSHAKE *hs) {
14021404
SSL *const ssl = hs->ssl;
14031405
assert(hs->cert_request);
14041406

1405-
int msg_ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
1407+
int msg_ret = ssl->method->ssl_get_message(ssl);
14061408
if (msg_ret <= 0) {
14071409
return msg_ret;
14081410
}
@@ -1430,6 +1432,10 @@ static int ssl3_get_client_certificate(SSL_HANDSHAKE *hs) {
14301432
return -1;
14311433
}
14321434

1435+
if (!ssl_hash_current_message(ssl)) {
1436+
return -1;
1437+
}
1438+
14331439
CBS certificate_msg;
14341440
CBS_init(&certificate_msg, ssl->init_msg, ssl->init_num);
14351441

@@ -1506,12 +1512,13 @@ static int ssl3_get_client_key_exchange(SSL_HANDSHAKE *hs) {
15061512
uint8_t psk[PSK_MAX_PSK_LEN];
15071513

15081514
if (hs->state == SSL3_ST_SR_KEY_EXCH_A) {
1509-
int ret = ssl->method->ssl_get_message(ssl, ssl_hash_message);
1515+
int ret = ssl->method->ssl_get_message(ssl);
15101516
if (ret <= 0) {
15111517
return ret;
15121518
}
15131519

1514-
if (!ssl_check_message_type(ssl, SSL3_MT_CLIENT_KEY_EXCHANGE)) {
1520+
if (!ssl_check_message_type(ssl, SSL3_MT_CLIENT_KEY_EXCHANGE) ||
1521+
!ssl_hash_current_message(ssl)) {
15151522
return -1;
15161523
}
15171524
}
@@ -1777,7 +1784,7 @@ static int ssl3_get_cert_verify(SSL_HANDSHAKE *hs) {
17771784
return 1;
17781785
}
17791786

1780-
int msg_ret = ssl->method->ssl_get_message(ssl, ssl_dont_hash_message);
1787+
int msg_ret = ssl->method->ssl_get_message(ssl);
17811788
if (msg_ret <= 0) {
17821789
return msg_ret;
17831790
}
@@ -1873,13 +1880,13 @@ static int ssl3_get_cert_verify(SSL_HANDSHAKE *hs) {
18731880
* sets the next_proto member in s if found */
18741881
static int ssl3_get_next_proto(SSL_HANDSHAKE *hs) {
18751882
SSL *const ssl = hs->ssl;
1876-
int ret =
1877-
ssl->method->ssl_get_message(ssl, ssl_hash_message);
1883+
int ret = ssl->method->ssl_get_message(ssl);
18781884
if (ret <= 0) {
18791885
return ret;
18801886
}
18811887

1882-
if (!ssl_check_message_type(ssl, SSL3_MT_NEXT_PROTO)) {
1888+
if (!ssl_check_message_type(ssl, SSL3_MT_NEXT_PROTO) ||
1889+
!ssl_hash_current_message(ssl)) {
18831890
return -1;
18841891
}
18851892

@@ -1904,7 +1911,7 @@ static int ssl3_get_next_proto(SSL_HANDSHAKE *hs) {
19041911
/* ssl3_get_channel_id reads and verifies a ClientID handshake message. */
19051912
static int ssl3_get_channel_id(SSL_HANDSHAKE *hs) {
19061913
SSL *const ssl = hs->ssl;
1907-
int msg_ret = ssl->method->ssl_get_message(ssl, ssl_dont_hash_message);
1914+
int msg_ret = ssl->method->ssl_get_message(ssl);
19081915
if (msg_ret <= 0) {
19091916
return msg_ret;
19101917
}

‎ssl/internal.h

+3-8
Original file line numberDiff line numberDiff line change
@@ -1224,11 +1224,6 @@ int tls12_check_peer_sigalg(SSL *ssl, int *out_alert, uint16_t sigalg);
12241224
/* From RFC4492, used in encoding the curve type in ECParameters */
12251225
#define NAMED_CURVE_TYPE 3
12261226

1227-
enum ssl_hash_message_t {
1228-
ssl_dont_hash_message,
1229-
ssl_hash_message,
1230-
};
1231-
12321227
typedef struct cert_st {
12331228
EVP_PKEY *privatekey;
12341229

@@ -1307,7 +1302,7 @@ struct ssl_protocol_method_st {
13071302
/* ssl_get_message reads the next handshake message. On success, it returns
13081303
* one and sets |ssl->s3->tmp.message_type|, |ssl->init_msg|, and
13091304
* |ssl->init_num|. Otherwise, it returns <= 0. */
1310-
int (*ssl_get_message)(SSL *ssl, enum ssl_hash_message_t hash_message);
1305+
int (*ssl_get_message)(SSL *ssl);
13111306
/* get_current_message sets |*out| to the current handshake message. This
13121307
* includes the protocol-specific message header. */
13131308
void (*get_current_message)(const SSL *ssl, CBS *out);
@@ -1768,7 +1763,7 @@ int ssl_verify_alarm_type(long type);
17681763

17691764
int ssl3_get_finished(SSL_HANDSHAKE *hs);
17701765
int ssl3_send_alert(SSL *ssl, int level, int desc);
1771-
int ssl3_get_message(SSL *ssl, enum ssl_hash_message_t hash_message);
1766+
int ssl3_get_message(SSL *ssl);
17721767
void ssl3_get_current_message(const SSL *ssl, CBS *out);
17731768
void ssl3_release_current_message(SSL *ssl, int free_buffer);
17741769

@@ -1854,7 +1849,7 @@ int dtls1_accept(SSL *ssl);
18541849
int dtls1_connect(SSL *ssl);
18551850
void dtls1_free(SSL *ssl);
18561851

1857-
int dtls1_get_message(SSL *ssl, enum ssl_hash_message_t hash_message);
1852+
int dtls1_get_message(SSL *ssl);
18581853
void dtls1_get_current_message(const SSL *ssl, CBS *out);
18591854
void dtls1_release_current_message(SSL *ssl, int free_buffer);
18601855
int dtls1_dispatch_alert(SSL *ssl);

‎ssl/s3_both.c

+3-13
Original file line numberDiff line numberDiff line change
@@ -398,7 +398,7 @@ int ssl3_send_finished(SSL_HANDSHAKE *hs) {
398398

399399
int ssl3_get_finished(SSL_HANDSHAKE *hs) {
400400
SSL *const ssl = hs->ssl;
401-
int ret = ssl->method->ssl_get_message(ssl, ssl_dont_hash_message);
401+
int ret = ssl->method->ssl_get_message(ssl);
402402
if (ret <= 0) {
403403
return ret;
404404
}
@@ -661,7 +661,7 @@ static int read_v2_client_hello(SSL *ssl) {
661661
return 1;
662662
}
663663

664-
int ssl3_get_message(SSL *ssl, enum ssl_hash_message_t hash_message) {
664+
int ssl3_get_message(SSL *ssl) {
665665
again:
666666
/* Re-create the handshake buffer if needed. */
667667
if (ssl->init_buf == NULL) {
@@ -681,14 +681,9 @@ int ssl3_get_message(SSL *ssl, enum ssl_hash_message_t hash_message) {
681681
}
682682

683683
if (ssl->s3->tmp.reuse_message) {
684-
/* A ssl_dont_hash_message call cannot be combined with reuse_message; the
685-
* ssl_dont_hash_message would have to have been applied to the previous
686-
* call. */
687-
assert(hash_message == ssl_hash_message);
684+
/* There must be a current message. */
688685
assert(ssl->init_msg != NULL);
689-
690686
ssl->s3->tmp.reuse_message = 0;
691-
hash_message = ssl_dont_hash_message;
692687
} else {
693688
ssl3_release_current_message(ssl, 0 /* don't free buffer */);
694689
}
@@ -732,11 +727,6 @@ int ssl3_get_message(SSL *ssl, enum ssl_hash_message_t hash_message) {
732727
goto again;
733728
}
734729

735-
/* Feed this message into MAC computation. */
736-
if (hash_message == ssl_hash_message && !ssl_hash_current_message(ssl)) {
737-
return -1;
738-
}
739-
740730
return 1;
741731
}
742732

‎ssl/s3_pkt.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -372,7 +372,7 @@ int ssl3_read_app_data(SSL *ssl, int *out_got_handshake, uint8_t *buf, int len,
372372
}
373373

374374
/* Parse post-handshake handshake messages. */
375-
int ret = ssl3_get_message(ssl, ssl_dont_hash_message);
375+
int ret = ssl3_get_message(ssl);
376376
if (ret <= 0) {
377377
return ret;
378378
}

‎ssl/tls13_both.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ int tls13_handshake(SSL_HANDSHAKE *hs) {
5858
}
5959

6060
case ssl_hs_read_message: {
61-
int ret = ssl->method->ssl_get_message(ssl, ssl_dont_hash_message);
61+
int ret = ssl->method->ssl_get_message(ssl);
6262
if (ret <= 0) {
6363
return ret;
6464
}

0 commit comments

Comments
 (0)