Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build fails with unset MBEDTLS_DHM_C but MBEDTLS_USE_PSA_CRYPTO set #9188

Closed
misch7 opened this issue May 27, 2024 · 1 comment · Fixed by #9189
Closed

Build fails with unset MBEDTLS_DHM_C but MBEDTLS_USE_PSA_CRYPTO set #9188

misch7 opened this issue May 27, 2024 · 1 comment · Fixed by #9189
Labels
bug component-tls size-xs Estimated task size: extra small (a few hours at most)

Comments

@misch7
Copy link
Contributor

misch7 commented May 27, 2024

Summary

Build fails with the following custom configuration:

  • set: MBEDTLS_USE_PSA_CRYPTO
  • unset: MBEDTLS_DHM_C
  • unset: MBEDTLS_KEY_EXCHANGE_DHE_PSK_ENABLED
  • unset: MBEDTLS_KEY_EXCHANGE_DHE_RSA_ENABLED

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
  1. Build release using CMake

Additional information

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

misch7 added a commit to misch7/mbedtls that referenced this issue May 27, 2024
…set (fixes Mbed-TLS#9188)

Signed-off-by: Michael Schuster <michael@schuster.ms>
@gilles-peskine-arm gilles-peskine-arm added bug component-tls size-xs Estimated task size: extra small (a few hours at most) labels May 27, 2024
@gilles-peskine-arm
Copy link
Contributor

To reproduce this warning, you need:

  • TLS 1.2 server support enabled (MBEDTLS_SSL_SRV_C and MBEDTLS_SSL_PROTO_TLS1_2);
  • ECDH-PSK key exchange enabled (MBEDTLS_KEY_EXCHANGE_ECDHE_PSK_ENABLED);
  • FFDH disabled (no PSA_WANT_ALG_FFDH or MBEDTLS_DHM_C);
  • RSA enabled so that PSA_EXPORT_PUBLIC_KEY_MAX_SIZE >= 256 (that's where 528 comes from in the diagnostic);
  • MBEDTLS_USE_PSA_CRYPTO enabled;
  • build with Clang (any vintage we have on the CI).

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 #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.

misch7 added a commit to misch7/mbedtls that referenced this issue Jun 5, 2024
…set (fixes Mbed-TLS#9188)

Signed-off-by: Michael Schuster <michael@schuster.ms>
misch7 added a commit to misch7/mbedtls that referenced this issue Jun 11, 2024
… set (fixes Mbed-TLS#9188)

Avoid compiler warning about size comparison (like in commit 7910cdd):

Clang builds fail, warning about comparing uint8_t to a size that may be >255.

Signed-off-by: Michael Schuster <michael@schuster.ms>
misch7 added a commit to misch7/mbedtls that referenced this issue Jul 18, 2024
… set (fixes Mbed-TLS#9188)

Avoid compiler warning about size comparison (like in commit 7910cdd):

Clang builds fail, warning about comparing uint8_t to a size that may be >255.

Signed-off-by: Michael Schuster <michael@schuster.ms>
misch7 added a commit to misch7/mbedtls that referenced this issue Jul 24, 2024
… set (fixes Mbed-TLS#9188)

Avoid compiler warning about size comparison (like in commit 7910cdd):

Clang builds fail, warning about comparing uint8_t to a size that may be >255.

Signed-off-by: Michael Schuster <michael@schuster.ms>
minosgalanakis pushed a commit to minosgalanakis/mbedtls that referenced this issue Aug 6, 2024
… set (fixes Mbed-TLS#9188)

Avoid compiler warning about size comparison (like in commit 7910cdd):

Clang builds fail, warning about comparing uint8_t to a size that may be >255.

Signed-off-by: Michael Schuster <michael@schuster.ms>
minosgalanakis pushed a commit to misch7/mbedtls that referenced this issue Aug 9, 2024
… set (fixes Mbed-TLS#9188)

Avoid compiler warning about size comparison (like in commit 7910cdd):

Clang builds fail, warning about comparing uint8_t to a size that may be >255.

Signed-off-by: Michael Schuster <michael@schuster.ms>
github-merge-queue bot pushed a commit that referenced this issue Aug 12, 2024
@github-project-automation github-project-automation bot moved this to Done in Barriers Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-tls size-xs Estimated task size: extra small (a few hours at most)
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants