From e8a557dc3c1ed16faff4aeb39268f8f5a3f8b81d Mon Sep 17 00:00:00 2001 From: Pauli Date: Tue, 16 Aug 2022 11:05:02 +1000 Subject: [PATCH] Coverity: misuses of time_t Coverity 1508506: 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. Coverity 1508534 & 1508540: Avoid problems when the lower 32 bits of time_t roll over by delaying the cast to integer until after the time delta has been computed. Reviewed-by: Ben Kaduk Reviewed-by: Matt Caswell (Merged from https://github.com/openssl/openssl/pull/19004) --- ssl/statem/extensions_clnt.c | 5 ++--- ssl/statem/extensions_srvr.c | 19 +++++++++---------- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/ssl/statem/extensions_clnt.c b/ssl/statem/extensions_clnt.c index ace5b58625f..3a315cb65fc 100644 --- a/ssl/statem/extensions_clnt.c +++ b/ssl/statem/extensions_clnt.c @@ -1005,7 +1005,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL_CONNECTION *s, WPACKET *pkt, X509 *x, size_t chainidx) { #ifndef OPENSSL_NO_TLS1_3 - uint32_t now, agesec, agems = 0; + uint32_t agesec, agems = 0; size_t reshashsize = 0, pskhashsize = 0, binderoffset, msglen; unsigned char *resbinder = NULL, *pskbinder = NULL, *msgstart = NULL; const EVP_MD *handmd = NULL, *mdres = NULL, *mdpsk = NULL; @@ -1062,8 +1062,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL_CONNECTION *s, WPACKET *pkt, * this in multiple places in the code, so portability shouldn't be an * issue. */ - now = (uint32_t)time(NULL); - agesec = now - (uint32_t)s->session->time; + agesec = (uint32_t)(time(NULL) - s->session->time); /* * We calculate the age in seconds but the server may work in ms. Due to * rounding errors we could overestimate the age by up to 1s. It is diff --git a/ssl/statem/extensions_srvr.c b/ssl/statem/extensions_srvr.c index 311b89878f1..747b9fe9a4b 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) /* @@ -699,7 +699,7 @@ int tls_parse_ctos_cookie(SSL_CONNECTION *s, PACKET *pkt, unsigned int context, 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; SSL *ssl = SSL_CONNECTION_GET_SSL(s); SSL_CTX *sctx = SSL_CONNECTION_GET_CTX(s); @@ -802,7 +802,7 @@ int tls_parse_ctos_cookie(SSL_CONNECTION *s, PACKET *pkt, unsigned int context, } 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) { @@ -811,7 +811,7 @@ int tls_parse_ctos_cookie(SSL_CONNECTION *s, PACKET *pkt, unsigned int context, } /* 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; @@ -1102,7 +1102,7 @@ int tls_parse_ctos_psk(SSL_CONNECTION *s, PACKET *pkt, unsigned int context, s->ext.early_data_ok = 1; s->ext.ticket_expected = 1; } else { - uint32_t ticket_age = 0, now, agesec, agems; + uint32_t ticket_age = 0, agesec, agems; int ret; /* @@ -1142,8 +1142,7 @@ int tls_parse_ctos_psk(SSL_CONNECTION *s, PACKET *pkt, unsigned int context, } ticket_age = (uint32_t)ticket_agel; - now = (uint32_t)time(NULL); - agesec = now - (uint32_t)sess->time; + agesec = (uint32_t)(time(NULL) - sess->time); agems = agesec * (uint32_t)1000; ticket_age -= sess->ext.tick_age_add; @@ -1764,7 +1763,7 @@ EXT_RETURN tls_construct_stoc_cookie(SSL_CONNECTION *s, WPACKET *pkt, &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, ERR_R_INTERNAL_ERROR); -- 2.47.2