diff options
author | Janis Danisevskis <jdanis@google.com> | 2019-03-25 15:37:43 -0700 |
---|---|---|
committer | android-build-merger <android-build-merger@google.com> | 2019-03-25 15:37:43 -0700 |
commit | d7c3ea15ee970cfacf9a5e4c4161fb79c1688e61 (patch) | |
tree | 3a5ecde928dd8354d90b26f4939e9681eef38be4 | |
parent | 58f3820418b52711bf4938b933b30c1e2d441236 (diff) | |
parent | 9a9794408c24301bd17a238e96aac88174d7a221 (diff) | |
download | security-d7c3ea15ee970cfacf9a5e4c4161fb79c1688e61.tar.gz |
Merge "Fix keystore wifi concurrency issue." am: dece12434d am: fe8fb1fc2c
am: 9a9794408c
Change-Id: Ic9c42a6876f056f70d25751c9feb2e650f753ac9
-rw-r--r-- | keystore/key_store_service.cpp | 38 | ||||
-rw-r--r-- | keystore/key_store_service.h | 13 |
2 files changed, 51 insertions, 0 deletions
diff --git a/keystore/key_store_service.cpp b/keystore/key_store_service.cpp index 0bd3e03a..5e7efab0 100644 --- a/keystore/key_store_service.cpp +++ b/keystore/key_store_service.cpp @@ -82,6 +82,7 @@ bool containsTag(const hidl_vec<KeyParameter>& params, Tag tag) { } #define AIDL_RETURN(rc) (*_aidl_return = KeyStoreServiceReturnCode(rc).getErrorCode(), Status::ok()) +#define KEYSTORE_SERVICE_LOCK std::lock_guard<std::mutex> keystore_lock(keystoreServiceMutex_) std::pair<KeyStoreServiceReturnCode, bool> hadFactoryResetSinceIdRotation() { struct stat sbuf; @@ -145,6 +146,7 @@ KeyStoreServiceReturnCode updateParamsForAttestation(uid_t callingUid, Authoriza } // anonymous namespace Status KeyStoreService::getState(int32_t userId, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkBinderPermission(P_GET_STATE)) { *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED); return Status::ok(); @@ -154,6 +156,7 @@ Status KeyStoreService::getState(int32_t userId, int32_t* aidl_return) { } Status KeyStoreService::get(const String16& name, int32_t uid, ::std::vector<uint8_t>* item) { + KEYSTORE_SERVICE_LOCK; uid_t targetUid = getEffectiveUid(uid); if (!checkBinderPermission(P_GET, targetUid)) { // see keystore/keystore.h @@ -183,6 +186,7 @@ Status KeyStoreService::get(const String16& name, int32_t uid, ::std::vector<uin Status KeyStoreService::insert(const String16& name, const ::std::vector<uint8_t>& item, int targetUid, int32_t flags, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; targetUid = getEffectiveUid(targetUid); KeyStoreServiceReturnCode result = checkBinderPermissionAndKeystoreState(P_INSERT, targetUid, flags & KEYSTORE_FLAG_ENCRYPTED); @@ -208,6 +212,7 @@ Status KeyStoreService::insert(const String16& name, const ::std::vector<uint8_t } Status KeyStoreService::del(const String16& name, int targetUid, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; targetUid = getEffectiveUid(targetUid); if (!checkBinderPermission(P_DELETE, targetUid)) { *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED); @@ -228,6 +233,7 @@ Status KeyStoreService::del(const String16& name, int targetUid, int32_t* aidl_r } Status KeyStoreService::exist(const String16& name, int targetUid, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; targetUid = getEffectiveUid(targetUid); if (!checkBinderPermission(P_EXIST, targetUid)) { *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED); @@ -243,6 +249,7 @@ Status KeyStoreService::exist(const String16& name, int targetUid, int32_t* aidl Status KeyStoreService::list(const String16& prefix, int32_t targetUid, ::std::vector<::android::String16>* matches) { + KEYSTORE_SERVICE_LOCK; targetUid = getEffectiveUid(targetUid); if (!checkBinderPermission(P_LIST, targetUid)) { return Status::fromServiceSpecificError( @@ -281,6 +288,7 @@ Status KeyStoreService::list(const String16& prefix, int32_t targetUid, */ Status KeyStoreService::listUidsOfAuthBoundKeys(std::vector<std::string>* uidsOut, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; const int32_t callingUid = IPCThreadState::self()->getCallingUid(); const int32_t userId = get_user_id(callingUid); const int32_t appId = get_app_id(callingUid); @@ -344,6 +352,7 @@ Status KeyStoreService::listUidsOfAuthBoundKeys(std::vector<std::string>* uidsOu } Status KeyStoreService::reset(int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkBinderPermission(P_RESET)) { *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED); return Status::ok(); @@ -357,6 +366,7 @@ Status KeyStoreService::reset(int32_t* aidl_return) { Status KeyStoreService::onUserPasswordChanged(int32_t userId, const String16& password, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkBinderPermission(P_PASSWORD)) { *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED); return Status::ok(); @@ -394,6 +404,7 @@ Status KeyStoreService::onUserPasswordChanged(int32_t userId, const String16& pa } Status KeyStoreService::onUserAdded(int32_t userId, int32_t parentId, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkBinderPermission(P_USER_CHANGED)) { *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED); return Status::ok(); @@ -419,6 +430,7 @@ Status KeyStoreService::onUserAdded(int32_t userId, int32_t parentId, int32_t* a } Status KeyStoreService::onUserRemoved(int32_t userId, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkBinderPermission(P_USER_CHANGED)) { *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED); return Status::ok(); @@ -430,6 +442,7 @@ Status KeyStoreService::onUserRemoved(int32_t userId, int32_t* aidl_return) { } Status KeyStoreService::lock(int32_t userId, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkBinderPermission(P_LOCK)) { *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED); return Status::ok(); @@ -449,6 +462,7 @@ Status KeyStoreService::lock(int32_t userId, int32_t* aidl_return) { } Status KeyStoreService::unlock(int32_t userId, const String16& pw, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkBinderPermission(P_UNLOCK)) { *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED); return Status::ok(); @@ -479,6 +493,7 @@ Status KeyStoreService::unlock(int32_t userId, const String16& pw, int32_t* aidl } Status KeyStoreService::isEmpty(int32_t userId, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkBinderPermission(P_IS_EMPTY)) { *aidl_return = static_cast<int32_t>(false); return Status::ok(); @@ -490,6 +505,7 @@ Status KeyStoreService::isEmpty(int32_t userId, int32_t* aidl_return) { Status KeyStoreService::grant(const String16& name, int32_t granteeUid, ::android::String16* aidl_return) { + KEYSTORE_SERVICE_LOCK; uid_t callingUid = IPCThreadState::self()->getCallingUid(); auto result = checkBinderPermissionAndKeystoreState(P_GRANT, /*targetUid=*/-1, /*checkUnlocked=*/false); @@ -510,6 +526,7 @@ Status KeyStoreService::grant(const String16& name, int32_t granteeUid, } Status KeyStoreService::ungrant(const String16& name, int32_t granteeUid, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; uid_t callingUid = IPCThreadState::self()->getCallingUid(); KeyStoreServiceReturnCode result = checkBinderPermissionAndKeystoreState(P_GRANT, /*targetUid=*/-1, /*checkUnlocked=*/false); @@ -530,6 +547,7 @@ Status KeyStoreService::ungrant(const String16& name, int32_t granteeUid, int32_ } Status KeyStoreService::getmtime(const String16& name, int32_t uid, int64_t* time) { + KEYSTORE_SERVICE_LOCK; uid_t targetUid = getEffectiveUid(uid); if (!checkBinderPermission(P_GET, targetUid)) { ALOGW("permission denied for %d: getmtime", targetUid); @@ -568,11 +586,13 @@ Status KeyStoreService::getmtime(const String16& name, int32_t uid, int64_t* tim } Status KeyStoreService::is_hardware_backed(const String16& keyType, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; *aidl_return = static_cast<int32_t>(mKeyStore->isHardwareBacked(keyType) ? 1 : 0); return Status::ok(); } Status KeyStoreService::clear_uid(int64_t targetUid64, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; uid_t targetUid = getEffectiveUid(targetUid64); if (!checkBinderPermissionSelfOrSystem(P_CLEAR_UID, targetUid)) { return AIDL_RETURN(ResponseCode::PERMISSION_DENIED); @@ -612,6 +632,7 @@ Status KeyStoreService::clear_uid(int64_t targetUid64, int32_t* _aidl_return) { Status KeyStoreService::addRngEntropy( const ::android::sp<::android::security::keystore::IKeystoreResponseCallback>& cb, const ::std::vector<uint8_t>& entropy, int32_t flags, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; auto device = mKeyStore->getDevice(flagsToSecurityLevel(flags)); if (!device) { return AIDL_RETURN(ErrorCode::HARDWARE_TYPE_UNAVAILABLE); @@ -628,6 +649,7 @@ Status KeyStoreService::generateKey( const ::android::sp<::android::security::keystore::IKeystoreKeyCharacteristicsCallback>& cb, const String16& name, const KeymasterArguments& params, const ::std::vector<uint8_t>& entropy, int uid, int flags, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; // TODO(jbires): remove this getCallingUid call upon implementation of b/25646100 uid_t originalUid = IPCThreadState::self()->getCallingUid(); uid = getEffectiveUid(uid); @@ -689,6 +711,7 @@ Status KeyStoreService::getKeyCharacteristics( const String16& name, const ::android::security::keymaster::KeymasterBlob& clientId, const ::android::security::keymaster::KeymasterBlob& appData, int32_t uid, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; uid_t targetUid = getEffectiveUid(uid); uid_t callingUid = IPCThreadState::self()->getCallingUid(); @@ -735,6 +758,7 @@ Status KeyStoreService::importKey( const ::android::sp<::android::security::keystore::IKeystoreKeyCharacteristicsCallback>& cb, const String16& name, const KeymasterArguments& params, int32_t format, const ::std::vector<uint8_t>& keyData, int uid, int flags, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; uid = getEffectiveUid(uid); auto logOnScopeExit = android::base::make_scope_guard([&] { if (__android_log_security()) { @@ -791,6 +815,7 @@ Status KeyStoreService::exportKey( const ::android::security::keymaster::KeymasterBlob& clientId, const ::android::security::keymaster::KeymasterBlob& appData, int32_t uid, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; uid_t targetUid = getEffectiveUid(uid); uid_t callingUid = IPCThreadState::self()->getCallingUid(); @@ -826,6 +851,7 @@ Status KeyStoreService::begin(const sp<IKeystoreOperationResultCallback>& cb, bool pruneable, const KeymasterArguments& params, const ::std::vector<uint8_t>& entropy, int32_t uid, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; uid_t callingUid = IPCThreadState::self()->getCallingUid(); uid_t targetUid = getEffectiveUid(uid); if (!is_granted_to(callingUid, targetUid)) { @@ -874,6 +900,7 @@ Status KeyStoreService::update(const ::android::sp<IKeystoreOperationResultCallb const ::android::sp<::android::IBinder>& token, const ::android::security::keymaster::KeymasterArguments& params, const ::std::vector<uint8_t>& input, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkAllowedOperationParams(params.getParameters())) { return AIDL_RETURN(ErrorCode::INVALID_ARGUMENT); } @@ -898,6 +925,7 @@ Status KeyStoreService::finish(const ::android::sp<IKeystoreOperationResultCallb const ::android::security::keymaster::KeymasterArguments& params, const ::std::vector<uint8_t>& signature, const ::std::vector<uint8_t>& entropy, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; if (!checkAllowedOperationParams(params.getParameters())) { return AIDL_RETURN(ErrorCode::INVALID_ARGUMENT); } @@ -921,6 +949,7 @@ Status KeyStoreService::finish(const ::android::sp<IKeystoreOperationResultCallb Status KeyStoreService::abort(const ::android::sp<IKeystoreResponseCallback>& cb, const ::android::sp<::android::IBinder>& token, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; auto dev = getOperationDevice(token); if (!dev) { return AIDL_RETURN(ErrorCode::INVALID_OPERATION_HANDLE); @@ -933,6 +962,7 @@ Status KeyStoreService::abort(const ::android::sp<IKeystoreResponseCallback>& cb Status KeyStoreService::addAuthToken(const ::std::vector<uint8_t>& authTokenAsVector, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; // TODO(swillden): When gatekeeper and fingerprint are ready, this should be updated to // receive a HardwareAuthToken, rather than an opaque byte array. @@ -983,6 +1013,7 @@ bool isDeviceIdAttestationRequested(const KeymasterArguments& params) { Status KeyStoreService::attestKey( const ::android::sp<::android::security::keystore::IKeystoreCertificateChainCallback>& cb, const String16& name, const KeymasterArguments& params, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; // check null output if method signature is updated and return ErrorCode::OUTPUT_PARAMETER_NULL if (!checkAllowedOperationParams(params.getParameters())) { return AIDL_RETURN(ErrorCode::INVALID_ARGUMENT); @@ -1038,6 +1069,7 @@ Status KeyStoreService::attestKey( Status KeyStoreService::attestDeviceIds( const ::android::sp<::android::security::keystore::IKeystoreCertificateChainCallback>& cb, const KeymasterArguments& params, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; // check null output if method signature is updated and return ErrorCode::OUTPUT_PARAMETER_NULL if (!checkAllowedOperationParams(params.getParameters())) { @@ -1130,6 +1162,7 @@ Status KeyStoreService::attestDeviceIds( } Status KeyStoreService::onDeviceOffBody(int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; // TODO(tuckeris): add permission check. This should be callable from ClockworkHome only. mKeyStore->getAuthTokenTable().onDeviceOffBody(); *aidl_return = static_cast<int32_t>(ResponseCode::NO_ERROR); @@ -1142,6 +1175,7 @@ Status KeyStoreService::importWrappedKey( const ::android::String16& wrappingKeyAlias, const ::std::vector<uint8_t>& maskingKey, const KeymasterArguments& params, int64_t rootSid, int64_t fingerprintSid, int32_t* _aidl_return) { + KEYSTORE_SERVICE_LOCK; uid_t callingUid = IPCThreadState::self()->getCallingUid(); @@ -1191,16 +1225,19 @@ Status KeyStoreService::presentConfirmationPrompt(const sp<IBinder>& listener, const ::std::vector<uint8_t>& extraData, const String16& locale, int32_t uiOptionsAsFlags, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; return mKeyStore->getConfirmationManager().presentConfirmationPrompt( listener, promptText, extraData, locale, uiOptionsAsFlags, aidl_return); } Status KeyStoreService::cancelConfirmationPrompt(const sp<IBinder>& listener, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; return mKeyStore->getConfirmationManager().cancelConfirmationPrompt(listener, aidl_return); } Status KeyStoreService::isConfirmationPromptSupported(bool* aidl_return) { + KEYSTORE_SERVICE_LOCK; return mKeyStore->getConfirmationManager().isConfirmationPromptSupported(aidl_return); } @@ -1318,6 +1355,7 @@ bool KeyStoreService::checkAllowedOperationParams(const hidl_vec<KeyParameter>& Status KeyStoreService::onKeyguardVisibilityChanged(bool isShowing, int32_t userId, int32_t* aidl_return) { + KEYSTORE_SERVICE_LOCK; mKeyStore->getEnforcementPolicy().set_device_locked(isShowing, userId); *aidl_return = static_cast<int32_t>(ResponseCode::NO_ERROR); diff --git a/keystore/key_store_service.h b/keystore/key_store_service.h index 2171213c..2fdc3dd2 100644 --- a/keystore/key_store_service.h +++ b/keystore/key_store_service.h @@ -35,6 +35,8 @@ #include <keystore/OperationResult.h> #include <keystore/keystore_return_types.h> +#include <mutex> + namespace keystore { // Class provides implementation for generated BnKeystoreService.h based on @@ -230,6 +232,17 @@ class KeyStoreService : public android::security::keystore::BnKeystoreService { sp<KeyStore> mKeyStore; + /** + * This mutex locks keystore operations from concurrent execution. + * The keystore service has always been conceptually single threaded. Even with the introduction + * of keymaster workers, it was assumed that the dispatcher thread executes exclusively on + * certain code paths. With the introduction of wifi Keystore service in the keystore process + * this assumption no longer holds as the hwbinder thread servicing this interface makes + * functions (rather than IPC) calls into keystore. This mutex protects the keystore logic + * from concurrent execution. + */ + std::mutex keystoreServiceMutex_; + std::mutex operationDeviceMapMutex_; std::map<sp<IBinder>, std::shared_ptr<KeymasterWorker>> operationDeviceMap_; |