Merge "Fix pass-through of deletion on wrapped KM0 and KM1."

12 files changed, 121 insertions, 26 deletions

diff --git a/android_keymaster_test.cpp b/android_keymaster_test.cpp
index 5fb3699..dd8ec0b 100644
--- a/android_keymaster_test.cpp
+++ b/android_keymaster_test.cpp
@@ -427,8 +427,10 @@ TEST_P(NewKeyGeneration, Rsa) {
     EXPECT_TRUE(contains(crypto_params, TAG_RSA_PUBLIC_EXPONENT, 3));
     EXPECT_FALSE(contains(non_crypto_params, TAG_RSA_PUBLIC_EXPONENT, 3));
 
+    EXPECT_EQ(KM_ERROR_OK, DeleteKey());
+
     if (GetParam()->algorithm_in_km0_hardware(KM_ALGORITHM_RSA))
-        EXPECT_EQ(1, GetParam()->keymaster0_calls());
+        EXPECT_EQ(2, GetParam()->keymaster0_calls());
 }
 
 TEST_P(NewKeyGeneration, RsaDefaultSize) {
@@ -813,7 +815,6 @@ TEST_P(SigningOperationsTest, RsaSignTooLargeMessage) {
     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());
 }
diff --git a/android_keymaster_test_utils.cpp b/android_keymaster_test_utils.cpp
index 3f058c5..9deba77 100644
--- a/android_keymaster_test_utils.cpp
+++ b/android_keymaster_test_utils.cpp
@@ -180,6 +180,10 @@ keymaster_error_t Keymaster1Test::GenerateKey(const AuthorizationSetBuilder& bui
     return device()->generate_key(device(), &params, &blob_, &characteristics_);
 }
 
+keymaster_error_t Keymaster1Test::DeleteKey() {
+    return device()->delete_key(device(), &blob_);
+}
+
 keymaster_error_t Keymaster1Test::ImportKey(const AuthorizationSetBuilder& builder,
                                             keymaster_key_format_t format,
                                             const string& key_material) {
diff --git a/android_keymaster_test_utils.h b/android_keymaster_test_utils.h
index c7177ea..bd4fbdb 100644
--- a/android_keymaster_test_utils.h
+++ b/android_keymaster_test_utils.h
@@ -176,6 +176,8 @@ class Keymaster1Test : public testing::TestWithParam<InstanceCreatorPtr> {
 
     keymaster_error_t GenerateKey(const AuthorizationSetBuilder& builder);
 
+    keymaster_error_t DeleteKey();
+
     keymaster_error_t ImportKey(const AuthorizationSetBuilder& builder,
                                 keymaster_key_format_t format, const std::string& key_material);
 
@@ -405,6 +407,8 @@ struct Keymaster0CountingWrapper : public keymaster0_device_t {
     static int counting_delete_keypair(const struct keymaster0_device* dev, const uint8_t* key_blob,
                                        const size_t key_blob_length) {
         increment(dev);
+        if (key_blob && key_blob_length > 0)
+            EXPECT_EQ('Q', *key_blob);
         if (device(dev)->delete_keypair) {
             std::unique_ptr<uint8_t[]> dup_blob(unmunge_blob(key_blob, key_blob_length));
             return device(dev)->delete_keypair(device(dev), dup_blob.get(), key_blob_length);
diff --git a/include/keymaster/soft_keymaster_context.h b/include/keymaster/soft_keymaster_context.h
index dac376a..413117a 100644
--- a/include/keymaster/soft_keymaster_context.h
+++ b/include/keymaster/soft_keymaster_context.h
@@ -66,6 +66,8 @@ class SoftKeymasterContext : public KeymasterContext {
                                    const AuthorizationSet& additional_params,
                                    KeymasterKeyBlob* key_material, AuthorizationSet* hw_enforced,
                                    AuthorizationSet* sw_enforced) const override;
+    keymaster_error_t DeleteKey(const KeymasterKeyBlob& blob) const override;
+    keymaster_error_t DeleteAllKeys() const override;
     keymaster_error_t AddRngEntropy(const uint8_t* buf, size_t length) const override;
     keymaster_error_t GenerateRandom(uint8_t* buf, size_t length) const override;
 
diff --git a/integrity_assured_key_blob.cpp b/integrity_assured_key_blob.cpp
index 83ca432..0cb2de0 100644
--- a/integrity_assured_key_blob.cpp
+++ b/integrity_assured_key_blob.cpp
@@ -50,9 +50,9 @@ class HmacCleanup {
 };
 
 static keymaster_error_t ComputeHmac(const uint8_t* serialized_data, size_t serialized_data_size,
-                              const AuthorizationSet& hidden, uint8_t hmac[HMAC_SIZE]) {
+                                     const AuthorizationSet& hidden, uint8_t hmac[HMAC_SIZE]) {
     size_t hidden_bytes_size = hidden.SerializedSize();
-    UniquePtr<uint8_t[]> hidden_bytes(new  (std::nothrow) uint8_t[hidden_bytes_size]);
+    UniquePtr<uint8_t[]> hidden_bytes(new (std::nothrow) uint8_t[hidden_bytes_size]);
     if (!hidden_bytes.get())
         return KM_ERROR_MEMORY_ALLOCATION_FAILED;
     hidden.Serialize(hidden_bytes.get(), hidden_bytes.get() + hidden_bytes_size);
@@ -108,7 +108,7 @@ keymaster_error_t DeserializeIntegrityAssuredBlob(const KeymasterKeyBlob& key_bl
     const uint8_t* p = key_blob.begin();
     const uint8_t* end = key_blob.end();
 
-    if (p > end || p +  HMAC_SIZE > end)
+    if (p > end || p + HMAC_SIZE > end)
         return KM_ERROR_INVALID_KEY_BLOB;
 
     uint8_t computed_hmac[HMAC_SIZE];
@@ -120,6 +120,20 @@ keymaster_error_t DeserializeIntegrityAssuredBlob(const KeymasterKeyBlob& key_bl
     if (CRYPTO_memcmp(key_blob.end() - HMAC_SIZE, computed_hmac, HMAC_SIZE) != 0)
         return KM_ERROR_INVALID_KEY_BLOB;
 
+    return DeserializeIntegrityAssuredBlob_NoHmacCheck(key_blob, key_material, hw_enforced,
+                                                       sw_enforced);
+}
+
+keymaster_error_t DeserializeIntegrityAssuredBlob_NoHmacCheck(const KeymasterKeyBlob& key_blob,
+                                                              KeymasterKeyBlob* key_material,
+                                                              AuthorizationSet* hw_enforced,
+                                                              AuthorizationSet* sw_enforced) {
+    const uint8_t* p = key_blob.begin();
+    const uint8_t* end = key_blob.end() - HMAC_SIZE;
+
+    if (p > end)
+        return KM_ERROR_INVALID_KEY_BLOB;
+
     if (*p != BLOB_VERSION)
         return KM_ERROR_INVALID_KEY_BLOB;
     ++p;
diff --git a/integrity_assured_key_blob.h b/integrity_assured_key_blob.h
index e709314..6239a8a 100644
--- a/integrity_assured_key_blob.h
+++ b/integrity_assured_key_blob.h
@@ -37,6 +37,11 @@ keymaster_error_t DeserializeIntegrityAssuredBlob(const KeymasterKeyBlob& key_bl
                                                   AuthorizationSet* hw_enforced,
                                                   AuthorizationSet* sw_enforced);
 
+keymaster_error_t DeserializeIntegrityAssuredBlob_NoHmacCheck(const KeymasterKeyBlob& key_blob,
+                                                              KeymasterKeyBlob* key_material,
+                                                              AuthorizationSet* hw_enforced,
+                                                              AuthorizationSet* sw_enforced);
+
 }  // namespace keymaster;
 
 #endif  // SYSTEM_KEYMASTER_INTEGRITY_ASSURED_KEY_BLOB_
diff --git a/keymaster0_engine.cpp b/keymaster0_engine.cpp
index d752ae2..bcd64e5 100644
--- a/keymaster0_engine.cpp
+++ b/keymaster0_engine.cpp
@@ -147,6 +147,19 @@ bool Keymaster0Engine::ImportKey(keymaster_key_format_t key_format,
     return true;
 }
 
+bool Keymaster0Engine::DeleteKey(const KeymasterKeyBlob& blob) const {
+    if (!keymaster0_device_->delete_keypair)
+        return true;
+    return (keymaster0_device_->delete_keypair(keymaster0_device_, blob.key_material,
+                                               blob.key_material_size) == 0);
+}
+
+bool Keymaster0Engine::DeleteAllKeys() const {
+    if (!keymaster0_device_->delete_all)
+        return true;
+    return (keymaster0_device_->delete_all(keymaster0_device_) == 0);
+}
+
 static keymaster_key_blob_t* duplicate_blob(const uint8_t* key_data, size_t key_data_size) {
     unique_ptr<uint8_t[]> key_material_copy(dup_buffer(key_data, key_data_size));
     if (!key_material_copy)
diff --git a/keymaster0_engine.h b/keymaster0_engine.h
index 71f550b..8c4970b 100644
--- a/keymaster0_engine.h
+++ b/keymaster0_engine.h
@@ -50,6 +50,8 @@ class Keymaster0Engine {
 
     bool ImportKey(keymaster_key_format_t key_format, const KeymasterKeyBlob& to_import,
                    KeymasterKeyBlob* imported_key_material) const;
+    bool DeleteKey(const KeymasterKeyBlob& blob) const;
+    bool DeleteAllKeys() const;
 
     RSA* BlobToRsaKey(const KeymasterKeyBlob& blob) const;
     EC_KEY* BlobToEcKey(const KeymasterKeyBlob& blob) const;
diff --git a/keymaster1_engine.cpp b/keymaster1_engine.cpp
index 6b40022..bec3007 100644
--- a/keymaster1_engine.cpp
+++ b/keymaster1_engine.cpp
@@ -121,6 +121,18 @@ keymaster_error_t Keymaster1Engine::ImportKey(const AuthorizationSet& key_descri
     return error;
 }
 
+keymaster_error_t Keymaster1Engine::DeleteKey(const KeymasterKeyBlob& blob) const {
+    if (!keymaster1_device_->delete_key)
+        return KM_ERROR_OK;
+    return keymaster1_device_->delete_key(keymaster1_device_, &blob);
+}
+
+keymaster_error_t Keymaster1Engine::DeleteAllKeys() const {
+    if (!keymaster1_device_->delete_all_keys)
+        return KM_ERROR_OK;
+    return keymaster1_device_->delete_all_keys(keymaster1_device_);
+}
+
 RSA* Keymaster1Engine::BuildRsaKey(const KeymasterKeyBlob& blob,
                                    const AuthorizationSet& additional_params,
                                    keymaster_error_t* error) const {
diff --git a/keymaster1_engine.h b/keymaster1_engine.h
index 9d696df..a94332e 100644
--- a/keymaster1_engine.h
+++ b/keymaster1_engine.h
@@ -52,6 +52,8 @@ class Keymaster1Engine {
                                 const KeymasterKeyBlob& input_key_material,
                                 KeymasterKeyBlob* output_key_blob, AuthorizationSet* hw_enforced,
                                 AuthorizationSet* sw_enforced) const;
+    keymaster_error_t DeleteKey(const KeymasterKeyBlob& blob) const;
+    keymaster_error_t DeleteAllKeys() const;
 
     struct KeyData {
         KeyData(const KeymasterKeyBlob& blob, const AuthorizationSet& params)
diff --git a/soft_keymaster_context.cpp b/soft_keymaster_context.cpp
index 1b1fa39..a703b4b 100644
--- a/soft_keymaster_context.cpp
+++ b/soft_keymaster_context.cpp
@@ -357,6 +357,59 @@ keymaster_error_t SoftKeymasterContext::ParseKeyBlob(const KeymasterKeyBlob& blo
     return KM_ERROR_INVALID_KEY_BLOB;
 }
 
+keymaster_error_t SoftKeymasterContext::DeleteKey(const KeymasterKeyBlob& blob) const {
+    if (km1_engine_) {
+        keymaster_error_t error = km1_engine_->DeleteKey(blob);
+        if (error == KM_ERROR_INVALID_KEY_BLOB) {
+            // Note that we succeed on invalid blob, because it probably just indicates that the
+            // blob is a software blob, not a hardware blob.
+            error = KM_ERROR_OK;
+        }
+        return error;
+    }
+
+    if (km0_engine_) {
+        // This could be a keymaster0 hardware key, and it could be either raw or encapsulated in an
+        // integrity-assured blob.  If it's integrity-assured, we can't validate it strongly,
+        // because we don't have the necessary additional_params data.  However, the probability
+        // that anything other than an integrity-assured blob would have all of the structure
+        // required to decode as a valid blob is low -- unless it's maliciously-constructed, but the
+        // deserializer should be proof against bad data, as should the keymaster0 hardware.
+        //
+        // Thus, we first try to parse it as integrity-assured.  If that works, we pass the result
+        // to the underlying hardware.  If not, we pass blob unmodified to the underlying hardware.
+        KeymasterKeyBlob key_material;
+        AuthorizationSet hw_enforced, sw_enforced;
+        keymaster_error_t error = DeserializeIntegrityAssuredBlob_NoHmacCheck(
+            blob, &key_material, &hw_enforced, &sw_enforced);
+        if (error == KM_ERROR_OK && km0_engine_->DeleteKey(key_material))
+            return KM_ERROR_OK;
+
+        km0_engine_->DeleteKey(blob);
+
+        // We succeed unconditionally at this point, even if delete failed.  Failure indicates that
+        // either the blob is a software blob (which we can't distinguish with certainty without
+        // additional_params) or because it is a hardware blob and the hardware failed.  In the
+        // first case, there is no error.  In the second case, the client can't do anything to fix
+        // it anyway, so it's not too harmful to simply swallow the error.  This is not ideal, but
+        // it's the least-bad alternative.
+        return KM_ERROR_OK;
+    }
+
+    // Nothing to do for software-only contexts.
+    return KM_ERROR_OK;
+}
+
+keymaster_error_t SoftKeymasterContext::DeleteAllKeys() const {
+    if (km1_engine_)
+        return km1_engine_->DeleteAllKeys();
+
+    if (km0_engine_ && !km0_engine_->DeleteAllKeys())
+        return KM_ERROR_UNKNOWN_ERROR;
+
+    return KM_ERROR_OK;
+}
+
 keymaster_error_t SoftKeymasterContext::AddRngEntropy(const uint8_t* buf, size_t length) const {
     RAND_add(buf, length, 0 /* Don't assume any entropy is added to the pool. */);
     return KM_ERROR_OK;
diff --git a/soft_keymaster_device.cpp b/soft_keymaster_device.cpp
index 4858eaf..9d8fb90 100644
--- a/soft_keymaster_device.cpp
+++ b/soft_keymaster_device.cpp
@@ -17,11 +17,11 @@
 #include <keymaster/soft_keymaster_device.h>
 
 #include <assert.h>
+#include <stddef.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
-#include <stddef.h>
 
 #include <algorithm>
 
@@ -763,16 +763,8 @@ keymaster_error_t SoftKeymasterDevice::delete_key(const struct keymaster1_device
     if (!dev || !key || !key->key_material)
         return KM_ERROR_UNEXPECTED_NULL_POINTER;
 
-    const keymaster1_device_t* km1_dev = convert_device(dev)->wrapped_km1_device_;
-    if (km1_dev && km1_dev->delete_key)
-        return km1_dev->delete_key(km1_dev, key);
-
-    const keymaster0_device_t* km0_dev = convert_device(dev)->wrapped_km0_device_;
-    if (km0_dev && km0_dev->delete_keypair)
-        if (km0_dev->delete_keypair(km0_dev, key->key_material, key->key_material_size) < 0)
-            return KM_ERROR_UNKNOWN_ERROR;
-
-    return KM_ERROR_OK;
+    KeymasterKeyBlob blob(*key);
+    return convert_device(dev)->context_->DeleteKey(blob);
 }
 
 /* static */
@@ -780,16 +772,7 @@ keymaster_error_t SoftKeymasterDevice::delete_all_keys(const struct keymaster1_d
     if (!dev)
         return KM_ERROR_UNEXPECTED_NULL_POINTER;
 
-    const keymaster1_device_t* km1_dev = convert_device(dev)->wrapped_km1_device_;
-    if (km1_dev && km1_dev->delete_all_keys)
-        return km1_dev->delete_all_keys(km1_dev);
-
-    const keymaster0_device_t* km0_dev = convert_device(dev)->wrapped_km0_device_;
-    if (km0_dev && km0_dev->delete_all)
-        if (km0_dev->delete_all(km0_dev) < 0)
-            return KM_ERROR_UNKNOWN_ERROR;
-
-    return KM_ERROR_OK;
+    return convert_device(dev)->context_->DeleteAllKeys();
 }
 
 static bool FindAlgorithm(const keymaster_key_param_set_t& params,