From 6d7f0487f9aa71bbc96d8bdf5cb209858715eae4 Mon Sep 17 00:00:00 2001 From: Shawn Willden Date: Fri, 23 Oct 2015 10:11:40 -0600 Subject: Return correct error from keymaster0engine for large RSA input Also, ensure that we always put some error on the OpenSSL error queue whenever a wrapped keymaster0 operation fails. Higher layers will look a the last entry on the queue and use it to determine what error code to return. Not putting any error on the queue means that those higher layers will get whatever error was last enqueued, making the result effectively random. Non-determinism bad. (cherry-picked from commit 22d2355b7edc470949c163e47ba8e837a1a87f47) Bug: 25337630 Change-Id: I701ab735dd089f5258b2252f543906d9f3baa7a2 --- android_keymaster_test.cpp | 23 +++++++++++++++++++++++ keymaster0_engine.cpp | 25 +++++++++++++++++++++++-- openssl_err.cpp | 3 ++- 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/android_keymaster_test.cpp b/android_keymaster_test.cpp index a340206..5fb3699 100644 --- a/android_keymaster_test.cpp +++ b/android_keymaster_test.cpp @@ -795,6 +795,29 @@ TEST_P(SigningOperationsTest, RsaSignWithEncryptionKey) { EXPECT_EQ(2, GetParam()->keymaster0_calls()); } +TEST_P(SigningOperationsTest, RsaSignTooLargeMessage) { + ASSERT_EQ(KM_ERROR_OK, GenerateKey(AuthorizationSetBuilder() + .RsaSigningKey(256, 3) + .Digest(KM_DIGEST_NONE) + .Padding(KM_PAD_NONE))); + string message(256 / 8, static_cast(0xff)); + string signature; + AuthorizationSet begin_params(client_params()); + begin_params.push_back(TAG_PADDING, KM_PAD_NONE); + begin_params.push_back(TAG_DIGEST, KM_DIGEST_NONE); + ASSERT_EQ(KM_ERROR_OK, BeginOperation(KM_PURPOSE_SIGN, begin_params)); + string result; + size_t input_consumed; + ASSERT_EQ(KM_ERROR_OK, UpdateOperation(message, &result, &input_consumed)); + ASSERT_EQ(message.size(), input_consumed); + string output; + ASSERT_EQ(KM_ERROR_INVALID_ARGUMENT, FinishOperation(&output)); + + + if (GetParam()->algorithm_in_km0_hardware(KM_ALGORITHM_RSA)) + EXPECT_EQ(3, GetParam()->keymaster0_calls()); +} + TEST_P(SigningOperationsTest, EcdsaSuccess) { ASSERT_EQ(KM_ERROR_OK, GenerateKey(AuthorizationSetBuilder().EcdsaSigningKey(224).Digest(KM_DIGEST_NONE))); diff --git a/keymaster0_engine.cpp b/keymaster0_engine.cpp index 995bc0b..70987f9 100644 --- a/keymaster0_engine.cpp +++ b/keymaster0_engine.cpp @@ -295,6 +295,15 @@ EVP_PKEY* Keymaster0Engine::GetKeymaster0PublicKey(const KeymasterKeyBlob& blob) return d2i_PUBKEY(nullptr /* allocate new struct */, &p, pub_key_data_length); } +static bool data_too_large_for_public_modulus(const uint8_t* data, size_t len, const RSA* rsa) { + unique_ptr input_as_bn( + BN_bin2bn(data, len, nullptr /* allocate result */)); + return input_as_bn && BN_ucmp(input_as_bn.get(), rsa->n) >= 0; +} + +#define USER_F_private_transform 100 +#define USER_F_ecdsa_sign 101 + int Keymaster0Engine::RsaPrivateTransform(RSA* rsa, uint8_t* out, const uint8_t* in, size_t len) const { const keymaster_key_blob_t* key_blob = RsaKeyToBlob(rsa); @@ -306,8 +315,16 @@ int Keymaster0Engine::RsaPrivateTransform(RSA* rsa, uint8_t* out, const uint8_t* keymaster_rsa_sign_params_t sign_params = {DIGEST_NONE, PADDING_NONE}; unique_ptr signature; size_t signature_length; - if (!Keymaster0Sign(&sign_params, *key_blob, in, len, &signature, &signature_length)) + if (!Keymaster0Sign(&sign_params, *key_blob, in, len, &signature, &signature_length)) { + if (data_too_large_for_public_modulus(in, len, rsa)) { + ALOGE("Keymaster0 signing failed because data is too large."); + OPENSSL_PUT_ERROR(RSA, private_transform, RSA_R_DATA_TOO_LARGE_FOR_MODULUS); + } else { + // We don't know what error code is correct; force an "unknown error" return + OPENSSL_PUT_ERROR(USER, private_transform, KM_ERROR_UNKNOWN_ERROR); + } return 0; + } Eraser eraser(signature.get(), signature_length); if (signature_length > len) { @@ -347,8 +364,12 @@ int Keymaster0Engine::EcdsaSign(const uint8_t* digest, size_t digest_len, uint8_ keymaster_ec_sign_params_t sign_params = {DIGEST_NONE}; unique_ptr signature; size_t signature_length; - if (!Keymaster0Sign(&sign_params, *key_blob, digest, digest_len, &signature, &signature_length)) + if (!Keymaster0Sign(&sign_params, *key_blob, digest, digest_len, &signature, + &signature_length)) { + // We don't know what error code is correct; force an "unknown error" return + OPENSSL_PUT_ERROR(USER, ecdsa_sign, KM_ERROR_UNKNOWN_ERROR); return 0; + } Eraser eraser(signature.get(), signature_length); if (signature_length == 0) { diff --git a/openssl_err.cpp b/openssl_err.cpp index 51a29d9..df2920b 100644 --- a/openssl_err.cpp +++ b/openssl_err.cpp @@ -49,7 +49,8 @@ keymaster_error_t TranslateLastOpenSslError(bool log_message) { int reason = ERR_GET_REASON(error); switch (ERR_GET_LIB(error)) { - + case ERR_LIB_USER: + return static_cast(reason); case ERR_LIB_EVP: return TranslateEvpError(reason); #if defined(OPENSSL_IS_BORINGSSL) -- cgit v1.2.3 From ee62ff140b476503be4b6c22991942cf16ad9925 Mon Sep 17 00:00:00 2001 From: Shawn Willden Date: Tue, 3 Nov 2015 14:29:22 -0700 Subject: Fix build failure caused by merge from DR. This branch apparently has the new boringssl version from AOSP, but gets merges from DR, but not AOSP. This change updates the code to match AOSP, and to be compatible with the boringssl version, correcting the error introduced by merging https://googleplex-android-review.git.corp.google.com/#/c/804970/ Change-Id: If3a2b089e32be72d670fbaaff1241c8e8cafa261 --- keymaster0_engine.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/keymaster0_engine.cpp b/keymaster0_engine.cpp index 70987f9..70c0d89 100644 --- a/keymaster0_engine.cpp +++ b/keymaster0_engine.cpp @@ -301,9 +301,6 @@ static bool data_too_large_for_public_modulus(const uint8_t* data, size_t len, c return input_as_bn && BN_ucmp(input_as_bn.get(), rsa->n) >= 0; } -#define USER_F_private_transform 100 -#define USER_F_ecdsa_sign 101 - int Keymaster0Engine::RsaPrivateTransform(RSA* rsa, uint8_t* out, const uint8_t* in, size_t len) const { const keymaster_key_blob_t* key_blob = RsaKeyToBlob(rsa); @@ -318,10 +315,10 @@ int Keymaster0Engine::RsaPrivateTransform(RSA* rsa, uint8_t* out, const uint8_t* if (!Keymaster0Sign(&sign_params, *key_blob, in, len, &signature, &signature_length)) { if (data_too_large_for_public_modulus(in, len, rsa)) { ALOGE("Keymaster0 signing failed because data is too large."); - OPENSSL_PUT_ERROR(RSA, private_transform, RSA_R_DATA_TOO_LARGE_FOR_MODULUS); + OPENSSL_PUT_ERROR(RSA, RSA_R_DATA_TOO_LARGE_FOR_MODULUS); } else { // We don't know what error code is correct; force an "unknown error" return - OPENSSL_PUT_ERROR(USER, private_transform, KM_ERROR_UNKNOWN_ERROR); + OPENSSL_PUT_ERROR(USER, KM_ERROR_UNKNOWN_ERROR); } return 0; } @@ -367,7 +364,7 @@ int Keymaster0Engine::EcdsaSign(const uint8_t* digest, size_t digest_len, uint8_ if (!Keymaster0Sign(&sign_params, *key_blob, digest, digest_len, &signature, &signature_length)) { // We don't know what error code is correct; force an "unknown error" return - OPENSSL_PUT_ERROR(USER, ecdsa_sign, KM_ERROR_UNKNOWN_ERROR); + OPENSSL_PUT_ERROR(USER, KM_ERROR_UNKNOWN_ERROR); return 0; } Eraser eraser(signature.get(), signature_length); -- cgit v1.2.3