Merge "Snap for 6533464 from 6a917e5245d0771b468d78c41b56039bf07c802a to sdk-release" into sdk-releaseplatform-tools-30.0.3platform-tools-30.0.2

13 files changed, 479 insertions, 53 deletions

diff --git a/METADATA b/METADATA
new file mode 100644
index 00000000..d97975ca
--- /dev/null
+++ b/METADATA
@@ -0,0 +1,3 @@
+third_party {
+  license_type: NOTICE
+}
diff --git a/identity/Credential.cpp b/identity/Credential.cpp
index 05c31d3d..59a4d81e 100644
--- a/identity/Credential.cpp
+++ b/identity/Credential.cpp
@@ -22,6 +22,7 @@
 
 #include <android/security/identity/ICredentialStore.h>
 
+#include <android/security/keystore/BnCredstoreTokenCallback.h>
 #include <android/security/keystore/IKeystoreService.h>
 #include <binder/IPCThreadState.h>
 #include <binder/IServiceManager.h>
@@ -29,6 +30,8 @@
 
 #include <cppbor.h>
 #include <cppbor_parse.h>
+#include <future>
+#include <tuple>
 
 #include "Credential.h"
 #include "CredentialData.h"
@@ -39,6 +42,8 @@ namespace security {
 namespace identity {
 
 using std::optional;
+using std::promise;
+using std::tuple;
 
 using android::security::keystore::IKeystoreService;
 
@@ -48,7 +53,9 @@ using ::android::hardware::identity::support::ecKeyPairGetPublicKey;
 using ::android::hardware::identity::support::sha256;
 
 using android::hardware::keymaster::V4_0::HardwareAuthToken;
+using android::hardware::keymaster::V4_0::VerificationToken;
 using AidlHardwareAuthToken = android::hardware::keymaster::HardwareAuthToken;
+using AidlVerificationToken = android::hardware::keymaster::VerificationToken;
 
 Credential::Credential(CipherSuite cipherSuite, const std::string& dataPath,
                        const std::string& credentialName)
@@ -116,12 +123,22 @@ Status Credential::selectAuthKey(bool allowUsingExhaustedKeys, int64_t* _aidl_re
     return Status::ok();
 }
 
+class CredstoreTokenCallback : public android::security::keystore::BnCredstoreTokenCallback,
+                               public promise<tuple<bool, vector<uint8_t>, vector<uint8_t>>> {
+  public:
+    CredstoreTokenCallback() {}
+    virtual Status onFinished(bool success, const vector<uint8_t>& authToken,
+                              const vector<uint8_t>& verificationToken) override {
+        this->set_value({success, authToken, verificationToken});
+        return Status::ok();
+    }
+};
+
 // Returns false if an error occurred communicating with keystore.
 //
-// Sets |authToken| to the empty vector if an auth token couldn't be obtained.
-//
-bool getAuthTokenFromKeystore(uint64_t challenge, uint64_t secureUserId,
-                              unsigned int authTokenMaxAgeMillis, vector<uint8_t>& authToken) {
+bool getTokensFromKeystore(uint64_t challenge, uint64_t secureUserId,
+                           unsigned int authTokenMaxAgeMillis, vector<uint8_t>& authToken,
+                           vector<uint8_t>& verificationToken) {
     sp<IServiceManager> sm = defaultServiceManager();
     sp<IBinder> binder = sm->getService(String16("android.security.keystore"));
     sp<IKeystoreService> keystore = interface_cast<IKeystoreService>(binder);
@@ -129,13 +146,27 @@ bool getAuthTokenFromKeystore(uint64_t challenge, uint64_t secureUserId,
         return false;
     }
 
-    vector<uint8_t> returnedAuthToken;
-    Status ret = keystore->getAuthTokenForCredstore(challenge, secureUserId, authTokenMaxAgeMillis,
-                                                    &returnedAuthToken);
-    if (!ret.isOk()) {
+    sp<CredstoreTokenCallback> callback = new CredstoreTokenCallback();
+    auto future = callback->get_future();
+
+    Status status =
+        keystore->getTokensForCredstore(challenge, secureUserId, authTokenMaxAgeMillis, callback);
+    if (!status.isOk()) {
+        return false;
+    }
+
+    auto fstatus = future.wait_for(std::chrono::milliseconds(5000));
+    if (fstatus != std::future_status::ready) {
+        LOG(ERROR) << "Waited 5 seconds from tokens for credstore, aborting";
+        return false;
+    }
+    auto [success, returnedAuthToken, returnedVerificationToken] = future.get();
+    if (!success) {
+        LOG(ERROR) << "Error getting tokens from credstore";
         return false;
     }
     authToken = returnedAuthToken;
+    verificationToken = returnedVerificationToken;
     return true;
 }
 
@@ -217,16 +248,37 @@ Status Credential::getEntries(const vector<uint8_t>& requestMessage,
         authTokenMaxAgeMillis = 10 * 1000;
     }
 
-    // Only get an authToken if it's actually needed.
+    // 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.
+    //
     AidlHardwareAuthToken aidlAuthToken;
+    AidlVerificationToken aidlVerificationToken;
+    aidlAuthToken.challenge = 0;
+    aidlAuthToken.userId = 0;
+    aidlAuthToken.authenticatorId = 0;
+    aidlAuthToken.authenticatorType =
+        ::android::hardware::keymaster::HardwareAuthenticatorType::NONE;
+    aidlAuthToken.timestamp.milliSeconds = 0;
+    aidlAuthToken.mac.clear();
+    aidlVerificationToken.challenge = 0;
+    aidlVerificationToken.timestamp.milliSeconds = 0;
+    aidlVerificationToken.securityLevel = ::android::hardware::keymaster::SecurityLevel::SOFTWARE;
+    aidlVerificationToken.mac.clear();
     if (userAuthNeeded) {
         vector<uint8_t> authTokenBytes;
-        if (!getAuthTokenFromKeystore(selectedChallenge_, data_->getSecureUserId(),
-                                      authTokenMaxAgeMillis, authTokenBytes)) {
-            LOG(ERROR) << "Error getting auth token from keystore";
+        vector<uint8_t> verificationTokenBytes;
+        if (!getTokensFromKeystore(selectedChallenge_, data_->getSecureUserId(),
+                                   authTokenMaxAgeMillis, authTokenBytes, verificationTokenBytes)) {
+            LOG(ERROR) << "Error getting tokens from keystore";
             return Status::fromServiceSpecificError(ICredentialStore::ERROR_GENERIC,
-                                                    "Error getting auth token from keystore");
+                                                    "Error getting tokens from keystore");
         }
+
+        // It's entirely possible getTokensFromKeystore() succeeded but didn't
+        // return any tokens (in which case the returned byte-vectors are
+        // empty). For example, this can happen if no auth token is available
+        // which satifies e.g. |authTokenMaxAgeMillis|.
+        //
         if (authTokenBytes.size() > 0) {
             HardwareAuthToken authToken =
                 android::hardware::keymaster::V4_0::support::hidlVec2AuthToken(authTokenBytes);
@@ -240,6 +292,22 @@ Status Credential::getEntries(const vector<uint8_t>& requestMessage,
             aidlAuthToken.timestamp.milliSeconds = int64_t(authToken.timestamp);
             aidlAuthToken.mac = authToken.mac;
         }
+
+        if (verificationTokenBytes.size() > 0) {
+            optional<VerificationToken> token =
+                android::hardware::keymaster::V4_0::support::deserializeVerificationToken(
+                    verificationTokenBytes);
+            if (!token) {
+                LOG(ERROR) << "Error deserializing verification token";
+                return Status::fromServiceSpecificError(ICredentialStore::ERROR_GENERIC,
+                                                        "Error deserializing verification token");
+            }
+            aidlVerificationToken.challenge = token->challenge;
+            aidlVerificationToken.timestamp.milliSeconds = token->timestamp;
+            aidlVerificationToken.securityLevel =
+                ::android::hardware::keymaster::SecurityLevel(token->securityLevel);
+            aidlVerificationToken.mac = token->mac;
+        }
     }
 
     // Note that the selectAuthKey() method is only called if a CryptoObject is involved at
@@ -261,7 +329,42 @@ Status Credential::getEntries(const vector<uint8_t>& requestMessage,
         signingKeyBlob = authKey->keyBlob;
     }
 
-    Status status =
+    // Pass the HAL enough information to allow calculating the size of
+    // DeviceNameSpaces ahead of time.
+    vector<RequestNamespace> halRequestNamespaces;
+    for (const RequestNamespaceParcel& rns : requestNamespaces) {
+        RequestNamespace ns;
+        ns.namespaceName = rns.namespaceName;
+        for (const RequestEntryParcel& rep : rns.entries) {
+            optional<EntryData> entryData = data_->getEntryData(rns.namespaceName, rep.name);
+            if (entryData) {
+                RequestDataItem di;
+                di.name = rep.name;
+                di.size = entryData.value().size;
+                di.accessControlProfileIds = entryData.value().accessControlProfileIds;
+                ns.items.push_back(di);
+            }
+        }
+        if (ns.items.size() > 0) {
+            halRequestNamespaces.push_back(ns);
+        }
+    }
+    // This is not catastrophic, we might be dealing with a version 1 implementation which
+    // doesn't have this method.
+    Status status = halBinder_->setRequestedNamespaces(halRequestNamespaces);
+    if (!status.isOk()) {
+        LOG(INFO) << "Failed setting expected requested namespaces, assuming V1 HAL "
+                  << "and continuing";
+    }
+
+    // Pass the verification token. Failure is OK, this method isn't in the V1 HAL.
+    status = halBinder_->setVerificationToken(aidlVerificationToken);
+    if (!status.isOk()) {
+        LOG(INFO) << "Failed setting verification token, assuming V1 HAL "
+                  << "and continuing";
+    }
+
+    status =
         halBinder_->startRetrieval(selectedProfiles, aidlAuthToken, requestMessage, signingKeyBlob,
                                    sessionTranscript, readerSignature, requestCounts);
     if (!status.isOk() && status.exceptionCode() == binder::Status::EX_SERVICE_SPECIFIC) {
diff --git a/identity/Credential.h b/identity/Credential.h
index a0d90634..e2880d98 100644
--- a/identity/Credential.h
+++ b/identity/Credential.h
@@ -38,6 +38,8 @@ using ::std::vector;
 using ::android::hardware::identity::CipherSuite;
 using ::android::hardware::identity::IIdentityCredential;
 using ::android::hardware::identity::IIdentityCredentialStore;
+using ::android::hardware::identity::RequestDataItem;
+using ::android::hardware::identity::RequestNamespace;
 
 class Credential : public BnCredential {
   public:
@@ -80,6 +82,11 @@ class Credential : public BnCredential {
     sp<CredentialData> data_;
 
     sp<IIdentityCredential> halBinder_;
+
+    ssize_t
+    calcExpectedDeviceNameSpacesSize(const vector<uint8_t>& requestMessage,
+                                     const vector<RequestNamespaceParcel>& requestNamespaces,
+                                     uint32_t authorizedAcps);
 };
 
 }  // namespace identity
diff --git a/identity/WritableCredential.cpp b/identity/WritableCredential.cpp
index dec95a6a..cb2d6ffd 100644
--- a/identity/WritableCredential.cpp
+++ b/identity/WritableCredential.cpp
@@ -39,10 +39,10 @@ using ::android::hardware::identity::SecureAccessControlProfile;
 using ::android::hardware::identity::support::chunkVector;
 
 WritableCredential::WritableCredential(const string& dataPath, const string& credentialName,
-                                       const string& /*docType*/, size_t dataChunkSize,
+                                       const string& docType, size_t dataChunkSize,
                                        sp<IWritableIdentityCredential> halBinder)
-    : dataPath_(dataPath), credentialName_(credentialName), dataChunkSize_(dataChunkSize),
-      halBinder_(halBinder) {}
+    : dataPath_(dataPath), credentialName_(credentialName), docType_(docType),
+      dataChunkSize_(dataChunkSize), halBinder_(halBinder) {}
 
 WritableCredential::~WritableCredential() {}
 
@@ -89,6 +89,62 @@ Status WritableCredential::getCredentialKeyCertificateChain(const vector<uint8_t
     return Status::ok();
 }
 
+ssize_t WritableCredential::calcExpectedProofOfProvisioningSize(
+    const vector<AccessControlProfileParcel>& accessControlProfiles,
+    const vector<EntryNamespaceParcel>& entryNamespaces) {
+
+    // Right now, we calculate the size by simply just calculating the
+    // CBOR. There's a little bit of overhead associated with this (as compared
+    // to just adding up sizes) but it's a lot simpler and robust. In the future
+    // if this turns out to be a problem, we can optimize it.
+    //
+
+    cppbor::Array acpArray;
+    for (const AccessControlProfileParcel& profile : accessControlProfiles) {
+        cppbor::Map map;
+        map.add("id", profile.id);
+        if (profile.readerCertificate.size() > 0) {
+            map.add("readerCertificate", cppbor::Bstr(profile.readerCertificate));
+        }
+        if (profile.userAuthenticationRequired) {
+            map.add("userAuthenticationRequired", profile.userAuthenticationRequired);
+            map.add("timeoutMillis", profile.userAuthenticationTimeoutMillis);
+        }
+        acpArray.add(std::move(map));
+    }
+
+    cppbor::Map dataMap;
+    for (const EntryNamespaceParcel& ensParcel : entryNamespaces) {
+        cppbor::Array entriesArray;
+        for (const EntryParcel& eParcel : ensParcel.entries) {
+            // TODO: ideally do do this without parsing the data (but still validate data is valid
+            // CBOR).
+            auto [itemForValue, _, _2] = cppbor::parse(eParcel.value);
+            if (itemForValue == nullptr) {
+                return -1;
+            }
+            cppbor::Map entryMap;
+            entryMap.add("name", eParcel.name);
+            entryMap.add("value", std::move(itemForValue));
+            cppbor::Array acpIdsArray;
+            for (int32_t id : eParcel.accessControlProfileIds) {
+                acpIdsArray.add(id);
+            }
+            entryMap.add("accessControlProfiles", std::move(acpIdsArray));
+            entriesArray.add(std::move(entryMap));
+        }
+        dataMap.add(ensParcel.namespaceName, std::move(entriesArray));
+    }
+
+    cppbor::Array array;
+    array.add("ProofOfProvisioning");
+    array.add(docType_);
+    array.add(std::move(acpArray));
+    array.add(std::move(dataMap));
+    array.add(false);  // testCredential
+    return array.encode().size();
+}
+
 Status
 WritableCredential::personalize(const vector<AccessControlProfileParcel>& accessControlProfiles,
                                 const vector<EntryNamespaceParcel>& entryNamespaces,
@@ -113,7 +169,21 @@ WritableCredential::personalize(const vector<AccessControlProfileParcel>& access
         entryCounts.push_back(ensParcel.entries.size());
     }
 
-    Status status = halBinder_->startPersonalization(accessControlProfiles.size(), entryCounts);
+    ssize_t expectedPoPSize =
+        calcExpectedProofOfProvisioningSize(accessControlProfiles, entryNamespaces);
+    if (expectedPoPSize < 0) {
+        return Status::fromServiceSpecificError(ICredentialStore::ERROR_GENERIC,
+                                                "Data is not valid CBOR");
+    }
+    // This is not catastrophic, we might be dealing with a version 1 implementation which
+    // doesn't have this method.
+    Status status = halBinder_->setExpectedProofOfProvisioningSize(expectedPoPSize);
+    if (!status.isOk()) {
+        LOG(INFO) << "Failed setting expected ProofOfProvisioning size, assuming V1 HAL "
+                  << "and continuing";
+    }
+
+    status = halBinder_->startPersonalization(accessControlProfiles.size(), entryCounts);
     if (!status.isOk()) {
         return halStatusToGenericError(status);
     }
diff --git a/identity/WritableCredential.h b/identity/WritableCredential.h
index 8b5e19e2..eb63aca2 100644
--- a/identity/WritableCredential.h
+++ b/identity/WritableCredential.h
@@ -50,10 +50,15 @@ class WritableCredential : public BnWritableCredential {
   private:
     string dataPath_;
     string credentialName_;
+    string docType_;
     size_t dataChunkSize_;
     sp<IWritableIdentityCredential> halBinder_;
     vector<uint8_t> attestationCertificate_;
 
+    ssize_t calcExpectedProofOfProvisioningSize(
+        const vector<AccessControlProfileParcel>& accessControlProfiles,
+        const vector<EntryNamespaceParcel>& entryNamespaces);
+
     Status ensureAttestationCertificateExists(const vector<uint8_t>& challenge);
 };
 
diff --git a/identity/main.cpp b/identity/main.cpp
index af03a306..8f4968db 100644
--- a/identity/main.cpp
+++ b/identity/main.cpp
@@ -53,6 +53,9 @@ int main(int argc, char* argv[]) {
     CHECK(ret == ::android::OK) << "Couldn't register binder service";
     LOG(ERROR) << "Registered binder service";
 
+    // This is needed for binder callbacks from keystore on a ICredstoreTokenCallback binder.
+    android::ProcessState::self()->startThreadPool();
+
     IPCThreadState::self()->joinThreadPool();
 
     return 0;
diff --git a/keystore/Android.bp b/keystore/Android.bp
index 61450471..b8817577 100644
--- a/keystore/Android.bp
+++ b/keystore/Android.bp
@@ -294,6 +294,7 @@ filegroup {
     name: "keystore_aidl",
     srcs: [
         "binder/android/security/IConfirmationPromptCallback.aidl",
+        "binder/android/security/keystore/ICredstoreTokenCallback.aidl",
         "binder/android/security/keystore/IKeystoreCertificateChainCallback.aidl",
         "binder/android/security/keystore/IKeystoreExportKeyCallback.aidl",
         "binder/android/security/keystore/IKeystoreKeyCharacteristicsCallback.aidl",
diff --git a/keystore/binder/android/security/keystore/ICredstoreTokenCallback.aidl b/keystore/binder/android/security/keystore/ICredstoreTokenCallback.aidl
new file mode 100644
index 00000000..b42e3d4c
--- /dev/null
+++ b/keystore/binder/android/security/keystore/ICredstoreTokenCallback.aidl
@@ -0,0 +1,25 @@
+/**
+ * Copyright (c) 2020, The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package android.security.keystore;
+
+
+/**
+ * @hide
+ */
+oneway interface ICredstoreTokenCallback {
+	void onFinished(boolean success, in byte[] authToken, in byte[] verificationToken);
+}
diff --git a/keystore/binder/android/security/keystore/IKeystoreService.aidl b/keystore/binder/android/security/keystore/IKeystoreService.aidl
index fcea115f..6edca564 100644
--- a/keystore/binder/android/security/keystore/IKeystoreService.aidl
+++ b/keystore/binder/android/security/keystore/IKeystoreService.aidl
@@ -19,6 +19,7 @@ package android.security.keystore;
 import android.security.keymaster.KeymasterArguments;
 import android.security.keymaster.KeymasterBlob;
 import android.security.keymaster.OperationResult;
+import android.security.keystore.ICredstoreTokenCallback;
 import android.security.keystore.IKeystoreResponseCallback;
 import android.security.keystore.IKeystoreKeyCharacteristicsCallback;
 import android.security.keystore.IKeystoreExportKeyCallback;
@@ -86,5 +87,6 @@ interface IKeystoreService {
     int listUidsOfAuthBoundKeys(out @utf8InCpp List<String> uids);
 
     // Called by credstore (and only credstore).
-    byte[] getAuthTokenForCredstore(in long challenge, in long secureUserId, in int authTokenMaxAgeMillis);
+    void getTokensForCredstore(in long challenge, in long secureUserId, in int authTokenMaxAgeMillis,
+                               in ICredstoreTokenCallback cb);
 }
diff --git a/keystore/key_store_service.cpp b/keystore/key_store_service.cpp
index 666b48a5..583f5b6e 100644
--- a/keystore/key_store_service.cpp
+++ b/keystore/key_store_service.cpp
@@ -57,6 +57,8 @@ using namespace android;
 namespace {
 
 using ::android::binder::Status;
+using android::hardware::keymaster::V4_0::support::authToken2HidlVec;
+using android::hardware::keymaster::V4_0::support::serializeVerificationToken;
 using android::security::keymaster::ExportResult;
 using android::security::keymaster::KeymasterArguments;
 using android::security::keymaster::KeymasterBlob;
@@ -64,6 +66,7 @@ using android::security::keymaster::KeymasterCertificateChain;
 using android::security::keymaster::operationFailed;
 using android::security::keymaster::OperationResult;
 using ConfirmationResponseCode = android::hardware::confirmationui::V1_0::ResponseCode;
+using ::android::security::keystore::ICredstoreTokenCallback;
 using ::android::security::keystore::IKeystoreOperationResultCallback;
 using ::android::security::keystore::IKeystoreResponseCallback;
 using ::android::security::keystore::KeystoreResponse;
@@ -943,9 +946,9 @@ Status KeyStoreService::addAuthToken(const ::std::vector<uint8_t>& authTokenAsVe
     return Status::ok();
 }
 
-Status KeyStoreService::getAuthTokenForCredstore(int64_t challenge, int64_t secureUserId,
-                                                 int32_t authTokenMaxAgeMillis,
-                                                 std::vector<uint8_t>* _aidl_return) {
+Status KeyStoreService::getTokensForCredstore(int64_t challenge, int64_t secureUserId,
+                                              int32_t authTokenMaxAgeMillis,
+                                              const ::android::sp<ICredstoreTokenCallback>& cb) {
     uid_t callingUid = IPCThreadState::self()->getCallingUid();
     if (callingUid != AID_CREDSTORE) {
         return Status::fromServiceSpecificError(static_cast<int32_t>(0));
@@ -953,29 +956,147 @@ Status KeyStoreService::getAuthTokenForCredstore(int64_t challenge, int64_t secu
 
     auto [err, authToken] = mKeyStore->getAuthTokenTable().FindAuthorizationForCredstore(
         challenge, secureUserId, authTokenMaxAgeMillis);
-    std::vector<uint8_t> ret;
-    if (err == AuthTokenTable::OK) {
-        ret = android::hardware::keymaster::V4_0::support::authToken2HidlVec(authToken);
+    // It's entirely possible we couldn't find an authToken (e.g. no user auth
+    // happened within the requested deadline) and in that case, we just
+    // callback immediately signaling success but just not returning any tokens.
+    if (err != AuthTokenTable::OK) {
+        cb->onFinished(true, {} /* serializedAuthToken */, {} /* serializedVerificationToken */);
+        return Status::ok();
+    }
+
+    // If we did find an authToken, get a verificationToken as well...
+    //
+    std::vector<uint8_t> serializedAuthToken = authToken2HidlVec(authToken);
+    std::vector<uint8_t> serializedVerificationToken;
+    std::shared_ptr<KeymasterWorker> dev = mKeyStore->getDevice(SecurityLevel::TRUSTED_ENVIRONMENT);
+    if (!dev) {
+        LOG(ERROR) << "Unable to get KM device for SecurityLevel::TRUSTED_ENVIRONMENT";
+        dev = mKeyStore->getDevice(SecurityLevel::SOFTWARE);
+        if (!dev) {
+            LOG(ERROR) << "Unable to get KM device for SecurityLevel::SOFTWARE";
+            cb->onFinished(false, {}, {});
+            return Status::fromServiceSpecificError(static_cast<int32_t>(0));
+        }
     }
-    *_aidl_return = ret;
+
+    dev->verifyAuthorization(
+        challenge, {} /* params */, authToken,
+        [serializedAuthToken, cb](KeyStoreServiceReturnCode rc, HardwareAuthToken,
+                                  VerificationToken verificationToken) {
+            if (rc != ErrorCode::OK) {
+                LOG(ERROR) << "verifyAuthorization failed, rc=" << rc;
+                cb->onFinished(false, {}, {});
+                return;
+            }
+            std::optional<std::vector<uint8_t>> serializedVerificationToken =
+                serializeVerificationToken(verificationToken);
+            if (!serializedVerificationToken) {
+                LOG(ERROR) << "Error serializing verificationToken";
+                cb->onFinished(false, {}, {});
+                return;
+            }
+            cb->onFinished(true, serializedAuthToken, serializedVerificationToken.value());
+        });
+
     return Status::ok();
 }
 
+bool isDeviceIdAttestationTag(Tag tag) {
+    switch (tag) {
+    case Tag::ATTESTATION_ID_BRAND:
+    case Tag::ATTESTATION_ID_DEVICE:
+    case Tag::ATTESTATION_ID_MANUFACTURER:
+    case Tag::ATTESTATION_ID_MODEL:
+    case Tag::ATTESTATION_ID_PRODUCT:
+    case Tag::ATTESTATION_ID_IMEI:
+    case Tag::ATTESTATION_ID_MEID:
+    case Tag::ATTESTATION_ID_SERIAL:
+        return true;
+    case Tag::INVALID:
+    case Tag::PURPOSE:
+    case Tag::ALGORITHM:
+    case Tag::KEY_SIZE:
+    case Tag::BLOCK_MODE:
+    case Tag::DIGEST:
+    case Tag::PADDING:
+    case Tag::CALLER_NONCE:
+    case Tag::MIN_MAC_LENGTH:
+    case Tag::EC_CURVE:
+    case Tag::RSA_PUBLIC_EXPONENT:
+    case Tag::INCLUDE_UNIQUE_ID:
+    case Tag::BLOB_USAGE_REQUIREMENTS:
+    case Tag::BOOTLOADER_ONLY:
+    case Tag::ROLLBACK_RESISTANCE:
+    case Tag::HARDWARE_TYPE:
+    case Tag::ACTIVE_DATETIME:
+    case Tag::ORIGINATION_EXPIRE_DATETIME:
+    case Tag::USAGE_EXPIRE_DATETIME:
+    case Tag::MIN_SECONDS_BETWEEN_OPS:
+    case Tag::MAX_USES_PER_BOOT:
+    case Tag::USER_ID:
+    case Tag::USER_SECURE_ID:
+    case Tag::NO_AUTH_REQUIRED:
+    case Tag::USER_AUTH_TYPE:
+    case Tag::AUTH_TIMEOUT:
+    case Tag::ALLOW_WHILE_ON_BODY:
+    case Tag::TRUSTED_USER_PRESENCE_REQUIRED:
+    case Tag::TRUSTED_CONFIRMATION_REQUIRED:
+    case Tag::UNLOCKED_DEVICE_REQUIRED:
+    case Tag::APPLICATION_ID:
+    case Tag::APPLICATION_DATA:
+    case Tag::CREATION_DATETIME:
+    case Tag::ORIGIN:
+    case Tag::ROOT_OF_TRUST:
+    case Tag::OS_VERSION:
+    case Tag::OS_PATCHLEVEL:
+    case Tag::UNIQUE_ID:
+    case Tag::ATTESTATION_CHALLENGE:
+    case Tag::ATTESTATION_APPLICATION_ID:
+    case Tag::VENDOR_PATCHLEVEL:
+    case Tag::BOOT_PATCHLEVEL:
+    case Tag::ASSOCIATED_DATA:
+    case Tag::NONCE:
+    case Tag::MAC_LENGTH:
+    case Tag::RESET_SINCE_ID_ROTATION:
+    case Tag::CONFIRMATION_TOKEN:
+        return false;
+        // no default, all values must be present in the switch, in this way the compiler ensures
+        // that new values added in the Tag enum are also added here.
+    }
+}
+
+// These are attestation id tags that are not unique per device and don't require special permission
+// to be attested. Any addition to this list needs privacy review and approval (PWG).
+bool isDevicePropertyAttestationTag(Tag tag) {
+    switch (tag) {
+    case Tag::ATTESTATION_ID_BRAND:
+    case Tag::ATTESTATION_ID_DEVICE:
+    case Tag::ATTESTATION_ID_MANUFACTURER:
+    case Tag::ATTESTATION_ID_MODEL:
+    case Tag::ATTESTATION_ID_PRODUCT:
+        return true;
+    default:
+        return false;
+    }
+}
+
 bool isDeviceIdAttestationRequested(const KeymasterArguments& params) {
     const hardware::hidl_vec<KeyParameter>& paramsVec = params.getParameters();
     for (size_t i = 0; i < paramsVec.size(); ++i) {
-        switch (paramsVec[i].tag) {
-        case Tag::ATTESTATION_ID_BRAND:
-        case Tag::ATTESTATION_ID_DEVICE:
-        case Tag::ATTESTATION_ID_MANUFACTURER:
-        case Tag::ATTESTATION_ID_MODEL:
-        case Tag::ATTESTATION_ID_PRODUCT:
-        case Tag::ATTESTATION_ID_IMEI:
-        case Tag::ATTESTATION_ID_MEID:
-        case Tag::ATTESTATION_ID_SERIAL:
+        if (isDeviceIdAttestationTag(paramsVec[i].tag)) {
+            return true;
+        }
+    }
+    return false;
+}
+
+// Device properties can be attested safely without special permission
+bool needsPermissionToAttestDeviceIds(const KeymasterArguments& params) {
+    const hardware::hidl_vec<KeyParameter>& paramsVec = params.getParameters();
+    for (size_t i = 0; i < paramsVec.size(); ++i) {
+        if (isDeviceIdAttestationTag(paramsVec[i].tag) &&
+            !isDevicePropertyAttestationTag(paramsVec[i].tag)) {
             return true;
-        default:
-            continue;
         }
     }
     return false;
@@ -991,7 +1112,7 @@ Status KeyStoreService::attestKey(
 
     uid_t callingUid = IPCThreadState::self()->getCallingUid();
 
-    if (isDeviceIdAttestationRequested(params) && (get_app_id(callingUid) != AID_SYSTEM)) {
+    if (needsPermissionToAttestDeviceIds(params) && (get_app_id(callingUid) != AID_SYSTEM)) {
         return AIDL_RETURN(KeyStoreServiceReturnCode(ErrorCode::INVALID_ARGUMENT));
     }
 
@@ -1055,14 +1176,19 @@ Status KeyStoreService::attestDeviceIds(
     }
 
     uid_t callingUid = IPCThreadState::self()->getCallingUid();
-    sp<IBinder> binder = defaultServiceManager()->getService(String16("permission"));
-    if (binder == nullptr) {
-        return AIDL_RETURN(ErrorCode::CANNOT_ATTEST_IDS);
-    }
-    if (!interface_cast<IPermissionController>(binder)->checkPermission(
-            String16("android.permission.READ_PRIVILEGED_PHONE_STATE"),
-            IPCThreadState::self()->getCallingPid(), callingUid)) {
-        return AIDL_RETURN(ErrorCode::CANNOT_ATTEST_IDS);
+
+    // Request special permission only for unique ids
+    if (needsPermissionToAttestDeviceIds(params)) {
+        sp<IBinder> binder = defaultServiceManager()->getService(String16("permission"));
+        if (binder == nullptr) {
+            return AIDL_RETURN(ErrorCode::CANNOT_ATTEST_IDS);
+        }
+
+        if (!interface_cast<IPermissionController>(binder)->checkPermission(
+                String16("android.permission.READ_PRIVILEGED_PHONE_STATE"),
+                IPCThreadState::self()->getCallingPid(), callingUid)) {
+            return AIDL_RETURN(ErrorCode::CANNOT_ATTEST_IDS);
+        }
     }
 
     AuthorizationSet mutableParams = params.getParameters();
@@ -1322,11 +1448,22 @@ bool KeyStoreService::checkAllowedOperationParams(const hidl_vec<KeyParameter>&
 }
 
 Status KeyStoreService::onKeyguardVisibilityChanged(bool isShowing, int32_t userId,
-                                                    int32_t* aidl_return) {
+                                                    int32_t* _aidl_return) {
+    if (isShowing) {
+        if (!checkBinderPermission(P_LOCK, UID_SELF)) {
+            LOG(WARNING) << "onKeyguardVisibilityChanged called with isShowing == true but "
+                            "without LOCK permission";
+            return AIDL_RETURN(ResponseCode::PERMISSION_DENIED);
+        }
+    } else {
+        if (!checkBinderPermission(P_UNLOCK, UID_SELF)) {
+            LOG(WARNING) << "onKeyguardVisibilityChanged called with isShowing == false but "
+                            "without UNLOCK permission";
+            return AIDL_RETURN(ResponseCode::PERMISSION_DENIED);
+        }
+    }
     mKeyStore->getEnforcementPolicy().set_device_locked(isShowing, userId);
-    *aidl_return = static_cast<int32_t>(ResponseCode::NO_ERROR);
-
-    return Status::ok();
+    return AIDL_RETURN(ResponseCode::NO_ERROR);
 }
 
 }  // namespace keystore
diff --git a/keystore/key_store_service.h b/keystore/key_store_service.h
index 8c1d5089..5fdddb99 100644
--- a/keystore/key_store_service.h
+++ b/keystore/key_store_service.h
@@ -132,9 +132,9 @@ class KeyStoreService : public android::security::keystore::BnKeystoreService {
           const ::android::sp<::android::IBinder>& token, int32_t* _aidl_return) override;
     ::android::binder::Status addAuthToken(const ::std::vector<uint8_t>& authToken,
                                            int32_t* _aidl_return) override;
-    ::android::binder::Status
-    getAuthTokenForCredstore(int64_t challenge, int64_t secureUserId, int32_t authTokenMaxAge,
-                             ::std::vector<uint8_t>* _aidl_return) override;
+    ::android::binder::Status getTokensForCredstore(
+        int64_t challenge, int64_t secureUserId, int32_t authTokenMaxAge,
+        const ::android::sp<::android::security::keystore::ICredstoreTokenCallback>& cb) override;
     ::android::binder::Status onUserAdded(int32_t userId, int32_t parentId,
                                           int32_t* _aidl_return) override;
     ::android::binder::Status onUserRemoved(int32_t userId, int32_t* _aidl_return) override;
diff --git a/keystore/tests/Android.bp b/keystore/tests/Android.bp
index eac6fe63..883e0200 100644
--- a/keystore/tests/Android.bp
+++ b/keystore/tests/Android.bp
@@ -13,6 +13,7 @@ cc_test {
         "auth_token_formatting_test.cpp",
         "blob_test.cpp",
         "confirmationui_rate_limiting_test.cpp",
+        "verification_token_seralization_test.cpp",
         "gtest_main.cpp",
     ],
     name: "keystore_unit_tests",
diff --git a/keystore/tests/verification_token_seralization_test.cpp b/keystore/tests/verification_token_seralization_test.cpp
new file mode 100644
index 00000000..276541a4
--- /dev/null
+++ b/keystore/tests/verification_token_seralization_test.cpp
@@ -0,0 +1,69 @@
+/*
+ * Copyright (C) 2015 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <gtest/gtest.h>
+
+#include <keymasterV4_0/keymaster_utils.h>
+
+namespace keystore {
+namespace test {
+
+using android::hardware::keymaster::V4_0::SecurityLevel;
+using android::hardware::keymaster::V4_0::VerificationToken;
+using android::hardware::keymaster::V4_0::support::deserializeVerificationToken;
+using android::hardware::keymaster::V4_0::support::serializeVerificationToken;
+using std::optional;
+using std::vector;
+
+TEST(VerificationTokenSeralizationTest, SerializationTest) {
+    VerificationToken token;
+    token.challenge = 12345;
+    token.timestamp = 67890;
+    token.securityLevel = SecurityLevel::TRUSTED_ENVIRONMENT;
+    token.mac.resize(32);
+    for (size_t n = 0; n < 32; n++) {
+        token.mac[n] = n;
+    }
+    optional<vector<uint8_t>> serialized = serializeVerificationToken(token);
+    ASSERT_TRUE(serialized.has_value());
+    optional<VerificationToken> deserialized = deserializeVerificationToken(serialized.value());
+    ASSERT_TRUE(deserialized.has_value());
+    ASSERT_EQ(token.challenge, deserialized.value().challenge);
+    ASSERT_EQ(token.timestamp, deserialized.value().timestamp);
+    ASSERT_EQ(token.securityLevel, deserialized.value().securityLevel);
+    ASSERT_EQ(0u, deserialized.value().parametersVerified.size());
+    ASSERT_EQ(token.mac, deserialized.value().mac);
+}
+
+TEST(VerificationTokenSeralizationTest, SerializationTestNoMac) {
+    VerificationToken token;
+    token.challenge = 12345;
+    token.timestamp = 67890;
+    token.securityLevel = SecurityLevel::TRUSTED_ENVIRONMENT;
+    token.mac.resize(0);
+    optional<vector<uint8_t>> serialized = serializeVerificationToken(token);
+    ASSERT_TRUE(serialized.has_value());
+    optional<VerificationToken> deserialized = deserializeVerificationToken(serialized.value());
+    ASSERT_TRUE(deserialized.has_value());
+    ASSERT_EQ(token.challenge, deserialized.value().challenge);
+    ASSERT_EQ(token.timestamp, deserialized.value().timestamp);
+    ASSERT_EQ(token.securityLevel, deserialized.value().securityLevel);
+    ASSERT_EQ(0u, deserialized.value().parametersVerified.size());
+    ASSERT_EQ(token.mac, deserialized.value().mac);
+}
+
+}  // namespace test
+}  // namespace keystore