search

ARMmbed / mbed-crypto

The development of Mbed Crypto has moved to Mbed TLS. No updates will be made to the mbed-crypto repository anymore.
Apache License 2.0
104 stars 98 forks source link

PSA_CRYPTO_GENERATOR_INIT does not initialize struct psa_crypto_generator_s completely #205

Open nanokatze opened 5 years ago

nanokatze commented 5 years ago

Description

Mbed Crypto 1.1.1 Toolchain GCC 9.1.1 with CompCert 3.5 Target Linux 5.1.20 x86_64 (Fedora 30 5.1.20-300)

Related: https://github.com/AbsInt/CompCert/issues/312

When compiling tag mbedcrypto-1.1.1 with CompCert 3.5, host and target are Fedora 30 5.1.20-300 x86_64, toolchain is taken from GCC 9.1.1, using

make test CC=ccomp CFLAGS='-O1 -fbitfields -fstruct-passing -finline-asm' WARNING_CFLAGS= DEBUG=1 -j

The library's test suite test_suite_psa_crypto has two tests fail:

PSA key derivation: HKDF SHA-256, RFC5869 #1, output 42+0 ......... FAILED
  ( ( psa_key_derivation( &generator, handle, alg, salt->x, salt->len, label->x, label->len, requested_capacity ) ) ) == ( ((psa_status_t)0) )
  at line 4248, suites/test_suite_psa_crypto.function

and

PSA key derivation: HKDF SHA-256, exercise HKDF-SHA-256 ........... FAILED
  ( ( psa_key_derivation( &generator, handle, alg, label, label_length, seed, seed_length, sizeof( output ) ) ) ) == ( ((psa_status_t)0) )
  at line 532, suites/test_suite_psa_crypto.function

Both tests pass on GCC and clang because it appears that those simply zero out memory of the whole structure, but on CompCert, where only select fields are initialized, code will read uninitialized memory when it accesses some fields of hkdf or tls12_prf. https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/tests/suites/test_suite_psa_crypto.function#L523 https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/tests/suites/test_suite_psa_crypto.function#L4216

Standard reference: ISO C 99, section 6.7.8, paragraph 17: Each brace-enclosed initializer list has an associated current object. When no designations are present, subobjects of the current object are initialized in order according to the type of the current object: array elements in increasing subscript order, structure members in declaration order, and the first named member of a union.

Issue request type

[ ] Question
[ ] Enhancement
[X] Bug
nanokatze commented 5 years ago

This issue probably should be generalized to other structures with union members as well.

gilles-peskine-arm commented 5 years ago

That issue has come up before (MSVC also doesn't zero out the whole structure) and we at the time decided that Mbed Crypto would not go out of its way to initialize the structure to all-bits-zero. (Because the content of the union depends on build options, determining its size at compile time is annoying.) If a test accesses an algorithm-dependent part of the structure, that's a bug in the test or in the library. Presumably in the library since psa_key_derivation is failing. Do you have a way to detect where exactly the code reads uninitialized memory?

Would you mind trying the branch https://github.com/ARMmbed/mbed-crypto/tree/psa-api-1.0-beta which has a newer key derivation API? If the bug is specific to the old generator API, I think we'll let it skip.

nanokatze commented 5 years ago

Do you have a way to detect where exactly the code reads uninitialized memory?

Yes. Running CompCert program under valgrind seems to work.

One of the tests follows this call chain

  1. https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/tests/suites/test_suite_psa_crypto.function#L532
  2. https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/library/psa_crypto.c#L4296
  3. https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/library/psa_crypto.c#L4211
  4. https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/library/psa_crypto.c#L4042
  5. https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/library/psa_crypto.c#L1985
  6. https://github.com/ARMmbed/mbed-crypto/blob/mbedcrypto-1.1.1/library/psa_crypto.c#L1390
gilles-peskine-arm commented 5 years ago

Oh, nice! Thank you!

I see what's going on: the HKDF code accesses low-level HMAC functions and violates one of their assumptions. Specifically, psa_hkdf_setup_internal calls psa_hmac_setup_internal, but unlike the MAC interface, it doesn't call psa_mac_init before, and so the alg field in the hash operation is never initialized to 0, which ends up triggering a BAD_STATE error in psa_hash_setup.

We really need to find a way to test this, or a to run static analyzer that catches it.

nanokatze commented 5 years ago

We really need to find a way to test this, or a to run static analyzer that catches it.

Unfortunately, -fsanitize=address nor undefined seem to catch this issue (when used in GCC and clang) and CompCert has no support for these sanitizers. I have found that feeding valgrind with CompCert's output seems to produce more or less useful results.

PSA key derivation: HKDF SHA-256, exercise HKDF-SHA-256 ........... ==28815== Conditional jump or move depends on uninitialised value(s)
==28815==    at 0x42305D: psa_hash_abort (psa_crypto.c:1331)
==28815==    by 0x42617B: psa_generator_abort (psa_crypto.c:3630)
==28815==    by 0x427579: psa_key_derivation (psa_crypto.c:4304)
==28815==    by 0x40286E: exercise_key (test_suite_psa_crypto.function:532)
==28815==    by 0x416EFE: test_derive_key_exercise (test_suite_psa_crypto.function:4415)
==28815==    by 0x416FD9: test_derive_key_exercise_wrapper (test_suite_psa_crypto.function:4431)
==28815==    by 0x419871: dispatch_test (main_test.function:189)
==28815==    by 0x41A910: execute_tests (host_test.function:572)
==28815==    by 0x41ACB9: main (main_test.function:258)
==28815== 
==28815== Conditional jump or move depends on uninitialised value(s)
==28815==    at 0x4230AA: psa_hash_abort (psa_crypto.c:1355)
==28815==    by 0x42617B: psa_generator_abort (psa_crypto.c:3630)
==28815==    by 0x427579: psa_key_derivation (psa_crypto.c:4304)
==28815==    by 0x40286E: exercise_key (test_suite_psa_crypto.function:532)
==28815==    by 0x416EFE: test_derive_key_exercise (test_suite_psa_crypto.function:4415)
==28815==    by 0x416FD9: test_derive_key_exercise_wrapper (test_suite_psa_crypto.function:4431)
==28815==    by 0x419871: dispatch_test (main_test.function:189)
==28815==    by 0x41A910: execute_tests (host_test.function:572)
==28815==    by 0x41ACB9: main (main_test.function:258)
==28815== 
==28815== Conditional jump or move depends on uninitialised value(s)
==28815==    at 0x4230C5: psa_hash_abort (psa_crypto.c:1366)
==28815==    by 0x42617B: psa_generator_abort (psa_crypto.c:3630)
==28815==    by 0x427579: psa_key_derivation (psa_crypto.c:4304)
==28815==    by 0x40286E: exercise_key (test_suite_psa_crypto.function:532)
==28815==    by 0x416EFE: test_derive_key_exercise (test_suite_psa_crypto.function:4415)
==28815==    by 0x416FD9: test_derive_key_exercise_wrapper (test_suite_psa_crypto.function:4431)
==28815==    by 0x419871: dispatch_test (main_test.function:189)
==28815==    by 0x41A910: execute_tests (host_test.function:572)
==28815==    by 0x41ACB9: main (main_test.function:258)
==28815== 
==28815== Conditional jump or move depends on uninitialised value(s)
==28815==    at 0x4230CD: psa_hash_abort (psa_crypto.c:1366)
==28815==    by 0x42617B: psa_generator_abort (psa_crypto.c:3630)
==28815==    by 0x427579: psa_key_derivation (psa_crypto.c:4304)
==28815==    by 0x40286E: exercise_key (test_suite_psa_crypto.function:532)
==28815==    by 0x416EFE: test_derive_key_exercise (test_suite_psa_crypto.function:4415)
==28815==    by 0x416FD9: test_derive_key_exercise_wrapper (test_suite_psa_crypto.function:4431)
==28815==    by 0x419871: dispatch_test (main_test.function:189)
==28815==    by 0x41A910: execute_tests (host_test.function:572)
==28815==    by 0x41ACB9: main (main_test.function:258)
==28815== 
FAILED
  ( ( psa_key_derivation( &generator, handle, alg, label, label_length, seed, seed_length, sizeof( output ) ) ) ) == ( ((psa_status_t)0) )
  at line 532, suites/test_suite_psa_crypto.function

This in fact picked up more stuff such as this:

PSA key agreement: ECDH SECP256R1 (RFC 5903) + HKDF-SHA-256: read   ==28815== Conditional jump or move depends on uninitialised value(s)
==28815==    at 0x423118: psa_hash_setup (psa_crypto.c:1388)
==28815==    by 0x423D15: psa_hmac_setup_internal (psa_crypto.c:1985)
==28815==    by 0x427245: psa_key_derivation_internal (psa_crypto.c:4042)
==28815==    by 0x427814: psa_key_agreement (psa_crypto.c:4399)
==28815==    by 0x417D61: test_key_agreement_output (test_suite_psa_crypto.function:4621)
==28815==    by 0x417F77: test_key_agreement_output_wrapper (test_suite_psa_crypto.function:4654)
==28815==    by 0x419871: dispatch_test (main_test.function:189)
==28815==    by 0x41A910: execute_tests (host_test.function:572)
==28815==    by 0x41ACB9: main (main_test.function:258)
==28815== 
PASS
nanokatze commented 5 years ago

Would you mind trying the branch https://github.com/ARMmbed/mbed-crypto/tree/psa-api-1.0-beta which has a newer key derivation API? If the bug is specific to the old generator API, I think we'll let it skip.

I have just compiled that tag and ran tests in valgrind, it has reported no errors.

ciarmcom commented 5 years ago

Internal Jira reference: https://jira.arm.com/browse/IOTCRYPT-853