gh-ARMmbed/mbed-crypto

Watch

Fork 0

PSA_CRYPTO_GENERATOR_INIT does not initialize struct psa_crypto_generator_s completely #205

New issue

Open

opened 2019-08-06 12:45:01 -05:00 by nanokatze · 7 comments

nanokatze commented

2019-08-06 12:45:01 -05:00

(Migrated from github.com)

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)

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

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

2019-08-06 12:49:43 -05:00

(Migrated from github.com)

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

gilles-peskine-arm commented

2019-08-06 13:06:47 -05:00

(Migrated from github.com)

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.

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

2019-08-06 13:21:13 -05:00

(Migrated from github.com)

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

> 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

2019-08-06 13:28:08 -05:00

(Migrated from github.com)

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.

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

2019-08-06 13:36:22 -05:00

(Migrated from github.com)

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

> 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

2019-08-06 14:37:19 -05:00

(Migrated from github.com)

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.

> 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

2019-08-06 15:46:52 -05:00

(Migrated from github.com)