From: Emilia Kasper Date: Tue, 18 Aug 2015 10:29:36 +0000 (+0200) Subject: PACKET: add methods for reading length-prefixed TLS vectors. X-Git-Tag: OpenSSL_1_1_0-pre1~776 X-Git-Url: https://git.librecmc.org/?a=commitdiff_plain;h=ec30e8566e43d888ab1d7cf01749a979896bb3d9;p=oweals%2Fopenssl.git PACKET: add methods for reading length-prefixed TLS vectors. Rewrite ssl3_get_client_hello to use the new methods. Reviewed-by: Matt Caswell --- diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h index 0d5d823083..3f03fa719c 100644 --- a/ssl/packet_locl.h +++ b/ssl/packet_locl.h @@ -87,6 +87,17 @@ __owur static inline size_t PACKET_remaining(const PACKET *pkt) return (size_t)(pkt->end - pkt->curr); } +/* + * Returns a pointer to the PACKET's current position. + * For use in non-PACKETized APIs. + * TODO(openssl-team): this should return 'const unsigned char*' but can't + * currently because legacy code passes 'unsigned char*'s around. + */ +static inline unsigned char *PACKET_data(const PACKET *pkt) +{ + return pkt->curr; +} + /* * Initialise a PACKET with |len| bytes held in |buf|. This does not make a * copy of the data so |buf| must be present for the whole time that the PACKET @@ -388,6 +399,77 @@ __owur static inline int PACKET_length(const PACKET *pkt, size_t *len) return 1; } +/* + * Reads a variable-length vector prefixed with a one-byte length, and stores + * the contents in |subpkt|. |pkt| can equal |subpkt|. + * Data is not copied: the |subpkt| packet will share its underlying buffer with + * the original |pkt|, so data wrapped by |pkt| must outlive the |subpkt|. + * Upon failure, the original |pkt| and |subpkt| are not modified. + */ +__owur static inline int PACKET_get_length_prefixed_1(PACKET *pkt, PACKET *subpkt) +{ + unsigned int length; + unsigned char *data; + PACKET tmp = *pkt; + if (!PACKET_get_1(&tmp, &length) || + !PACKET_get_bytes(&tmp, &data, (size_t)length)) { + return 0; + } + + *pkt = tmp; + subpkt->start = subpkt->curr = data; + subpkt->end = subpkt->start + length; + + return 1; +} + +/* + * Reads a variable-length vector prefixed with a two-byte length, and stores + * the contents in |subpkt|. |pkt| can equal |subpkt|. + * Data is not copied: the |subpkt| packet will share its underlying buffer with + * the original |pkt|, so data wrapped by |pkt| must outlive the |subpkt|. + * Upon failure, the original |pkt| and |subpkt| are not modified. + */ +__owur static inline int PACKET_get_length_prefixed_2(PACKET *pkt, PACKET *subpkt) +{ + unsigned int length; + unsigned char *data; + PACKET tmp = *pkt; + if (!PACKET_get_net_2(&tmp, &length) || + !PACKET_get_bytes(&tmp, &data, (size_t)length)) { + return 0; + } + + *pkt = tmp; + subpkt->start = subpkt->curr = data; + subpkt->end = subpkt->start + length; + + return 1; +} + +/* + * Reads a variable-length vector prefixed with a three-byte length, and stores + * the contents in |subpkt|. |pkt| can equal |subpkt|. + * Data is not copied: the |subpkt| packet will share its underlying buffer with + * the original |pkt|, so data wrapped by |pkt| must outlive the |subpkt|. + * Upon failure, the original |pkt| and |subpkt| are not modified. + */ +__owur static inline int PACKET_get_length_prefixed_3(PACKET *pkt, PACKET *subpkt) +{ + unsigned long length; + unsigned char *data; + PACKET tmp = *pkt; + if (!PACKET_get_net_3(&tmp, &length) || + !PACKET_get_bytes(&tmp, &data, (size_t)length)) { + return 0; + } + + *pkt = tmp; + subpkt->start = subpkt->curr = data; + subpkt->end = subpkt->start + length; + + return 1; +} # ifdef __cplusplus } # endif diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 2d15c63b6b..74c369604c 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -838,19 +838,16 @@ int ssl3_send_hello_request(SSL *s) int ssl3_get_client_hello(SSL *s) { int i, ok, al = SSL_AD_INTERNAL_ERROR, ret = -1; - unsigned int j, cipherlen, complen; - unsigned int cookie_len = 0; + unsigned int j, complen = 0; long n; unsigned long id; SSL_CIPHER *c; #ifndef OPENSSL_NO_COMP - unsigned char *q = NULL; SSL_COMP *comp = NULL; #endif STACK_OF(SSL_CIPHER) *ciphers = NULL; int protverr = 1; - PACKET pkt; - unsigned char *sess, *cdata; + PACKET pkt, cipher_suite, compression; if (s->state == SSL3_ST_SR_CLNT_HELLO_C && !s->first_packet) goto retry_cert; @@ -1013,30 +1010,31 @@ int ssl3_get_client_hello(SSL *s) * Note, this is only for SSLv3+ using the backward compatible format. * Real SSLv2 is not supported, and is rejected above. */ - unsigned int csl, sil, cl; + unsigned int cipher_len, session_id_len, challenge_len; - if (!PACKET_get_net_2(&pkt, &csl) - || !PACKET_get_net_2(&pkt, &sil) - || !PACKET_get_net_2(&pkt, &cl)) { + if (!PACKET_get_net_2(&pkt, &cipher_len) + || !PACKET_get_net_2(&pkt, &session_id_len) + || !PACKET_get_net_2(&pkt, &challenge_len)) { SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_RECORD_LENGTH_MISMATCH); al = SSL_AD_DECODE_ERROR; goto f_err; } - if (csl == 0) { + if (cipher_len == 0) { /* we need at least one cipher */ al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_NO_CIPHERS_SPECIFIED); goto f_err; } - if (!PACKET_get_bytes(&pkt, &cdata, csl)) { + if (!PACKET_get_sub_packet(&pkt, &cipher_suite, cipher_len)) { SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_RECORD_LENGTH_MISMATCH); al = SSL_AD_DECODE_ERROR; goto f_err; } - if (ssl_bytes_to_cipher_list(s, cdata, csl, &(ciphers), 1) == NULL) { + if (ssl_bytes_to_cipher_list(s, PACKET_data(&cipher_suite), + cipher_len, &(ciphers), 1) == NULL) { goto err; } @@ -1044,7 +1042,7 @@ int ssl3_get_client_hello(SSL *s) * Ignore any session id. We don't allow resumption in a backwards * compatible ClientHello */ - if (!PACKET_forward(&pkt, sil)) { + if (!PACKET_forward(&pkt, session_id_len)) { SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_RECORD_LENGTH_MISMATCH); al = SSL_AD_DECODE_ERROR; goto f_err; @@ -1055,27 +1053,24 @@ int ssl3_get_client_hello(SSL *s) goto err; /* Load the client random */ - i = (cl > SSL3_RANDOM_SIZE) ? SSL3_RANDOM_SIZE : cl; + i = challenge_len > SSL3_RANDOM_SIZE ? SSL3_RANDOM_SIZE : challenge_len; memset(s->s3->client_random, 0, SSL3_RANDOM_SIZE); if (!PACKET_peek_copy_bytes(&pkt, s->s3->client_random + SSL3_RANDOM_SIZE - i, i) - || !PACKET_forward(&pkt, cl) + || !PACKET_forward(&pkt, challenge_len) || PACKET_remaining(&pkt) != 0) { SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_RECORD_LENGTH_MISMATCH); al = SSL_AD_DECODE_ERROR; goto f_err; } - - /* No compression, so set complen to 0 */ - complen = 0; } else { /* If we get here we've got SSLv3+ in an SSLv3+ record */ - + PACKET session_id; + unsigned int cookie_len; /* load the client random and get the session-id */ if (!PACKET_copy_bytes(&pkt, s->s3->client_random, SSL3_RANDOM_SIZE) - || !PACKET_get_1(&pkt, &j) - || !PACKET_get_bytes(&pkt, &sess, j)) { + || !PACKET_get_length_prefixed_1(&pkt, &session_id)) { al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_TOO_SHORT); goto f_err; @@ -1117,7 +1112,13 @@ int ssl3_get_client_hello(SSL *s) if (!ssl_get_new_session(s, 1)) goto err; } else { - i = ssl_get_prev_session(s, &pkt, sess, j); + /* + * TODO(openssl-team): ssl_get_prev_session passes a non-const + * 'unsigned char*' session id to a user callback. Grab a copy of + * the data? + */ + i = ssl_get_prev_session(s, &pkt, PACKET_data(&session_id), + PACKET_remaining(&session_id)); /* * Only resume if the session's version matches the negotiated * version. @@ -1140,11 +1141,13 @@ int ssl3_get_client_hello(SSL *s) } if (SSL_IS_DTLS(s)) { - if (!PACKET_get_1(&pkt, &cookie_len)) { + PACKET cookie; + if (!PACKET_get_length_prefixed_1(&pkt, &cookie)) { al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_TOO_SHORT); goto f_err; } + cookie_len = PACKET_remaining(&cookie); /* * The ClientHello may contain a cookie even if the * HelloVerify message has not been sent--make sure that it @@ -1161,10 +1164,13 @@ int ssl3_get_client_hello(SSL *s) if ((SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) && cookie_len > 0) { /* Get cookie */ - if (!PACKET_copy_bytes(&pkt, s->d1->rcvd_cookie, - cookie_len)) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_TOO_SHORT); + /* + * TODO(openssl-team): rcvd_cookie appears unused outside this + * function. Remove the field? + */ + if (!PACKET_copy_bytes(&cookie, s->d1->rcvd_cookie, cookie_len)) { + al = SSL_AD_INTERNAL_ERROR; + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, ERR_R_INTERNAL_ERROR); goto f_err; } @@ -1187,15 +1193,7 @@ int ssl3_get_client_hello(SSL *s) } /* Set to -2 so if successful we return 2 */ ret = -2; - } else { - /* Skip over cookie */ - if (!PACKET_forward(&pkt, cookie_len)) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_TOO_SHORT); - goto f_err; - } } - if (s->method->version == DTLS_ANY_VERSION) { /* Select version to use */ if (s->client_version <= DTLS1_2_VERSION && @@ -1223,26 +1221,21 @@ int ssl3_get_client_hello(SSL *s) } } - if (!PACKET_get_net_2(&pkt, &cipherlen)) { + if (!PACKET_get_length_prefixed_2(&pkt, &cipher_suite)) { al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_TOO_SHORT); goto f_err; } - if (cipherlen == 0) { + if (PACKET_remaining(&cipher_suite) == 0) { al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_NO_CIPHERS_SPECIFIED); goto f_err; } - if (!PACKET_get_bytes(&pkt, &cdata, cipherlen)) { - /* not enough data */ - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_MISMATCH); - goto f_err; - } - - if (ssl_bytes_to_cipher_list(s, cdata, cipherlen, &(ciphers), 0) == NULL) { + if (ssl_bytes_to_cipher_list(s, PACKET_data(&cipher_suite), + PACKET_remaining(&cipher_suite), + &(ciphers), 0) == NULL) { goto err; } @@ -1301,19 +1294,21 @@ int ssl3_get_client_hello(SSL *s) } /* compression */ - if (!PACKET_get_1(&pkt, &complen) - || !PACKET_get_bytes(&pkt, &cdata, complen)) { + if (!PACKET_get_length_prefixed_1(&pkt, &compression)) { /* not enough data */ al = SSL_AD_DECODE_ERROR; + /* + * TODO(openssl-team): + * SSL_R_LENGTH_TOO_SHORT and SSL_R_LENGTH_MISMATCH are used + * interchangeably. Pick one. + */ SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_LENGTH_MISMATCH); goto f_err; } -#ifndef OPENSSL_NO_COMP - q = cdata; -#endif + complen = PACKET_remaining(&compression); for (j = 0; j < complen; j++) { - if (cdata[j] == 0) + if (PACKET_data(&compression)[j] == 0) break; } @@ -1415,7 +1410,7 @@ int ssl3_get_client_hello(SSL *s) } /* Look for resumed method in compression list */ for (k = 0; k < complen; k++) { - if (q[k] == comp_id) + if (PACKET_data(&compression)[k] == comp_id) break; } if (k >= complen) { @@ -1436,7 +1431,7 @@ int ssl3_get_client_hello(SSL *s) comp = sk_SSL_COMP_value(s->ctx->comp_methods, m); v = comp->id; for (o = 0; o < complen; o++) { - if (v == q[o]) { + if (v == PACKET_data(&compression)[o]) { done = 1; break; } diff --git a/test/packettest.c b/test/packettest.c index c3ac53bcb0..b3f7bbb19e 100644 --- a/test/packettest.c +++ b/test/packettest.c @@ -281,6 +281,85 @@ static int test_PACKET_buf_init() return 1; } +static int test_PACKET_get_length_prefixed_1() +{ + unsigned char buf[BUF_LEN]; + const size_t len = 16; + unsigned int i; + PACKET pkt, short_pkt, subpkt; + + buf[0] = len; + for (i = 1; i < BUF_LEN; i++) { + buf[i] = (i * 2) & 0xff; + } + + if ( !PACKET_buf_init(&pkt, buf, BUF_LEN) + || !PACKET_buf_init(&short_pkt, buf, len) + || !PACKET_get_length_prefixed_1(&pkt, &subpkt) + || PACKET_remaining(&subpkt) != len + || !PACKET_get_net_2(&subpkt, &i) + || i != 0x0204 + || PACKET_get_length_prefixed_1(&short_pkt, &subpkt) + || PACKET_remaining(&short_pkt) != len) { + fprintf(stderr, "test_PACKET_get_length_prefixed_1() failed\n"); + return 0; + } + + return 1; +} + +static int test_PACKET_get_length_prefixed_2() +{ + unsigned char buf[1024]; + const size_t len = 516; /* 0x0204 */ + unsigned int i; + PACKET pkt, short_pkt, subpkt; + + for (i = 1; i <= 1024; i++) { + buf[i-1] = (i * 2) & 0xff; + } + + if ( !PACKET_buf_init(&pkt, buf, 1024) + || !PACKET_buf_init(&short_pkt, buf, len) + || !PACKET_get_length_prefixed_2(&pkt, &subpkt) + || PACKET_remaining(&subpkt) != len + || !PACKET_get_net_2(&subpkt, &i) + || i != 0x0608 + || PACKET_get_length_prefixed_2(&short_pkt, &subpkt) + || PACKET_remaining(&short_pkt) != len) { + fprintf(stderr, "test_PACKET_get_length_prefixed_2() failed\n"); + return 0; + } + + return 1; +} + +static int test_PACKET_get_length_prefixed_3() +{ + unsigned char buf[1024]; + const size_t len = 516; /* 0x000204 */ + unsigned int i; + PACKET pkt, short_pkt, subpkt; + + for (i = 0; i < 1024; i++) { + buf[i] = (i * 2) & 0xff; + } + + if ( !PACKET_buf_init(&pkt, buf, 1024) + || !PACKET_buf_init(&short_pkt, buf, len) + || !PACKET_get_length_prefixed_3(&pkt, &subpkt) + || PACKET_remaining(&subpkt) != len + || !PACKET_get_net_2(&subpkt, &i) + || i != 0x0608 + || PACKET_get_length_prefixed_3(&short_pkt, &subpkt) + || PACKET_remaining(&short_pkt) != len) { + fprintf(stderr, "test_PACKET_get_length_prefixed_3() failed\n"); + return 0; + } + + return 1; +} + int main(int argc, char **argv) { unsigned char buf[BUF_LEN]; @@ -309,7 +388,10 @@ int main(int argc, char **argv) || !test_PACKET_get_sub_packet(&pkt, start) || !test_PACKET_get_bytes(&pkt, start) || !test_PACKET_copy_bytes(&pkt, start) - || !test_PACKET_move_funcs(&pkt, start)) { + || !test_PACKET_move_funcs(&pkt, start) + || !test_PACKET_get_length_prefixed_1() + || !test_PACKET_get_length_prefixed_2() + || !test_PACKET_get_length_prefixed_3()) { return 1; } printf("PASS\n");