diff options
author | Pauli <pauli@openssl.org> | 2022-08-16 11:05:02 +1000 |
---|---|---|
committer | Pauli <pauli@openssl.org> | 2022-08-22 14:46:00 +1000 |
commit | 07ecb790b02c0e8781ae591e7efb2a4f93177354 (patch) | |
tree | c14d4265abe42ec9c6d9be1255832b60049ee9cf | |
parent | 552603edfed18f30466277d29b70939390fea65b (diff) |
Coverity 1508506: misuse of time_t
Fixes a bug in the cookie code which would have caused problems for ten
minutes before and after the lower 32 bits of time_t rolled over.
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/19022)
-rw-r--r-- | ssl/packet.c | 6 | ||||
-rw-r--r-- | ssl/packet_local.h | 37 | ||||
-rw-r--r-- | ssl/statem/extensions_srvr.c | 14 |
3 files changed, 46 insertions, 11 deletions
diff --git a/ssl/packet.c b/ssl/packet.c index 1ddde969f3..691a82b706 100644 --- a/ssl/packet.c +++ b/ssl/packet.c @@ -161,7 +161,7 @@ int WPACKET_set_flags(WPACKET *pkt, unsigned int flags) } /* Store the |value| of length |len| at location |data| */ -static int put_value(unsigned char *data, size_t value, size_t len) +static int put_value(unsigned char *data, uint64_t value, size_t len) { for (data += len - 1; len > 0; len--) { *data = (unsigned char)(value & 0xff); @@ -306,12 +306,12 @@ int WPACKET_start_sub_packet(WPACKET *pkt) return WPACKET_start_sub_packet_len__(pkt, 0); } -int WPACKET_put_bytes__(WPACKET *pkt, unsigned int val, size_t size) +int WPACKET_put_bytes__(WPACKET *pkt, uint64_t val, size_t size) { unsigned char *data; /* Internal API, so should not fail */ - if (!ossl_assert(size <= sizeof(unsigned int)) + if (!ossl_assert(size <= sizeof(uint64_t)) || !WPACKET_allocate_bytes(pkt, size, &data) || !put_value(data, val, size)) return 0; diff --git a/ssl/packet_local.h b/ssl/packet_local.h index 1b6c2fb9bc..e93680d8bb 100644 --- a/ssl/packet_local.h +++ b/ssl/packet_local.h @@ -227,6 +227,28 @@ __owur static ossl_inline int PACKET_peek_net_4(const PACKET *pkt, return 1; } +/* + * Peek ahead at 8 bytes in network order from |pkt| and store the value in + * |*data| + */ +__owur static ossl_inline int PACKET_peek_net_8(const PACKET *pkt, + uint64_t *data) +{ + if (PACKET_remaining(pkt) < 8) + return 0; + + *data = ((uint64_t)(*pkt->curr)) << 56; + *data |= ((uint64_t)(*(pkt->curr + 1))) << 48; + *data |= ((uint64_t)(*(pkt->curr + 2))) << 40; + *data |= ((uint64_t)(*(pkt->curr + 3))) << 32; + *data |= ((uint64_t)(*(pkt->curr + 4))) << 24; + *data |= ((uint64_t)(*(pkt->curr + 5))) << 16; + *data |= ((uint64_t)(*(pkt->curr + 6))) << 8; + *data |= *(pkt->curr + 7); + + return 1; +} + /* Equivalent of n2l */ /* Get 4 bytes in network order from |pkt| and store the value in |*data| */ __owur static ossl_inline int PACKET_get_net_4(PACKET *pkt, unsigned long *data) @@ -250,6 +272,17 @@ __owur static ossl_inline int PACKET_get_net_4_len(PACKET *pkt, size_t *data) return ret; } + +/* Get 8 bytes in network order from |pkt| and store the value in |*data| */ +__owur static ossl_inline int PACKET_get_net_8(PACKET *pkt, uint64_t *data) +{ + if (!PACKET_peek_net_8(pkt, data)) + return 0; + + packet_forward(pkt, 8); + + return 1; +} /* Peek ahead at 1 byte from |pkt| and store the value in |*data| */ __owur static ossl_inline int PACKET_peek_1(const PACKET *pkt, @@ -808,7 +841,7 @@ int WPACKET_sub_reserve_bytes__(WPACKET *pkt, size_t len, * 1 byte will fail. Don't call this directly. Use the convenience macros below * instead. */ -int WPACKET_put_bytes__(WPACKET *pkt, unsigned int val, size_t bytes); +int WPACKET_put_bytes__(WPACKET *pkt, uint64_t val, size_t bytes); /* * Convenience macros for calling WPACKET_put_bytes with different @@ -822,6 +855,8 @@ int WPACKET_put_bytes__(WPACKET *pkt, unsigned int val, size_t bytes); WPACKET_put_bytes__((pkt), (val), 3) #define WPACKET_put_bytes_u32(pkt, val) \ WPACKET_put_bytes__((pkt), (val), 4) +#define WPACKET_put_bytes_u64(pkt, val) \ + WPACKET_put_bytes__((pkt), (val), 8) /* Set a maximum size that we will not allow the WPACKET to grow beyond */ int WPACKET_set_max_size(WPACKET *pkt, size_t maxsize); diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index f110053273..93a9b675e6 100644 --- a/ssl/statem/extensions_srvr.c +++ b/ssl/statem/extensions_srvr.c @@ -12,16 +12,16 @@ #include "statem_local.h" #include "internal/cryptlib.h" -#define COOKIE_STATE_FORMAT_VERSION 0 +#define COOKIE_STATE_FORMAT_VERSION 1 /* * 2 bytes for packet length, 2 bytes for format version, 2 bytes for * protocol version, 2 bytes for group id, 2 bytes for cipher id, 1 byte for - * key_share present flag, 4 bytes for timestamp, 2 bytes for the hashlen, + * key_share present flag, 8 bytes for timestamp, 2 bytes for the hashlen, * EVP_MAX_MD_SIZE for transcript hash, 1 byte for app cookie length, app cookie * length bytes, SHA256_DIGEST_LENGTH bytes for the HMAC of the whole thing. */ -#define MAX_COOKIE_SIZE (2 + 2 + 2 + 2 + 2 + 1 + 4 + 2 + EVP_MAX_MD_SIZE + 1 \ +#define MAX_COOKIE_SIZE (2 + 2 + 2 + 2 + 2 + 1 + 8 + 2 + EVP_MAX_MD_SIZE + 1 \ + SSL_COOKIE_LENGTH + SHA256_DIGEST_LENGTH) /* @@ -741,7 +741,7 @@ int tls_parse_ctos_cookie(SSL *s, PACKET *pkt, unsigned int context, X509 *x, unsigned char hmac[SHA256_DIGEST_LENGTH]; unsigned char hrr[MAX_HRR_SIZE]; size_t rawlen, hmaclen, hrrlen, ciphlen; - unsigned long tm, now; + uint64_t tm, now; /* Ignore any cookie if we're not set up to verify it */ if (s->ctx->verify_stateless_cookie_cb == NULL @@ -851,7 +851,7 @@ int tls_parse_ctos_cookie(SSL *s, PACKET *pkt, unsigned int context, X509 *x, } if (!PACKET_get_1(&cookie, &key_share) - || !PACKET_get_net_4(&cookie, &tm) + || !PACKET_get_net_8(&cookie, &tm) || !PACKET_get_length_prefixed_2(&cookie, &chhash) || !PACKET_get_length_prefixed_1(&cookie, &appcookie) || PACKET_remaining(&cookie) != SHA256_DIGEST_LENGTH) { @@ -861,7 +861,7 @@ int tls_parse_ctos_cookie(SSL *s, PACKET *pkt, unsigned int context, X509 *x, } /* We tolerate a cookie age of up to 10 minutes (= 60 * 10 seconds) */ - now = (unsigned long)time(NULL); + now = time(NULL); if (tm > now || (now - tm) > 600) { /* Cookie is stale. Ignore it */ return 1; @@ -1799,7 +1799,7 @@ EXT_RETURN tls_construct_stoc_cookie(SSL *s, WPACKET *pkt, unsigned int context, &ciphlen) /* Is there a key_share extension present in this HRR? */ || !WPACKET_put_bytes_u8(pkt, s->s3->peer_tmp == NULL) - || !WPACKET_put_bytes_u32(pkt, (unsigned int)time(NULL)) + || !WPACKET_put_bytes_u64(pkt, time(NULL)) || !WPACKET_start_sub_packet_u16(pkt) || !WPACKET_reserve_bytes(pkt, EVP_MAX_MD_SIZE, &hashval1)) { SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_F_TLS_CONSTRUCT_STOC_COOKIE, |