Compiling mbedtls with latest GCC (master/HEAD) shows alarming out of bound warnings

[mirko]

Description

• Type: Bug (potentially)
• Priority: if not a false positive, probably major/blocker

---

Bug

OS
linux

mbed TLS build:
Version: tag: mbedtls-2.18.1,
OS version: Debian Linux (buster)
Configuration: please attach config.h file where possible
Compiler: gcc (57d705da)
Compiler configure options: ./configure --build=x86_64-linux-gnu --enable-languages=c,c++ --enable-multiarch
Environment: mbedtls in above version (vanilla) as part of the micropython project (UNIX port)

Compiling with above mentioned GCC version, I'm getting alarming warnings about out of bound accesses:

CC ../../lib/mbedtls/library/aes.c
CC ../../lib/mbedtls/library/aesni.c
CC ../../lib/mbedtls/library/arc4.c
CC ../../lib/mbedtls/library/asn1parse.c
CC ../../lib/mbedtls/library/asn1write.c
CC ../../lib/mbedtls/library/base64.c
CC ../../lib/mbedtls/library/bignum.c
CC ../../lib/mbedtls/library/blowfish.c
CC ../../lib/mbedtls/library/camellia.c
CC ../../lib/mbedtls/library/ccm.c
CC ../../lib/mbedtls/library/certs.c
CC ../../lib/mbedtls/library/chacha20.c
CC ../../lib/mbedtls/library/chachapoly.c
CC ../../lib/mbedtls/library/cipher.c
CC ../../lib/mbedtls/library/cipher_wrap.c
CC ../../lib/mbedtls/library/cmac.c
CC ../../lib/mbedtls/library/ctr_drbg.c
CC ../../lib/mbedtls/library/debug.c
CC ../../lib/mbedtls/library/des.c
CC ../../lib/mbedtls/library/dhm.c
CC ../../lib/mbedtls/library/ecdh.c
CC ../../lib/mbedtls/library/ecdsa.c
CC ../../lib/mbedtls/library/ecjpake.c
CC ../../lib/mbedtls/library/ecp.c
CC ../../lib/mbedtls/library/ecp_curves.c
CC ../../lib/mbedtls/library/entropy.c
CC ../../lib/mbedtls/library/entropy_poll.c
CC ../../lib/mbedtls/library/error.c
CC ../../lib/mbedtls/library/gcm.c
CC ../../lib/mbedtls/library/havege.c
CC ../../lib/mbedtls/library/hmac_drbg.c
CC ../../lib/mbedtls/library/md2.c
CC ../../lib/mbedtls/library/md4.c
CC ../../lib/mbedtls/library/md5.c
CC ../../lib/mbedtls/library/md.c
CC ../../lib/mbedtls/library/md_wrap.c
CC ../../lib/mbedtls/library/oid.c
CC ../../lib/mbedtls/library/padlock.c
CC ../../lib/mbedtls/library/pem.c
CC ../../lib/mbedtls/library/pk.c
CC ../../lib/mbedtls/library/pkcs11.c
CC ../../lib/mbedtls/library/pkcs12.c
CC ../../lib/mbedtls/library/pkcs5.c
CC ../../lib/mbedtls/library/pkparse.c
CC ../../lib/mbedtls/library/pk_wrap.c
CC ../../lib/mbedtls/library/pkwrite.c
CC ../../lib/mbedtls/library/platform.c
CC ../../lib/mbedtls/library/platform_util.c
CC ../../lib/mbedtls/library/poly1305.c
CC ../../lib/mbedtls/library/ripemd160.c
CC ../../lib/mbedtls/library/rsa.c
CC ../../lib/mbedtls/library/rsa_internal.c
CC ../../lib/mbedtls/library/sha1.c
CC ../../lib/mbedtls/library/sha256.c
CC ../../lib/mbedtls/library/sha512.c
CC ../../lib/mbedtls/library/ssl_cache.c
CC ../../lib/mbedtls/library/ssl_ciphersuites.c
CC ../../lib/mbedtls/library/ssl_cli.c
CC ../../lib/mbedtls/library/ssl_cookie.c
CC ../../lib/mbedtls/library/ssl_srv.c
CC ../../lib/mbedtls/library/ssl_ticket.c
CC ../../lib/mbedtls/library/ssl_tls.c
CC ../../lib/mbedtls/library/timing.c
../../lib/mbedtls/library/ssl_tls.c:1686:67: error: argument 2 of type ‘unsigned char[36]’ with mismatched bound [-Werror=array-parameter=]
 1686 | void ssl_calc_verify_tls( mbedtls_ssl_context *ssl, unsigned char hash[36] )
      |                                                     ~~~~~~~~~~~~~~^~~~~~~~
../../lib/mbedtls/library/ssl_tls.c:806:57: note: previously declared as ‘unsigned char *’
  806 | static void ssl_calc_verify_tls( mbedtls_ssl_context *, unsigned char * );
      |                                                         ^~~~~~~~~~~~~~~
../../lib/mbedtls/library/ssl_tls.c:1714:74: error: argument 2 of type ‘unsigned char[32]’ with mismatched bound [-Werror=array-parameter=]
 1714 | void ssl_calc_verify_tls_sha256( mbedtls_ssl_context *ssl, unsigned char hash[32] )
      |                                                            ~~~~~~~~~~~~~~^~~~~~~~
../../lib/mbedtls/library/ssl_tls.c:813:63: note: previously declared as ‘unsigned char *’
  813 | static void ssl_calc_verify_tls_sha256( mbedtls_ssl_context *,unsigned char * );
      |                                                               ^~~~~~~~~~~~~~~
../../lib/mbedtls/library/ssl_tls.c:1757:74: error: argument 2 of type ‘unsigned char[48]’ with mismatched bound [-Werror=array-parameter=]
 1757 | void ssl_calc_verify_tls_sha384( mbedtls_ssl_context *ssl, unsigned char hash[48] )
      |                                                            ~~~~~~~~~~~~~~^~~~~~~~
../../lib/mbedtls/library/ssl_tls.c:819:64: note: previously declared as ‘unsigned char *’
  819 | static void ssl_calc_verify_tls_sha384( mbedtls_ssl_context *, unsigned char * );
      |                                                                ^~~~~~~~~~~~~~~
CC ../../lib/mbedtls/library/x509.c
CC ../../lib/mbedtls/library/x509_create.c
CC ../../lib/mbedtls/library/x509_crl.c
CC ../../lib/mbedtls/library/x509_crt.c
CC ../../lib/mbedtls/library/x509_csr.c
CC ../../lib/mbedtls/library/x509write_crt.c
CC ../../lib/mbedtls/library/x509write_csr.c
CC ../../lib/mbedtls/library/xtea.c
../../lib/mbedtls/library/ssl_tls.c: In function ‘ssl_calc_finished_tls_sha384’:
../../lib/mbedtls/library/ssl_tls.c:7586:5: error: ‘mbedtls_sha512_finish_ret’ accessing 64 bytes in a region of size 48 [-Werror=stringop-overflow=]
 7586 |     mbedtls_sha512_finish_ret( &sha512, padbuf );
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/mbedtls/library/ssl_tls.c:7586:5: note: referencing argument 2 of type ‘unsigned char *’
In file included from ../../lib/mbedtls/include/mbedtls/ssl_internal.h:53,
                 from ../../lib/mbedtls/library/ssl_tls.c:48:
../../lib/mbedtls/include/mbedtls/sha512.h:141:5: note: in a call to function ‘mbedtls_sha512_finish_ret’
  141 | int mbedtls_sha512_finish_ret( mbedtls_sha512_context *ctx,
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/mbedtls/library/ssl_tls.c: In function ‘ssl_calc_verify_tls_sha384’:
../../lib/mbedtls/library/ssl_tls.c:1788:5: error: ‘mbedtls_sha512_finish_ret’ accessing 64 bytes in a region of size 48 [-Werror=stringop-overflow=]
 1788 |     mbedtls_sha512_finish_ret( &sha512, hash );
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/mbedtls/library/ssl_tls.c:1788:5: note: referencing argument 2 of type ‘unsigned char *’
In file included from ../../lib/mbedtls/include/mbedtls/ssl_internal.h:53,
                 from ../../lib/mbedtls/library/ssl_tls.c:48:
../../lib/mbedtls/include/mbedtls/sha512.h:141:5: note: in a call to function ‘mbedtls_sha512_finish_ret’
  141 | int mbedtls_sha512_finish_ret( mbedtls_sha512_context *ctx,
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

I know those checks/warnings can be false positives and I've seen that with OpenSSL quite a few times. For mbedtls this is new to me, though. I wasn't able to properly verify this being an actual bug or a false alarm.

[gilles-peskine-arm]

TLS only uses SHA-384, which has 48 bytes of output, but SHA-384 is provided by the SHA-512 module, and SHA-512 produces 64 bytes of output. This sometimes leads static analysis to believe that a 64-byte buffer is required, because it doesn't know that the calling code only wants a SHA-384 and so 48 bytes are enough. There's already a workaround to avoid warnings with released versions of gcc (or clang?) in ssl_calc_finished_tls_sha384. Looks like we'll have to add more similar workarounds then.

Please note that we're unlikely to start testing with unreleased compilers, or to spend energy on warnings from unreleased compilers. But if you have a patch that silences this particular build of GCC and seems ok from a readability perspective, please submit it.

[mirko]

Thanks for the prompt answer.
It seems #3848 already addressed this issue in and also got backported to 2.16 and 2.7. Apparently not 2.18 though. I guess due to some release policies?

[mirko]

While the questions still stands (why no merged into 2.17, 2.18), even on branch 2.16 (where mentioend PR is merged into), I'm still getting part of above issues:

../../lib/mbedtls/library/ssl_tls.c: In function ‘ssl_calc_finished_tls_sha384’:
../../lib/mbedtls/library/ssl_tls.c:6408:5: error: ‘mbedtls_sha512_finish_ret’ accessing 64 bytes in a region of size 48 [-Werror=stringop-overflow=]
 6408 |     finish_sha384( &sha512, padbuf );
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../lib/mbedtls/library/ssl_tls.c:6408:5: note: referencing argument 2 of type ‘unsigned char *’
In file included from ../../lib/mbedtls/include/mbedtls/ssl_internal.h:74,
                 from ../../lib/mbedtls/library/ssl_tls.c:73:
../../lib/mbedtls/include/mbedtls/sha512.h:165:5: note: in a call to function ‘mbedtls_sha512_finish_ret’
  165 | int mbedtls_sha512_finish_ret( mbedtls_sha512_context *ctx,
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors

which can obviously be silenced by:

diff --git a/library/ssl_tls.c b/library/ssl_tls.c
index c749a8611..4a8ab6054 100644
--- a/library/ssl_tls.c
+++ b/library/ssl_tls.c
@@ -6372,7 +6372,7 @@ static void ssl_calc_finished_tls_sha384(
     int len = 12;
     const char *sender;
     mbedtls_sha512_context sha512;
-    unsigned char padbuf[48];
+    unsigned char padbuf[64];
     /*
      * For SHA-384, we can save 16 bytes by keeping padbuf 48 bytes long.
      * However, to avoid stringop-overflow warning in gcc, we have to cast

though I believe that's not the way to go here, as from your comment 48 bytes really should be enough in this context(?)

[gilles-peskine-arm]

Mbed TLS 2.17 and 2.18 are old releases. We only maintain the long-time support releases: 2.7 (until next month), 2.16, and the development branch.

#3848 took care of one case. I guess newer builds of gcc find more occurrences of the same root cause.

Allocating an extra 16 bytes would silence gcc, but these bytes aren't used, so we'd rather avoid this solution.

[gilles-peskine-arm]

GCC 11 has been released and still emits the false positives. Ideas to fix this without changing the prototype of mbedtls_sha512_finish_ret (which is part of the public API of Mbed TLS) and without allocating 16 more bytes unnecessarily are welcome.

[gilles-peskine-arm]

How about https://github.com/gilles-peskine-arm/mbedtls/tree/ssl-sha384-stringop-overflow-warning? Which could get in through #4493.

[gilles-peskine-arm]

This was fixed by #4493 and #4506.