From 4bd16463b84efb13ce5fb35add284e284b0fd819 Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Thu, 17 Sep 2015 18:11:46 +0200 Subject: [PATCH] Remove PACKET_(get|goto)_bookmark The bookmark API results in a lot of boilerplate error checking that can be much more easily achieved with a simple struct copy. It also lays the path for removing the third PACKET field. Reviewed-by: Rich Salz --- ssl/packet_locl.h | 19 ----- ssl/s3_clnt.c | 38 ++++----- ssl/s3_srvr.c | 14 +--- ssl/t1_lib.c | 9 +-- test/packettest.c | 202 +++++++++++++++++++++++----------------------- 5 files changed, 122 insertions(+), 160 deletions(-) diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h index 3200c22846..7a72f8e658 100644 --- a/ssl/packet_locl.h +++ b/ssl/packet_locl.h @@ -421,25 +421,6 @@ __owur static inline int PACKET_forward(PACKET *pkt, size_t len) return 1; } -/* Store a bookmark for the current reading position in |*bm| */ -__owur static inline int PACKET_get_bookmark(const PACKET *pkt, size_t *bm) -{ - *bm = pkt->curr - pkt->start; - - return 1; -} - -/* Set the current reading position to the bookmark |bm| */ -__owur static inline int PACKET_goto_bookmark(PACKET *pkt, size_t bm) -{ - if (bm > (size_t)(pkt->end - pkt->start)) - return 0; - - pkt->curr = pkt->start + bm; - - return 1; -} - /* * Stores the total length of the packet we have in the underlying buffer in * |*len| diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 0195d09958..7cfff635f0 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -1102,10 +1102,9 @@ int ssl3_get_server_hello(SSL *s) if (s->version >= TLS1_VERSION && s->tls_session_secret_cb && s->session->tlsext_tick) { SSL_CIPHER *pref_cipher = NULL; - size_t bookm; - if (!PACKET_get_bookmark(&pkt, &bookm) - || !PACKET_forward(&pkt, j) - || !PACKET_get_bytes(&pkt, &cipherchars, ciphercharlen)) { + PACKET bookmark = pkt; + if (!PACKET_forward(&pkt, j) + || !PACKET_get_bytes(&pkt, &cipherchars, ciphercharlen)) { SSLerr(SSL_F_SSL3_GET_SERVER_HELLO, SSL_R_LENGTH_MISMATCH); al = SSL_AD_DECODE_ERROR; goto f_err; @@ -1122,11 +1121,7 @@ int ssl3_get_server_hello(SSL *s) al = SSL_AD_INTERNAL_ERROR; goto f_err; } - if (!PACKET_goto_bookmark(&pkt, bookm)) { - SSLerr(SSL_F_SSL3_GET_SERVER_HELLO, ERR_R_INTERNAL_ERROR); - al = SSL_AD_INTERNAL_ERROR; - goto f_err; - } + pkt = bookmark; } /* Get the session id */ @@ -1462,9 +1457,9 @@ int ssl3_get_key_exchange(SSL *s) int curve_nid = 0; unsigned int encoded_pt_len = 0; #endif - PACKET pkt; + PACKET pkt, save_param_start; unsigned char *data, *param; - size_t startparam, endparam; + size_t param_len; EVP_MD_CTX_init(&md_ctx); @@ -1496,12 +1491,12 @@ int ssl3_get_key_exchange(SSL *s) return (1); } - if (!PACKET_buf_init(&pkt, s->init_msg, n) - || !PACKET_get_bookmark(&pkt, &startparam)) { + if (!PACKET_buf_init(&pkt, s->init_msg, n)) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); al = SSL_AD_INTERNAL_ERROR; goto f_err; } + save_param_start = pkt; #ifndef OPENSSL_NO_RSA RSA_free(s->s3->peer_rsa_tmp); @@ -1894,10 +1889,11 @@ int ssl3_get_key_exchange(SSL *s) } #endif /* !OPENSSL_NO_EC */ - if (!PACKET_get_bookmark(&pkt, &endparam)) { - SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); - goto f_err; - } + /* + * |pkt| now points to the beginning of the signature, so the difference + * equals the length of the parameters. + */ + param_len = PACKET_remaining(&save_param_start) - PACKET_remaining(&pkt); /* if it was signed, check the signature */ if (pkey != NULL) { @@ -1939,8 +1935,8 @@ int ssl3_get_key_exchange(SSL *s) SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, SSL_R_WRONG_SIGNATURE_LENGTH); goto f_err; } - if (!PACKET_goto_bookmark(&pkt, startparam) - || !PACKET_get_bytes(&pkt, ¶m, endparam - startparam)) { + pkt = save_param_start; + if (!PACKET_get_bytes(&pkt, ¶m, param_len)) { al = SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); goto f_err; @@ -1960,7 +1956,7 @@ int ssl3_get_key_exchange(SSL *s) SSL3_RANDOM_SIZE); EVP_DigestUpdate(&md_ctx, &(s->s3->server_random[0]), SSL3_RANDOM_SIZE); - EVP_DigestUpdate(&md_ctx, param, endparam - startparam); + EVP_DigestUpdate(&md_ctx, param, param_len); EVP_DigestFinal_ex(&md_ctx, q, &size); q += size; j += size; @@ -1986,7 +1982,7 @@ int ssl3_get_key_exchange(SSL *s) SSL3_RANDOM_SIZE); EVP_VerifyUpdate(&md_ctx, &(s->s3->server_random[0]), SSL3_RANDOM_SIZE); - EVP_VerifyUpdate(&md_ctx, param, endparam - startparam); + EVP_VerifyUpdate(&md_ctx, param, param_len); if (EVP_VerifyFinal(&md_ctx, data, (int)i, pkey) <= 0) { /* bad signature */ al = SSL_AD_DECRYPT_ERROR; diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 18d3be1b2a..aea72794ef 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -2481,14 +2481,9 @@ int ssl3_get_client_key_exchange(SSL *s) if (alg_k & (SSL_kDHE | SSL_kDHr | SSL_kDHd | SSL_kDHEPSK)) { int idx = -1; EVP_PKEY *skey = NULL; - size_t bookm; + PACKET bookmark = pkt; unsigned char shared[(OPENSSL_DH_MAX_MODULUS_BITS + 7) / 8]; - if (!PACKET_get_bookmark(&pkt, &bookm)) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR); - goto f_err; - } if (!PACKET_get_net_2(&pkt, &i)) { if (alg_k & (SSL_kDHE | SSL_kDHEPSK)) { al = SSL_AD_HANDSHAKE_FAILURE; @@ -2504,12 +2499,7 @@ int ssl3_get_client_key_exchange(SSL *s) SSL_R_DH_PUBLIC_VALUE_LENGTH_IS_WRONG); goto err; } else { - if (!PACKET_goto_bookmark(&pkt, bookm)) { - al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, - ERR_R_INTERNAL_ERROR); - goto f_err; - } + pkt = bookmark; i = PACKET_remaining(&pkt); } } diff --git a/ssl/t1_lib.c b/ssl/t1_lib.c index 95b4fb694d..463f34e687 100644 --- a/ssl/t1_lib.c +++ b/ssl/t1_lib.c @@ -2934,7 +2934,7 @@ int tls1_process_ticket(SSL *s, PACKET *pkt, unsigned char *session_id, int len, SSL_SESSION **ret) { unsigned int i; - size_t bookmark = 0; + PACKET bookmark = *pkt; int retv = -1; *ret = NULL; @@ -2949,10 +2949,6 @@ int tls1_process_ticket(SSL *s, PACKET *pkt, unsigned char *session_id, if ((s->version <= SSL3_VERSION)) return 0; - if (!PACKET_get_bookmark(pkt, &bookmark)) { - return -1; - } - /* Skip past DTLS cookie */ if (SSL_IS_DTLS(s)) { if (!PACKET_get_1(pkt, &i) @@ -3043,8 +3039,7 @@ int tls1_process_ticket(SSL *s, PACKET *pkt, unsigned char *session_id, } retv = 0; end: - if (!PACKET_goto_bookmark(pkt, bookmark)) - return -1; + *pkt = bookmark; return retv; } diff --git a/test/packettest.c b/test/packettest.c index 9844b203f2..19a7599353 100644 --- a/test/packettest.c +++ b/test/packettest.c @@ -61,13 +61,16 @@ #define BUF_LEN 255 -static int test_PACKET_remaining(PACKET *pkt) +static int test_PACKET_remaining(unsigned char buf[BUF_LEN]) { - if ( PACKET_remaining(pkt) != BUF_LEN - || !PACKET_forward(pkt, BUF_LEN - 1) - || PACKET_remaining(pkt) != 1 - || !PACKET_forward(pkt, 1) - || PACKET_remaining(pkt) != 0) { + PACKET pkt; + + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || PACKET_remaining(&pkt) != BUF_LEN + || !PACKET_forward(&pkt, BUF_LEN - 1) + || PACKET_remaining(&pkt) != 1 + || !PACKET_forward(&pkt, 1) + || PACKET_remaining(&pkt) != 0) { fprintf(stderr, "test_PACKET_remaining() failed\n"); return 0; } @@ -75,17 +78,18 @@ static int test_PACKET_remaining(PACKET *pkt) return 1; } -static int test_PACKET_get_1(PACKET *pkt, size_t start) +static int test_PACKET_get_1(unsigned char buf[BUF_LEN]) { unsigned int i; + PACKET pkt; - if ( !PACKET_goto_bookmark(pkt, start) - || !PACKET_get_1(pkt, &i) + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_get_1(&pkt, &i) || i != 0x02 - || !PACKET_forward(pkt, BUF_LEN - 2) - || !PACKET_get_1(pkt, &i) + || !PACKET_forward(&pkt, BUF_LEN - 2) + || !PACKET_get_1(&pkt, &i) || i != 0xfe - || PACKET_get_1(pkt, &i)) { + || PACKET_get_1(&pkt, &i)) { fprintf(stderr, "test_PACKET_get_1() failed\n"); return 0; } @@ -93,17 +97,18 @@ static int test_PACKET_get_1(PACKET *pkt, size_t start) return 1; } -static int test_PACKET_get_4(PACKET *pkt, size_t start) +static int test_PACKET_get_4(unsigned char buf[BUF_LEN]) { unsigned long i; + PACKET pkt; - if ( !PACKET_goto_bookmark(pkt, start) - || !PACKET_get_4(pkt, &i) + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_get_4(&pkt, &i) || i != 0x08060402UL - || !PACKET_forward(pkt, BUF_LEN - 8) - || !PACKET_get_4(pkt, &i) + || !PACKET_forward(&pkt, BUF_LEN - 8) + || !PACKET_get_4(&pkt, &i) || i != 0xfefcfaf8UL - || PACKET_get_4(pkt, &i)) { + || PACKET_get_4(&pkt, &i)) { fprintf(stderr, "test_PACKET_get_4() failed\n"); return 0; } @@ -111,17 +116,18 @@ static int test_PACKET_get_4(PACKET *pkt, size_t start) return 1; } -static int test_PACKET_get_net_2(PACKET *pkt, size_t start) +static int test_PACKET_get_net_2(unsigned char buf[BUF_LEN]) { unsigned int i; + PACKET pkt; - if ( !PACKET_goto_bookmark(pkt, start) - || !PACKET_get_net_2(pkt, &i) + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_get_net_2(&pkt, &i) || i != 0x0204 - || !PACKET_forward(pkt, BUF_LEN - 4) - || !PACKET_get_net_2(pkt, &i) + || !PACKET_forward(&pkt, BUF_LEN - 4) + || !PACKET_get_net_2(&pkt, &i) || i != 0xfcfe - || PACKET_get_net_2(pkt, &i)) { + || PACKET_get_net_2(&pkt, &i)) { fprintf(stderr, "test_PACKET_get_net_2() failed\n"); return 0; } @@ -129,17 +135,18 @@ static int test_PACKET_get_net_2(PACKET *pkt, size_t start) return 1; } -static int test_PACKET_get_net_3(PACKET *pkt, size_t start) +static int test_PACKET_get_net_3(unsigned char buf[BUF_LEN]) { unsigned long i; + PACKET pkt; - if ( !PACKET_goto_bookmark(pkt, start) - || !PACKET_get_net_3(pkt, &i) + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_get_net_3(&pkt, &i) || i != 0x020406UL - || !PACKET_forward(pkt, BUF_LEN - 6) - || !PACKET_get_net_3(pkt, &i) + || !PACKET_forward(&pkt, BUF_LEN - 6) + || !PACKET_get_net_3(&pkt, &i) || i != 0xfafcfeUL - || PACKET_get_net_3(pkt, &i)) { + || PACKET_get_net_3(&pkt, &i)) { fprintf(stderr, "test_PACKET_get_net_3() failed\n"); return 0; } @@ -147,17 +154,18 @@ static int test_PACKET_get_net_3(PACKET *pkt, size_t start) return 1; } -static int test_PACKET_get_net_4(PACKET *pkt, size_t start) +static int test_PACKET_get_net_4(unsigned char buf[BUF_LEN]) { unsigned long i; + PACKET pkt; - if ( !PACKET_goto_bookmark(pkt, start) - || !PACKET_get_net_4(pkt, &i) + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_get_net_4(&pkt, &i) || i != 0x02040608UL - || !PACKET_forward(pkt, BUF_LEN - 8) - || !PACKET_get_net_4(pkt, &i) + || !PACKET_forward(&pkt, BUF_LEN - 8) + || !PACKET_get_net_4(&pkt, &i) || i != 0xf8fafcfeUL - || PACKET_get_net_4(pkt, &i)) { + || PACKET_get_net_4(&pkt, &i)) { fprintf(stderr, "test_PACKET_get_net_4() failed\n"); return 0; } @@ -165,22 +173,22 @@ static int test_PACKET_get_net_4(PACKET *pkt, size_t start) return 1; } -static int test_PACKET_get_sub_packet(PACKET *pkt, size_t start) +static int test_PACKET_get_sub_packet(unsigned char buf[BUF_LEN]) { - PACKET subpkt; + PACKET pkt, subpkt; unsigned long i; - if ( !PACKET_goto_bookmark(pkt, start) - || !PACKET_get_sub_packet(pkt, &subpkt, 4) + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_get_sub_packet(&pkt, &subpkt, 4) || !PACKET_get_net_4(&subpkt, &i) || i != 0x02040608UL || PACKET_remaining(&subpkt) - || !PACKET_forward(pkt, BUF_LEN - 8) - || !PACKET_get_sub_packet(pkt, &subpkt, 4) + || !PACKET_forward(&pkt, BUF_LEN - 8) + || !PACKET_get_sub_packet(&pkt, &subpkt, 4) || !PACKET_get_net_4(&subpkt, &i) || i != 0xf8fafcfeUL || PACKET_remaining(&subpkt) - || PACKET_get_sub_packet(pkt, &subpkt, 4)) { + || PACKET_get_sub_packet(&pkt, &subpkt, 4)) { fprintf(stderr, "test_PACKET_get_sub_packet() failed\n"); return 0; } @@ -188,20 +196,21 @@ static int test_PACKET_get_sub_packet(PACKET *pkt, size_t start) return 1; } -static int test_PACKET_get_bytes(PACKET *pkt, size_t start) +static int test_PACKET_get_bytes(unsigned char buf[BUF_LEN]) { unsigned char *bytes; + PACKET pkt; - if ( !PACKET_goto_bookmark(pkt, start) - || !PACKET_get_bytes(pkt, &bytes, 4) + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_get_bytes(&pkt, &bytes, 4) || bytes[0] != 2 || bytes[1] != 4 || bytes[2] != 6 || bytes[3] != 8 - || PACKET_remaining(pkt) != BUF_LEN -4 - || !PACKET_forward(pkt, BUF_LEN - 8) - || !PACKET_get_bytes(pkt, &bytes, 4) + || PACKET_remaining(&pkt) != BUF_LEN -4 + || !PACKET_forward(&pkt, BUF_LEN - 8) + || !PACKET_get_bytes(&pkt, &bytes, 4) || bytes[0] != 0xf8 || bytes[1] != 0xfa || bytes[2] != 0xfc || bytes[3] != 0xfe - || PACKET_remaining(pkt)) { + || PACKET_remaining(&pkt)) { fprintf(stderr, "test_PACKET_get_bytes() failed\n"); return 0; } @@ -209,20 +218,21 @@ static int test_PACKET_get_bytes(PACKET *pkt, size_t start) return 1; } -static int test_PACKET_copy_bytes(PACKET *pkt, size_t start) +static int test_PACKET_copy_bytes(unsigned char buf[BUF_LEN]) { unsigned char bytes[4]; + PACKET pkt; - if ( !PACKET_goto_bookmark(pkt, start) - || !PACKET_copy_bytes(pkt, bytes, 4) + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_copy_bytes(&pkt, bytes, 4) || bytes[0] != 2 || bytes[1] != 4 || bytes[2] != 6 || bytes[3] != 8 - || PACKET_remaining(pkt) != BUF_LEN - 4 - || !PACKET_forward(pkt, BUF_LEN - 8) - || !PACKET_copy_bytes(pkt, bytes, 4) + || PACKET_remaining(&pkt) != BUF_LEN - 4 + || !PACKET_forward(&pkt, BUF_LEN - 8) + || !PACKET_copy_bytes(&pkt, bytes, 4) || bytes[0] != 0xf8 || bytes[1] != 0xfa || bytes[2] != 0xfc || bytes[3] != 0xfe - || PACKET_remaining(pkt)) { + || PACKET_remaining(&pkt)) { fprintf(stderr, "test_PACKET_copy_bytes() failed\n"); return 0; } @@ -230,22 +240,24 @@ static int test_PACKET_copy_bytes(PACKET *pkt, size_t start) return 1; } -static int test_PACKET_memdup(PACKET *pkt, size_t start) +static int test_PACKET_memdup(unsigned char buf[BUF_LEN]) { unsigned char *data = NULL; size_t len; - if ( !PACKET_goto_bookmark(pkt, start) - || !PACKET_memdup(pkt, &data, &len) + PACKET pkt; + + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_memdup(&pkt, &data, &len) || len != BUF_LEN - || memcmp(data, PACKET_data(pkt), len) - || !PACKET_forward(pkt, 10) - || !PACKET_memdup(pkt, &data, &len) + || memcmp(data, PACKET_data(&pkt), len) + || !PACKET_forward(&pkt, 10) + || !PACKET_memdup(&pkt, &data, &len) || len != BUF_LEN - 10 - || memcmp(data, PACKET_data(pkt), len) - || !PACKET_back(pkt, 1) - || !PACKET_memdup(pkt, &data, &len) + || memcmp(data, PACKET_data(&pkt), len) + || !PACKET_back(&pkt, 1) + || !PACKET_memdup(&pkt, &data, &len) || len != BUF_LEN - 9 - || memcmp(data, PACKET_data(pkt), len)) { + || memcmp(data, PACKET_data(&pkt), len)) { fprintf(stderr, "test_PACKET_memdup() failed\n"); OPENSSL_free(data); return 0; @@ -282,25 +294,21 @@ static int test_PACKET_strndup() return 1; } -static int test_PACKET_move_funcs(PACKET *pkt, size_t start) +static int test_PACKET_move_funcs(unsigned char buf[BUF_LEN]) { unsigned char *byte; - size_t bm; + PACKET pkt; - if ( !PACKET_goto_bookmark(pkt, start) - || PACKET_back(pkt, 1) - || !PACKET_forward(pkt, 1) - || !PACKET_get_bytes(pkt, &byte, 1) + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || PACKET_back(&pkt, 1) + || !PACKET_forward(&pkt, 1) + || !PACKET_get_bytes(&pkt, &byte, 1) || byte[0] != 4 - || !PACKET_get_bookmark(pkt, &bm) - || !PACKET_forward(pkt, BUF_LEN - 2) - || PACKET_forward(pkt, 1) - || !PACKET_back(pkt, 1) - || !PACKET_get_bytes(pkt, &byte, 1) - || byte[0] != 0xfe - || !PACKET_goto_bookmark(pkt, bm) - || !PACKET_get_bytes(pkt, &byte, 1) - || byte[0] != 6) { + || !PACKET_forward(&pkt, BUF_LEN - 2) + || PACKET_forward(&pkt, 1) + || !PACKET_back(&pkt, 1) + || !PACKET_get_bytes(&pkt, &byte, 1) + || byte[0] != 0xfe) { fprintf(stderr, "test_PACKET_move_funcs() failed\n"); return 0; } @@ -416,33 +424,25 @@ int main(int argc, char **argv) { unsigned char buf[BUF_LEN]; unsigned int i; - size_t start = 0; - PACKET pkt; for (i=1; i<=BUF_LEN; i++) { buf[i-1] = (i * 2) & 0xff; } i = 0; - if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) - || !PACKET_get_bookmark(&pkt, &start)) { - fprintf(stderr, "setup failed\n"); - return 0; - } - if ( !test_PACKET_buf_init() - || !test_PACKET_remaining(&pkt) - || !test_PACKET_get_1(&pkt, start) - || !test_PACKET_get_4(&pkt, start) - || !test_PACKET_get_net_2(&pkt, start) - || !test_PACKET_get_net_3(&pkt, start) - || !test_PACKET_get_net_4(&pkt, start) - || !test_PACKET_get_sub_packet(&pkt, start) - || !test_PACKET_get_bytes(&pkt, start) - || !test_PACKET_copy_bytes(&pkt, start) - || !test_PACKET_memdup(&pkt, start) + || !test_PACKET_remaining(buf) + || !test_PACKET_get_1(buf) + || !test_PACKET_get_4(buf) + || !test_PACKET_get_net_2(buf) + || !test_PACKET_get_net_3(buf) + || !test_PACKET_get_net_4(buf) + || !test_PACKET_get_sub_packet(buf) + || !test_PACKET_get_bytes(buf) + || !test_PACKET_copy_bytes(buf) + || !test_PACKET_memdup(buf) || !test_PACKET_strndup() - || !test_PACKET_move_funcs(&pkt, start) + || !test_PACKET_move_funcs(buf) || !test_PACKET_get_length_prefixed_1() || !test_PACKET_get_length_prefixed_2() || !test_PACKET_get_length_prefixed_3()) { -- 2.25.1