From 6d41fc80e6152a6bf9d062b2a8e835a388ed0062 Mon Sep 17 00:00:00 2001 From: Emilia Kasper Date: Tue, 1 Sep 2015 18:19:14 +0200 Subject: [PATCH] PACKET: add PACKET_memdup and PACKET_strndup Use each once in s3_srvr.c to show how they work. Also fix a bug introduced in c3fc7eeab884b6876a1b4006163f190d325aa047 and made apparent by this change: ssl3_get_next_proto wasn't updating next_proto_negotiated_len Reviewed-by: Matt Caswell --- ssl/packet_locl.h | 56 +++++++++++++++++++++++++++++++++++++++++++++-- ssl/s3_srvr.c | 41 +++++++++++----------------------- test/packettest.c | 53 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 30 deletions(-) diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h index 3f03fa719c..3200c22846 100644 --- a/ssl/packet_locl.h +++ b/ssl/packet_locl.h @@ -335,9 +335,12 @@ __owur static inline int PACKET_peek_copy_bytes(const PACKET *pkt, return 1; } -/* Read |len| bytes from |pkt| and copy them to |data| */ +/* + * Read |len| bytes from |pkt| and copy them to |data|. + * The caller is responsible for ensuring that |data| can hold |len| bytes. + */ __owur static inline int PACKET_copy_bytes(PACKET *pkt, unsigned char *data, - size_t len) + size_t len) { if (!PACKET_peek_copy_bytes(pkt, data, len)) return 0; @@ -347,6 +350,55 @@ __owur static inline int PACKET_copy_bytes(PACKET *pkt, unsigned char *data, return 1; } +/* + * Copy |pkt| bytes to a newly allocated buffer and store a pointer to the + * result in |*data|, and the length in |len|. + * If |*data| is not NULL, the old data is OPENSSL_free'd. + * If the packet is empty, or malloc fails, |*data| will be set to NULL. + * Returns 1 if the malloc succeeds and 0 otherwise. + * Does not forward PACKET position (because it is typically the last thing + * done with a given PACKET). + */ +__owur static inline int PACKET_memdup(const PACKET *pkt, unsigned char **data, + size_t *len) +{ + size_t length; + + OPENSSL_free(*data); + *data = NULL; + *len = 0; + + length = PACKET_remaining(pkt); + + if (length == 0) + return 1; + + *data = BUF_memdup(pkt->curr, length); + + if (*data == NULL) + return 0; + + *len = length; + return 1; +} + +/* + * Read a C string from |pkt| and copy to a newly allocated, NUL-terminated + * buffer. Store a pointer to the result in |*data|. + * If |*data| is not NULL, the old data is OPENSSL_free'd. + * If the data in |pkt| does not contain a NUL-byte, the entire data is + * copied and NUL-terminated. + * Returns 1 if the malloc succeeds and 0 otherwise. + * Does not forward PACKET position (because it is typically the last thing done + * with a given PACKET). + */ +__owur static inline int PACKET_strndup(const PACKET *pkt, char **data) +{ + OPENSSL_free(*data); + *data = BUF_strndup((const char*)pkt->curr, PACKET_remaining(pkt)); + return (*data != NULL); +} + /* Move the current reading position back |len| bytes */ __owur static inline int PACKET_back(PACKET *pkt, size_t len) { diff --git a/ssl/s3_srvr.c b/ssl/s3_srvr.c index 74c369604c..16f4db975c 100644 --- a/ssl/s3_srvr.c +++ b/ssl/s3_srvr.c @@ -2250,13 +2250,14 @@ int ssl3_get_client_key_exchange(SSL *s) if (alg_k & SSL_PSK) { unsigned char psk[PSK_MAX_PSK_LEN]; size_t psklen; + PACKET psk_identity; - if (!PACKET_get_net_2(&pkt, &i)) { + if (!PACKET_get_length_prefixed_2(&pkt, &psk_identity)) { al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH); goto f_err; } - if (i > PSK_MAX_IDENTITY_LEN) { + if (PACKET_remaining(&psk_identity) > PSK_MAX_IDENTITY_LEN) { al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_DATA_LENGTH_TOO_LONG); @@ -2269,21 +2270,10 @@ int ssl3_get_client_key_exchange(SSL *s) goto f_err; } - OPENSSL_free(s->session->psk_identity); - s->session->psk_identity = OPENSSL_malloc(i + 1); - if (s->session->psk_identity == NULL) { + if (!PACKET_strndup(&psk_identity, &s->session->psk_identity)) { al = SSL_AD_INTERNAL_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, - ERR_R_MALLOC_FAILURE); - goto f_err; - } - if (!PACKET_copy_bytes(&pkt, (unsigned char *)s->session->psk_identity, - i)) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_LENGTH_MISMATCH); goto f_err; } - s->session->psk_identity[i] = '\0'; psklen = s->psk_server_callback(s, s->session->psk_identity, psk, sizeof(psk)); @@ -3455,9 +3445,9 @@ int ssl3_send_cert_status(SSL *s) int ssl3_get_next_proto(SSL *s) { int ok; - unsigned int proto_len, padding_len; long n; - PACKET pkt; + PACKET pkt, next_proto, padding; + size_t next_proto_len; /* * Clients cannot send a NextProtocol message if we didn't see the @@ -3506,25 +3496,20 @@ int ssl3_get_next_proto(SSL *s) * uint8 padding_len; * uint8 padding[padding_len]; */ - if (!PACKET_get_1(&pkt, &proto_len)){ + if (!PACKET_get_length_prefixed_1(&pkt, &next_proto) + || !PACKET_get_length_prefixed_1(&pkt, &padding) + || PACKET_remaining(&pkt) > 0) { SSLerr(SSL_F_SSL3_GET_NEXT_PROTO, SSL_R_LENGTH_MISMATCH); goto err; } - s->next_proto_negotiated = OPENSSL_malloc(proto_len); - if (s->next_proto_negotiated == NULL) { - SSLerr(SSL_F_SSL3_GET_NEXT_PROTO, ERR_R_MALLOC_FAILURE); + if (!PACKET_memdup(&next_proto, &s->next_proto_negotiated, + &next_proto_len)) { + s->next_proto_negotiated_len = 0; goto err; } - if (!PACKET_copy_bytes(&pkt, s->next_proto_negotiated, proto_len) - || !PACKET_get_1(&pkt, &padding_len) - || PACKET_remaining(&pkt) != padding_len) { - OPENSSL_free(s->next_proto_negotiated); - s->next_proto_negotiated = NULL; - SSLerr(SSL_F_SSL3_GET_NEXT_PROTO, SSL_R_LENGTH_MISMATCH); - goto err; - } + s->next_proto_negotiated_len = (unsigned char)next_proto_len; return 1; err: diff --git a/test/packettest.c b/test/packettest.c index b3f7bbb19e..23b60857f1 100644 --- a/test/packettest.c +++ b/test/packettest.c @@ -230,6 +230,57 @@ static int test_PACKET_copy_bytes(PACKET *pkt, size_t start) return 1; } +static int test_PACKET_memdup(PACKET *pkt, size_t start) +{ + unsigned char *data = NULL; + size_t len; + if ( !PACKET_goto_bookmark(pkt, start) + || !PACKET_memdup(pkt, &data, &len) + || len != BUF_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) + || len != BUF_LEN - 9 + || memcmp(data, PACKET_data(pkt), len)) { + fprintf(stderr, "test_PACKET_memdup() failed\n"); + OPENSSL_free(data); + return 0; + } + + OPENSSL_free(data); + return 1; +} + +static int test_PACKET_strndup() +{ + char buf[10], buf2[10]; + memset(buf, 'x', 10); + memset(buf2, 'y', 10); + buf2[5] = '\0'; + char *data = NULL; + PACKET pkt; + + if ( !PACKET_buf_init(&pkt, (unsigned char*)buf, 10) + || !PACKET_strndup(&pkt, &data) + || strlen(data) != 10 + || strncmp(data, buf, 10) + || !PACKET_buf_init(&pkt, (unsigned char*)buf2, 10) + || !PACKET_strndup(&pkt, &data) + || strlen(data) != 5 + || strcmp(data, buf2)) { + fprintf(stderr, "test_PACKET_strndup failed\n"); + OPENSSL_free(data); + return 0; + } + + OPENSSL_free(data); + return 1; +} + static int test_PACKET_move_funcs(PACKET *pkt, size_t start) { unsigned char *byte; @@ -388,6 +439,8 @@ 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_memdup(&pkt, start) + || !test_PACKET_strndup() || !test_PACKET_move_funcs(&pkt, start) || !test_PACKET_get_length_prefixed_1() || !test_PACKET_get_length_prefixed_2() -- 2.25.1