[DO NOT MERGE] Fix keychain key upgrade issue

Fix issues in keystore where it didn't handle key upgrade for granted keys
or keys with effective uid (keys under WiFi uid)  correctly.

Test: manual
Bug: 66094261
Change-Id: I6709b7562d47ad6156bee88a9e2d961f8a4a797d
Merged-In: I6709b7562d47ad6156bee88a9e2d961f8a4a797d

keystore/key_store_service.cpp

@@ -191,16 +191,21 @@ KeyStoreServiceReturnCode KeyStoreService::del(const String16& name, int targetU
     }
     String8 name8(name);
     ALOGI("del %s %d", name8.string(), targetUid);
-    String8 filename(mKeyStore->getKeyNameForUidWithDir(name8, targetUid, ::TYPE_ANY));
+    auto filename = mKeyStore->getBlobFileNameIfExists(name8, targetUid, ::TYPE_ANY);
-    ResponseCode result = mKeyStore->del(filename.string(), ::TYPE_ANY, get_user_id(targetUid));
+    if (!filename.isOk()) return ResponseCode::KEY_NOT_FOUND;
+
+    ResponseCode result = mKeyStore->del(filename.value().string(), ::TYPE_ANY,
+            get_user_id(targetUid));
     if (result != ResponseCode::NO_ERROR) {
         return result;
     }
 
-    // Also delete any characteristics files
+    filename = mKeyStore->getBlobFileNameIfExists(name8, targetUid, ::TYPE_KEY_CHARACTERISTICS);
-    String8 chrFilename(
+    if (filename.isOk()) {
-        mKeyStore->getKeyNameForUidWithDir(name8, targetUid, ::TYPE_KEY_CHARACTERISTICS));
+        return mKeyStore->del(filename.value().string(), ::TYPE_KEY_CHARACTERISTICS,
-    return mKeyStore->del(chrFilename.string(), ::TYPE_KEY_CHARACTERISTICS, get_user_id(targetUid));
+                get_user_id(targetUid));
+    }
+    return ResponseCode::NO_ERROR;
 }
 
 KeyStoreServiceReturnCode KeyStoreService::exist(const String16& name, int targetUid) {
@@ -209,13 +214,8 @@ KeyStoreServiceReturnCode KeyStoreService::exist(const String16& name, int targe
         return ResponseCode::PERMISSION_DENIED;
     }
 
-    String8 name8(name);
+    auto filename = mKeyStore->getBlobFileNameIfExists(String8(name), targetUid, ::TYPE_ANY);
-    String8 filename(mKeyStore->getKeyNameForUidWithDir(name8, targetUid, ::TYPE_ANY));
+    return filename.isOk() ? ResponseCode::NO_ERROR : ResponseCode::KEY_NOT_FOUND;
-
-    if (access(filename.string(), R_OK) == -1) {
-        return (errno != ENOENT) ? ResponseCode::SYSTEM_ERROR : ResponseCode::KEY_NOT_FOUND;
-    }
-    return ResponseCode::NO_ERROR;
 }
 
 KeyStoreServiceReturnCode KeyStoreService::list(const String16& prefix, int targetUid,
@@ -555,17 +555,16 @@ int64_t KeyStoreService::getmtime(const String16& name, int32_t uid) {
         return -1L;
     }
 
-    String8 name8(name);
+    auto filename = mKeyStore->getBlobFileNameIfExists(String8(name), targetUid, ::TYPE_ANY);
-    String8 filename(mKeyStore->getKeyNameForUidWithDir(name8, targetUid, ::TYPE_ANY));
 
-    if (access(filename.string(), R_OK) == -1) {
+    if (!filename.isOk()) {
-        ALOGW("could not access %s for getmtime", filename.string());
+        ALOGW("could not access %s for getmtime", filename.value().string());
         return -1L;
     }
 
-    int fd = TEMP_FAILURE_RETRY(open(filename.string(), O_NOFOLLOW, O_RDONLY));
+    int fd = TEMP_FAILURE_RETRY(open(filename.value().string(), O_NOFOLLOW, O_RDONLY));
     if (fd < 0) {
-        ALOGW("could not open %s for getmtime", filename.string());
+        ALOGW("could not open %s for getmtime", filename.value().string());
         return -1L;
     }
 
@@ -573,7 +572,7 @@ int64_t KeyStoreService::getmtime(const String16& name, int32_t uid) {
     int ret = fstat(fd, &s);
     close(fd);
     if (ret == -1) {
-        ALOGW("could not stat %s for getmtime", filename.string());
+        ALOGW("could not stat %s for getmtime", filename.value().string());
         return -1L;
     }
 
@@ -1868,8 +1867,12 @@ KeyStoreServiceReturnCode KeyStoreService::upgradeKeyBlob(const String16& name,
             return;
         }
 
-        String8 filename(mKeyStore->getKeyNameForUidWithDir(name8, uid, ::TYPE_KEYMASTER_10));
+        auto filename = mKeyStore->getBlobFileNameIfExists(name8, uid, ::TYPE_KEYMASTER_10);
-        error = mKeyStore->del(filename.string(), ::TYPE_ANY, get_user_id(uid));
+        if (!filename.isOk()) {
+            ALOGI("trying to upgrade a non existing blob");
+            return;
+        }
+        error = mKeyStore->del(filename.value().string(), ::TYPE_ANY, get_user_id(uid));
         if (!error.isOk()) {
             ALOGI("upgradeKeyBlob keystore->del failed %d", (int)error);
             return;
@@ -1882,7 +1885,7 @@ KeyStoreServiceReturnCode KeyStoreService::upgradeKeyBlob(const String16& name,
         newBlob.setSuperEncrypted(blob->isSuperEncrypted());
         newBlob.setCriticalToDeviceEncryption(blob->isCriticalToDeviceEncryption());
 
-        error = mKeyStore->put(filename.string(), &newBlob, get_user_id(uid));
+        error = mKeyStore->put(filename.value().string(), &newBlob, get_user_id(uid));
         if (!error.isOk()) {
             ALOGI("upgradeKeyBlob keystore->put failed %d", (int)error);
             return;

keystore/keystore.cpp

@@ -159,6 +159,42 @@ android::String8 KeyStore::getKeyNameForUidWithDir(
     }
 }
 
+NullOr<android::String8> KeyStore::getBlobFileNameIfExists(const android::String8& alias, uid_t uid,
+                                                          const BlobType type) {
+    android::String8 filepath8(getKeyNameForUidWithDir(alias, uid, type));
+
+    if (!access(filepath8.string(), R_OK | W_OK)) return filepath8;
+
+    // If this is one of the legacy UID->UID mappings, use it.
+    uid_t euid = get_keystore_euid(uid);
+    if (euid != uid) {
+        filepath8 = getKeyNameForUidWithDir(alias, euid, type);
+        if (!access(filepath8.string(), R_OK | W_OK)) return filepath8;
+    }
+
+    // They might be using a granted key (<uid>_<granted_alias>), try parsing the alias.
+    char* end;
+    uid_t granter_uid = strtoul(alias, &end, 10);
+    // Does the alias look like a granted key?
+    if (end[0] != '_' || end[1] == 0 || (!granter_uid)) {
+        return {};
+    }
+    android::String8 granted_alias(end + 1);
+
+    // We might be asked to get a characteristics file, but still need to check grant
+    // on the key file itself.
+    android::String8 keyfilepath8 = getKeyNameForUidWithDir(granted_alias, granter_uid, ::TYPE_ANY);
+    if (!hasGrant(keyfilepath8.string(), uid)) {
+        return {};
+    }
+    // Get the granted blob file path of the desired type
+    filepath8 = getKeyNameForUidWithDir(granted_alias, granter_uid, type);
+    if (!access(filepath8.string(), R_OK | W_OK)) return filepath8;
+
+    return {};
+}
+
+
 void KeyStore::resetUser(uid_t userId, bool keepUnenryptedEntries) {
     android::String8 prefix("");
     android::Vector<android::String16> aliases;
@@ -517,39 +553,13 @@ bool KeyStore::isHardwareBacked(const android::String16& /*keyType*/) const {
 
 ResponseCode KeyStore::getKeyForName(Blob* keyBlob, const android::String8& keyName,
                                      const uid_t uid, const BlobType type) {
-    android::String8 filepath8(getKeyNameForUidWithDir(keyName, uid, type));
+    auto filepath8 = getBlobFileNameIfExists(keyName, uid, type);
     uid_t userId = get_user_id(uid);
 
-    ResponseCode responseCode = get(filepath8.string(), keyBlob, type, userId);
+    if (filepath8.isOk())
-    if (responseCode == ResponseCode::NO_ERROR) {
+        return get(filepath8.value().string(), keyBlob, type, userId);
-        return responseCode;
-    }
 
-    // If this is one of the legacy UID->UID mappings, use it.
+    return ResponseCode::KEY_NOT_FOUND;
-    uid_t euid = get_keystore_euid(uid);
-    if (euid != uid) {
-        filepath8 = getKeyNameForUidWithDir(keyName, euid, type);
-        responseCode = get(filepath8.string(), keyBlob, type, userId);
-        if (responseCode == ResponseCode::NO_ERROR) {
-            return responseCode;
-        }
-    }
-
-    // They might be using a granted key.
-    android::String8 filename8 = getKeyName(keyName, type);
-    char* end;
-    strtoul(filename8.string(), &end, 10);
-    if (end[0] != '_' || end[1] == 0) {
-        return ResponseCode::KEY_NOT_FOUND;
-    }
-    filepath8 = android::String8::format("%s/%s", getUserState(userId)->getUserDirName(),
-                                         filename8.string());
-    if (!hasGrant(filepath8.string(), uid)) {
-        return responseCode;
-    }
-
-    // It is a granted key. Try to load it.
-    return get(filepath8.string(), keyBlob, type, userId);
 }
 
 UserState* KeyStore::getUserState(uid_t userId) {
@@ -607,7 +617,7 @@ const grant_t* KeyStore::getGrant(const char* filename, uid_t uid) const {
 }
 
 bool KeyStore::upgradeBlob(const char* filename, Blob* blob, const uint8_t oldVersion,
-                           const BlobType type, uid_t uid) {
+                           const BlobType type, uid_t userId) {
     bool updated = false;
     uint8_t version = oldVersion;
 
@@ -617,7 +627,7 @@ bool KeyStore::upgradeBlob(const char* filename, Blob* blob, const uint8_t oldVe
 
         blob->setType(type);
         if (type == TYPE_KEY_PAIR) {
-            importBlobAsKey(blob, filename, uid);
+            importBlobAsKey(blob, filename, userId);
         }
         version = 1;
         updated = true;
@@ -649,7 +659,7 @@ struct BIO_Delete {
 };
 typedef UniquePtr<BIO, BIO_Delete> Unique_BIO;
 
-ResponseCode KeyStore::importBlobAsKey(Blob* blob, const char* filename, uid_t uid) {
+ResponseCode KeyStore::importBlobAsKey(Blob* blob, const char* filename, uid_t userId) {
     // We won't even write to the blob directly with this BIO, so const_cast is okay.
     Unique_BIO b(BIO_new_mem_buf(const_cast<uint8_t*>(blob->getValue()), blob->getLength()));
     if (b.get() == NULL) {
@@ -677,13 +687,13 @@ ResponseCode KeyStore::importBlobAsKey(Blob* blob, const char* filename, uid_t u
         return ResponseCode::SYSTEM_ERROR;
     }
 
-    ResponseCode rc = importKey(pkcs8key.get(), len, filename, get_user_id(uid),
+    ResponseCode rc = importKey(pkcs8key.get(), len, filename, userId,
                                 blob->isEncrypted() ? KEYSTORE_FLAG_ENCRYPTED : KEYSTORE_FLAG_NONE);
     if (rc != ResponseCode::NO_ERROR) {
         return rc;
     }
 
-    return get(filename, blob, TYPE_KEY_PAIR, uid);
+    return get(filename, blob, TYPE_KEY_PAIR, userId);
 }
 
 void KeyStore::readMetaData() {

keystore/keystore.h

@@ -74,6 +74,8 @@ class KeyStore {
                                       const BlobType type);
     android::String8 getKeyNameForUidWithDir(const android::String8& keyName, uid_t uid,
                                              const BlobType type);
+    NullOr<android::String8> getBlobFileNameIfExists(const android::String8& alias, uid_t uid,
+                                                    const BlobType type);
 
     /*
      * Delete entries owned by userId. If keepUnencryptedEntries is true