Fix hash buffer size with macro derived from configuration

[leorosen]

There are two pre-existing macros that define has buffer size requirement: PSA_HASH_MAX_SIZE and MBEDTLS_MD_MAX_SIZE for PSA and legacy cyphers respectfully. Using one of these macros (per configuration) instead of a hard-coded value of '48'.

Note that because SHA512 and SHA384 are co-dependent, the buffer size will be equal 64 bytes if any of these are enabled or 32 if the configuration only supports SHA256.

Signed-off-by: Leonid Rozenboim [email protected]

Description

See issue #6152

Status

READY

Requires Backporting

Yes

Migrations

NO

Additional comments

Issue detected by Coverity static code analysis.

Todos

• [ ] Backported

Steps to test or reproduce

Activate SHA512 algo #6152

[leorosen]

To address the doubts several of you had expressed regarding the need for this change, here is a small experiment I performed on my laptop:

diff --git a/library/ssl_tls12_client.c b/library/ssl_tls12_client.c
index e992e4bce..dce475ac9 100644
--- a/library/ssl_tls12_client.c
+++ b/library/ssl_tls12_client.c
@@ -47,6 +47,7 @@
 #include <string.h>
 
 #include <stdint.h>
+#include <assert.h>
 
 #if defined(MBEDTLS_HAVE_TIME)
 #include "mbedtls/platform_time.h"
@@ -3359,7 +3360,7 @@ static int ssl_write_certificate_verify( mbedtls_ssl_context *ssl )
     const mbedtls_ssl_ciphersuite_t *ciphersuite_info =
         ssl->handshake->ciphersuite_info;
     size_t n = 0, offset = 0;
-    unsigned char hash[HASH_MAX_SIZE];
+    unsigned char hash[HASH_MAX_SIZE] = {0};
     unsigned char *hash_start = hash;
     mbedtls_md_type_t md_alg = MBEDTLS_MD_NONE;
     size_t hashlen;
@@ -3418,6 +3419,8 @@ sign:
 #endif
 
     ssl->handshake->calc_verify( ssl, hash, &hashlen );
+    assert(hashlen == 48);
+    assert(hash[48] == 0 && hash[49] == 0);
 
     /*
      * digitally-signed struct {

Running the tests resulted in the following:

test_suite_random ................................................. PASS
test_suite_rsa .................................................... PASS
test_suite_shax ................................................... PASS
test_suite_ssl .................................................... Assertion failed: (hashlen == 48), function ssl_write_certificate_verify, file ssl_tls12_client.c, line 3422.
FAIL
Use of uninitialized value $skipped in subtraction (-) at scripts/run-test-suites.pl line 125.
Use of uninitialized value $tests in subtraction (-) at scripts/run-test-suites.pl line 125.
test_suite_timing ................................................. PASS
test_suite_version ................................................ PASS

[leorosen]

Here is another experiment for good measure:

diff --git a/library/ssl_tls12_client.c b/library/ssl_tls12_client.c
index e992e4bce..a3d2e5f32 100644
--- a/library/ssl_tls12_client.c
+++ b/library/ssl_tls12_client.c
@@ -47,6 +47,7 @@
 #include <string.h>
 
 #include <stdint.h>
+#include <assert.h>
 
 #if defined(MBEDTLS_HAVE_TIME)
 #include "mbedtls/platform_time.h"
@@ -2391,7 +2392,7 @@ start_processing:
     if( mbedtls_ssl_ciphersuite_uses_server_signature( ciphersuite_info ) )
     {
         size_t sig_len, hashlen;
-        unsigned char hash[HASH_MAX_SIZE];
+        unsigned char hash[HASH_MAX_SIZE] = {0};
         mbedtls_md_type_t md_alg = MBEDTLS_MD_NONE;
         mbedtls_pk_type_t pk_alg = MBEDTLS_PK_NONE;
         unsigned char *params = ssl->in_msg + mbedtls_ssl_hs_hdr_len( ssl );
@@ -2465,6 +2466,11 @@ start_processing:
                                                           md_alg );
             if( ret != 0 )
                 return( ret );
+
+            unsigned x = 0, i;
+            for (i = 48; i < 64; i++) x |= hash[i];
+            assert(x == 0);
+            assert(hashlen == 48);
         }
         else
         {

and here is the test result:

test_suite_rsa .................................................... PASS
test_suite_shax ................................................... PASS
test_suite_ssl .................................................... Assertion failed: (x == 0), function ssl_parse_server_key_exchange, file ssl_tls12_client.c, line 2472.
FAIL
Use of uninitialized value $skipped in subtraction (-) at scripts/run-test-suites.pl line 125.
Use of uninitialized value $tests in subtraction (-) at scripts/run-test-suites.pl line 125.
test_suite_timing ................................................. PASS

I sincerely hope this helps.

[gilles-peskine-arm]

+    assert(hashlen == 48);

Well, that's going to fail in test cases that use SHA-256, the other hash that's supported by the protocol here.

[leorosen]

+    assert(hashlen == 48);

Well, that's going to fail in test cases that use SHA-256, the other hash that's supported by the protocol here.

Thank you for this correction, should have used assert(hashlen <= 48

[leorosen]

Please see these two branches which during test fail on assertion, proving that the suspected buffer overflow happens: https://github.com/leorosen/mbedtls/tree/leorosen-stack-watch-ssl_tls12_client

Test moving clients handshake to state: CERTIFICATE_REQUEST ....... Assertion failed: (x == 0), function ssl_parse_server_key_exchange, file ssl_tls12_client.c, line 2475. https://github.com/leorosen/mbedtls/tree/leorosen-stack-watch-ssl_tls12_server

Test moving clients handshake to state: CERTIFICATE_REQUEST ....... Assertion failed: (x == 0), function ssl_prepare_server_key_exchange, file ssl_tls12_server.c, line 3114.

[gilles-peskine-arm]

The assertion that's failing is in ssl_parse_server_key_exchange, and it's failing because you're passing the wrong size to HASH_BUF_CHECK. In that function, hash is an array of PSA_HASH_MAX_SIZE or MBEDTLS_MD_MAX_SIZE bytes, not an array of 48 bytes. The hash algorithm can be any algorithm in the TLS HashAlgorithm list (or a proprietary extension, but we don't have any).

In ssl_write_certificate_verify, the hash is a 48-byte buffer, which is filled by ssl->handshake->calc_verify. The only implementations of calc_verify are ssl_calc_verify_tls_sha256 and ssl_calc_verify_tls_sha384. While the protocol allows any TLS HashAlgorithm here, Mbed TLS only supports the PRF hashes at this point. The correct symbolic constant for the hash length would be max(MAX_TLS12_PRF_HASH_LENGTH, MAX_SUPPORTED_HASH_LENGTH) and not max(MAX_TLS12_SIGALG_HASH_LENGTH, MAX_SUPPORTED_HASH_LENGTH), let alone MAX_SUPPORTED_HASH_LENGTH.

So, once again, there is no buffer overflow.

In the extremely unlikely case that a future extension to the protocol allowed other hashes, if we forgot to update the buffer size accordingly, this would be caught in testing (we do test runs with ASan, MSan and Valgrind). So I'm not worried at all about future-proofing here,

Replacing the hard-coded numbers by symbolic constant would be a good thing. But they have to be the right constants!

[tom-cosgrove-arm]

we do test runs with ASan, MSan and Valgrind

Would these catch a stack buffer overflow? IIRC ASan instruments malloc() and friends; MSan is for uninitialised access, and Valgrind doesn't catch stack buffer overflows either.

OTOH, stack cookies should do that, but - depending on other arrays on the stack in the same function - might not get triggered either

[gilles-peskine-arm]

@tom-cosgrove-arm Asan detects some stack buffer overflows. Experimentally, for example, if I change hash[48] to hash[47] in ssl_write_certificate_verify(), then in an Asan build, tests/compat.sh -p mbedTLS -V YES -t ECDSA fails with ASan errors.

[leorosen]

``