Define MBEDTLS_PK_SIGNATURE_MAX_SIZE #315

2019-11-05T10:36:19-06:00

gilles-peskine-arm commented

2019-11-05 10:36:19 -06:00

(Migrated from github.com)

Define MBEDTLS_PK_SIGNATURE_MAX_SIZE, which is (larger than) the maximum size of a signature produced by mbedtls_pk_sign().

Fix the sample programs pk_sign and pk_verify to use this size, instead of MBEDTLS_MPI_MAX_SIZE which 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 in config-suite-b.h with ASan enabled if MBEDTLS_PK_SIGNATURE_MAX_SIZE has the value MBEDTLS_MPI_MAX_SIZE which was previously used in programs/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:

In the Mbed TLS LTS branches, pk_sign was fixed but not pk_verify.
To make the TLS code more robust, especially when using PSA via pk, change it to use MBEDTLS_PK_SIGNATURE_MAX_SIZE.
A few bits of https://github.com/ARMmbed/mbedtls-restricted/pull/573 are not addressed here because they weren't needed to fix IOTSSL-2857. I'll make a separate PR for those.

Define `MBEDTLS_PK_SIGNATURE_MAX_SIZE`, which is (larger than) the maximum size of a signature produced by `mbedtls_pk_sign()`. Fix the sample programs `pk_sign` and `pk_verify` to use this size, instead of `MBEDTLS_MPI_MAX_SIZE` which 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 in `config-suite-b.h` with ASan enabled if `MBEDTLS_PK_SIGNATURE_MAX_SIZE` has the value `MBEDTLS_MPI_MAX_SIZE` which was previously used in `programs/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: * In the Mbed TLS LTS branches, pk_sign was fixed but not pk_verify. * To make the TLS code more robust, especially when using PSA via pk, change it to use `MBEDTLS_PK_SIGNATURE_MAX_SIZE`. * A few bits of https://github.com/ARMmbed/mbedtls-restricted/pull/573 are not addressed here because they weren't needed to fix IOTSSL-2857. I'll make a separate PR for those.

dgreen-arm (Migrated from github.com) reviewed

2019-11-08 07:06:25 -06:00

include/mbedtls/pk.h

					
				@ -104,0 +125,4 @@

				#define MBEDTLS_PK_SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE

				#endif

				#if defined(MBEDTLS_ECDSA_C) &&                                 \

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?

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?

AndrzejKurek (Migrated from github.com) reviewed

2019-11-08 07:09:55 -06:00

tests/suites/test_suite_pk.function

					
				@ -928,1 +943,4 @@

				                 rnd_std_rand, NULL, rs_ctx ) == sign_ret );

				    if( sign_ret == 0 )

				        TEST_ASSERT( sig_len <= MBEDTLS_PK_SIGNATURE_MAX_SIZE );

				    else

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?

AndrzejKurek (Migrated from github.com) reviewed

2019-11-08 07:13:54 -06:00

library/pk_wrap.c

					
				@ -775,2 +775,4 @@

				    *sig_len = rsa_alt->key_len_func( rsa_alt->key );

				    if( *sig_len > MBEDTLS_PK_SIGNATURE_MAX_SIZE )

				        return( MBEDTLS_ERR_PK_BAD_INPUT_DATA );

Shouldn't we reset the sig_len in such case?

AndrzejKurek (Migrated from github.com) reviewed

2019-11-08 07:13:59 -06:00

library/pk_wrap.c

Shouldn't we reset the sig_len in such case?

gilles-peskine-arm (Migrated from github.com) reviewed

2019-11-08 08:07:50 -06:00

tests/suites/test_suite_pk.function

					
				@ -928,1 +943,4 @@

				                 rnd_std_rand, NULL, rs_ctx ) == sign_ret );

				    if( sign_ret == 0 )

				        TEST_ASSERT( sig_len <= MBEDTLS_PK_SIGNATURE_MAX_SIZE );

				    else

This can be triggered if mbedtls_pk_sign is buggy, which is the point of a test.

If mbedtls_pk_sign writes less than MAX_SIZE bytes to its output buffer but sets sig_len to this value, the test fails here. This is a possible failure mode, but not a likely one.
If mbedtls_pk_sign writes more than MAX_SIZE bytes to its output buffer, there's a buffer overflow. Since we can't be sure how much mbedtls_pk_sign will 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 with config-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.

This can be triggered if `mbedtls_pk_sign` is buggy, which is the point of a test. * If `mbedtls_pk_sign` writes less than `MAX_SIZE` bytes to its output buffer but sets `sig_len` to this value, the test fails here. This is a possible failure mode, but not a likely one. * If `mbedtls_pk_sign` writes more than `MAX_SIZE` bytes to its output buffer, there's a buffer overflow. Since we can't be sure how much `mbedtls_pk_sign` will 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 with `config-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.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-11-08 08:08:54 -06:00

library/pk_wrap.c

					
				@ -775,2 +775,4 @@

				    *sig_len = rsa_alt->key_len_func( rsa_alt->key );

				    if( *sig_len > MBEDTLS_PK_SIGNATURE_MAX_SIZE )

				        return( MBEDTLS_ERR_PK_BAD_INPUT_DATA );

*sig_len is unspecified on error. In the mbedtls API, we usually go for minimal code size and leave whatever was there as long as it isn't sensitive.

`*sig_len` is unspecified on error. In the mbedtls API, we usually go for minimal code size and leave whatever was there as long as it isn't sensitive.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-11-08 08:11:19 -06:00

include/mbedtls/pk.h

					
				@ -104,0 +125,4 @@

				#define MBEDTLS_PK_SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE

				#endif

				#if defined(MBEDTLS_ECDSA_C) &&                                 \

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.

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.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-11-08 08:14:24 -06:00

include/mbedtls/pk.h

					
				@ -104,0 +125,4 @@

				#define MBEDTLS_PK_SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE

				#endif

				#if defined(MBEDTLS_ECDSA_C) &&                                 \

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.

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.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-11-08 09:00:31 -06:00

include/mbedtls/pk.h

					
				@ -104,0 +125,4 @@

				#define MBEDTLS_PK_SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE

				#endif

				#if defined(MBEDTLS_ECDSA_C) &&                                 \

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.

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.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-11-08 09:25:37 -06:00

include/mbedtls/pk.h

					
				@ -104,0 +125,4 @@

				#define MBEDTLS_PK_SIGNATURE_MAX_SIZE MBEDTLS_MPI_MAX_SIZE

				#endif

				#if defined(MBEDTLS_ECDSA_C) &&                                 \

Fixed.

AndrzejKurek (Migrated from github.com) reviewed

2019-11-08 09:39:14 -06:00

dgreen-arm commented

2019-11-08 09:48:09 -06:00

(Migrated from github.com)

CI looks pretty unhappy about something in the pk test suite
PSA wrapped sign .................................................. FAILED

CI looks pretty unhappy about something in the pk test suite `PSA wrapped sign .................................................. FAILED`

gilles-peskine-arm commented

2019-11-08 10:36:23 -06:00

(Migrated from github.com)

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 full, is not such a good predictor of all configurations working. Who'd'a thunk it.

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 `full`, is not such a good predictor of all configurations working. Who'd'a thunk it.

gilles-peskine-arm commented

2019-11-08 12:36:51 -06:00

(Migrated from github.com)

CI says it's still not right. I'll analyze another day.

gilles-peskine-arm commented

2019-11-12 07:46:45 -06:00

(Migrated from github.com)

@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.

dgreen-arm (Migrated from github.com) reviewed

2019-11-12 08:20:52 -06:00

AndrzejKurek (Migrated from github.com) reviewed

2019-11-13 01:15:52 -06:00

include/mbedtls/pk.h

					
				@ -104,0 +138,4 @@

				/* PSA_ASYMMETRIC_SIGNATURE_MAX_SIZE is the maximum size of a signature made

				 * through the PSA API in the PSA representation. */

				#undef MBEDTLS_PK_SIGNATURE_MAX_SIZE

				#define MBEDTLS_PK_SIGNATURE_MAX_SIZE PSA_ASYMMETRIC_SIGNATURE_MAX_SIZE

Shouldn't there be a +11 here?

AndrzejKurek (Migrated from github.com) reviewed

2019-11-13 01:19:03 -06:00

library/pk_wrap.c

					
				@ -774,6 +774,8 @@ static int rsa_alt_sign_wrap( void *ctx, mbedtls_md_type_t md_alg,

				#endif /* SIZE_MAX > UINT_MAX */

				    *sig_len = rsa_alt->key_len_func( rsa_alt->key );

				    if( *sig_len > MBEDTLS_PK_SIGNATURE_MAX_SIZE )

What if we support a feature that makes MBEDTLS_PK_SIGNATURE_MAX_SIZE bigger, but is not needed in this case?
You've mentioned that You may use a smaller buffer if it is large enough given the key type. but we have a generic check here.

What if we support a feature that makes `MBEDTLS_PK_SIGNATURE_MAX_SIZE` bigger, but is not needed in this case? You've mentioned that `You may use a smaller buffer if it is large enough given the key type.` but we have a generic check here.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-11-13 04:02:02 -06:00

include/mbedtls/pk.h

					
				@ -104,0 +138,4 @@

				/* PSA_ASYMMETRIC_SIGNATURE_MAX_SIZE is the maximum size of a signature made

				 * through the PSA API in the PSA representation. */

				#undef MBEDTLS_PK_SIGNATURE_MAX_SIZE

				#define MBEDTLS_PK_SIGNATURE_MAX_SIZE PSA_ASYMMETRIC_SIGNATURE_MAX_SIZE

Actually, there shouldn't be a + 11 above. I've amended the corresponding commit.

I'm not adding a non-regression test for this because it's currently impossible: this could only trigger if it was possible to enable a PSA signature method without enabling internal RSA or ECDSA.

Actually, there shouldn't be a `+ 11` above. I've amended the corresponding commit. I'm not adding a non-regression test for this because it's currently impossible: this could only trigger if it was possible to enable a PSA signature method without enabling internal RSA or ECDSA.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-11-13 04:17:14 -06:00

library/pk_wrap.c

					
				@ -774,6 +774,8 @@ static int rsa_alt_sign_wrap( void *ctx, mbedtls_md_type_t md_alg,

				#endif /* SIZE_MAX > UINT_MAX */

				    *sig_len = rsa_alt->key_len_func( rsa_alt->key );

				    if( *sig_len > MBEDTLS_PK_SIGNATURE_MAX_SIZE )

This is a sanity check. Ideally this function would check that the signature size is less than the buffer size, but this is impossible since it doesn't know the buffer size. The proper fix is to change the API to pass the buffer size, and we're doing this through PSA. At least here an application can choose to use MAX_SIZE as the buffer size and maybe use too much RAM but be sure that there's no risk of a buffer overflow.

I don't see any real potential for improvement here. I could add a separate constant MBEDTLS_PK_RSA_ALT_SIGNATURE_MAX_SIZE, but what would be the point? The main benefit of using the pk module is to use it without caring about the precise key type. A guarantee which is only valid for a specific key type doesn't help much.

This is a sanity check. Ideally this function would check that the signature size is less than the buffer size, but this is impossible since it doesn't know the buffer size. The proper fix is to change the API to pass the buffer size, and we're doing this through PSA. At least here an application can choose to use `MAX_SIZE` as the buffer size and maybe use too much RAM but be sure that there's no risk of a buffer overflow. I don't see any real potential for improvement here. I could add a separate constant `MBEDTLS_PK_RSA_ALT_SIGNATURE_MAX_SIZE`, but what would be the point? The main benefit of using the pk module is to use it without caring about the precise key type. A guarantee which is only valid for a specific key type doesn't help much.