From 272cc19ab5dcf8a7dc1fc80323e1f4348512c544 Mon Sep 17 00:00:00 2001 From: Ashley Duncan Date: Fri, 11 Feb 2022 09:57:18 +1300 Subject: [PATCH 1/6] Fixed undefined behavior in ssl_read if buf parameter is NULL. Signed-off-by: Ashley Duncan --- library/ssl_msg.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index a38e764403..8d35c9c00b 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5429,8 +5429,10 @@ int mbedtls_ssl_read(mbedtls_ssl_context *ssl, unsigned char *buf, size_t len) n = (len < ssl->in_msglen) ? len : ssl->in_msglen; - memcpy(buf, ssl->in_offt, n); - ssl->in_msglen -= n; + if (buf) { + memcpy(buf, ssl->in_offt, n); + ssl->in_msglen -= n; + } /* Zeroising the plaintext buffer to erase unused application data from the memory. */ From cf01d78e7ef546b934b0727da3ea372efe3ce94b Mon Sep 17 00:00:00 2001 From: ashesman Date: Thu, 17 Feb 2022 11:08:27 +1300 Subject: [PATCH 2/6] Update library/ssl_msg.c Co-authored-by: Gilles Peskine Signed-off-by: Dave Rodgman --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 8d35c9c00b..db0299edad 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5429,7 +5429,7 @@ int mbedtls_ssl_read(mbedtls_ssl_context *ssl, unsigned char *buf, size_t len) n = (len < ssl->in_msglen) ? len : ssl->in_msglen; - if (buf) { + if (len != 0) { memcpy(buf, ssl->in_offt, n); ssl->in_msglen -= n; } From 13938b84e95c5508f497ebfcc831901b3a355d00 Mon Sep 17 00:00:00 2001 From: Ashley Duncan Date: Thu, 17 Feb 2022 11:10:33 +1300 Subject: [PATCH 3/6] Added changelog entry. Signed-off-by: Ashley Duncan --- ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt diff --git a/ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt b/ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt new file mode 100644 index 0000000000..392a91b729 --- /dev/null +++ b/ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt @@ -0,0 +1,2 @@ +Bugfix + * Fixed undefined behavior in mbedtls_ssl_read if len argument is 0 From 1215557e91e551031d89cb89e235ec3483ac7a91 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 24 Feb 2023 15:41:34 +0000 Subject: [PATCH 4/6] Add corresponding fix for mbedtls_ssl_write Signed-off-by: Dave Rodgman --- library/ssl_msg.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index db0299edad..8a2ab7b9ba 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5508,7 +5508,9 @@ static int ssl_write_real(mbedtls_ssl_context *ssl, */ ssl->out_msglen = len; ssl->out_msgtype = MBEDTLS_SSL_MSG_APPLICATION_DATA; - memcpy(ssl->out_msg, buf, len); + if (len > 0) { + memcpy(ssl->out_msg, buf, len); + } if ((ret = mbedtls_ssl_write_record(ssl, SSL_FORCE_FLUSH)) != 0) { MBEDTLS_SSL_DEBUG_RET(1, "mbedtls_ssl_write_record", ret); From fb07c37cb101798d004191372aa1ccd573feed1b Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 24 Feb 2023 15:43:43 +0000 Subject: [PATCH 5/6] Improve changelog Signed-off-by: Dave Rodgman --- ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt b/ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt index 392a91b729..1f2c563be3 100644 --- a/ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt +++ b/ChangeLog.d/mbedtls_ssl_read_undefined_behavior.txt @@ -1,2 +1,3 @@ Bugfix - * Fixed undefined behavior in mbedtls_ssl_read if len argument is 0 + * Fix undefined behavior in mbedtls_ssl_read() and mbedtls_ssl_write() if + len argument is 0 and buffer is NULL. From cd09d68eb1de3493c74ba72d5d69d6ab9a8bd9c4 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 24 Feb 2023 15:41:55 +0000 Subject: [PATCH 6/6] Add tests Signed-off-by: Dave Rodgman --- tests/suites/test_suite_ssl.function | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/suites/test_suite_ssl.function b/tests/suites/test_suite_ssl.function index 1dec18d389..39825ed969 100644 --- a/tests/suites/test_suite_ssl.function +++ b/tests/suites/test_suite_ssl.function @@ -1052,6 +1052,12 @@ int mbedtls_ssl_write_fragment(mbedtls_ssl_context *ssl, unsigned char *buf, int buf_len, int *written, const int expected_fragments) { + /* Verify that calling mbedtls_ssl_write with a NULL buffer and zero length is + * a valid no-op for TLS connections. */ + if (ssl->conf->transport != MBEDTLS_SSL_TRANSPORT_DATAGRAM) { + TEST_ASSERT(mbedtls_ssl_write(ssl, NULL, 0) == 0); + } + int ret = mbedtls_ssl_write(ssl, buf + *written, buf_len - *written); if (ret > 0) { *written += ret; @@ -1090,6 +1096,12 @@ int mbedtls_ssl_read_fragment(mbedtls_ssl_context *ssl, unsigned char *buf, int buf_len, int *read, int *fragments, const int expected_fragments) { + /* Verify that calling mbedtls_ssl_write with a NULL buffer and zero length is + * a valid no-op for TLS connections. */ + if (ssl->conf->transport != MBEDTLS_SSL_TRANSPORT_DATAGRAM) { + TEST_ASSERT(mbedtls_ssl_read(ssl, NULL, 0) == 0); + } + int ret = mbedtls_ssl_read(ssl, buf + *read, buf_len - *read); if (ret > 0) { (*fragments)++;