Mbed-TLS / mbedtls

An open source, portable, easy to use, readable and flexible TLS library, and reference implementation of the PSA Cryptography API. Releases are on a varying cadence, typically around 3 - 6 months between releases.
https://www.trustedfirmware.org/projects/mbed-tls/
Other
5.04k stars 2.51k forks source link

Build fails with unset MBEDTLS_DHM_C but MBEDTLS_USE_PSA_CRYPTO set #9188

Open misch7 opened 1 month ago

misch7 commented 1 month ago

Summary

Build fails with the following custom configuration:

System information

Mbed TLS version: v3.6.0 and development branch Operating system and version: macOS 12.6 Configuration: please see "Steps to reproduce" section below Compiler and options: Apple clang version 14.0.0 (clang-1400.0.29.202), Xcode 14.2 (14C18), SDK: MacOSX13.1.sdk

Expected behavior

Compilation should work with MBEDTLS_USE_PSA_CRYPTO set, regardless of MBEDTLS_DHM_C being set or unset.

Actual behavior

Build fails with following output:

[105/118] Building C object library/CMakeFiles/mbedtls.dir/ssl_tls12_server.c.o
FAILED: library/CMakeFiles/mbedtls.dir/ssl_tls12_server.c.o 
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc  -I../include -I../library -Ilibrary -I../3rdparty/everest/include -I../3rdparty/p256-m -I../3rdparty/p256-m/p256-m -arch x86_64 -mmacosx-version-min=10.11 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -Ofast -fPIC -fvisibility=hidden -ffunction-sections -fdata-sections -Wall -Wextra -Wwrite-strings -Wpointer-arith -Wimplicit-fallthrough -Wshadow -Wvla -Wformat=2 -Wno-format-nonliteral -Werror -Wmissing-declarations -Wmissing-prototypes -Wdocumentation -Wno-documentation-deprecated-sync -Wunreachable-code -O2 -arch x86_64 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.1.sdk -mmacosx-version-min=10.11 -std=c99 -MD -MT library/CMakeFiles/mbedtls.dir/ssl_tls12_server.c.o -MF library/CMakeFiles/mbedtls.dir/ssl_tls12_server.c.o.d -o library/CMakeFiles/mbedtls.dir/ssl_tls12_server.c.o -c ../library/ssl_tls12_server.c
../library/ssl_tls12_server.c:3949:25: error: result of comparison of constant 528 with expression of type 'uint8_t' (aka 'unsigned char') is always false [-Werror,-Wtautological-constant-out-of-range-compare]
        if (ecpoint_len > sizeof(handshake->xxdh_psa_peerkey)) {
            ~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.

Steps to reproduce

  1. Clone repo and checkout development or mbedtls-3.6 branch
  2. Set custom config:
    # Run in Terminal inside the repo folder:
    scripts/config.pl unset MBEDTLS_DHM_C && \
    scripts/config.pl unset MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED && \
    scripts/config.pl unset MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED && \
    scripts/config.pl set MBEDTLS_USE_PSA_CRYPTO
  3. Build release using CMake

Additional information

I've already created a working local patch and the PR will follow soon.

gilles-peskine-arm commented 1 month ago

To reproduce this warning, you need:

We don't get this warning in the CI because we don't have a build with all of these characteristics. Checking the outcome file from https://github.com/Mbed-TLS/mbedtls/pull/9172, only the following builds have MBEDTLS_RSA_C but not MBEDTLS_DHM_C:

component_test_full_no_bignum
component_test_psa_crypto_config_accel_ffdh
config-no-entropy.h

full_no_bignum having RSA enabled is a bug, but anyway it legitimately doesn't have DHM. psa_crypto_config_accel_ffdh has FFDH, just accelerated. config-no-entropy.h doesn't have TLS.

So we have a test coverage gap. I need to think a little what new configuration(s) we should be testing. Considering how the relative sizes of various things in TLS depend on whether RSA or FFDH are enabled, there are potential buffer overflows that we aren't checking.