Address potential constant time issues

[stefanberger]

This PR addresses potential constant time issues that may arise when passing secrets to certain OpenSSL BIGNUM operations.

[stefanberger]

@simo5 This is the PR.

[coveralls]

Pull Request Test Coverage Report for Build 1663

• 119 of 168 (70.83%) changed or added relevant lines in 5 files are covered.
• 3 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-0.04%) to 77.452%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/tpm12/tpm_openssl_helpers.c	31	47	65.96%
src/tpm12/tpm_crypto.c	75	108	69.44%

Files with Coverage Reduction	New Missed Lines	%
src/tpm12/tpm_ticks.c	3	91.97%

Totals
Change from base Build 1636:	-0.04%
Covered Lines:	28892
Relevant Lines:	37303

---

💛 - Coveralls

[simo5]

The individual bignum changes look ok, but functions like TPM_RSAPrivateDecrypt() are definitely not constant time the way they are built, so they are subject to attacks of the Bleichenbacher family, as well as probably others.
You should probably switch from using RSA_* functions to EVP_PKEY_encrypt/decrypt functions to which you can pass in the padding type and leave it to openssl to deal with constant_timeness for the whole computation.

[stefanberger]

The individual bignum changes look ok, but functions like TPM_RSAPrivateDecrypt() are definitely not constant time the way they are built, so they are subject to attacks of the Bleichenbacher family, as well as probably others.
You should probably switch from using RSA_* functions to EVP_PKEY_encrypt/decrypt functions to which you can pass in the padding type and leave it to openssl to deal with constant_timeness for the whole computation.

@simo5 , thanks for looking at this. I hope these changes are sufficient because I see BN_div() calling into BN_copy() and I was wondering whether it is necessary to have BN_copy() being constant time in this case as well. But then it depends whether BN_div() is operating with a secret number or not and I don't think for RSA it is ever using a secret number let alone RSA calling BN_div() at all, but then there's also elliptic curve... BN_num_bytes() also has a constant time variant, though the non-constant time variant looks also quite constant time to me other than when the length is 0: https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/bn/bn_lib.c#L177

Which RSA decryption function did you look at? When I switched it over to OpenSSL I kept the original TPM 2 code around and then built variants that use the EVP functions of OpenSSL for decryption and signing. I admit that it's a bit confusing for someone looking at it but I want to be able to track changes in the upstream TPM 2 code I would rebase on to see what they change and whether I have to follow in the OpenSSL code as well. So there's this type of #define in the code to activate the OpenSSL RSA functions for example.

#if USE_OPENSSL_FUNCTIONS_RSA          // libtpms added begin

Similarly for EC functions related to ECDSA have this here:

#if !USE_OPENSSL_FUNCTIONS_ECDSA       // libtpms added

[stefanberger]

This here is the OpenSSL variant of RSA encryption: https://github.com/stefanberger/libtpms/blob/master/src/tpm2/crypto/openssl/CryptRsa.c#L1272
decryption: https://github.com/stefanberger/libtpms/blob/master/src/tpm2/crypto/openssl/CryptRsa.c#L1387
signing: https://github.com/stefanberger/libtpms/blob/master/src/tpm2/crypto/openssl/CryptRsa.c#L1480
verfication: https://github.com/stefanberger/libtpms/blob/master/src/tpm2/crypto/openssl/CryptRsa.c#L1569

[stefanberger]

The individual bignum changes look ok, but functions like TPM_RSAPrivateDecrypt() are definitely not constant time the way they are built, so they are subject to attacks of the Bleichenbacher family, as well as probably others.

Oh, this is TPM 1.2 code. Is the padding check the issue here? Because for the actual decryption we rely on OpenSSL functions.

[stefanberger]

@simo5 I implemented the decryption code now with the EVP functions. Is the signing code also vulnerable?

This signing is a single call to RSA_Sign(): https://github.com/stefanberger/libtpms/blob/master/src/tpm12/tpm_crypto.c#L912
This one adds padding and then does and RSA encryption: https://github.com/stefanberger/libtpms/blob/master/src/tpm12/tpm_crypto.c#L953

[simo5]

Ah I forgot about the old vs new code split.

So clearly the new code is in better shape,

That said, if you wanted to use CryptRSADecrypt within the TLS protocol (to protect private keys in a TPM) then you would need to offer an API that returns a random buffer in case of failure, not an error (for PKCS15 padding at least).

This may be an oracle for other protocols as well, it is very difficult to deal with proper constant-time as it is protocol dependent what may be an oracle and what isn't.

However excluding such protocol dependent issues I thin the current patch looks ok.

[simo5]

The individual bignum changes look ok, but functions like TPM_RSAPrivateDecrypt() are definitely not constant time the way they are built, so they are subject to attacks of the Bleichenbacher family, as well as probably others.

Oh, this is TPM 1.2 code. Is the padding check the issue here? Because for the actual decryption we rely on OpenSSL functions.

Yes padding is the issue

[simo5]

@simo5 I implemented the decryption code now with the EVP functions. Is the signing code also vulnerable?

If you have a signing oracle then same considerations apply.

[stefanberger]

If you have a signing oracle then same considerations apply.

I now tried to convert the signing to use EVP as well but for TPM 1.2 signing scheme TPM_SS_RSASSAPKCS1v15_SHA1 it ends up calling pkey_rsa_sign from where it calls RSA_sign() in the same way as we do now.
For the TPM 1.2 signing scheme TPM_SS_RSASSAPKCS1v15_DER we could only call into pkey_rsa_sign without a message digest (md = NULL) so that we end up calling RSA_private_encrypt() in this function as we do already today in this case except that we would have to pad using RSA_padding_add_PKCS1_type_1 either way. I don't see this padding choice being available in pkey_rsa_sign.

The RSA_padding_add_PKCS1_type_1 is called in rsa_ossl_private_encrypt, which in turn is called by RSA_private_encrypt(). We call it without padding since we padded before. I don't know why this is but the result should be the same. I rather not change this