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

jack-fortanix · 2020-01-22T22:37:41Z

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 #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 ARMmbed#347

Currently they are ignored in the serialized key and then regenerated from P/Q/D. But that exposes key loading to a side channel attack on the modular inversion and GCD bignum functions when QP is computed. [And probably similar issues for DP/DQ during the division step but no attack there has been published yet.] Backport of ARMmbed/mbed-crypto#352

gilles-peskine-arm · 2020-01-23T08:51:19Z

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

yanesca · 2020-01-23T14:37:50Z

library/pkparse.c

@@ -769,14 +769,29 @@ static int pk_parse_key_pkcs1_der( mbedtls_rsa_context *rsa,
        goto cleanup;
    p += len;

-    /* Complete the RSA private key */
-    if( ( ret = mbedtls_rsa_complete( rsa ) ) != 0 )
+    /* Import DP */


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 · 2020-01-23T23:10:44Z

@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 · 2020-01-23T23:50:43Z

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 · 2020-01-24T00:01:08Z

@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 · 2020-01-24T00:31:13Z

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 · 2020-01-27T22:54:54Z

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 · 2020-01-28T12:54:11Z

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

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

library/pkparse.c

gilles-peskine-arm

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

library/rsa.c

jack-fortanix · 2020-01-28T19:44:19Z

Updated

yanesca

Looks good to me!

yanesca · 2020-01-29T09:51:20Z

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?

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 ARMmbed/mbed-crypto#347 Backport of ARMmbed/mbed-crypto#352

yanesca · 2020-01-30T12:26:49Z

Thank you for submitting the backports!

Currently they are ignored in the serialized key and then regenerated from P/Q/D. But that exposes key loading to a side channel attack on the modular inversion and GCD bignum functions when QP is computed. [And probably similar issues for DP/DQ during the division step but no attack there has been published yet.] Backport of ARMmbed/mbed-crypto#352

= mbed TLS 2.16.5 branch released 2020-02-20 Security * Fix potential memory overread when performing an ECDSA signature operation. The overread only happens with cryptographically low probability (of the order of 2^-n where n is the bitsize of the curve) unless the RNG is broken, and could result in information disclosure or denial of service (application crash or extra resource consumption). Found by Auke Zeilstra and Peter Schwabe, using static analysis. * To avoid a side channel vulnerability when parsing an RSA private key, read all the CRT parameters from the DER structure rather than reconstructing them. Found by Alejandro Cabrera Aldaya and Billy Bob Brumley. Reported and fix contributed by Jack Lloyd. ARMmbed/mbed-crypto#352 Bugfix * Fix an unchecked call to mbedtls_md() in the x509write module. * Fix a bug in mbedtls_pk_parse_key() that would cause it to accept some RSA keys that would later be rejected by functions expecting private keys. Found by Catena cyber using oss-fuzz (issue 20467). * Fix a bug in mbedtls_pk_parse_key() that would cause it to accept some RSA keys with invalid values by silently fixing those values.

security/mbedtls: security fix Revisions pulled up: - security/mbedtls/Makefile 1.12 - security/mbedtls/PLIST 1.6 - security/mbedtls/distinfo 1.8 --- Module Name: pkgsrc Committed By: nia Date: Sat Feb 29 11:45:02 UTC 2020 Modified Files: pkgsrc/security/mbedtls: Makefile PLIST distinfo Log Message: mbedtls: Update to 2.16.5 = mbed TLS 2.16.5 branch released 2020-02-20 Security * Fix potential memory overread when performing an ECDSA signature operation. The overread only happens with cryptographically low probability (of the order of 2^-n where n is the bitsize of the curve) unless the RNG is broken, and could result in information disclosure or denial of service (application crash or extra resource consumption). Found by Auke Zeilstra and Peter Schwabe, using static analysis. * To avoid a side channel vulnerability when parsing an RSA private key, read all the CRT parameters from the DER structure rather than reconstructing them. Found by Alejandro Cabrera Aldaya and Billy Bob Brumley. Reported and fix contributed by Jack Lloyd. ARMmbed/mbed-crypto#352 Bugfix * Fix an unchecked call to mbedtls_md() in the x509write module. * Fix a bug in mbedtls_pk_parse_key() that would cause it to accept some RSA keys that would later be rejected by functions expecting private keys. Found by Catena cyber using oss-fuzz (issue 20467). * Fix a bug in mbedtls_pk_parse_key() that would cause it to accept some RSA keys with invalid values by silently fixing those values.

90: Fix ECDSA side channel r=jethrogb a=jack-fortanix Backport of ARMmbed/mbed-crypto@247c4d3 which addresses the attack described in https://eprint.iacr.org/2020/055.pdf 91: Parse RSA CRT parameters from PKCS1 private keys r=jethrogb a=jack-fortanix Currently they are ignored in the serialized key and then regenerated from P/Q/D. But that exposes key loading to a side channel attack on the modular inversion and GCD bignum functions when QP is computed. [And probably similar issues for DP/DQ during the division step but no attack there has been published yet.] Also this reduces computational overhead of loading RSA private keys from memory which will be nice for us in roche. Backport of ARMmbed/mbed-crypto#352 Co-authored-by: Jack Lloyd <jack.lloyd@fortanix.com>

90: Fix ECDSA side channel r=jethrogb a=jack-fortanix Backport of ARMmbed/mbed-crypto@247c4d3 which addresses the attack described in https://eprint.iacr.org/2020/055.pdf 91: Parse RSA CRT parameters from PKCS1 private keys r=jethrogb a=jack-fortanix Currently they are ignored in the serialized key and then regenerated from P/Q/D. But that exposes key loading to a side channel attack on the modular inversion and GCD bignum functions when QP is computed. [And probably similar issues for DP/DQ during the division step but no attack there has been published yet.] Also this reduces computational overhead of loading RSA private keys from memory which will be nice for us in roche. Backport of ARMmbed/mbed-crypto#352 94: Fix docs on macro invocations r=jethrogb a=jethrogb Co-authored-by: Jack Lloyd <jack.lloyd@fortanix.com> Co-authored-by: Jethro Beekman <jethro@fortanix.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 ARMmbed/mbed-crypto#347 Backport of ARMmbed/mbed-crypto#352

* ARMmbed#352: Parse RSA parameters DP, DQ and QP from PKCS1 private keys * ARMmbed#263: Introduce ASN.1 SEQUENCE traversal API * ARMmbed#345: Fix possible error code mangling in psa_mac_verify_finish * ARMmbed#357: Update Mbed Crypto with latest Mbed TLS changes as of 2020-02-03 * ARMmbed#350: test_suite_asn1parse: improve testing of trailing garbage in parse_prefixes * ARMmbed#346: Improve robustness and testing of mbedtls_mpi_copy

security/mbedtls: security fix Revisions pulled up: - security/mbedtls/Makefile 1.12 - security/mbedtls/PLIST 1.6 - security/mbedtls/distinfo 1.8 --- Module Name: pkgsrc Committed By: nia Date: Sat Feb 29 11:45:02 UTC 2020 Modified Files: pkgsrc/security/mbedtls: Makefile PLIST distinfo Log Message: mbedtls: Update to 2.16.5 = mbed TLS 2.16.5 branch released 2020-02-20 Security * Fix potential memory overread when performing an ECDSA signature operation. The overread only happens with cryptographically low probability (of the order of 2^-n where n is the bitsize of the curve) unless the RNG is broken, and could result in information disclosure or denial of service (application crash or extra resource consumption). Found by Auke Zeilstra and Peter Schwabe, using static analysis. * To avoid a side channel vulnerability when parsing an RSA private key, read all the CRT parameters from the DER structure rather than reconstructing them. Found by Alejandro Cabrera Aldaya and Billy Bob Brumley. Reported and fix contributed by Jack Lloyd. ARMmbed/mbed-crypto#352 Bugfix * Fix an unchecked call to mbedtls_md() in the x509write module. * Fix a bug in mbedtls_pk_parse_key() that would cause it to accept some RSA keys that would later be rejected by functions expecting private keys. Found by Catena cyber using oss-fuzz (issue 20467). * Fix a bug in mbedtls_pk_parse_key() that would cause it to accept some RSA keys with invalid values by silently fixing those values.

jack-fortanix mentioned this pull request Jan 22, 2020

Plan to address RSA attacks of eprint.iacr.org/2020/055? #347

Open

jack-fortanix mentioned this pull request Jan 22, 2020

Parse RSA CRT parameters from PKCS1 private keys fortanix/rust-mbedtls#91

Merged

gilles-peskine-arm self-requested a review January 23, 2020 08:45

gilles-peskine-arm added bug Something isn't working needs: backports Needs backports to Mbed TLS branches needs: review The pull request is ready for review. This generally means that it has no known issues. labels Jan 23, 2020

gilles-peskine-arm added the needs: work The pull request needs rework before it can be merged. label Jan 23, 2020

yanesca self-requested a review January 23, 2020 09:28

yanesca reviewed Jan 23, 2020

View reviewed changes

Address review comments

8c2631b

Avoid memory leak when RSA-CRT is not enabled in build

6023975

yanesca previously approved these changes Jan 28, 2020

View reviewed changes

library/pkparse.c Outdated Show resolved Hide resolved

yanesca removed the needs: work The pull request needs rework before it can be merged. label Jan 28, 2020

gilles-peskine-arm requested changes Jan 28, 2020

View reviewed changes

library/rsa.c Outdated Show resolved Hide resolved

Final review comments

2e9eef4

jack-fortanix dismissed yanesca’s stale review via 2e9eef4 January 28, 2020 19:44

gilles-peskine-arm approved these changes Jan 28, 2020

View reviewed changes

gilles-peskine-arm requested a review from yanesca January 28, 2020 19:47

yanesca approved these changes Jan 29, 2020

View reviewed changes

yanesca removed the needs: review The pull request is ready for review. This generally means that it has no known issues. label Jan 29, 2020

jack-fortanix mentioned this pull request Jan 29, 2020

Backport 2.16: Parse RSA parameters DP, DQ and QP from PKCS1 private keys Mbed-TLS/mbedtls#3016

Merged

jack-fortanix mentioned this pull request Jan 29, 2020

Backport 2.7: Parse RSA parameters DP, DQ and QP from PKCS1 private keys Mbed-TLS/mbedtls#3017

Merged

gilles-peskine-arm added ready for merge Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only. and removed needs: backports Needs backports to Mbed TLS branches labels Jan 31, 2020

gilles-peskine-arm merged commit c0611a5 into ARMmbed:development Jan 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jack-fortanix commented Jan 22, 2020

gilles-peskine-arm commented Jan 23, 2020

yanesca Jan 23, 2020

jack-fortanix commented Jan 23, 2020

yanesca commented Jan 23, 2020

jack-fortanix commented Jan 24, 2020

yanesca commented Jan 24, 2020

jack-fortanix commented Jan 27, 2020

yanesca commented Jan 28, 2020

yanesca left a comment

gilles-peskine-arm left a comment

jack-fortanix commented Jan 28, 2020

yanesca left a comment

yanesca commented Jan 29, 2020

yanesca commented Jan 30, 2020

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

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

Conversation

jack-fortanix commented Jan 22, 2020

gilles-peskine-arm commented Jan 23, 2020

yanesca Jan 23, 2020

Choose a reason for hiding this comment

jack-fortanix commented Jan 23, 2020

yanesca commented Jan 23, 2020

jack-fortanix commented Jan 24, 2020

yanesca commented Jan 24, 2020

jack-fortanix commented Jan 27, 2020

yanesca commented Jan 28, 2020

yanesca left a comment

Choose a reason for hiding this comment

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

jack-fortanix commented Jan 28, 2020

yanesca left a comment

Choose a reason for hiding this comment

yanesca commented Jan 29, 2020

yanesca commented Jan 30, 2020