wolfSSL / wolfssl

The wolfSSL library is a small, fast, portable implementation of TLS/SSL for embedded devices to the cloud. wolfSSL supports up to TLS 1.3 and DTLS 1.3!
https://www.wolfssl.com
GNU General Public License v2.0
2.33k stars 824 forks source link

[Bug]: STM32 ECC broken on all curves except four #6396

Closed maxgerhardt closed 11 months ago

maxgerhardt commented 1 year ago

Contact Details

maximilian.gerhardt@rub.de

Version

5.6.0-stable

Description

This is by far the most absurd thing I've seen in quite some time.

When compiling for STM32 (as I'm doing with my STM32WL target explained in #6386) and WolfSSL was default-configured with ECC with the NIST P-256 and P-384 curves, the self test function happily goes through.

------------------------------------------------------------------------------
 wolfSSL version 5.6.0
------------------------------------------------------------------------------
..
ECC      test passed!

When it configured for BrainPool P256 R1 and 364 respectively by using

#define HAVE_ECC_BRAINPOOL
//we don't need the NIST curves, only brainpool
#define NO_ECC_SECP
#define WOLFSSL_CUSTOM_CURVES

the test fails.

ecc_test_curve_size 32 failed!: -9938
ECC      test failed!
 error = -9938

Debugging it, springs to mind that during the ecc_test() function, it tries to generate a ECC key using the RNG to generate a random integer (0 .. order of the generating point / base) and then does a EC point multiplication with the base point. It then, in the function wc_ecc_mulmod_ex in stm32.c, tries

    /* find STM32_PKA friendly parameters for the selected curve */
    if (0 != stm32_get_ecc_specs(&prime, &coef, &coefB, &coef_sign,
            &gen_x, &gen_y, &order, size)) {
        return ECC_BAD_ARG_E;
    }

to get the parameters needed for the PKA hardware. Keep in mind we're working with the Brainpool256R1 here. Looking into that function, it does the following:

https://github.com/wolfSSL/wolfssl/blob/c95371636752af2313ebf8b476941c3791ed77ed/wolfcrypt/src/port/st/stm32.c#L749-L764

https://github.com/wolfSSL/wolfssl/blob/c95371636752af2313ebf8b476941c3791ed77ed/wolfcrypt/src/port/st/stm32.c#L653-L668

These are not the parameters for BrainPool P256R1.

>>> "Modulus for NIST P-256"
>>> 0xffffffff00000001000000000000000000000000ffffffffffffffffffffffff
115792089210356248762697446949407573530086143415290314195533631308867097853951
>>>
>>> "Modulus for Brainpool P256R1"
>>> 0xA9FB57DBA1EEA9BC3E660A909D838D726E3BF623D52620282013481D1F6E5377
76884956397045344220809746629001649093037950200943055203735601445031516197751

In fact, the function doesn't even have any parameter that would be able to identify the curve since it only receives the size information about the curve. Subsequently, a EC point multiplication is performed as if we were on NIST P-256, which leads to failures of course.

..How can it be that stm32.c tries to identify a curve purely based on its bytesize alone and does hardcoded returns of NIST-P curve parameters? This is broken for all bust NIST-P curves then.

For the signature verfication and generation functions, the right curves paramter are already saved in the ecc_key object with the ->dp member as hex strings. You could convert those back to binary and load them into the PublicKeyAccelerator hardware.

The absolute least it could have done is check the curve paramter of the dp object for the NIST-P ID and return a fatal error, informing the user.

Reproduction steps

  1. Create a STM32 + WolfSSL project with a chip that has the PKA hardware
  2. Run the wolfcrypt_test() function, ECC test should pass
  3. Configure WolfSSL for brainpool curves per above macros
  4. Run the wolfcrypt_test() function again -- ECC should fail

Relevant log output

No response

dgarske commented 1 year ago

Hi @maxgerhardt ,

The PKA ECC support for STM32 was originally designed for the NIST prime curves. Have you confirmed the PKA hardware is capable of the brainpool curves? To solve this we'd pass in the curve_id and lookup based on it, vs just the key size. Can you tell us more about your project? I'm looking into getting STM32WL boards for us to test with and add support on. Feel free to send a note to support @ wolfssl dot com if you would prefer to keep the details private.

Thanks, David Garske, wolfSSL

maxgerhardt commented 1 year ago

I just did a very rough hack in which I try to detect the used curve based on curve_id, or, in the raw "multiply curve point by integer k" where the curve ID is not available but the modulus and the "a" parameter of the curve is, load the Brainpool or NIST parameters in it, and that worked. Brainpool support is also explicitly mentioned in the reference manual as recommended curves.

image

I see the PKA hardware of my STM32WL chip as general-purpose, being able to load arbitrary numbers for modulus, curve parameters, ECC point and scalar, so I think any curve (within the bit-size limitiation of the peripheral) should work perfectly fine.

image

After doing that hack and playing around with the static memory allocator settings, self tests indeed go through.

------------------------------------------------------------------------------
 wolfSSL version 5.6.0
------------------------------------------------------------------------------
error    test passed!
MEMORY   test passed!
base64   test passed!
asn      test passed!
RANDOM   test passed!
SHA-256  test passed!
SHA-384  test passed!
Hash     test passed!
HMAC-SHA256 test passed!
HMAC-SHA384 test passed!
GMAC     test passed!
AES      test passed!
AES256   test passed!
AES-GCM  test passed!
ECC      test passed!
ECC buffer test passed!
logging  test passed!
mutex    test passed!
memcb    test passed!
Test complete

I'm working on doing the patches properly. This could should work for arbitrary numbers, so no curve detection and loading in of static data should be done, the function should convert the given parameters to the STM32 format and load it into the peripheral. This will lead to a bit of a performance hit (converting the same modulus over and over again for the same curve...) but at least it's correct in the general case. And correctness comes before speed.

dgarske commented 1 year ago

Hi @maxgerhardt ,

That is excellent. We've got STM32WL boards coming that should arrive tomorrow. If you decide to upstream the patches just open a PR. We'll then need to get a signed contributor agreement from you. If you open a PR please email support @ wolfssl dot com to get that process started.

Thanks, David Garske, wolfSSL

dgarske commented 11 months ago

Hi @maxgerhardt ,

I've pushed fixes to support non NIST Prime and custom curves to https://github.com/wolfSSL/wolfssl/pull/6937

Thanks, David Garske, wolfSSL