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.

Sign up for GitHub

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

Jump to bottom

PSA_CRYPTO_GENERATOR_INIT does not initialize struct psa_crypto_generator_s completely #205

Open
nanokatze opened this issue Aug 6, 2019 · 7 comments
Open

PSA_CRYPTO_GENERATOR_INIT does not initialize struct psa_crypto_generator_s completely #205

nanokatze opened this issue Aug 6, 2019 · 7 comments
Labels
bug Something isn't working Jira status: OPEN mirrored type: bug

Comments

@nanokatze
Copy link

nanokatze commented Aug 6, 2019 •
edited
Loading

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: AbsInt/CompCert#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 nanokatze changed the title PSA_CRYPTO_GENERATOR_INIT does not initialize struct psa_crypto_generator_s completely (+ more possible issues) PSA_CRYPTO_GENERATOR_INIT does not initialize struct psa_crypto_generator_s completely Aug 6, 2019
@nanokatze
Copy link
Author

nanokatze commented Aug 6, 2019 •
edited
Loading

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

@gilles-peskine-arm
Copy link
Collaborator

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
Copy link
Author

nanokatze commented Aug 6, 2019 •
edited
Loading

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
Copy link
Collaborator

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.

@gilles-peskine-arm gilles-peskine-arm added the bug Something isn't working label Aug 6, 2019
@nanokatze
Copy link
Author

nanokatze commented Aug 6, 2019 •
edited
Loading

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
Copy link
Author

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
Copy link
Member

ciarmcom commented Aug 6, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working Jira status: OPEN mirrored type: bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ciarmcom @nanokatze @gilles-peskine-arm