Simplify ssl_get_message somewhat.
It still places the current message all over the place, but remove the
bizarre init_num/error/ok split. Now callers get the message length out
of init_num, which mirrors init_msg. Also fix some signedness.
Change-Id: Ic2e97b6b99e234926504ff217b8aedae85ba6596
Reviewed-on: https://boringssl-review.googlesource.com/8690
Reviewed-by: David Benjamin <davidben@google.com>
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index 5148fad..2ae5ab1 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -3938,10 +3938,15 @@
int state; /* where we are */
BUF_MEM *init_buf; /* buffer used during init */
- uint8_t *init_msg; /* pointer to handshake message body, set by
- ssl3_get_message() */
- int init_num; /* amount read/written */
- int init_off; /* amount read/written */
+
+ /* init_msg is a pointer to the current handshake message body. */
+ const uint8_t *init_msg;
+ /* init_num is the length of the current handshake message body. */
+ uint32_t init_num;
+
+ /* init_off, in DTLS, is the number of bytes of the current message that have
+ * been written. */
+ uint32_t init_off;
struct ssl3_state_st *s3; /* SSLv3 variables */
struct dtls1_state_st *d1; /* DTLSv1 variables */
@@ -4189,9 +4194,9 @@
* established connection state in case of renegotiations. */
struct {
uint8_t finish_md[EVP_MAX_MD_SIZE];
- int finish_md_len;
+ uint8_t finish_md_len;
uint8_t peer_finish_md[EVP_MAX_MD_SIZE];
- int peer_finish_md_len;
+ uint8_t peer_finish_md_len;
int message_type;
diff --git a/ssl/d1_both.c b/ssl/d1_both.c
index 3da1fd8..2248f8c 100644
--- a/ssl/d1_both.c
+++ b/ssl/d1_both.c
@@ -398,13 +398,8 @@
return 1;
}
-/* dtls1_get_message reads a handshake message of message type |msg_type| (any
- * if |msg_type| == -1). Read an entire handshake message. Handshake messages
- * arrive in fragments. */
-long dtls1_get_message(SSL *ssl, int msg_type,
- enum ssl_hash_message_t hash_message, int *ok) {
- *ok = 0;
-
+int dtls1_get_message(SSL *ssl, int msg_type,
+ enum ssl_hash_message_t hash_message) {
if (ssl->s3->tmp.reuse_message) {
/* A ssl_dont_hash_message call cannot be combined with reuse_message; the
* ssl_dont_hash_message would have to have been applied to the previous
@@ -449,8 +444,7 @@
ssl_do_msg_callback(ssl, 0 /* read */, ssl->version, SSL3_RT_HANDSHAKE,
frag->data, ssl->init_num + DTLS1_HM_HEADER_LENGTH);
- *ok = 1;
- return ssl->init_num;
+ return 1;
}
int dtls1_hash_current_message(SSL *ssl) {
diff --git a/ssl/handshake_client.c b/ssl/handshake_client.c
index 3c90310..ba4ac57 100644
--- a/ssl/handshake_client.c
+++ b/ssl/handshake_client.c
@@ -681,15 +681,13 @@
}
static int dtls1_get_hello_verify(SSL *ssl) {
- long n;
- int al, ok = 0;
+ int al;
CBS hello_verify_request, cookie;
uint16_t server_version;
- n = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message, &ok);
-
- if (!ok) {
- return n;
+ int ret = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message);
+ if (ret <= 0) {
+ return ret;
}
if (ssl->s3->tmp.message_type != DTLS1_MT_HELLO_VERIFY_REQUEST) {
@@ -698,7 +696,7 @@
return 1;
}
- CBS_init(&hello_verify_request, ssl->init_msg, n);
+ CBS_init(&hello_verify_request, ssl->init_msg, ssl->init_num);
if (!CBS_get_u16(&hello_verify_request, &server_version) ||
!CBS_get_u8_length_prefixed(&hello_verify_request, &cookie) ||
@@ -728,16 +726,14 @@
STACK_OF(SSL_CIPHER) *sk;
const SSL_CIPHER *c;
CERT *ct = ssl->cert;
- int al = SSL_AD_INTERNAL_ERROR, ok;
- long n;
+ int al = SSL_AD_INTERNAL_ERROR;
CBS server_hello, server_random, session_id;
uint16_t server_wire_version, server_version, cipher_suite;
uint8_t compression_method;
- n = ssl->method->ssl_get_message(ssl, SSL3_MT_SERVER_HELLO, ssl_hash_message,
- &ok);
-
- if (!ok) {
+ int ret =
+ ssl->method->ssl_get_message(ssl, SSL3_MT_SERVER_HELLO, ssl_hash_message);
+ if (ret <= 0) {
uint32_t err = ERR_peek_error();
if (ERR_GET_LIB(err) == ERR_LIB_SSL &&
ERR_GET_REASON(err) == SSL_R_SSLV3_ALERT_HANDSHAKE_FAILURE) {
@@ -749,10 +745,10 @@
* See https://crbug.com/446505. */
OPENSSL_PUT_ERROR(SSL, SSL_R_HANDSHAKE_FAILURE_ON_CLIENT_HELLO);
}
- return n;
+ return ret;
}
- CBS_init(&server_hello, ssl->init_msg, n);
+ CBS_init(&server_hello, ssl->init_msg, ssl->init_num);
if (!CBS_get_u16(&server_hello, &server_wire_version) ||
!CBS_get_bytes(&server_hello, &server_random, SSL3_RANDOM_SIZE) ||
@@ -955,22 +951,20 @@
}
static int ssl3_get_server_certificate(SSL *ssl) {
- int al, ok, ret = -1;
- unsigned long n;
+ int al, ret = -1;
X509 *x = NULL;
STACK_OF(X509) *sk = NULL;
EVP_PKEY *pkey = NULL;
CBS cbs, certificate_list;
const uint8_t *data;
- n = ssl->method->ssl_get_message(ssl, SSL3_MT_CERTIFICATE, ssl_hash_message,
- &ok);
-
- if (!ok) {
- return n;
+ int msg_ret =
+ ssl->method->ssl_get_message(ssl, SSL3_MT_CERTIFICATE, ssl_hash_message);
+ if (msg_ret <= 0) {
+ return msg_ret;
}
- CBS_init(&cbs, ssl->init_msg, n);
+ CBS_init(&cbs, ssl->init_msg, ssl->init_num);
sk = sk_X509_new_null();
if (sk == NULL) {
@@ -1045,15 +1039,13 @@
}
static int ssl3_get_cert_status(SSL *ssl) {
- int ok, al;
- long n;
+ int al;
CBS certificate_status, ocsp_response;
uint8_t status_type;
- n = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message, &ok);
-
- if (!ok) {
- return n;
+ int ret = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message);
+ if (ret <= 0) {
+ return ret;
}
if (ssl->s3->tmp.message_type != SSL3_MT_CERTIFICATE_STATUS) {
@@ -1063,7 +1055,7 @@
return 1;
}
- CBS_init(&certificate_status, ssl->init_msg, n);
+ CBS_init(&certificate_status, ssl->init_msg, ssl->init_num);
if (!CBS_get_u8(&certificate_status, &status_type) ||
status_type != TLSEXT_STATUSTYPE_ocsp ||
!CBS_get_u24_length_prefixed(&certificate_status, &ocsp_response) ||
@@ -1102,15 +1094,15 @@
}
static int ssl3_get_server_key_exchange(SSL *ssl) {
- int al, ok;
+ int al;
EVP_PKEY *pkey = NULL;
DH *dh = NULL;
EC_KEY *ecdh = NULL;
EC_POINT *srvr_ecpoint = NULL;
- long n = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message, &ok);
- if (!ok) {
- return n;
+ int ret = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message);
+ if (ret <= 0) {
+ return ret;
}
if (ssl->s3->tmp.message_type != SSL3_MT_SERVER_KEY_EXCHANGE) {
@@ -1134,7 +1126,7 @@
/* Retain a copy of the original CBS to compute the signature over. */
CBS server_key_exchange;
- CBS_init(&server_key_exchange, ssl->init_msg, n);
+ CBS_init(&server_key_exchange, ssl->init_msg, ssl->init_num);
CBS server_key_exchange_orig = server_key_exchange;
uint32_t alg_k = ssl->s3->tmp.new_cipher->algorithm_mkey;
@@ -1373,14 +1365,13 @@
}
static int ssl3_get_certificate_request(SSL *ssl) {
- int ok, ret = 0;
+ int ret = 0;
X509_NAME *xn = NULL;
STACK_OF(X509_NAME) *ca_sk = NULL;
- long n = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message, &ok);
-
- if (!ok) {
- return n;
+ int msg_ret = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message);
+ if (msg_ret <= 0) {
+ return msg_ret;
}
ssl->s3->tmp.cert_request = 0;
@@ -1400,7 +1391,7 @@
}
CBS cbs;
- CBS_init(&cbs, ssl->init_msg, n);
+ CBS_init(&cbs, ssl->init_msg, ssl->init_num);
ca_sk = sk_X509_NAME_new(ca_dn_cmp);
if (ca_sk == NULL) {
@@ -1481,18 +1472,14 @@
}
static int ssl3_get_server_hello_done(SSL *ssl) {
- int ok;
- long n;
-
- n = ssl->method->ssl_get_message(ssl, SSL3_MT_SERVER_HELLO_DONE,
- ssl_hash_message, &ok);
-
- if (!ok) {
- return n;
+ int ret = ssl->method->ssl_get_message(ssl, SSL3_MT_SERVER_HELLO_DONE,
+ ssl_hash_message);
+ if (ret <= 0) {
+ return ret;
}
- if (n > 0) {
- /* should contain no data */
+ /* ServerHelloDone is empty. */
+ if (ssl->init_num > 0) {
ssl3_send_alert(ssl, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR);
OPENSSL_PUT_ERROR(SSL, SSL_R_LENGTH_MISMATCH);
return -1;
@@ -1993,17 +1980,16 @@
}
static int ssl3_get_new_session_ticket(SSL *ssl) {
- int ok, al;
- long n = ssl->method->ssl_get_message(ssl, SSL3_MT_NEW_SESSION_TICKET,
- ssl_hash_message, &ok);
-
- if (!ok) {
- return n;
+ int al;
+ int ret = ssl->method->ssl_get_message(ssl, SSL3_MT_NEW_SESSION_TICKET,
+ ssl_hash_message);
+ if (ret <= 0) {
+ return ret;
}
CBS new_session_ticket, ticket;
uint32_t ticket_lifetime_hint;
- CBS_init(&new_session_ticket, ssl->init_msg, n);
+ CBS_init(&new_session_ticket, ssl->init_msg, ssl->init_num);
if (!CBS_get_u32(&new_session_ticket, &ticket_lifetime_hint) ||
!CBS_get_u16_length_prefixed(&new_session_ticket, &ticket) ||
CBS_len(&new_session_ticket) != 0) {
diff --git a/ssl/handshake_server.c b/ssl/handshake_server.c
index 6dea88e..30f0797 100644
--- a/ssl/handshake_server.c
+++ b/ssl/handshake_server.c
@@ -507,8 +507,7 @@
}
static int ssl3_get_client_hello(SSL *ssl) {
- int ok, al = SSL_AD_INTERNAL_ERROR, ret = -1;
- long n;
+ int al = SSL_AD_INTERNAL_ERROR, ret = -1;
const SSL_CIPHER *c;
STACK_OF(SSL_CIPHER) *ciphers = NULL;
struct ssl_early_callback_ctx early_ctx;
@@ -522,27 +521,22 @@
* be handled by a different method. If we are SSLv3, we will respond with
* SSLv3, even if prompted with TLSv1. */
switch (ssl->state) {
- case SSL3_ST_SR_CLNT_HELLO_A:
- n = ssl->method->ssl_get_message(ssl, SSL3_MT_CLIENT_HELLO,
- ssl_hash_message, &ok);
-
- if (!ok) {
- return n;
+ case SSL3_ST_SR_CLNT_HELLO_A: {
+ int msg_ret = ssl->method->ssl_get_message(ssl, SSL3_MT_CLIENT_HELLO,
+ ssl_hash_message);
+ if (msg_ret <= 0) {
+ return msg_ret;
}
ssl->state = SSL3_ST_SR_CLNT_HELLO_B;
+ }
/* fallthrough */
case SSL3_ST_SR_CLNT_HELLO_B:
case SSL3_ST_SR_CLNT_HELLO_C:
- /* We have previously parsed the ClientHello message, and can't call
- * ssl_get_message again without hashing the message into the Finished
- * digest again. */
- n = ssl->init_num;
-
memset(&early_ctx, 0, sizeof(early_ctx));
early_ctx.ssl = ssl;
early_ctx.client_hello = ssl->init_msg;
- early_ctx.client_hello_len = n;
+ early_ctx.client_hello_len = ssl->init_num;
if (!ssl_early_callback_init(&early_ctx)) {
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, SSL_R_CLIENTHELLO_PARSE_FAILED);
@@ -575,7 +569,7 @@
return -1;
}
- CBS_init(&client_hello, ssl->init_msg, n);
+ CBS_init(&client_hello, ssl->init_msg, ssl->init_num);
if (!CBS_get_u16(&client_hello, &client_wire_version) ||
!CBS_get_bytes(&client_hello, &client_random, SSL3_RANDOM_SIZE) ||
!CBS_get_u8_length_prefixed(&client_hello, &session_id) ||
@@ -1248,19 +1242,18 @@
}
static int ssl3_get_client_certificate(SSL *ssl) {
- int ok, al, ret = -1;
+ int al, ret = -1;
X509 *x = NULL;
- unsigned long n;
STACK_OF(X509) *sk = NULL;
SHA256_CTX sha256;
CBS certificate_msg, certificate_list;
int is_first_certificate = 1;
assert(ssl->s3->tmp.cert_request);
- n = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message, &ok);
- if (!ok) {
- return n;
+ int msg_ret = ssl->method->ssl_get_message(ssl, -1, ssl_hash_message);
+ if (msg_ret <= 0) {
+ return msg_ret;
}
if (ssl->s3->tmp.message_type != SSL3_MT_CERTIFICATE) {
@@ -1283,7 +1276,7 @@
goto f_err;
}
- CBS_init(&certificate_msg, ssl->init_msg, n);
+ CBS_init(&certificate_msg, ssl->init_msg, ssl->init_num);
sk = sk_X509_new_null();
if (sk == NULL) {
@@ -1399,11 +1392,10 @@
uint8_t psk[PSK_MAX_PSK_LEN];
if (ssl->state == SSL3_ST_SR_KEY_EXCH_A) {
- int ok;
- const long n = ssl->method->ssl_get_message(
- ssl, SSL3_MT_CLIENT_KEY_EXCHANGE, ssl_hash_message, &ok);
- if (!ok) {
- return n;
+ int ret = ssl->method->ssl_get_message(ssl, SSL3_MT_CLIENT_KEY_EXCHANGE,
+ ssl_hash_message);
+ if (ret <= 0) {
+ return ret;
}
}
@@ -1656,8 +1648,7 @@
}
static int ssl3_get_cert_verify(SSL *ssl) {
- int al, ok, ret = 0;
- long n;
+ int al, ret = 0;
CBS certificate_verify, signature;
X509 *peer = ssl->session->peer;
EVP_PKEY *pkey = NULL;
@@ -1670,11 +1661,10 @@
return 1;
}
- n = ssl->method->ssl_get_message(ssl, SSL3_MT_CERTIFICATE_VERIFY,
- ssl_dont_hash_message, &ok);
-
- if (!ok) {
- return n;
+ int msg_ret = ssl->method->ssl_get_message(ssl, SSL3_MT_CERTIFICATE_VERIFY,
+ ssl_dont_hash_message);
+ if (msg_ret <= 0) {
+ return msg_ret;
}
/* Filter out unsupported certificate types. */
@@ -1689,7 +1679,7 @@
goto f_err;
}
- CBS_init(&certificate_verify, ssl->init_msg, n);
+ CBS_init(&certificate_verify, ssl->init_msg, ssl->init_num);
/* Determine the digest type if needbe. */
uint16_t signature_algorithm = 0;
@@ -1784,19 +1774,14 @@
/* ssl3_get_next_proto reads a Next Protocol Negotiation handshake message. It
* sets the next_proto member in s if found */
static int ssl3_get_next_proto(SSL *ssl) {
- int ok;
- long n;
- CBS next_protocol, selected_protocol, padding;
-
- n = ssl->method->ssl_get_message(ssl, SSL3_MT_NEXT_PROTO, ssl_hash_message,
- &ok);
-
- if (!ok) {
- return n;
+ int ret =
+ ssl->method->ssl_get_message(ssl, SSL3_MT_NEXT_PROTO, ssl_hash_message);
+ if (ret <= 0) {
+ return ret;
}
- CBS_init(&next_protocol, ssl->init_msg, n);
-
+ CBS next_protocol, selected_protocol, padding;
+ CBS_init(&next_protocol, ssl->init_msg, ssl->init_num);
if (!CBS_get_u8_length_prefixed(&next_protocol, &selected_protocol) ||
!CBS_get_u8_length_prefixed(&next_protocol, &padding) ||
CBS_len(&next_protocol) != 0 ||
@@ -1810,8 +1795,7 @@
/* ssl3_get_channel_id reads and verifies a ClientID handshake message. */
static int ssl3_get_channel_id(SSL *ssl) {
- int ret = -1, ok;
- long n;
+ int ret = -1;
uint8_t channel_id_hash[EVP_MAX_MD_SIZE];
size_t channel_id_hash_len;
const uint8_t *p;
@@ -1823,11 +1807,10 @@
BIGNUM x, y;
CBS encrypted_extensions, extension;
- n = ssl->method->ssl_get_message(ssl, SSL3_MT_CHANNEL_ID,
- ssl_dont_hash_message, &ok);
-
- if (!ok) {
- return n;
+ int msg_ret = ssl->method->ssl_get_message(ssl, SSL3_MT_CHANNEL_ID,
+ ssl_dont_hash_message);
+ if (msg_ret <= 0) {
+ return msg_ret;
}
/* Before incorporating the EncryptedExtensions message to the handshake
@@ -1841,7 +1824,7 @@
return -1;
}
- CBS_init(&encrypted_extensions, ssl->init_msg, n);
+ CBS_init(&encrypted_extensions, ssl->init_msg, ssl->init_num);
/* EncryptedExtensions could include multiple extensions, but the only
* extension that could be negotiated is Channel ID, so there can only be one
diff --git a/ssl/internal.h b/ssl/internal.h
index bdc1230..e15555e 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -834,8 +834,12 @@
int (*begin_handshake)(SSL *ssl);
/* finish_handshake is called when a handshake completes. */
void (*finish_handshake)(SSL *ssl);
- long (*ssl_get_message)(SSL *ssl, int msg_type,
- enum ssl_hash_message_t hash_message, int *ok);
+ /* ssl_get_message reads the next handshake message. If |msg_type| is not -1,
+ * the message must have the specified type. On success, it returns one and
+ * sets |ssl->s3->tmp.message_type|, |ssl->init_msg|, and |ssl->init_num|.
+ * Otherwise, it returns <= 0. */
+ int (*ssl_get_message)(SSL *ssl, int msg_type,
+ enum ssl_hash_message_t hash_message);
/* hash_current_message incorporates the current handshake message into the
* handshake hash. It returns one on success and zero on allocation
* failure. */
@@ -1025,8 +1029,8 @@
int ssl3_send_change_cipher_spec(SSL *ssl);
void ssl3_cleanup_key_block(SSL *ssl);
int ssl3_send_alert(SSL *ssl, int level, int desc);
-long ssl3_get_message(SSL *ssl, int msg_type,
- enum ssl_hash_message_t hash_message, int *ok);
+int ssl3_get_message(SSL *ssl, int msg_type,
+ enum ssl_hash_message_t hash_message);
int ssl3_hash_current_message(SSL *ssl);
/* ssl3_cert_verify_hash writes the SSL 3.0 CertificateVerify hash into the
@@ -1105,8 +1109,7 @@
int dtls1_connect(SSL *ssl);
void dtls1_free(SSL *ssl);
-long dtls1_get_message(SSL *ssl, int mt, enum ssl_hash_message_t hash_message,
- int *ok);
+int dtls1_get_message(SSL *ssl, int mt, enum ssl_hash_message_t hash_message);
int dtls1_hash_current_message(SSL *ssl);
int dtls1_dispatch_alert(SSL *ssl);
diff --git a/ssl/s3_both.c b/ssl/s3_both.c
index 42ec70e..b5c2ed5 100644
--- a/ssl/s3_both.c
+++ b/ssl/s3_both.c
@@ -261,15 +261,11 @@
}
int ssl3_get_finished(SSL *ssl) {
- int al, finished_len, ok;
- long message_len;
- uint8_t *p;
-
- message_len = ssl->method->ssl_get_message(ssl, SSL3_MT_FINISHED,
- ssl_dont_hash_message, &ok);
-
- if (!ok) {
- return message_len;
+ int al;
+ int ret = ssl->method->ssl_get_message(ssl, SSL3_MT_FINISHED,
+ ssl_dont_hash_message);
+ if (ret <= 0) {
+ return ret;
}
/* Snapshot the finished hash before incorporating the new message. */
@@ -278,17 +274,15 @@
goto err;
}
- p = ssl->init_msg;
- finished_len = ssl->s3->tmp.peer_finish_md_len;
-
- if (finished_len != message_len) {
+ size_t finished_len = ssl->s3->tmp.peer_finish_md_len;
+ if (finished_len != ssl->init_num) {
al = SSL_AD_DECODE_ERROR;
OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_DIGEST_LENGTH);
goto f_err;
}
int finished_ret =
- CRYPTO_memcmp(p, ssl->s3->tmp.peer_finish_md, finished_len);
+ CRYPTO_memcmp(ssl->init_msg, ssl->s3->tmp.peer_finish_md, finished_len);
#if defined(BORINGSSL_UNSAFE_FUZZER_MODE)
finished_ret = 0;
#endif
@@ -516,12 +510,8 @@
return 1;
}
-/* Obtain handshake message of message type |msg_type| (any if |msg_type| ==
- * -1). */
-long ssl3_get_message(SSL *ssl, int msg_type,
- enum ssl_hash_message_t hash_message, int *ok) {
- *ok = 0;
-
+int ssl3_get_message(SSL *ssl, int msg_type,
+ enum ssl_hash_message_t hash_message) {
again:
if (ssl->server && !ssl->s3->v2_hello_done) {
/* Bypass the record layer for the first message to handle V2ClientHello. */
@@ -601,8 +591,7 @@
return -1;
}
- *ok = 1;
- return ssl->init_num;
+ return 1;
}
int ssl3_hash_current_message(SSL *ssl) {
