X-Git-Url: https://git.saurik.com/apple/security.git/blobdiff_plain/dd5fb164cf5b32c462296bc65e289e100f74b59a..refs/heads/master:/securityd/src/agentquery.cpp?ds=sidebyside

diff --git a/securityd/src/agentquery.cpp b/securityd/src/agentquery.cpp
index afacd8b3..3c813f4c 100644
--- a/securityd/src/agentquery.cpp
+++ b/securityd/src/agentquery.cpp
@@ -150,7 +150,8 @@ SecurityAgentXPCConnection::activate(bool ignoreUid)
 			xpc_connection_set_target_uid(mXPCConnection, targetUid);
 			secnotice("SecurityAgentXPCConnection", "Creating a standard security agent");
 		} else {
-			mXPCConnection = xpc_connection_create_mach_service(SECURITYAGENT_LOGINWINDOW_BOOTSTRAP_NAME_BASE, NULL, 0);
+			mXPCConnection = xpc_connection_create_mach_service(SECURITYAGENT_LOGINWINDOW_BOOTSTRAP_NAME_BASE, NULL,
+                                                                XPC_CONNECTION_MACH_SERVICE_PRIVILEGED);
 			xpc_connection_set_instance(mXPCConnection, sessionUUID);
 			secnotice("SecurityAgentXPCConnection", "Creating a loginwindow security agent");
 		}
@@ -321,6 +322,10 @@ static void xpcArrayToAuthItemSet(AuthItemSet *setToBuild, xpc_object_t input) {
         bool sensitive = xpc_dictionary_get_value(item, AUTH_XPC_ITEM_SENSITIVE_VALUE_LENGTH);
         if (sensitive) {
             size_t sensitiveLength = (size_t)xpc_dictionary_get_uint64(item, AUTH_XPC_ITEM_SENSITIVE_VALUE_LENGTH);
+            if (sensitiveLength > length) {
+                secnotice("SecurityAgentXPCQuery", "Sensitive data len %zu is not valid", sensitiveLength);
+                return true;
+            }
             dataCopy = malloc(sensitiveLength);
             memcpy(dataCopy, data, sensitiveLength);
             memset_s((void *)data, length, 0, sensitiveLength); // clear the sensitive data, memset_s is never optimized away
@@ -490,13 +495,13 @@ QueryKeychainUse::QueryKeychainUse(bool needPass, const Database *db)
 	// if passphrase checking requested, save KeychainDatabase reference
 	// (will quietly disable check if db isn't a keychain)
 
-    // Always require password, <rdar://problem/34677969> Always require the user's password on keychain approval dialogs
-	// if (needPass)
-		mPassphraseCheck = dynamic_cast<const KeychainDatabase *>(db);
+    // Always require password due to <rdar://problem/34677969>
+    mPassphraseCheck = dynamic_cast<const KeychainDatabase *>(db);
 
     setTerminateOnSleep(true);
 }
 
+// Callers to this function must hold the common lock
 Reason QueryKeychainUse::queryUser (const char *database, const char *description, AclAuthorization action)
 {
     Reason reason = SecurityAgent::noReason;
@@ -537,7 +542,12 @@ Reason QueryKeychainUse::queryUser (const char *database, const char *descriptio
             hints.erase(retryHint); hints.insert(retryHint); // replace
 
             setInput(hints, context);
-			invoke();
+
+            {
+                // Must drop the common lock while showing UI.
+                StSyncLock<Mutex, Mutex> syncLock(const_cast<KeychainDatabase*>(mPassphraseCheck)->common().uiLock(), const_cast<KeychainDatabase*>(mPassphraseCheck)->common());
+                invoke();
+            }
 
             if (retryCount > kMaximumAuthorizationTries)
 			{
@@ -551,8 +561,16 @@ Reason QueryKeychainUse::queryUser (const char *database, const char *descriptio
 				continue;
 
 			passwordItem->getCssmData(data);
+
+            // decode() replaces the master key, so do this only if we know the passphrase is correct.
+            // I suspect decode() is redundant but something might rely on its side effects so let's keep it.
+            if (const_cast<KeychainDatabase*>(mPassphraseCheck)->validatePassphrase(data) && const_cast<KeychainDatabase*>(mPassphraseCheck)->decode(data)) {
+                reason = SecurityAgent::noReason;
+            } else {
+                reason = SecurityAgent::invalidPassphrase;
+            }
 		}
-		while ((reason = (const_cast<KeychainDatabase*>(mPassphraseCheck)->decode(data) ? SecurityAgent::noReason : SecurityAgent::invalidPassphrase)));
+        while (reason != SecurityAgent::noReason);
         
         readChoice();
 	}
@@ -649,6 +667,11 @@ Reason QueryOld::operator () ()
 //
 Reason QueryUnlock::accept(CssmManagedData &passphrase)
 {
+    // Must hold the 'common' lock to call decode; otherwise there's a data corruption issue
+    StLock<Mutex> _(safer_cast<KeychainDatabase &>(database).common());
+
+    // Calling validatePassphrase here throws when trying to constitute a key.
+    // Unsure why but since this is for the KC unlock path and not a validation path the wrong password won't make things worse.
 	if (safer_cast<KeychainDatabase &>(database).decode(passphrase))
 		return SecurityAgent::noReason;
 	else
@@ -1049,7 +1072,7 @@ Reason QueryDBBlobSecret::accept(CssmManagedData &passphrase,
 
 // @@@  no pluggable authentication possible!
 Reason
-QueryKeychainAuth::operator () (const char *database, const char *description, AclAuthorization action, const char *prompt)
+QueryKeychainAuth::performQuery(const KeychainDatabase& db, const char *description, AclAuthorization action, const char *prompt)
 {
     Reason reason = SecurityAgent::noReason;
     AuthItemSet hints, context;
@@ -1058,7 +1081,7 @@ QueryKeychainAuth::operator () (const char *database, const char *description, A
 	string password;
 
     using CommonCriteria::Securityd::KeychainAuthLogger;
-    KeychainAuthLogger logger(mAuditToken, (short)AUE_ssauthint, database, description);
+    KeychainAuthLogger logger(mAuditToken, (short)AUE_ssauthint, db.dbName(), description);
 
     hints.insert(mClientHints.begin(), mClientHints.end());
 
@@ -1071,15 +1094,16 @@ QueryKeychainAuth::operator () (const char *database, const char *description, A
 	hints.insert(AuthItemRef(AGENT_HINT_KEYCHAIN_ITEM_NAME, AuthValueOverlay(description ? (uint32_t)strlen(description) : 0, const_cast<char*>(description))));
 
 	// keychain name into hints
-	hints.insert(AuthItemRef(AGENT_HINT_KEYCHAIN_PATH, AuthValueOverlay(database ? (uint32_t)strlen(database) : 0, const_cast<char*>(database))));
+	hints.insert(AuthItemRef(AGENT_HINT_KEYCHAIN_PATH, AuthValueOverlay(db.dbName() ? (uint32_t)strlen(db.dbName()) : 0, const_cast<char*>(db.dbName()))));
 
     create("builtin", "confirm-access-user-password");
 
     AuthItem *usernameItem;
     AuthItem *passwordItem;
 
+    // This entire do..while requires the UI lock because we do accept() in the condition, which in some cases is reentrant
+    StSyncLock<Mutex, Mutex> syncLock(db.common().uiLock(), db.common());
     do {
-
         AuthItemRef triesHint(AGENT_HINT_TRIES, AuthValueOverlay(sizeof(retryCount), &retryCount));
         hints.erase(triesHint); hints.insert(triesHint); // replace
 
@@ -1115,6 +1139,7 @@ QueryKeychainAuth::operator () (const char *database, const char *description, A
         usernameItem->getString(username);
         passwordItem->getString(password);
     } while ((reason = accept(username, password)));
+    syncLock.unlock();
 
     if (SecurityAgent::noReason == reason)
         logger.logSuccess();