From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Hao Ke Date: Tue, 4 Oct 2022 19:43:58 +0000 Subject: [PATCH] Add safety checks on KEY_INTENT mismatch. For many years, Parcel mismatch typed exploits has been using the AccoungManagerService's passing of KEY_INTENT workflow, as a foothold of launching arbitrary intents. We are adding an extra check on the service side to simulate the final deserialization of the KEY_INTENT value, to make sure the client side won't get a mismatched KEY_INTENT value. Bug: 250588548 Bug: 240138294 Test: atest CtsAccountManagerTestCases Test: local test, also see b/250588548 Change-Id: I433e34f6e21ce15c89825044a15b1dec46bb25cc (cherry picked from commit eb9a0566a583fa13f8aff671c41f78a9e33eab82) Merged-In: I433e34f6e21ce15c89825044a15b1dec46bb25cc --- .../accounts/AccountManagerService.java | 34 ++++++++++++++++--- 1 file changed, 30 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/accounts/AccountManagerService.java b/services/core/java/com/android/server/accounts/AccountManagerService.java index 239297cc420a..7273e3ea5ffc 100644 --- a/services/core/java/com/android/server/accounts/AccountManagerService.java +++ b/services/core/java/com/android/server/accounts/AccountManagerService.java @@ -89,6 +89,7 @@ import android.os.UserHandle; import android.os.UserManager; import android.os.storage.StorageManager; import android.text.TextUtils; +import android.util.EventLog; import android.util.Log; import android.util.Pair; import android.util.Slog; @@ -2545,7 +2546,7 @@ public class AccountManagerService */ if (!checkKeyIntent( Binder.getCallingUid(), - intent)) { + result)) { onError(AccountManager.ERROR_CODE_INVALID_RESPONSE, "invalid intent in bundle returned"); return; @@ -2960,7 +2961,7 @@ public class AccountManagerService && (intent = result.getParcelable(AccountManager.KEY_INTENT)) != null) { if (!checkKeyIntent( Binder.getCallingUid(), - intent)) { + result)) { onError(AccountManager.ERROR_CODE_INVALID_RESPONSE, "invalid intent in bundle returned"); return; @@ -4230,7 +4231,13 @@ public class AccountManagerService * into launching aribtrary intents on the device via by tricking to click authenticator * supplied entries in the system Settings app. */ - protected boolean checkKeyIntent(int authUid, Intent intent) { + protected boolean checkKeyIntent(int authUid, Bundle bundle) { + if (!checkKeyIntentParceledCorrectly(bundle)) { + EventLog.writeEvent(0x534e4554, "250588548", authUid, ""); + return false; + } + + Intent intent = bundle.getParcelable(AccountManager.KEY_INTENT); // Explicitly set an empty ClipData to ensure that we don't offer to // promote any Uris contained inside for granting purposes if (intent.getClipData() == null) { @@ -4263,6 +4270,25 @@ public class AccountManagerService } } + /** + * Simulate the client side's deserialization of KEY_INTENT value, to make sure they don't + * violate our security policy. + * + * In particular we want to make sure the Authenticator doesn't trick users + * into launching arbitrary intents on the device via exploiting any other Parcel read/write + * mismatch problems. + */ + private boolean checkKeyIntentParceledCorrectly(Bundle bundle) { + Parcel p = Parcel.obtain(); + p.writeBundle(bundle); + p.setDataPosition(0); + Bundle simulateBundle = p.readBundle(); + p.recycle(); + Intent intent = bundle.getParcelable(AccountManager.KEY_INTENT); + Intent simulateIntent = simulateBundle.getParcelable(AccountManager.KEY_INTENT); + return (intent.filterEquals(simulateIntent)); + } + private void close() { synchronized (mSessions) { if (mSessions.remove(toString()) == null) { @@ -4408,7 +4434,7 @@ public class AccountManagerService && (intent = result.getParcelable(AccountManager.KEY_INTENT)) != null) { if (!checkKeyIntent( Binder.getCallingUid(), - intent)) { + result)) { onError(AccountManager.ERROR_CODE_INVALID_RESPONSE, "invalid intent in bundle returned"); return;