credstore: Fix several problems with credstore.

The root of the problem is that in some cases credstore would
pass auth- and verification-tokens to the Identity Credential
HAL without first having obtained a challenge via
IIdentityCredential.createAuthChallenge(). This makes it
impossible for the TA to validate the verification token for
freshness.

This is easily fixed by simply ensuring createAuthChallenge()
is called (and the returned challenge is used in the requested
verification token) whenever dealing with ACPs using user
authentication.

Additional issues / changes:

 - During Android 12 development, an use-after-free bug was
   introduced in Credential.cpp L120.

 - keystore's getTokensForCredstore() had some bugs in how to
   select an auth-token, in particular mixing authTokenAgeMillis
   (milliseconds) with time_t values (seconds)
   - as a result, keystore would sometimes return tokens older
     than what credstore requested. This wasn't actually problem
      because the TA would check it anyway.
   - we now precisely define semantics in IKeystoreService.aidl

 - Another potential use-after-free bug was found in
   Credential.cpp L767

None of the fixes for these bugs affect CTS or VTS tests.

Bug: 181893400
Test: atest android.security.identity.cts on emulator
Test: atest android.security.identity.cts on crosshatch (w/ Android 11 era HAL)
Test: CtsVerifier's Identity Credential Authentication test crosshatch (w/ Android 11 era HAL)
Change-Id: I45a3fd16eff3b6a232d8b8c88f2e3dd3619a9c03

6 files changed, 103 insertions, 49 deletions

diff --git a/identity/Credential.cpp b/identity/Credential.cpp
index 4a2bae17..a3c72edc 100644
--- a/identity/Credential.cpp
+++ b/identity/Credential.cpp
@@ -117,26 +117,42 @@ Status Credential::selectAuthKey(bool allowUsingExhaustedKeys, bool allowUsingEx
                                                 "Error loading data for credential");
     }
 
-    selectedAuthKey_ = data->selectAuthKey(allowUsingExhaustedKeys, allowUsingExpiredKeys);
-    if (selectedAuthKey_ == nullptr) {
+    // We just check if a key is available, we actually don't store it since we
+    // don't keep CredentialData around between binder calls.
+    const AuthKeyData* authKey =
+        data->selectAuthKey(allowUsingExhaustedKeys, allowUsingExpiredKeys);
+    if (authKey == nullptr) {
         return Status::fromServiceSpecificError(
             ICredentialStore::ERROR_NO_AUTHENTICATION_KEY_AVAILABLE,
             "No suitable authentication key available");
     }
 
+    if (!ensureChallenge()) {
+        return Status::fromServiceSpecificError(ICredentialStore::ERROR_GENERIC,
+                                                "Error getting challenge (bug in HAL or TA)");
+    }
+    *_aidl_return = selectedChallenge_;
+    return Status::ok();
+}
+
+bool Credential::ensureChallenge() {
+    if (selectedChallenge_ != 0) {
+        return true;
+    }
+
     int64_t challenge;
     Status status = halBinder_->createAuthChallenge(&challenge);
     if (!status.isOk()) {
-        return halStatusToGenericError(status);
+        LOG(ERROR) << "Error getting challenge: " << status.exceptionMessage();
+        return false;
     }
     if (challenge == 0) {
-        return Status::fromServiceSpecificError(ICredentialStore::ERROR_GENERIC,
-                                                "Returned challenge is 0 (bug in HAL or TA)");
+        LOG(ERROR) << "Returned challenge is 0 (bug in HAL or TA)";
+        return false;
     }
 
     selectedChallenge_ = challenge;
-    *_aidl_return = challenge;
-    return Status::ok();
+    return true;
 }
 
 class CredstoreTokenCallback : public android::security::keystore::BnCredstoreTokenCallback,
@@ -279,13 +295,6 @@ Status Credential::getEntries(const vector<uint8_t>& requestMessage,
         }
     }
 
-    // If requesting a challenge-based authToken the idea is that authentication
-    // happens as part of the transaction. As such, authTokenMaxAgeMillis should
-    // be nearly zero. We'll use 10 seconds for this.
-    if (userAuthNeeded && selectedChallenge_ != 0) {
-        authTokenMaxAgeMillis = 10 * 1000;
-    }
-
     // Reset tokens and only get them if they're actually needed, e.g. if user authentication
     // is needed in any of the access control profiles for data items being requested.
     //
@@ -303,6 +312,28 @@ Status Credential::getEntries(const vector<uint8_t>& requestMessage,
     aidlVerificationToken.securityLevel = ::android::hardware::keymaster::SecurityLevel::SOFTWARE;
     aidlVerificationToken.mac.clear();
     if (userAuthNeeded) {
+        // If user authentication is needed, always get a challenge from the
+        // HAL/TA since it'll need it to check the returned VerificationToken
+        // for freshness.
+        if (!ensureChallenge()) {
+            return Status::fromServiceSpecificError(ICredentialStore::ERROR_GENERIC,
+                                                    "Error getting challenge (bug in HAL or TA)");
+        }
+
+        // Note: if all selected profiles require auth-on-every-presentation
+        // then authTokenMaxAgeMillis will be 0 (because timeoutMillis for each
+        // profile is 0). Which means that keystore will only return an
+        // AuthToken if its challenge matches what we pass, regardless of its
+        // age. This is intended b/c the HAL/TA will check not care about
+        // the age in this case, it only cares that the challenge matches.
+        //
+        // Otherwise, if one or more of the profiles is auth-with-a-timeout then
+        // authTokenMaxAgeMillis will be set to the largest of those
+        // timeouts. We'll get an AuthToken which satisfies this deadline if it
+        // exists. This authToken _may_ have the requested challenge but it's
+        // not a guarantee and it's also not required.
+        //
+
         vector<uint8_t> authTokenBytes;
         vector<uint8_t> verificationTokenBytes;
         if (!getTokensFromKeystore(selectedChallenge_, data->getSecureUserId(),
@@ -320,6 +351,7 @@ Status Credential::getEntries(const vector<uint8_t>& requestMessage,
         if (authTokenBytes.size() > 0) {
             HardwareAuthToken authToken =
                 android::hardware::keymaster::V4_0::support::hidlVec2AuthToken(authTokenBytes);
+
             // Convert from HIDL to AIDL...
             aidlAuthToken.challenge = int64_t(authToken.challenge);
             aidlAuthToken.userId = int64_t(authToken.userId);
@@ -351,15 +383,25 @@ Status Credential::getEntries(const vector<uint8_t>& requestMessage,
     // Note that the selectAuthKey() method is only called if a CryptoObject is involved at
     // the Java layer. So we could end up with no previously selected auth key and we may
     // need one.
-    const AuthKeyData* authKey = selectedAuthKey_;
-    if (sessionTranscript.size() > 0) {
-        if (authKey == nullptr) {
-            authKey = data->selectAuthKey(allowUsingExhaustedKeys, allowUsingExpiredKeys);
-            if (authKey == nullptr) {
-                return Status::fromServiceSpecificError(
-                    ICredentialStore::ERROR_NO_AUTHENTICATION_KEY_AVAILABLE,
-                    "No suitable authentication key available");
-            }
+    //
+    const AuthKeyData* authKey =
+        data->selectAuthKey(allowUsingExhaustedKeys, allowUsingExpiredKeys);
+    if (authKey == nullptr) {
+        // If no authKey is available, consider it an error only when a
+        // SessionTranscript was provided.
+        //
+        // We allow no SessionTranscript to be provided because it makes
+        // the API simpler to deal with insofar it can be used without having
+        // to generate any authentication keys.
+        //
+        // In this "no SessionTranscript is provided" mode we don't return
+        // DeviceNameSpaces nor a MAC over DeviceAuthentication so we don't
+        // need a device key.
+        //
+        if (sessionTranscript.size() > 0) {
+            return Status::fromServiceSpecificError(
+                ICredentialStore::ERROR_NO_AUTHENTICATION_KEY_AVAILABLE,
+                "No suitable authentication key available and one is needed");
         }
     }
     vector<uint8_t> signingKeyBlob;
@@ -750,31 +792,36 @@ Status Credential::update(sp<IWritableCredential>* _aidl_return) {
     //
     // It is because of this we need to set the CredentialKey certificate chain,
     // keyCount, and maxUsesPerKey below.
-    sp<WritableCredential> writableCredential =
-        new WritableCredential(dataPath_, credentialName_, docType.value(), true, hwInfo_,
-                               halWritableCredential, halApiVersion_);
+    sp<WritableCredential> writableCredential = new WritableCredential(
+        dataPath_, credentialName_, docType.value(), true, hwInfo_, halWritableCredential);
 
     writableCredential->setAttestationCertificate(data->getAttestationCertificate());
     auto [keyCount, maxUsesPerKey] = data->getAvailableAuthenticationKeys();
     writableCredential->setAvailableAuthenticationKeys(keyCount, maxUsesPerKey);
 
-    // Because its data has changed, we need to reconnect to the HAL when the
-    // credential has been updated... otherwise the remote object will have
-    // stale data for future calls (e.g. getAuthKeysNeedingCertification().
+    // Because its data has changed, we need to replace the binder for the
+    // IIdentityCredential when the credential has been updated... otherwise the
+    // remote object will have stale data for future calls, for example
+    // getAuthKeysNeedingCertification().
     //
-    // The joys and pitfalls of mutable objects...
+    // The way this is implemented is that setCredentialToReloadWhenUpdated()
+    // instructs the WritableCredential to call writableCredentialPersonalized()
+    // on |this|.
     //
-    writableCredential->setCredentialUpdatedCallback([this] {
-        Status status = this->ensureOrReplaceHalBinder();
-        if (!status.isOk()) {
-            LOG(ERROR) << "Error loading credential";
-        }
-    });
+    //
+    writableCredential->setCredentialToReloadWhenUpdated(this);
 
     *_aidl_return = writableCredential;
     return Status::ok();
 }
 
+void Credential::writableCredentialPersonalized() {
+    Status status = ensureOrReplaceHalBinder();
+    if (!status.isOk()) {
+        LOG(ERROR) << "Error reloading credential";
+    }
+}
+
 }  // namespace identity
 }  // namespace security
 }  // namespace android
diff --git a/identity/Credential.h b/identity/Credential.h
index 7f085150..a76f3cc0 100644
--- a/identity/Credential.h
+++ b/identity/Credential.h
@@ -50,6 +50,7 @@ class Credential : public BnCredential {
     ~Credential();
 
     Status ensureOrReplaceHalBinder();
+    void writableCredentialPersonalized();
 
     // ICredential overrides
     Status createEphemeralKeyPair(vector<uint8_t>* _aidl_return) override;
@@ -94,12 +95,13 @@ class Credential : public BnCredential {
     HardwareInformation hwInfo_;
     sp<IIdentityCredentialStore> halStoreBinder_;
 
-    const AuthKeyData* selectedAuthKey_ = nullptr;
     uint64_t selectedChallenge_ = 0;
 
     sp<IIdentityCredential> halBinder_;
     int halApiVersion_;
 
+    bool ensureChallenge();
+
     ssize_t
     calcExpectedDeviceNameSpacesSize(const vector<uint8_t>& requestMessage,
                                      const vector<RequestNamespaceParcel>& requestNamespaces,
diff --git a/identity/CredentialData.h b/identity/CredentialData.h
index b037997b..24b55d3d 100644
--- a/identity/CredentialData.h
+++ b/identity/CredentialData.h
@@ -55,7 +55,7 @@ struct AuthKeyData {
 
     vector<uint8_t> certificate;
     vector<uint8_t> keyBlob;
-    int64_t expirationDateMillisSinceEpoch;
+    int64_t expirationDateMillisSinceEpoch = 0;
     vector<uint8_t> staticAuthenticationData;
     vector<uint8_t> pendingCertificate;
     vector<uint8_t> pendingKeyBlob;
diff --git a/identity/CredentialStore.cpp b/identity/CredentialStore.cpp
index f77294ee..509e022c 100644
--- a/identity/CredentialStore.cpp
+++ b/identity/CredentialStore.cpp
@@ -90,7 +90,7 @@ Status CredentialStore::createCredential(const std::string& credentialName,
     }
 
     sp<IWritableCredential> writableCredential = new WritableCredential(
-        dataPath_, credentialName, docType, false, hwInfo_, halWritableCredential, halApiVersion_);
+        dataPath_, credentialName, docType, false, hwInfo_, halWritableCredential);
     *_aidl_return = writableCredential;
     return Status::ok();
 }
diff --git a/identity/WritableCredential.cpp b/identity/WritableCredential.cpp
index d0688b8d..a300e514 100644
--- a/identity/WritableCredential.cpp
+++ b/identity/WritableCredential.cpp
@@ -41,15 +41,14 @@ using ::android::hardware::identity::support::chunkVector;
 WritableCredential::WritableCredential(const string& dataPath, const string& credentialName,
                                        const string& docType, bool isUpdate,
                                        HardwareInformation hwInfo,
-                                       sp<IWritableIdentityCredential> halBinder, int halApiVersion)
+                                       sp<IWritableIdentityCredential> halBinder)
     : dataPath_(dataPath), credentialName_(credentialName), docType_(docType), isUpdate_(isUpdate),
-      hwInfo_(std::move(hwInfo)), halBinder_(halBinder), halApiVersion_(halApiVersion) {}
+      hwInfo_(std::move(hwInfo)), halBinder_(halBinder) {}
 
 WritableCredential::~WritableCredential() {}
 
-void WritableCredential::setCredentialUpdatedCallback(
-    std::function<void()>&& onCredentialUpdatedCallback) {
-    onCredentialUpdatedCallback_ = onCredentialUpdatedCallback;
+void WritableCredential::setCredentialToReloadWhenUpdated(sp<Credential> credential) {
+    credentialToReloadWhenUpdated_ = credential;
 }
 
 Status WritableCredential::ensureAttestationCertificateExists(const vector<uint8_t>& challenge) {
@@ -268,7 +267,10 @@ WritableCredential::personalize(const vector<AccessControlProfileParcel>& access
                                                 "Error saving credential data to disk");
     }
 
-    onCredentialUpdatedCallback_();
+    if (credentialToReloadWhenUpdated_) {
+        credentialToReloadWhenUpdated_->writableCredentialPersonalized();
+        credentialToReloadWhenUpdated_.clear();
+    }
 
     *_aidl_return = proofOfProvisioningSignature;
     return Status::ok();
diff --git a/identity/WritableCredential.h b/identity/WritableCredential.h
index 6ff31aec..838b9564 100644
--- a/identity/WritableCredential.h
+++ b/identity/WritableCredential.h
@@ -24,6 +24,8 @@
 
 #include <android/hardware/identity/IIdentityCredentialStore.h>
 
+#include "Credential.h"
+
 namespace android {
 namespace security {
 namespace identity {
@@ -38,13 +40,15 @@ class WritableCredential : public BnWritableCredential {
   public:
     WritableCredential(const string& dataPath, const string& credentialName, const string& docType,
                        bool isUpdate, HardwareInformation hwInfo,
-                       sp<IWritableIdentityCredential> halBinder, int halApiVersion);
+                       sp<IWritableIdentityCredential> halBinder);
     ~WritableCredential();
 
     // Used when updating a credential
     void setAttestationCertificate(const vector<uint8_t>& attestationCertificate);
     void setAvailableAuthenticationKeys(int keyCount, int maxUsesPerKey);
-    void setCredentialUpdatedCallback(std::function<void()>&& onCredentialUpdatedCallback);
+
+    // Used by Credential::update()
+    void setCredentialToReloadWhenUpdated(sp<Credential> credential);
 
     // IWritableCredential overrides
     Status getCredentialKeyCertificateChain(const vector<uint8_t>& challenge,
@@ -61,13 +65,12 @@ class WritableCredential : public BnWritableCredential {
     bool isUpdate_;
     HardwareInformation hwInfo_;
     sp<IWritableIdentityCredential> halBinder_;
-    int halApiVersion_;
 
     vector<uint8_t> attestationCertificate_;
     int keyCount_ = 0;
     int maxUsesPerKey_ = 1;
 
-    std::function<void()> onCredentialUpdatedCallback_ = []() {};
+    sp<Credential> credentialToReloadWhenUpdated_;
 
     ssize_t calcExpectedProofOfProvisioningSize(
         const vector<AccessControlProfileParcel>& accessControlProfiles,