diff options
author | Rubin Xu <rubinxu@google.com> | 2017-10-19 15:55:17 +0100 |
---|---|---|
committer | Nikoli Cartagena <dargeren@google.com> | 2017-11-13 17:12:22 -0800 |
commit | 438ea20a7236acfffc573a756988e23dddcf3712 (patch) | |
tree | b0d5ff7230d038fea5118b7ac5be85ad5987cf04 | |
parent | 0ab28b78bd06a06a0ffa150cef5876d56212902a (diff) | |
download | security-nougat-mr2-security-release.tar.gz |
[DO NOT MERGE] Fix keychain key upgrade issueandroid-7.1.2_r39android-7.1.2_r38android-7.1.2_r37nougat-mr2-security-release
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
(cherry picked from commit 58291e0ff3302d9502a13c0ff16bbcaa7c15f8bd)
-rw-r--r-- | keystore/key_store_service.cpp | 44 | ||||
-rw-r--r-- | keystore/keystore.cpp | 79 | ||||
-rw-r--r-- | keystore/keystore.h | 2 |
3 files changed, 71 insertions, 54 deletions
diff --git a/keystore/key_store_service.cpp b/keystore/key_store_service.cpp index 020bfe4c..8fef42f1 100644 --- a/keystore/key_store_service.cpp +++ b/keystore/key_store_service.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#define LOG_TAG "keystore" + #include "key_store_service.h" #include <fcntl.h> @@ -111,16 +113,21 @@ int32_t KeyStoreService::del(const String16& name, int targetUid) { return ::PERMISSION_DENIED; } String8 name8(name); - String8 filename(mKeyStore->getKeyNameForUidWithDir(name8, targetUid, ::TYPE_ANY)); - int32_t result = mKeyStore->del(filename.string(), ::TYPE_ANY, get_user_id(targetUid)); - if (result != ::NO_ERROR) { + String8 filename = mKeyStore->getBlobFileNameIfExists(name8, targetUid, ::TYPE_ANY); + if (filename.isEmpty()) return ResponseCode::KEY_NOT_FOUND; + + ResponseCode result = mKeyStore->del(filename.string(), ::TYPE_ANY, + get_user_id(targetUid)); + if (result != ResponseCode::NO_ERROR) { return result; } - // Also delete any characteristics files - String8 chrFilename(mKeyStore->getKeyNameForUidWithDir( - name8, targetUid, ::TYPE_KEY_CHARACTERISTICS)); - return mKeyStore->del(chrFilename.string(), ::TYPE_KEY_CHARACTERISTICS, get_user_id(targetUid)); + filename = mKeyStore->getBlobFileNameIfExists(name8, targetUid, ::TYPE_KEY_CHARACTERISTICS); + if (!filename.isEmpty()) { + return mKeyStore->del(filename.string(), ::TYPE_KEY_CHARACTERISTICS, + get_user_id(targetUid)); + } + return ResponseCode::NO_ERROR; } int32_t KeyStoreService::exist(const String16& name, int targetUid) { @@ -129,13 +136,8 @@ int32_t KeyStoreService::exist(const String16& name, int targetUid) { return ::PERMISSION_DENIED; } - String8 name8(name); - String8 filename(mKeyStore->getKeyNameForUidWithDir(name8, targetUid, ::TYPE_ANY)); - - if (access(filename.string(), R_OK) == -1) { - return (errno != ENOENT) ? ::SYSTEM_ERROR : ::KEY_NOT_FOUND; - } - return ::NO_ERROR; + String8 filename = mKeyStore->getBlobFileNameIfExists(String8(name), targetUid, ::TYPE_ANY); + return (!filename.isEmpty()) ? ResponseCode::NO_ERROR : ResponseCode::KEY_NOT_FOUND; } int32_t KeyStoreService::list(const String16& prefix, int targetUid, Vector<String16>* matches) { @@ -467,10 +469,9 @@ int64_t KeyStoreService::getmtime(const String16& name, int32_t uid) { return -1L; } - String8 name8(name); - String8 filename(mKeyStore->getKeyNameForUidWithDir(name8, targetUid, ::TYPE_ANY)); + String8 filename = mKeyStore->getBlobFileNameIfExists(String8(name), targetUid, ::TYPE_ANY); - if (access(filename.string(), R_OK) == -1) { + if (filename.isEmpty()) { ALOGW("could not access %s for getmtime", filename.string()); return -1L; } @@ -1647,12 +1648,17 @@ int32_t KeyStoreService::upgradeKeyBlob(const String16& name, uid_t uid, UniquePtr<uint8_t, Malloc_Delete> upgraded_key_deleter( const_cast<uint8_t*>(upgraded_key.key_material)); - rc = del(name, uid); + String8 filename = mKeyStore->getBlobFileNameIfExists(name8, uid, ::TYPE_KEYMASTER_10); + if (filename.isEmpty()) { + ALOGI("trying to upgrade a non existing blob"); + return KEY_NOT_FOUND; + } + rc = mKeyStore->del(filename.string(), ::TYPE_ANY, get_user_id(uid)); if (rc != ::NO_ERROR) { + ALOGI("upgradeKeyBlob keystore->del failed %d", rc); return rc; } - String8 filename(mKeyStore->getKeyNameForUidWithDir(name8, uid, ::TYPE_KEYMASTER_10)); Blob newBlob(upgraded_key.key_material, upgraded_key.key_material_size, nullptr /* info */, 0 /* infoLength */, ::TYPE_KEYMASTER_10); newBlob.setFallback(blob->isFallback()); diff --git a/keystore/keystore.cpp b/keystore/keystore.cpp index d0a61bf6..a7a81e87 100644 --- a/keystore/keystore.cpp +++ b/keystore/keystore.cpp @@ -150,6 +150,41 @@ android::String8 KeyStore::getKeyNameForUidWithDir( } } +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(); + } + 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 android::String8(); + } + // 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 android::String8(); +} + void KeyStore::resetUser(uid_t userId, bool keepUnenryptedEntries) { android::String8 prefix(""); android::Vector<android::String16> aliases; @@ -508,39 +543,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 (responseCode == NO_ERROR) { - return responseCode; - } - - // If this is one of the legacy UID->UID mappings, use it. - uid_t euid = get_keystore_euid(uid); - if (euid != uid) { - filepath8 = getKeyNameForUidWithDir(keyName, euid, type); - responseCode = get(filepath8.string(), keyBlob, type, userId); - if (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 KEY_NOT_FOUND; - } - filepath8 = android::String8::format("%s/%s", getUserState(userId)->getUserDirName(), - filename8.string()); - if (!hasGrant(filepath8.string(), uid)) { - return responseCode; - } + if (!filepath8.isEmpty()) + return get(filepath8.string(), keyBlob, type, userId); - // It is a granted key. Try to load it. - return get(filepath8.string(), keyBlob, type, userId); + return ResponseCode::KEY_NOT_FOUND; } UserState* KeyStore::getUserState(uid_t userId) { @@ -598,7 +607,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; @@ -608,7 +617,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; @@ -640,7 +649,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) { @@ -668,13 +677,13 @@ ResponseCode KeyStore::importBlobAsKey(Blob* blob, const char* filename, uid_t u return 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 != NO_ERROR) { return rc; } - return get(filename, blob, TYPE_KEY_PAIR, uid); + return get(filename, blob, TYPE_KEY_PAIR, userId); } void KeyStore::readMetaData() { diff --git a/keystore/keystore.h b/keystore/keystore.h index 278a0a0d..fee4527f 100644 --- a/keystore/keystore.h +++ b/keystore/keystore.h @@ -58,6 +58,8 @@ class KeyStore { const BlobType type); android::String8 getKeyNameForUidWithDir(const android::String8& keyName, uid_t uid, const BlobType type); + android::String8 getBlobFileNameIfExists(const android::String8& alias, uid_t uid, + const BlobType type); /* * Delete entries owned by userId. If keepUnencryptedEntries is true |