chromium/src
0
Fork 0
Code Activity

Force crypto::AppleKeychain access to be guarded by a Big Global Lock

Browse Source 
Apple removed the Big Global Lock guarding the Security.framework API,
but there are a number of thread-unsafe places in the API. Additionally,
it seems that OS X 10.8.2 has introduced some deadlock potential, so
force calls to be serialized behind a Chrome-supplied Big Global Lock
until it's safe to do otherwise.

BUG=151707
TEST=See bug


Review URL: https://chromiumcodereview.appspot.com/11016004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@159839 0039d316-1c4b-4281-b951-d872f2087c98
This commit is contained in:
rsleevi@chromium.org
2012-10-03 05:46:45 +00:00
parent 77e91500ae
commit d6e8fe6819
4 changed files with 64 additions and 23 deletions

13
crypto/apple_keychain_mac.mm

@ -6,6 +6,9 @@
#import <Foundation/Foundation.h>
#include "base/synchronization/lock.h"
#include "crypto/mac_security_services_lock.h"
namespace crypto {
AppleKeychain::AppleKeychain() {}
@ -19,6 +22,7 @@ OSStatus AppleKeychain::ItemCopyAttributesAndData(
SecKeychainAttributeList** attrList,
UInt32* length,
void** outData) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainItemCopyAttributesAndData(itemRef, info, itemClass,
attrList, length, outData);
}
@ -28,6 +32,7 @@ OSStatus AppleKeychain::ItemModifyAttributesAndData(
const SecKeychainAttributeList* attrList,
UInt32 length,
const void* data) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainItemModifyAttributesAndData(itemRef, attrList, length,
data);
}
@ -35,10 +40,12 @@ OSStatus AppleKeychain::ItemModifyAttributesAndData(
OSStatus AppleKeychain::ItemFreeAttributesAndData(
SecKeychainAttributeList* attrList,
void* data) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainItemFreeAttributesAndData(attrList, data);
}
OSStatus AppleKeychain::ItemDelete(SecKeychainItemRef itemRef) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainItemDelete(itemRef);
}
@ -47,12 +54,14 @@ OSStatus AppleKeychain::SearchCreateFromAttributes(
SecItemClass itemClass,
const SecKeychainAttributeList* attrList,
SecKeychainSearchRef* searchRef) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainSearchCreateFromAttributes(keychainOrArray, itemClass,
attrList, searchRef);
}
OSStatus AppleKeychain::SearchCopyNext(SecKeychainSearchRef searchRef,
SecKeychainItemRef* itemRef) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainSearchCopyNext(searchRef, itemRef);
}
@ -72,6 +81,7 @@ OSStatus AppleKeychain::AddInternetPassword(
UInt32 passwordLength,
const void* passwordData,
SecKeychainItemRef* itemRef) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainAddInternetPassword(keychain,
serverNameLength, serverName,
securityDomainLength, securityDomain,
@ -90,6 +100,7 @@ OSStatus AppleKeychain::FindGenericPassword(CFTypeRef keychainOrArray,
UInt32* passwordLength,
void** passwordData,
SecKeychainItemRef* itemRef) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainFindGenericPassword(keychainOrArray,
serviceNameLength,
serviceName,
@ -102,6 +113,7 @@ OSStatus AppleKeychain::FindGenericPassword(CFTypeRef keychainOrArray,
OSStatus AppleKeychain::ItemFreeContent(SecKeychainAttributeList* attrList,
void* data) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainItemFreeContent(attrList, data);
}
@ -113,6 +125,7 @@ OSStatus AppleKeychain::AddGenericPassword(SecKeychainRef keychain,
UInt32 passwordLength,
const void* passwordData,
SecKeychainItemRef* itemRef) const {
base::AutoLock lock(GetMacSecurityServicesLock());
return SecKeychainAddGenericPassword(keychain,
serviceNameLength,
serviceName,

10
net/base/cert_verify_proc_mac.cc

@ -16,6 +16,8 @@
#include "base/mac/scoped_cftyperef.h"
#include "base/sha1.h"
#include "base/string_piece.h"
#include "base/synchronization/lock.h"
#include "crypto/mac_security_services_lock.h"
#include "crypto/nss_util.h"
#include "crypto/sha2.h"
#include "net/base/asn1_util.h"
@ -362,11 +364,9 @@ int CertVerifyProcMac::VerifyInternal(X509Certificate* cert,
// chain building.
ScopedCFTypeRef<CFArrayRef> cert_array(cert->CreateOSCertChainForCert());
// From here on, only one thread can be active at a time. We have had a number
// of sporadic crashes in the SecTrustEvaluate call below, way down inside
// Apple's cert code, which we suspect are caused by a thread-safety issue.
// So as a speculative fix allow only one thread to use SecTrust on this cert.
base::AutoLock lock(verification_lock_);
// Serialize all calls that may use the Keychain, to work around various
// issues in OS X 10.6+ with multi-threaded access to Security.framework.
base::AutoLock lock(crypto::GetMacSecurityServicesLock());
SecTrustRef trust_ref = NULL;
status = SecTrustCreateWithCertificates(cert_array, trust_policies,

6
net/base/cert_verify_proc_mac.h

@ -7,8 +7,6 @@
#include "net/base/cert_verify_proc.h"
#include "base/synchronization/lock.h"
namespace net {
// Performs certificate path construction and validation using OS X's
@ -26,10 +24,6 @@ class CertVerifyProcMac : public CertVerifyProc {
int flags,
CRLSet* crl_set,
CertVerifyResult* verify_result) OVERRIDE;
// Blocks multiple threads from verifying the cert simultaneously.
// (Marked mutable because it's used in a const method.)
mutable base::Lock verification_lock_;
};
} // namespace net

58
net/base/x509_certificate_mac.cc

@ -18,8 +18,10 @@
#include "base/memory/singleton.h"
#include "base/pickle.h"
#include "base/sha1.h"
#include "base/synchronization/lock.h"
#include "base/sys_string_conversions.h"
#include "crypto/cssm_init.h"
#include "crypto/mac_security_services_lock.h"
#include "crypto/nss_util.h"
#include "crypto/rsa_private_key.h"
#include "net/base/x509_util_mac.h"
@ -93,6 +95,7 @@ OSStatus CopyCertChain(SecCertificateRef cert_handle,
CFArrayRef* out_cert_chain) {
DCHECK(cert_handle);
DCHECK(out_cert_chain);
// Create an SSL policy ref configured for client cert evaluation.
SecPolicyRef ssl_policy;
OSStatus result = x509_util::CreateSSLClientPolicy(&ssl_policy);
@ -105,7 +108,11 @@ OSStatus CopyCertChain(SecCertificateRef cert_handle,
NULL, const_cast<const void**>(reinterpret_cast<void**>(&cert_handle)),
1, &kCFTypeArrayCallBacks));
SecTrustRef trust_ref = NULL;
result = SecTrustCreateWithCertificates(input_certs, ssl_policy, &trust_ref);
{
base::AutoLock lock(crypto::GetMacSecurityServicesLock());
result = SecTrustCreateWithCertificates(input_certs, ssl_policy,
&trust_ref);
}
if (result)
return result;
ScopedCFTypeRef<SecTrustRef> trust(trust_ref);
@ -113,10 +120,17 @@ OSStatus CopyCertChain(SecCertificateRef cert_handle,
// Evaluate trust, which creates the cert chain.
SecTrustResultType status;
CSSM_TP_APPLE_EVIDENCE_INFO* status_chain;
result = SecTrustEvaluate(trust, &status);
{
base::AutoLock lock(crypto::GetMacSecurityServicesLock());
result = SecTrustEvaluate(trust, &status);
}
if (result)
return result;
return SecTrustGetResult(trust, &status, out_cert_chain, &status_chain);
{
base::AutoLock lock(crypto::GetMacSecurityServicesLock());
result = SecTrustGetResult(trust, &status, out_cert_chain, &status_chain);
}
return result;
}
// Returns true if |purpose| is listed as allowed in |usage|. This
@ -162,8 +176,13 @@ void AddCertificatesFromBytes(const char* data, size_t length,
kCFAllocatorNull));
CFArrayRef items = NULL;
OSStatus status = SecKeychainItemImport(local_data, NULL, &input_format,
NULL, 0, NULL, NULL, &items);
OSStatus status;
{
base::AutoLock lock(crypto::GetMacSecurityServicesLock());
status = SecKeychainItemImport(local_data, NULL, &input_format,
NULL, 0, NULL, NULL, &items);
}
if (status) {
OSSTATUS_DLOG(WARNING, status)
<< "Unable to import items from data of length " << length;
@ -693,17 +712,29 @@ bool X509Certificate::GetSSLClientCertificates(
// argument is ignored and filtering unimplemented. See
// SecIdentity.cpp in libsecurity_keychain, specifically
// _SecIdentityCopyPreferenceMatchingName().
if (SecIdentityCopyPreference(domain_str, 0, NULL, &identity) == noErr)
preferred_identity.reset(identity);
{
base::AutoLock lock(crypto::GetMacSecurityServicesLock());
if (SecIdentityCopyPreference(domain_str, 0, NULL, &identity) == noErr)
preferred_identity.reset(identity);
}
}
// Now enumerate the identities in the available keychains.
SecIdentitySearchRef search = nil;
OSStatus err = SecIdentitySearchCreate(NULL, CSSM_KEYUSE_SIGN, &search);
SecIdentitySearchRef search = NULL;
OSStatus err;
{
base::AutoLock lock(crypto::GetMacSecurityServicesLock());
err = SecIdentitySearchCreate(NULL, CSSM_KEYUSE_SIGN, &search);
}
if (err)
return false;
ScopedCFTypeRef<SecIdentitySearchRef> scoped_search(search);
while (!err) {
SecIdentityRef identity = NULL;
err = SecIdentitySearchCopyNext(search, &identity);
{
base::AutoLock lock(crypto::GetMacSecurityServicesLock());
err = SecIdentitySearchCopyNext(search, &identity);
}
if (err)
break;
ScopedCFTypeRef<SecIdentityRef> scoped_identity(identity);
@ -755,8 +786,11 @@ bool X509Certificate::GetSSLClientCertificates(
CFArrayRef X509Certificate::CreateClientCertificateChain() const {
// Initialize the result array with just the IdentityRef of the receiver:
SecIdentityRef identity;
OSStatus result =
SecIdentityCreateWithCertificate(NULL, cert_handle_, &identity);
OSStatus result;
{
base::AutoLock lock(crypto::GetMacSecurityServicesLock());
result = SecIdentityCreateWithCertificate(NULL, cert_handle_, &identity);
}
if (result) {
OSSTATUS_LOG(ERROR, result) << "SecIdentityCreateWithCertificate error";
return NULL;