From 2590aebfd9c5169e51724208481e3102217fa3c7 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 30 Jan 2025 17:59:48 +0100 Subject: [PATCH 1/7] dtls13: don't overrun hdr->epoch --- src/dtls13.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/dtls13.c b/src/dtls13.c index d2516564b..a9beec6ca 100644 --- a/src/dtls13.c +++ b/src/dtls13.c @@ -185,7 +185,8 @@ int Dtls13RlAddPlaintextHeader(WOLFSSL* ssl, byte* out, /* seq[0] combines the epoch and 16 MSB of sequence number. We write on the epoch field and will overflow to the first two bytes of the sequence number */ - c32toa(seq[0], hdr->epoch); + c16toa((word16)(seq[0] >> 16), hdr->epoch); + c16toa((word16)seq[0], hdr->sequenceNumber); c32toa(seq[1], &hdr->sequenceNumber[2]); c16toa(length, hdr->length); From d91141fe053f44895c01797619fd987c3cf1590f Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 30 Jan 2025 18:00:32 +0100 Subject: [PATCH 2/7] api: pass in sizeof(tmp) instead of 1024 to attempt to satisfy Coverity --- tests/api.c | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/tests/api.c b/tests/api.c index 05ad49af0..cd4d4f3f7 100644 --- a/tests/api.c +++ b/tests/api.c @@ -53356,54 +53356,54 @@ static int test_wolfSSL_a2i_ASN1_INTEGER(void) ExpectIntEQ(a2i_ASN1_INTEGER(bio, NULL, NULL, -1), 0); ExpectIntEQ(a2i_ASN1_INTEGER(NULL, ai, NULL, -1), 0); ExpectIntEQ(a2i_ASN1_INTEGER(NULL, NULL, tmp, -1), 0); - ExpectIntEQ(a2i_ASN1_INTEGER(NULL, NULL, NULL, 1024), 0); - ExpectIntEQ(a2i_ASN1_INTEGER(NULL, ai, tmp, 1024), 0); - ExpectIntEQ(a2i_ASN1_INTEGER(bio, NULL, tmp, 1024), 0); - ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, NULL, 1024), 0); + ExpectIntEQ(a2i_ASN1_INTEGER(NULL, NULL, NULL, sizeof(tmp)), 0); + ExpectIntEQ(a2i_ASN1_INTEGER(NULL, ai, tmp, sizeof(tmp)), 0); + ExpectIntEQ(a2i_ASN1_INTEGER(bio, NULL, tmp, sizeof(tmp)), 0); + ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, NULL, sizeof(tmp)), 0); ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, -1), 0); ExpectIntEQ(i2a_ASN1_INTEGER(NULL, NULL), 0); ExpectIntEQ(i2a_ASN1_INTEGER(bio, NULL), 0); ExpectIntEQ(i2a_ASN1_INTEGER(NULL, ai), 0); /* No data to read from BIO. */ - ExpectIntEQ(a2i_ASN1_INTEGER(out, ai, tmp, 1024), 0); + ExpectIntEQ(a2i_ASN1_INTEGER(out, ai, tmp, sizeof(tmp)), 0); /* read first line */ - ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, 1024), 1); + ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, sizeof(tmp)), 1); ExpectIntEQ(i2a_ASN1_INTEGER(out, ai), 6); - XMEMSET(tmp, 0, 1024); - tmpSz = BIO_read(out, tmp, 1024); + XMEMSET(tmp, 0, sizeof(tmp)); + tmpSz = BIO_read(out, tmp, sizeof(tmp)); ExpectIntEQ(tmpSz, 6); ExpectIntEQ(XMEMCMP(tmp, expected1, tmpSz), 0); /* fail on second line (not % 2) */ - ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, 1024), 0); + ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, sizeof(tmp)), 0); /* read 3rd long line */ - ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, 1024), 1); + ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, sizeof(tmp)), 1); ExpectIntEQ(i2a_ASN1_INTEGER(out, ai), 30); - XMEMSET(tmp, 0, 1024); - tmpSz = BIO_read(out, tmp, 1024); + XMEMSET(tmp, 0, sizeof(tmp)); + tmpSz = BIO_read(out, tmp, sizeof(tmp)); ExpectIntEQ(tmpSz, 30); ExpectIntEQ(XMEMCMP(tmp, expected2, tmpSz), 0); /* fail on empty line */ - ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, 1024), 0); + ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, sizeof(tmp)), 0); BIO_free(bio); bio = NULL; /* Make long integer, requiring dynamic memory, even longer. */ ExpectNotNull(bio = BIO_new_mem_buf(longStr, -1)); - ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, 1024), 1); + ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, sizeof(tmp)), 1); ExpectIntEQ(i2a_ASN1_INTEGER(out, ai), 48); - XMEMSET(tmp, 0, 1024); - tmpSz = BIO_read(out, tmp, 1024); + XMEMSET(tmp, 0, sizeof(tmp)); + tmpSz = BIO_read(out, tmp, sizeof(tmp)); ExpectIntEQ(tmpSz, 48); - ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, 1024), 1); + ExpectIntEQ(a2i_ASN1_INTEGER(bio, ai, tmp, sizeof(tmp)), 1); ExpectIntEQ(i2a_ASN1_INTEGER(out, ai), 56); - XMEMSET(tmp, 0, 1024); - tmpSz = BIO_read(out, tmp, 1024); + XMEMSET(tmp, 0, sizeof(tmp)); + tmpSz = BIO_read(out, tmp, sizeof(tmp)); ExpectIntEQ(tmpSz, 56); ExpectIntEQ(wolfSSL_ASN1_INTEGER_set(ai, 1), 1); BIO_free(bio); From 2865b0c79b0192c90f831f2a1f957aa8488e7f8d Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 30 Jan 2025 18:01:10 +0100 Subject: [PATCH 3/7] api: check fd values as recv and send can't take in negative fd --- tests/api.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/tests/api.c b/tests/api.c index cd4d4f3f7..1d63ae409 100644 --- a/tests/api.c +++ b/tests/api.c @@ -90726,9 +90726,10 @@ static void test_wolfSSL_dtls13_fragments_spammer(WOLFSSL* ssl) XMEMSET(&delay, 0, sizeof(delay)); delay.tv_nsec = 10000000; /* wait 0.01 seconds */ c16toa(msg_number, b + msg_offset); - sendSz = BuildTls13Message(ssl, sendBuf, sendSz, b, + ret = sendSz = BuildTls13Message(ssl, sendBuf, sendSz, b, (int)idx, handshake, 0, 0, 0); - ret = (int)send(fd, sendBuf, (size_t)sendSz, 0); + if (sendSz > 0) + ret = (int)send(fd, sendBuf, (size_t)sendSz, 0); nanosleep(&delay, NULL); } } @@ -90954,8 +90955,9 @@ static byte test_AEAD_done = 0; static int test_AEAD_cbiorecv(WOLFSSL *ssl, char *buf, int sz, void *ctx) { - int ret = (int)recv(wolfSSL_get_fd(ssl), buf, sz, 0); - if (ret > 0) { + int fd = wolfSSL_get_fd(ssl); + int ret = -1; + if (fd >= 0 && (ret = (int)recv(fd, buf, sz, 0)) > 0) { if (test_AEAD_fail_decryption) { /* Modify the packet to trigger a decryption failure */ buf[ret/2] ^= 0xFF; @@ -91271,12 +91273,16 @@ static void test_wolfSSL_dtls_send_ch_with_invalid_cookie(WOLFSSL* ssl) }; fd = wolfSSL_get_wfd(ssl); - ret = (int)send(fd, ch_msh_invalid_cookie, sizeof(ch_msh_invalid_cookie), 0); - AssertIntGT(ret, 0); - /* should reply with an illegal_parameter reply */ - ret = (int)recv(fd, alert_reply, sizeof(alert_reply), 0); - AssertIntEQ(ret, sizeof(expected_alert_reply)); - AssertIntEQ(XMEMCMP(alert_reply, expected_alert_reply, sizeof(expected_alert_reply)), 0); + if (fd >= 0) { + ret = (int)send(fd, ch_msh_invalid_cookie, + sizeof(ch_msh_invalid_cookie), 0); + AssertIntGT(ret, 0); + /* should reply with an illegal_parameter reply */ + ret = (int)recv(fd, alert_reply, sizeof(alert_reply), 0); + AssertIntEQ(ret, sizeof(expected_alert_reply)); + AssertIntEQ(XMEMCMP(alert_reply, expected_alert_reply, + sizeof(expected_alert_reply)), 0); + } } #endif From e4b7a5319163ebaa6d1b106d11b0d017365f753f Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 30 Jan 2025 18:01:51 +0100 Subject: [PATCH 4/7] api: make sure len doesn't overrun the input buffer --- tests/api.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/api.c b/tests/api.c index 1d63ae409..42122e34e 100644 --- a/tests/api.c +++ b/tests/api.c @@ -99175,6 +99175,8 @@ static int test_dtls_frag_ch_count_records(byte* b, int len) records++; dtlsRH = (DtlsRecordLayerHeader*)b; recordLen = (dtlsRH->length[0] << 8) | dtlsRH->length[1]; + if (recordLen > (size_t)len) + break; b += sizeof(DtlsRecordLayerHeader) + recordLen; len -= sizeof(DtlsRecordLayerHeader) + recordLen; } From 3cd64581ebe87aec0f08563d11c51f311d781aee Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 30 Jan 2025 18:02:34 +0100 Subject: [PATCH 5/7] dtls: better sanitize incoming messages in stateless handling --- src/dtls.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/dtls.c b/src/dtls.c index a0ba10e84..d5b8b0d4e 100644 --- a/src/dtls.c +++ b/src/dtls.c @@ -365,7 +365,8 @@ static int FindExtByType(WolfSSL_ConstVector* ret, word16 extType, ato16(exts.elements + idx, &type); idx += OPAQUE16_LEN; idx += ReadVector16(exts.elements + idx, &ext); - if (idx > exts.size) + if (idx > exts.size || + ext.elements + ext.size > exts.elements + exts.size) return BUFFER_ERROR; if (type == extType) { XMEMCPY(ret, &ext, sizeof(ext)); @@ -498,7 +499,7 @@ static int TlsCheckSupportedVersion(const WOLFSSL* ssl, ch->extension, &tlsxFound); if (ret != 0) return ret; - if (!tlsxFound) { + if (!tlsxFound || tlsxSupportedVersions.elements == NULL) { *isTls13 = 0; return 0; } From 9a8bc248dec1417ff937d35bfb8022380393f6e2 Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 30 Jan 2025 18:02:46 +0100 Subject: [PATCH 6/7] dtls: remove dead code --- src/dtls.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/dtls.c b/src/dtls.c index d5b8b0d4e..cd3b4b8c4 100644 --- a/src/dtls.c +++ b/src/dtls.c @@ -848,8 +848,6 @@ static int SendStatelessReplyDtls13(const WOLFSSL* ssl, WolfSSL_CH* ch) WOLFSSL* nonConstSSL = (WOLFSSL*)ssl; TLSX* sslExts = nonConstSSL->extensions; - if (ret != 0) - goto dtls13_cleanup; nonConstSSL->options.tls = 1; nonConstSSL->options.tls1_1 = 1; nonConstSSL->options.tls1_3 = 1; From c36d23029f5d1f8fa103329e2442ac6217fb3fcd Mon Sep 17 00:00:00 2001 From: Juliusz Sosinowicz Date: Thu, 30 Jan 2025 18:03:28 +0100 Subject: [PATCH 7/7] dtls: malloc needs to allocate the size of the dereferenced object --- src/dtls.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/dtls.c b/src/dtls.c index cd3b4b8c4..8c063bd85 100644 --- a/src/dtls.c +++ b/src/dtls.c @@ -1220,7 +1220,7 @@ int TLSX_ConnectionID_Use(WOLFSSL* ssl) info = (CIDInfo*)XMALLOC(sizeof(CIDInfo), ssl->heap, DYNAMIC_TYPE_TLSX); if (info == NULL) return MEMORY_ERROR; - ext = (WOLFSSL**)XMALLOC(sizeof(WOLFSSL**), ssl->heap, DYNAMIC_TYPE_TLSX); + ext = (WOLFSSL**)XMALLOC(sizeof(WOLFSSL*), ssl->heap, DYNAMIC_TYPE_TLSX); if (ext == NULL) { XFREE(info, ssl->heap, DYNAMIC_TYPE_TLSX); return MEMORY_ERROR;