Commit 3ed935aa authored by Avi Drissman's avatar Avi Drissman Committed by Commit Bot

Fix unsafe and leaky uses of CFDictionaries

1. Creating a CFDictionary with null callbacks is almost certainly
not something you want to do. It is not bridgeable with NSDictionary,
and the normal semantics of retain/release do not apply. It is
*unsafe* to put autoreleased items into such a dictionary.
There is no guarantee that the items won't be deallocated out from
underneath the dictionary.

2. CFDictionarySetValue does *not* take ownership. No matter if
the dictionary has retain/release semantics or not, assuming
ownership takeover will leak.

Bug: none
Change-Id: I6e3cae093e61ab2a42af7b0052231d00fa4a71b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2391208Reviewed-by: default avatarLeonard Grey <lgrey@chromium.org>
Reviewed-by: default avatarMartin Kreichgauer <martinkr@google.com>
Commit-Queue: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804583}
parent 69317269
......@@ -54,12 +54,13 @@ const std::vector<uint8_t> kUserId = {10, 11, 12, 13, 14, 15};
// credentials in the non-legacy keychain that are tagged with the keychain
// access group used in this test.
base::ScopedCFTypeRef<CFMutableDictionaryRef> BaseQuery() {
base::ScopedCFTypeRef<CFMutableDictionaryRef> query(
CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
base::ScopedCFTypeRef<CFMutableDictionaryRef> query(CFDictionaryCreateMutable(
kCFAllocatorDefault, 0, &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
CFDictionarySetValue(query, kSecClass, kSecClassKey);
base::ScopedCFTypeRef<CFStringRef> access_group_ref(
base::SysUTF8ToCFStringRef(kKeychainAccessGroup));
CFDictionarySetValue(query, kSecAttrAccessGroup, access_group_ref.release());
CFDictionarySetValue(query, kSecAttrAccessGroup, access_group_ref);
CFDictionarySetValue(query, kSecAttrNoLegacy, @YES);
CFDictionarySetValue(query, kSecReturnAttributes, @YES);
CFDictionarySetValue(query, kSecMatchLimit, kSecMatchLimitAll);
......
......@@ -32,8 +32,9 @@ namespace {
base::ScopedCFTypeRef<CFMutableDictionaryRef> DefaultKeychainQuery(
const AuthenticatorConfig& config,
const std::string& rp_id) {
base::ScopedCFTypeRef<CFMutableDictionaryRef> query(
CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
base::ScopedCFTypeRef<CFMutableDictionaryRef> query(CFDictionaryCreateMutable(
kCFAllocatorDefault, 0, &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
CFDictionarySetValue(query, kSecClass, kSecClassKey);
CFDictionarySetValue(query, kSecAttrAccessGroup,
base::SysUTF8ToNSString(config.keychain_access_group));
......@@ -79,8 +80,9 @@ QueryKeychainItemsForProfile(const std::string& keychain_access_group,
// keychain access group.
std::vector<base::ScopedCFTypeRef<CFDictionaryRef>> result;
base::ScopedCFTypeRef<CFMutableDictionaryRef> query(
CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
base::ScopedCFTypeRef<CFMutableDictionaryRef> query(CFDictionaryCreateMutable(
kCFAllocatorDefault, 0, &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
CFDictionarySetValue(query, kSecClass, kSecClassKey);
CFDictionarySetValue(query, kSecAttrAccessGroup,
base::SysUTF8ToNSString(keychain_access_group));
......@@ -178,7 +180,9 @@ bool DoDeleteWebAuthnCredentials(const std::string& keychain_access_group,
continue;
}
base::ScopedCFTypeRef<CFMutableDictionaryRef> delete_query(
CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
CFDictionaryCreateMutable(kCFAllocatorDefault, 0,
&kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
CFDictionarySetValue(delete_query, kSecClass, kSecClassKey);
CFDictionarySetValue(delete_query, kSecAttrApplicationLabel,
sec_attr_app_label);
......@@ -233,7 +237,9 @@ TouchIdCredentialStore::CreateCredential(
CredentialMetadata::FromPublicKeyCredentialUserEntity(user, is_resident));
base::ScopedCFTypeRef<CFMutableDictionaryRef> params(
CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
CFDictionaryCreateMutable(kCFAllocatorDefault, 0,
&kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
CFDictionarySetValue(params, kSecAttrKeyType,
kSecAttrKeyTypeECSECPrimeRandom);
CFDictionarySetValue(params, kSecAttrKeySizeInBits, @256);
......
......@@ -79,7 +79,9 @@ bool CanCreateSecureEnclaveKeyPair() {
// bindings. Instead, attempt to create an ephemeral key pair in the secure
// enclave.
base::ScopedCFTypeRef<CFMutableDictionaryRef> params(
CFDictionaryCreateMutable(kCFAllocatorDefault, 0, nullptr, nullptr));
CFDictionaryCreateMutable(kCFAllocatorDefault, 0,
&kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
CFDictionarySetValue(params, kSecAttrKeyType,
kSecAttrKeyTypeECSECPrimeRandom);
CFDictionarySetValue(params, kSecAttrKeySizeInBits, @256);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment