Could they bypass all the intent checks?

[lowrebswrd]

Hi, Michal. I'd like to ask one question. For an unpatched version of code corresponding to CVE-2023-45777.

https://android.googlesource.com/platform/frameworks/base/+/b0f6558fb36eb76df35c516ec5a65030a34a8734/services/core/java/com/android/server/accounts/AccountManagerService.java

Please look at the following code, you said getParcelable without the second typed parameter will return null. But getParcelable(AccountManager.KEY_INTENT,
Intent.class) will throw exception. as readValue will check the intent.class And the following 1,2,3,4 will not bypass the intent check. Am I right？ I cannot understand it.

        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);//1. intent will return null because it has an ingenious layout.
            if (intent != null && intent.getClass() != Intent.class) {
                return false;
            }
            Intent simulateIntent = simulateBundle.getParcelable(AccountManager.KEY_INTENT,
                    Intent.class); //2. As you know, it will throw exception because it cannot bypass the check of readValue() function. The process will crash. How could you reach 3. and all return null?
            if (intent == null) {
                return (simulateIntent == null); //3. Couldn't be here.
            }
            if (!intent.filterEquals(simulateIntent)) {
                return false;
            }
            if (intent.getSelector() != simulateIntent.getSelector()) {
                return false;
            }
            int prohibitedFlags = Intent.FLAG_GRANT_READ_URI_PERMISSION
                    | Intent.FLAG_GRANT_WRITE_URI_PERMISSION
                    | Intent.FLAG_GRANT_PERSISTABLE_URI_PERMISSION
                    | Intent.FLAG_GRANT_PREFIX_URI_PERMISSION;
            return (simulateIntent.getFlags() & prohibitedFlags) == 0;
        }
      
			
	      protected boolean checkKeyIntent(int authUid, Bundle bundle) {
            if (!checkKeyIntentParceledCorrectly(bundle)) {
            	EventLog.writeEvent(0x534e4554, "250588548", authUid, "");
                return false;
            }
            Intent intent = bundle.getParcelable(AccountManager.KEY_INTENT, Intent.class);  //4. Because the second typed parameter which is Intent.class not equal to array of SecImageClipData  will has an exception.
            if (intent == null) {
                return true;
            }
			

[michalbednarski]

Let's take a look:

First we have just delegation to BaseBundle#get(String,Class)

public <T> T getParcelable(@Nullable String key, @NonNull Class<T> clazz) {
    // ...
    return get(key, clazz);
}

Then that method delegates to getValue(), but swallowing ClassCastException (or BadTypeParcelableException)

<T> T get(@Nullable String key, @NonNull Class<T> clazz) {
    unparcel();
    try {
        return getValue(key, requireNonNull(clazz));
    } catch (ClassCastException | BadTypeParcelableException e) {
        typeWarning(key, clazz.getCanonicalName(), e);
        return null;
    }
}

getValue() delegates to getValueAt()

final <T> T getValue(String key, @Nullable Class<T> clazz, @Nullable Class<?>... itemTypes) {
    int i = mMap.indexOfKey(key);
    return (i >= 0) ? getValueAt(i, clazz, itemTypes) : null;
}

getValueAt() doesn't enter object instanceof BiFunction as value was already replaced by mMap.setValueAt(i, object); during previous invocation, ClassCastException is triggered by clazz.cast(object), but it is caught by getValue()

final <T> T getValueAt(int i, @Nullable Class<T> clazz, @Nullable Class<?>... itemTypes) {
    Object object = mMap.valueAt(i);
    if (object instanceof BiFunction<?, ?, ?>) {
        try {
            object = ((BiFunction<Class<?>, Class<?>[], ?>) object).apply(clazz, itemTypes);
        } catch (BadParcelableException e) {
            if (sShouldDefuse) {
                Log.w(TAG, "Failed to parse item " + mMap.keyAt(i) + ", returning null.", e);
                return null;
            } else {
                throw e;
            }
        }
        mMap.setValueAt(i, object);
    }
    return (clazz != null) ? clazz.cast(object) : (T) object;
}

[lowrebswrd]

Thanks for the detail and in getting back to me. So as what you said, bundle.getParcelable(AccountManager.KEY_INTENT); and simulateBundle.getParcelable(AccountManager.KEY_INTENT,Intent.class) that both return null, then bypass the check checkKeyIntentParceledCorrectly and checkKeyIntent. Google also gives this patch:

             p.setDataPosition(0);
             Bundle simulateBundle = p.readBundle();
             p.recycle();
-            Intent intent = bundle.getParcelable(AccountManager.KEY_INTENT);
+            Intent intent = bundle.getParcelable(AccountManager.KEY_INTENT, Intent.class);

Add intent.class for the second typed argument, can we say the patch didn't work as all return null.

[michalbednarski]

-            Intent intent = bundle.getParcelable(AccountManager.KEY_INTENT);
+            Intent intent = bundle.getParcelable(AccountManager.KEY_INTENT, Intent.class);

This patch is exactly what this writeup is about, it is true that both old and new version return null, however the point is what getParcelable does besides returning value

[lowrebswrd]

Thank you!