Parse RSA parameters DP, DQ and QP from PKCS1 private keys #352

2020-01-22T16:37:41-06:00

jack-fortanix commented

2020-01-22 16:37:41 -06:00

(Migrated from github.com)

Otherwise these values are recomputed in mbedtls_rsa_deduce_crt, which currently suffers from side channel issues in the computation of QP (see https://eprint.iacr.org/2020/055). By loading the pre-computed values not only is the side channel avoided, but runtime overhead of loading RSA keys is reduced.

Discussion in https://github.com/ARMmbed/mbed-crypto/issues/347

Otherwise these values are recomputed in `mbedtls_rsa_deduce_crt`, which currently suffers from side channel issues in the computation of QP (see https://eprint.iacr.org/2020/055). By loading the pre-computed values not only is the side channel avoided, but runtime overhead of loading RSA keys is reduced. Discussion in https://github.com/ARMmbed/mbed-crypto/issues/347

gilles-peskine-arm commented

2020-01-23 02:51:19 -06:00

(Migrated from github.com)

Thanks for the patch! Unfortunately, the CI points out that it doesn't build when MBEDTLS_RSA_NO_CRT is defined.

var/lib/build/library/rsa.c: In function 'mbedtls_rsa_complete':
/var/lib/build/library/rsa.c:252:67: error: variable 'have_QP' set but not used [-Werror=unused-but-set-variable]
     int have_N, have_P, have_Q, have_D, have_E, have_DP, have_DQ, have_QP;
                                                                   ^
/var/lib/build/library/rsa.c:252:58: error: variable 'have_DQ' set but not used [-Werror=unused-but-set-variable]
     int have_N, have_P, have_Q, have_D, have_E, have_DP, have_DQ, have_QP;
                                                          ^
/var/lib/build/library/rsa.c:252:49: error: variable 'have_DP' set but not used [-Werror=unused-but-set-variable]
     int have_N, have_P, have_Q, have_D, have_E, have_DP, have_DQ, have_QP;
                                                 ^
cc1: all warnings being treated as errors
library/CMakeFiles/mbedcrypto.dir/build.make:1358: recipe for target 'library/CMakeFiles/mbedcrypto.dir/rsa.c.o' failed
make[2]: *** [library/CMakeFiles/mbedcrypto.dir/rsa.c.o] Error 1
make[2]: *** Waiting for unfinished jobs....
CMakeFiles/Makefile2:268: recipe for target 'library/CMakeFiles/mbedcrypto.dir/all' failed
make[1]: *** [library/CMakeFiles/mbedcrypto.dir/all] Error 2
make: *** [all] Error 2
Makefile:138: recipe for target 'all' failed
^^^^test_rsa_no_crt: build: Default + RSA_NO_CRT (ASan build): command make -> 2^^^^

To reproduce:

make clean
scripts/config.py set MBEDTLS_RSA_NO_CRT
make all test

To run all the tests we run with CRT usage disabled:

tests/scripts/all.sh test_rsa_no_crt

Thanks for the patch! Unfortunately, the CI points out that it doesn't build when `MBEDTLS_RSA_NO_CRT` is defined. ``` var/lib/build/library/rsa.c: In function 'mbedtls_rsa_complete': /var/lib/build/library/rsa.c:252:67: error: variable 'have_QP' set but not used [-Werror=unused-but-set-variable] int have_N, have_P, have_Q, have_D, have_E, have_DP, have_DQ, have_QP; ^ /var/lib/build/library/rsa.c:252:58: error: variable 'have_DQ' set but not used [-Werror=unused-but-set-variable] int have_N, have_P, have_Q, have_D, have_E, have_DP, have_DQ, have_QP; ^ /var/lib/build/library/rsa.c:252:49: error: variable 'have_DP' set but not used [-Werror=unused-but-set-variable] int have_N, have_P, have_Q, have_D, have_E, have_DP, have_DQ, have_QP; ^ cc1: all warnings being treated as errors library/CMakeFiles/mbedcrypto.dir/build.make:1358: recipe for target 'library/CMakeFiles/mbedcrypto.dir/rsa.c.o' failed make[2]: *** [library/CMakeFiles/mbedcrypto.dir/rsa.c.o] Error 1 make[2]: *** Waiting for unfinished jobs.... CMakeFiles/Makefile2:268: recipe for target 'library/CMakeFiles/mbedcrypto.dir/all' failed make[1]: *** [library/CMakeFiles/mbedcrypto.dir/all] Error 2 make: *** [all] Error 2 Makefile:138: recipe for target 'all' failed ^^^^test_rsa_no_crt: build: Default + RSA_NO_CRT (ASan build): command make -> 2^^^^ ``` To reproduce: ``` make clean scripts/config.py set MBEDTLS_RSA_NO_CRT make all test ``` To run all the tests we run with CRT usage disabled: ``` tests/scripts/all.sh test_rsa_no_crt ```

👍 1

yanesca (Migrated from github.com) reviewed

2020-01-23 08:37:51 -06:00

library/pkparse.c

					
				@ -777,0 +784,4 @@

				    /* Import DP */

				    if( ( ret = mbedtls_asn1_get_mpi( &p, end, &rsa->DP ) ) != 0)

				       goto cleanup;

Could we please add a comment here as a reminder for the future to ensure that nobody "optimises" this code out?

Basically explaining that although we could compute these values, we choose not to because this way it is faster and reduces the attack surface exposed to side channel attacks.

Could we please add a comment here as a reminder for the future to ensure that nobody "optimises" this code out? Basically explaining that although we could compute these values, we choose not to because this way it is faster and reduces the attack surface exposed to side channel attacks.

jack-fortanix commented

2020-01-23 17:10:44 -06:00

(Migrated from github.com)

@gilles-peskine-arm I addressed the build problem but it looks like Jenkins is still failing, can you forward the issue here?

tests/scripts/all.sh test_rsa_no_crt was able to reproduce the error you were seeing, but after fixing it a later build fails with

/home/jack/sw/mbed-crypto/tests/suites/test_suite_psa_crypto.function: In function ‘test_generate_random’:
suites/helpers.function:154:25: error: argument 2 value ‘18446744073709551597’ exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=]
             ( pointer ) = mbedtls_calloc( sizeof( *( pointer ) ), \
             ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                           ( length ) );           \
                                           ~~~~~~~~~~~~
/home/jack/sw/mbed-crypto/tests/suites/test_suite_psa_crypto.function:5161:5: note: in expansion of macro ‘ASSERT_ALLOC’
     ASSERT_ALLOC( changed, bytes );
     ^~~~~~~~~~~~
In file included from suites/helpers.function:5:0:
/usr/include/stdlib.h:541:14: note: in a call to allocation function ‘calloc’ declared here
 extern void *calloc (size_t __nmemb, size_t __size)
              ^~~~~~
cc1: all warnings being treated as errors

This appears to be a GCM test so I don't think has anything to do with my change - and the same error appears with development branch.

@gilles-peskine-arm I addressed the build problem but it looks like Jenkins is still failing, can you forward the issue here? `tests/scripts/all.sh test_rsa_no_crt` was able to reproduce the error you were seeing, but after fixing it a later build fails with ``` /home/jack/sw/mbed-crypto/tests/suites/test_suite_psa_crypto.function: In function ‘test_generate_random’: suites/helpers.function:154:25: error: argument 2 value ‘18446744073709551597’ exceeds maximum object size 9223372036854775807 [-Werror=alloc-size-larger-than=] ( pointer ) = mbedtls_calloc( sizeof( *( pointer ) ), \ ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ( length ) ); \ ~~~~~~~~~~~~ /home/jack/sw/mbed-crypto/tests/suites/test_suite_psa_crypto.function:5161:5: note: in expansion of macro ‘ASSERT_ALLOC’ ASSERT_ALLOC( changed, bytes ); ^~~~~~~~~~~~ In file included from suites/helpers.function:5:0: /usr/include/stdlib.h:541:14: note: in a call to allocation function ‘calloc’ declared here extern void *calloc (size_t __nmemb, size_t __size) ^~~~~~ cc1: all warnings being treated as errors ``` This appears to be a GCM test so I don't think has anything to do with my change - and the same error appears with `development` branch.

yanesca commented

2020-01-23 17:50:43 -06:00

(Migrated from github.com)

Unfortunately all.sh is very sensitive to the OS and tools used and can have such problems. As a workaround you could delete the generate_random function from tests/suites/test_suite_psa_crypto.function and the entries starting with generate_random from tests/suites/test_suite_psa_crypto.data then tests/scripts/all.sh test_rsa_no_crt should be able to reproduce the failure.

For more detailed test output you can run the individual test functions with -v option. For example: cd tests/ && ./test_suite_pk -v.

Unfortunately `all.sh` is very sensitive to the OS and tools used and can have such problems. As a workaround you could delete the `generate_random` function from `tests/suites/test_suite_psa_crypto.function` and the entries starting with `generate_random` from `tests/suites/test_suite_psa_crypto.data` then `tests/scripts/all.sh test_rsa_no_crt` should be able to reproduce the failure. For more detailed test output you can run the individual test functions with `-v` option. For example: `cd tests/ && ./test_suite_pk -v`.

jack-fortanix commented

2020-01-23 18:01:08 -06:00

(Migrated from github.com)

@yanesca Thanks, your suggestion resolved the build problem in the random tests. Unfortunately? now all.sh passes for me so I still don't know why the Jenkins builds are failing.

@yanesca Thanks, your suggestion resolved the build problem in the random tests. Unfortunately? now `all.sh` passes for me so I still don't know why the Jenkins builds are failing.

yanesca commented

2020-01-23 18:31:13 -06:00

(Migrated from github.com)

I copied the CI output here:
https://gist.github.com/yanesca/f250b5faa8e390b76eede7520e4ec84c

In the first couple of lines it prints out the OS and tools versions. Also here is my local environment (slightly more recent than the CI), I could reproduce with these too (I encountered the same compilation problem though):

* uname: /bin/uname: Linux mbedTLS-devPC1 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
                                                                      
* gcc: /usr/bin/gcc: gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0

* ldd: /usr/bin/ldd: ldd (Ubuntu GLIBC 2.27-3ubuntu1) 2.27

* asan: Version: 5.5.0-12ubuntu1

The PR should be ready after fixing this test. This won't make the CI green though, because we have an unrelated failure in the CI.

I copied the CI output here: https://gist.github.com/yanesca/f250b5faa8e390b76eede7520e4ec84c In the first couple of lines it prints out the OS and tools versions. Also here is my local environment (slightly more recent than the CI), I could reproduce with these too (I encountered the same compilation problem though): ``` * uname: /bin/uname: Linux mbedTLS-devPC1 4.15.0-74-generic #84-Ubuntu SMP Thu Dec 19 08:06:28 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux * gcc: /usr/bin/gcc: gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0 * ldd: /usr/bin/ldd: ldd (Ubuntu GLIBC 2.27-3ubuntu1) 2.27 * asan: Version: 5.5.0-12ubuntu1 ``` The PR should be ready after fixing this test. This won't make the CI green though, because we have an unrelated failure in the CI.

jack-fortanix commented

2020-01-27 16:54:54 -06:00

(Migrated from github.com)

I was not able to reproduce until I edited all.sh to add -fsanitize=leak - for whatever reason it doesn't show up with just ASan. Anyway should be resolved now.

I was not able to reproduce until I edited `all.sh` to add `-fsanitize=leak` - for whatever reason it doesn't show up with just ASan. Anyway should be resolved now.

yanesca commented

2020-01-28 06:54:11 -06:00

(Migrated from github.com)

Yes, it is resolved, the CI is only failing on known issues that are unrelated to this PR.

Aren't you on OS X by any chance? Leak analyser is turned off by default in OS X ASan:
https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer

Yes, it is resolved, the CI is only failing on known issues that are unrelated to this PR. Aren't you on OS X by any chance? Leak analyser is turned off by default in OS X ASan: https://github.com/google/sanitizers/wiki/AddressSanitizerLeakSanitizer

yanesca (Migrated from github.com) reviewed

2020-01-28 06:56:56 -06:00

yanesca (Migrated from github.com) left a comment

No description provided.

I have found a minor snag, other than that it looks good to me. (It would be nice to have it resolved, but it is not a blocker for merging.)

gilles-peskine-arm (Migrated from github.com) requested changes

2020-01-28 11:13:01 -06:00

gilles-peskine-arm (Migrated from github.com) left a comment

As noted by @yanesca, please use mbedtls_asn1_get_mpi where applicable.

As noted by @yanesca, please use `mbedtls_asn1_get_mpi` where applicable.

library/rsa.c

					
				@ -325,7 +334,7 @@ int mbedtls_rsa_complete( mbedtls_rsa_context *ctx )

				     */

Style:

    if( is_priv && ! ( have_DP && have_DQ && have_QP ) )

Style: ```suggestion if( is_priv && ! ( have_DP && have_DQ && have_QP ) ) ```

jack-fortanix commented

2020-01-28 13:44:19 -06:00

(Migrated from github.com)

Updated

gilles-peskine-arm (Migrated from github.com) approved these changes

2020-01-28 13:47:02 -06:00

yanesca (Migrated from github.com) approved these changes

2020-01-29 03:47:26 -06:00

yanesca (Migrated from github.com) left a comment

Looks good to me!

yanesca commented

2020-01-29 03:51:20 -06:00

(Migrated from github.com)

The only thing left on this PR is to do the backports. @jack-fortanix Could you please raise PRs with the same content in the Mbed TLS repository targeting the mbedtls-2.7 and mbedtls-2.16 branches?

The only thing left on this PR is to do the backports. @jack-fortanix Could you please raise PRs with the same content in the Mbed TLS repository targeting the `mbedtls-2.7` and `mbedtls-2.16` branches?

👍 1

yanesca commented

2020-01-30 06:26:49 -06:00

(Migrated from github.com)