Define MBEDTLS_PK_SIGNATURE_MAX_SIZE #315
Conversation
Based on the buffer size used in the pk_sign sample program, this is MBEDTLS_MPI_MAX_SIZE.
gilles-peskine-arm
added
bug
| #if defined(MBEDTLS_RSA_C) && \ | ||
| MBEDTLS_MPI_MAX_SIZE > MBEDTLS_PK_SIGNATURE_MAX_SIZE | ||
| #undef MBEDTLS_PK_SIGNATURE_MAX_SIZE | ||
| #define MBEDTLS_PK_SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE |
There was a problem hiding this comment.
These lines appear to unset MBEDTLS_PK_SIGNATURE_MAX_SIZE and then reset it back to MBEDTLS_MPI_MAX_SIZE. Am I missing something here?
There was a problem hiding this comment.
It's a coincidence. The undef/define gymnastic is necessary to redefine the value. While you can express max(…) using C operators, it doesn't scale. It's still ok for max(x1,x2) (x1 > x2 ? x1 : x2) but it quickly grows unmanageable for max(x1, x2, x3, …).
I'll add a comment to explain what's going on.
There was a problem hiding this comment.
Ah, but no, the code isn't working as intended! It's always setting MBEDTLS_PK_SIGNATURE_MAX_SIZE to be at least MBEDTLS_MPI_MAX_SIZE, which I wanted to avoid in the fairly common case where MBEDTLS_MPI_MAX_SIZE is kept to a large value but RSA signature is disabled and MBEDTLS_PK_SIGNATURE_MAX_SIZE encompasses ECDSA only with a much smaller maximum size.
There was a problem hiding this comment.
Uh, actually, this fallback was working as intended, sort of. It's not a fallback in the sense of “if nothing else is defined”, but something that can arise even if nothing is defined.
Except that the RSA_ALT case can't happen if RSA_ALT is not enabled, so that part was wrong.
tests/suites/test_suite_pk.function
Outdated
|
|
||
| TEST_ASSERT( mbedtls_pk_sign( &pk, MBEDTLS_MD_SHA256, hash, sizeof hash, | ||
| sig, &sig_len, rnd_std_rand, NULL ) == sign_ret ); | ||
| TEST_ASSERT( sig_len <= MBEDTLS_PK_SIGNATURE_MAX_SIZE ); |
There was a problem hiding this comment.
Is is even possible for this to be ever triggered? Won't it be a buffer overflow in such case? Can max(MBEDTLS_ECDSA_MAX_LEN, MBEDTLS_MPI_MAX_SIZE) be higher than MBEDTLS_PK_SIGNATURE_MAX_SIZE?
There was a problem hiding this comment.
This can be triggered if mbedtls_pk_sign is buggy, which is the point of a test.
- If
mbedtls_pk_signwrites less thanMAX_SIZEbytes to its output buffer but setssig_lento this value, the test fails here. This is a possible failure mode, but not a likely one. - If
mbedtls_pk_signwrites more thanMAX_SIZEbytes to its output buffer, there's a buffer overflow. Since we can't be sure how muchmbedtls_pk_signwill write, the potential buffer overflow if the code is buggy is unavoidable. I could add a safety margin and hope that the overflow doesn't go beyond that, but I don't think it's very useful: ASan detects it nicely (try it out with the commit that adds this test withconfig-suite-b.h).
max(MBEDTLS_ECDSA_MAX_LEN, MBEDTLS_MPI_MAX_SIZE) can be higher than MBEDTLS_PK_SIGNATURE_MAX_SIZE, but only if ECDSA or RSA is disabled, or if the definition of MBEDTLS_PK_SIGNATURE_MAX_SIZE is buggy.
|
CI looks pretty unhappy about something in the pk test suite |
gilles-peskine-arm
added
needs: work
|
Turns out that checking the value of a variable before setting it doesn't work. And locally testing a bunch of “exotic” configuration, but not |
gilles-peskine-arm
added
needs: review
|
CI says it's still not right. I'll analyze another day. |
gilles-peskine-arm
added
needs: work
In pk_sign_verify, if mbedtls_pk_sign() failed, sig_len was passed to mbedtls_pk_verify_restartable() without having been initialized. This worked only because in the only test case that expects signature to fail, the verify implementation doesn't look at sig_len before failing for the expected reason. The value of sig_len if sign() fails is undefined, so set sig_len to something sensible.
Add a check that the signature size from pk_sign is less than the documented maximum size. Reduce the stack consumption in pk_sign_verify.
gilles-peskine-arm
force-pushed
the
pk_signature_max_size
branch
from
bec2bb3 to
dd25db0
Compare
No intended behavior change.
This reveals that MBEDTLS_PK_SIGNATURE_MAX_SIZE is too small.
The original definition of MBEDTLS_PK_SIGNATURE_MAX_SIZE only took RSA into account. An ECDSA signature may be larger than the maximum possible RSA signature size, depending on build options; for example this is the case with config-suite-b.h.
mbedtls_pk_sign does not take the size of its output buffer as a parameter. We guarantee that MBEDTLS_PK_SIGNATURE_MAX_SIZE is enough. For RSA and ECDSA signatures made in software, this is ensured by the way MBEDTLS_PK_SIGNATURE_MAX_SIZE is defined at compile time. For signatures made through RSA-alt and PSA, this is not guaranteed robustly at compile time, but we can test it at runtime, so do that.
PSA_ASYMMETRIC_SIGNATURE_MAX_SIZE was taking the maximum ECDSA key size as the ECDSA signature size. Fix it to use the actual maximum size of an ECDSA signature.
gilles-peskine-arm
force-pushed
the
pk_signature_max_size
branch
from
dd25db0 to
be37b07
Compare
gilles-peskine-arm
added
needs: review
|
@AndrzejKurek @dgreen-arm CI passed! Please re-review. I tweaked some commits; the original branch is at https://github.com/gilles-peskine-arm/mbed-crypto/tree/pk_signature_max_size-1. |
The initial value for the max calculation needs to be 0. The fallback needs to come last. With the old code, the value was never smaller than the fallback. For RSA_ALT, use MPI_MAX_SIZE. Only use this if RSA_ALT is enabled. For PSA, check PSA_ASYMMETRIC_SIGNATURE_MAX_SIZE, and separately check the special case of ECDSA where PSA and mbedtls have different representations for the signature.
Clarify the documentation regarding the signature size. Also fix minor niggles about references to mbedtls_pk_sign().
gilles-peskine-arm
force-pushed
the
pk_signature_max_size
branch
from
be37b07 to
9db14fa
Compare
gilles-peskine-arm
removed
the
needs: review
* ARMmbed#292: Make psa_close_key(0) and psa_destroy_key(0) succeed * ARMmbed#299: Allow xxx_drbg_set_entropy_len before xxx_drbg_seed * ARMmbed#259: Check `len` against buffers size upper bound in PSA tests * ARMmbed#288: Add ECDSA tests with hash and key of different lengths * ARMmbed#305: CTR_DRBG: grab a nonce from the entropy source if needed * ARMmbed#316: Stop transactions from being reentrant * ARMmbed#317: getting_started: Make it clear that keys are passed in * ARMmbed#314: Fix pk_write with EC key to use a constant size for the private value * ARMmbed#298: Test a build without any asymmetric cryptography * ARMmbed#284: Fix some possibly-undefined variable warnings * ARMmbed#315: Define MBEDTLS_PK_SIGNATURE_MAX_SIZE * ARMmbed#318: Finish side-porting commits from mbedtls-restricted that missed the split
3 participants
Define
MBEDTLS_PK_SIGNATURE_MAX_SIZE, which is (larger than) the maximum size of a signature produced bymbedtls_pk_sign().Fix the sample programs
pk_signandpk_verifyto use this size, instead ofMBEDTLS_MPI_MAX_SIZEwhich is typically too small in builds without RSA.Add unit tests that would catch an incorrect value for
MBEDTLS_PK_SIGNATURE_MAX_SIZE. Non-regression: one of the new test cases fails inconfig-suite-b.hwith ASan enabled ifMBEDTLS_PK_SIGNATURE_MAX_SIZEhas the valueMBEDTLS_MPI_MAX_SIZEwhich was previously used inprograms/pkey/pk_sign.Internal ref: IOTSSL-2857
This was fixed in a different way in Mbed TLS 2.7, 2.16 and development (released in 2.19) (private link: https://github.com/ARMmbed/mbedtls-restricted/pull/573). We lost the fix to development due to a mistake when splitting Mbed Crypto and Mbed TLS (internal ref: IOTCRYPT-969).
Goal this PR: fix IOTSSL-2857 on the crypto side in a way that's better for users and easier to test than the original fix. Note that on the TLS side, the fix made in X.509 via #r573 is sufficient to fix IOTSSL-2857.
Follow-ups:
MBEDTLS_PK_SIGNATURE_MAX_SIZE.