From ef4f067c03543d8c8f2f8218bc69af12692ba000 Mon Sep 17 00:00:00 2001 From: Max Bires Date: Wed, 29 Nov 2017 14:38:48 -0800 Subject: [PATCH] Fixing security vuln by tightening race condition window. A proper fix for this feature requires reworking binder permission checking to take the selinux context and not the pid. This is feature work that should be done for P to properly fix these race conditions that occur elsewhere in the code. Bug: 68217699 Test: KeyStore keygen permissions cannot be bypassed through PID cycling Change-Id: I1ba5210010d6c413c9b1dbde3df0cc566400bfac --- keystore/key_store_service.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/keystore/key_store_service.cpp b/keystore/key_store_service.cpp index c2879c8f..ac10921e 100644 --- a/keystore/key_store_service.cpp +++ b/keystore/key_store_service.cpp @@ -797,6 +797,8 @@ KeyStoreService::generateKey(const String16& name, const KeymasterArguments& par const ::std::vector& entropy, int uid, int flags, android::security::keymaster::KeyCharacteristics* outCharacteristics, int32_t* aidl_return) { + // TODO(jbires): remove this getCallingUid call upon implementation of b/25646100 + uid_t originalUid = IPCThreadState::self()->getCallingUid(); uid = getEffectiveUid(uid); KeyStoreServiceReturnCode rc = checkBinderPermissionAndKeystoreState(P_INSERT, uid, flags & KEYSTORE_FLAG_ENCRYPTED); @@ -811,7 +813,9 @@ KeyStoreService::generateKey(const String16& name, const KeymasterArguments& par } if (containsTag(params.getParameters(), Tag::INCLUDE_UNIQUE_ID)) { - if (!checkBinderPermission(P_GEN_UNIQUE_ID)) { + //TODO(jbires): remove uid checking upon implementation of b/25646100 + if (!checkBinderPermission(P_GEN_UNIQUE_ID) && + originalUid != IPCThreadState::self()->getCallingUid()) { *aidl_return = static_cast(ResponseCode::PERMISSION_DENIED); return Status::ok(); }