From f0a8c86ede36903b84d3cad0e41889734814cb83 Mon Sep 17 00:00:00 2001 From: "stuart.morgan%alumni.case.edu" Date: Sun, 1 Apr 2007 22:38:57 +0000 Subject: [PATCH] Camino only - Bug 372153: Don't try to access keychain data until necessary, and don't keep trying after being denied. r=josh sr=pink git-svn-id: svn://10.0.0.236/trunk@223986 18797224-902f-48f8-a5cc-f745e15eee43 --- mozilla/camino/src/formfill/KeychainItem.h | 1 + mozilla/camino/src/formfill/KeychainItem.m | 48 ++++++++++++++----- .../camino/src/formfill/KeychainService.mm | 15 ++++-- 3 files changed, 48 insertions(+), 16 deletions(-) diff --git a/mozilla/camino/src/formfill/KeychainItem.h b/mozilla/camino/src/formfill/KeychainItem.h index 38d69e20770..baff769e138 100644 --- a/mozilla/camino/src/formfill/KeychainItem.h +++ b/mozilla/camino/src/formfill/KeychainItem.h @@ -44,6 +44,7 @@ @private SecKeychainItemRef mKeychainItemRef; // strong BOOL mDataLoaded; + BOOL mPasswordLoaded; NSString* mUsername; // strong NSString* mPassword; // strong NSString* mHost; // strong diff --git a/mozilla/camino/src/formfill/KeychainItem.m b/mozilla/camino/src/formfill/KeychainItem.m index 76877eaa230..a8b257b6aba 100644 --- a/mozilla/camino/src/formfill/KeychainItem.m +++ b/mozilla/camino/src/formfill/KeychainItem.m @@ -41,6 +41,7 @@ @interface KeychainItem(Private) - (KeychainItem*)initWithRef:(SecKeychainItemRef)ref; - (void)loadKeychainData; +- (void)loadKeychainPassword; - (BOOL)setAttributeType:(SecKeychainAttrType)type toString:(NSString*)value; - (BOOL)setAttributeType:(SecKeychainAttrType)type toValue:(void*)valuePtr withLength:(UInt32)length; @end @@ -204,15 +205,11 @@ attrInfo.format = NULL; SecKeychainAttributeList *attrList; - UInt32 passwordLength; - char* passwordData; - OSStatus result = SecKeychainItemCopyAttributesAndData(mKeychainItemRef, &attrInfo, NULL, &attrList, - &passwordLength, (void**)(&passwordData)); + OSStatus result = SecKeychainItemCopyAttributesAndData(mKeychainItemRef, &attrInfo, NULL, + &attrList, NULL, NULL); [mUsername autorelease]; mUsername = nil; - [mPassword autorelease]; - mPassword = nil; [mHost autorelease]; mHost = nil; [mComment autorelease]; @@ -221,9 +218,8 @@ mSecurityDomain = nil; if (result != noErr) { - NSLog(@"Couldn't load keychain data"); + NSLog(@"Couldn't load keychain data (error %d)", result); mUsername = [[NSString alloc] init]; - mPassword = [[NSString alloc] init]; mHost = [[NSString alloc] init]; mComment = [[NSString alloc] init]; mSecurityDomain = [[NSString alloc] init]; @@ -249,11 +245,39 @@ else if (attr.tag == kSecCreatorItemAttr) mCreator = attr.data ? *((OSType*)(attr.data)) : 0; } - mPassword = [[NSString alloc] initWithBytes:passwordData length:passwordLength encoding:NSUTF8StringEncoding]; - SecKeychainItemFreeAttributesAndData(attrList, (void*)passwordData); mDataLoaded = YES; } +// Password is fetched separately, since trying to read the password will +// trigger an auth request if we aren't on the trust list. +- (void)loadKeychainPassword +{ + if (!mKeychainItemRef) + return; + UInt32 passwordLength; + char* passwordData; + OSStatus result = SecKeychainItemCopyAttributesAndData(mKeychainItemRef, NULL, NULL, NULL, + &passwordLength, (void**)(&passwordData)); + + [mPassword autorelease]; + mPassword = nil; + + if (result == noErr) { + mPassword = [[NSString alloc] initWithBytes:passwordData + length:passwordLength + encoding:NSUTF8StringEncoding]; + SecKeychainItemFreeAttributesAndData(NULL, (void*)passwordData); + } + else { + // Being denied access isn't a failure case, so don't log it. + if (result != errSecAuthFailed) + NSLog(@"Couldn't load keychain data (%d)", result); + } + // Mark it as loaded either way, so that we can return nil password as an + // indicator that the item is inaccessible. + mPasswordLoaded = YES; +} + - (NSString*)username { if (!mDataLoaded) @@ -263,8 +287,8 @@ - (NSString*)password { - if (!mDataLoaded) - [self loadKeychainData]; + if (!mPasswordLoaded) + [self loadKeychainPassword]; return mPassword; } diff --git a/mozilla/camino/src/formfill/KeychainService.mm b/mozilla/camino/src/formfill/KeychainService.mm index ec3c2f6987a..c0de3566f31 100644 --- a/mozilla/camino/src/formfill/KeychainService.mm +++ b/mozilla/camino/src/formfill/KeychainService.mm @@ -241,18 +241,23 @@ int KeychainPrefChangedCallback(const char* inPref, void* unused) } newKeychainItems = [matchingItems arrayByAddingObjectsFromArray:genericItems]; } + + // Find the best matching keychain entry. The password is always checked to + // ensure that it's not nil before returning that item (which triggers an auth + // request if necsessary) so that we know we are always returning something + // that is actually useful. // First, check for a new-style Camino keychain entry KeychainItem* item; NSEnumerator* keychainEnumerator = [newKeychainItems objectEnumerator]; while ((item = [keychainEnumerator nextObject])) { - if ([item creator] == kCaminoKeychainCreatorCode) + if (([item creator] == kCaminoKeychainCreatorCode) && [item password]) return item; } // If there isn't one, check for an old-style Camino entry item = [self findLegacyKeychainEntryForHost:host port:port]; - if (item) + if (item && [item password]) return item; // Finally, check for a new style entry created by something other than Camino. @@ -267,14 +272,16 @@ int KeychainPrefChangedCallback(const char* inPref, void* unused) // items; that's just the way they roll. This fragile method is the best we can do. if ([[item password] isEqualToString:@" "]) continue; - return item; + if ([item password]) + return item; } } keychainEnumerator = [newKeychainItems objectEnumerator]; while ((item = [keychainEnumerator nextObject])) { if ([[item password] isEqualToString:@" "]) continue; - return item; + if ([item password]) + return item; } return nil;