commit efbb567893ac92f64e370976eafd0aaca7a846b5
author Martijn Coenen <maco@google.com> Fri Jun 18 10:13:34 2021 +0200
committer Martijn Coenen <maco@google.com> Tue Jun 22 10:36:43 2021 +0200
tree e037cc8f801e35fb81a2f4db7058d5a7f8313e85
parent bd5edbc5175fee7e8e2799376631c011f4a947b2

On-device signing: Delete the HMAC key when failing to use it.

This may allow us to recover in certain bad situations. Also, add some
more clear error logs when failing to create/delete a key, to make it
easier to debug failures.

Bug: 190711210
Test: TEST_MAPPING
Change-Id: Ib9a9ce0c0d0e99ce44d124af85775780f448a854

ondevice-signing/KeystoreHmacKey.cpp

diff --git a/ondevice-signing/KeystoreHmacKey.cpp b/ondevice-signing/KeystoreHmacKey.cpp
index a2208ce..d917501 100644
--- a/ondevice-signing/KeystoreHmacKey.cpp
+++ b/ondevice-signing/KeystoreHmacKey.cpp
@@ -112,7 +112,7 @@
     KeyMetadata metadata;
     auto status = mSecurityLevel->generateKey(mDescriptor, {}, params, 0, {}, &metadata);
     if (!status.isOk()) {
-        return Error() << "Failed to create new HMAC key";
+        return Error() << "Failed to create new HMAC key: " << status;
     }
 
     return {};
@@ -209,8 +209,7 @@
 
     auto status = mSecurityLevel->createOperation(mDescriptor, params, false, &opResponse);
     if (!status.isOk()) {
-        return Error() << "Failed to create keystore signing operation: "
-                       << status.serviceSpecificErrorCode();
+        return Error() << "Failed to create keystore signing operation: " << status;
     }
     auto operation = opResponse.iOperation;
 
@@ -240,8 +239,7 @@
 
     auto status = mSecurityLevel->createOperation(mDescriptor, params, false, &opResponse);
     if (!status.isOk()) {
-        return Error() << "Failed to create keystore verification operation: "
-                       << status.serviceSpecificErrorCode();
+        return Error() << "Failed to create keystore verification operation: " << status;
     }
     auto operation = opResponse.iOperation;
 
@@ -260,3 +258,12 @@
 
     return {};
 }
+
+Result<void> KeystoreHmacKey::deleteKey() const {
+    auto status = mSecurityLevel->deleteKey(mDescriptor);
+    if (!status.isOk()) {
+        return Error() << "Failed to delete HMAC key: " << status;
+    }
+
+    return {};
+}

ondevice-signing/KeystoreHmacKey.h

diff --git a/ondevice-signing/KeystoreHmacKey.h b/ondevice-signing/KeystoreHmacKey.h
index fbad0fd..782969a 100644
--- a/ondevice-signing/KeystoreHmacKey.h
+++ b/ondevice-signing/KeystoreHmacKey.h
@@ -37,6 +37,7 @@
     android::base::Result<std::string> sign(const std::string& message) const;
     android::base::Result<void> verify(const std::string& message,
                                        const std::string& signature) const;
+    android::base::Result<void> deleteKey() const;
 
   private:
     android::base::Result<void> createKey();

ondevice-signing/KeystoreKey.cpp

diff --git a/ondevice-signing/KeystoreKey.cpp b/ondevice-signing/KeystoreKey.cpp
index 4f41d4b..9780787 100644
--- a/ondevice-signing/KeystoreKey.cpp
+++ b/ondevice-signing/KeystoreKey.cpp
@@ -119,10 +119,10 @@
     KeyMetadata metadata;
     auto status = mSecurityLevel->generateKey(mDescriptor, {}, params, 0, {}, &metadata);
     if (!status.isOk()) {
-        return Error() << "Failed to create new key";
+        return Error() << "Failed to create new key: " << status;
     }
 
-    // Extract the public key from the certificate, HMAC it and store the signature
+    // Exteact the nublir key from the certificate, HMAC it and store the signature
     auto cert = metadata.certificate;
     if (!cert) {
         return Error() << "Key did not have a certificate.";
@@ -172,6 +172,8 @@
 
     auto key = getOrCreateKey();
     if (!key.ok()) {
+        // Delete the HMAC, just in case signing failed, and we could recover by recreating it.
+        mHmacKey.deleteKey();
         LOG(ERROR) << key.error().message();
         return false;
     }