Mysterious patch

Let's start this time with the patch that appeared as fix for CVE-2023-45777 in Android Security Bulletin:

diff --git a/services/core/java/com/android/server/accounts/AccountManagerService.java b/services/core/java/com/android/server/accounts/AccountManagerService.java
index 7a19d034c2c8..5238595fe2a2 100644
--- a/services/core/java/com/android/server/accounts/AccountManagerService.java
+++ b/services/core/java/com/android/server/accounts/AccountManagerService.java
@@ -4923,7 +4923,7 @@ public class AccountManagerService
             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);
             if (intent != null && intent.getClass() != Intent.class) {
                 return false;
             }

Few people were puzzled by it enough to ask me, previously I've replied to them with some hints and now I'm publishing full writeup for this issue

But first lets provide some context about what is going on in this patch

This is change in checkKeyIntent() method. This method performs multiple checks to ensure that Intent provided by application is safe for system to launch (using privileges of system)

First, this method uses checkKeyIntentParceledCorrectly() which serializes and deserializes again Bundle which we're checking and checks if Intent taken from Bundle before that matches Intent from Bundle after such cycle. Since launch of Intent happens in other system app processes than one which performs validation, it was previously possible to construct Bundle-s which appeared safe during validation inside AccountManagerService, but contained different Intent after being sent to next process. This simulates sending Bundle to next process in order to detect such situations.

After checkKeyIntentParceledCorrectly() we have bundle.getParcelable() call, which this patch switches from deprecated version that could construct any object to one that validates that object that is about to be deserialized is of type which was specified in second parameter

That version with type parameter was introduced in Android 13, as part of larger Parcel/Bundle hardening. In particular, before Android 13 when Bundle was sent between processes, it kept raw copy of whole serialized data until any item was accessed, at which point every value was deserialized. Now when any value is accessed for first time after Bundle has been received, only String keys and the values of primitive types are deserialized, while non-primitive values are left as LazyValue-s, which have their length stored as part of serialized data in order to ensure that even when serialization/deserialization logic is mismatched, such mismatches won't affect other entries

Before we dive in, lets have a look at LazyValue: In it's source code we've got nice comment explaining it's data structure

                     |   4B   |   4B   |
mSource = Parcel{... |  type  | length | object | ...}
                     a        b        c        d
length = d - c
mPosition = a
mLength = d - a

mPosition and mLength describe location of whole LazyValue data in original Parcel, including type and length. "length" (without "m" at beginning) refers to length value as written to Parcel and excludes header (type and length)

If Bundle containing LazyValue is being forwarded to another process, whole LazyValue including type and length fields is copied verbatim from Bundle.mParcelledData to destination Parcel

When Bundle item represented by LazyValue is accessed, Parcel is rewound to mPosition and readValue() is called. If type argument is passed to bundle.getParcelable(), it is propagated to readValue() which will both ensure that type about to be unparcelled is expected one as well as verify after unparcelling that unparcelled value type is expected one. After unparcelling LazyValue is replaced so next time Bundle is written to Parcel, value will be serialized through writeValue() again

Use of typed Bundle.get*()/Parcel.read*() parameter is mostly relevant for methods such as Parcel.readParcelableList(), which returns ArrayList and due to Java Type Erasure even if you did something like List<SomeParcelableType> field = parcel.readParcelableList();, the <SomeParcelableType> part wasn't enforced at runtime and such List could contain any Parcelable classes available in system and therefore all createFromParcel/writeToParcel available in system could be used as part of serialization/deserialization of type that contained such List

You might also want to check out presentation from Android Security and Privacy team about introduction of these mechanisms (slides, video)

Here however use of typed version appears to be redundant, as we also explicitly check type of returned object. So what is going on and what vulnerability is being fixed here?

Side effects

Take a look at patch from beginning again

• If value deserialized under "intent" key is an Intent
  • It will be validated to point to component it is safe for system to launch
  • If mismatch could be triggered from Intent object we'd have much bigger problem
• If value being deserialized isn't Intent
  • In order to do anything bad, we'd need to have an Intent inside Bundle after it gets sent to another process, but type of Parcelable is saved at earlier offset than any possible mismatch and LazyValue length-prefixing prevents us from modifying next key-value pairs in case of writeToParcel/createFromParcel mismatch

So, what dangerous thing call to bundle.getParcelable(AccountManager.KEY_INTENT) without type argument could do here?

[Answer in next paragraph, try to guess before reading on. If I'd have a fursona this would be place for some art]

The answer is calling unrelated createFromParcel() that actually modifies of raw data of LazyValue that is stored under different key and will be passed verbatim to next process