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

We have a createFromParcel() implementation that can actually call writeInt() on provided Parcel

But not due to writeInt being mistakenly placed, but due to unrestricted reflection. In particular inside PackageParser we have following code:

final Class<T> cls = (Class<T>) Class.forName(componentName);
final Constructor<T> cons = cls.getConstructor(Parcel.class);

intentsList = new ArrayList<>(N);
for (int i = 0; i < N; ++i) {
    intentsList.add(cons.newInstance(in));
}

We can have Parcel object which was passed to createFromParcel passed to any available in system public constructor that accepts single Parcel argument

And then we have following code:

public PooledStringWriter(Parcel out) {
    mOut = out;
    mPool = new HashMap<>();
    mStart = out.dataPosition();
    out.writeInt(0); // reserve space for final pool size.
}

We've got constructor that calls writeInt(0) on provided Parcel, however there are few things that complicate exploitation

First of all, while it isn't directly visible in source code, immediately after newInstance() is called, a cast is performed and ClassCastException is thrown

Swallow the Exception

I needed something that would during createFromParcel call createFromParcel of another class under try block and then fail to propagate caught Exception

This is part where exploit doesn't actually work on pure AOSP, I've used Samsung specific class

I've included copy of relevant parts of that class in this repo

This repo also includes script that integrates it into AOSP, so for testing you can run it (pass path to your AOSP checkout as argument, e.g. ./make-aosp-buggy.sh /path/to/aosp), revert change described at beginning of writeup and run this exploit against your AOSP build

I've previously used OutputConfiguration class from AOSP for Exception swallowing, before Android 13 swallowing an Exception in createFromParcel() combined with allowing construction of other Parcelable-s is vulnerability in itself, however in case of SemImageClipData Exception swallowing wasn't present on these Android versions

There is however important difference between SemImageClipData and previously used OutputConfiguration: even though SemImageClipData catches an Exception, it still returns non-null object and if it will be later cast to another type, that would trigger ClassCastException which is what we're trying to avoid

Java Type Erasure strikes back

Java Type Erasure means that generic methods don't actually know about generic type used by caller. This usually was helping exploitation

// When we read some List, this actually didn't check if list contains only SomeParcelableType
List<SomeParcelableType> myList = sourceParcel.readParcelableList();

// Above is why Android 13 has introduced typed methods that enforce type at runtime
List<SomeParcelableType> myList = sourceParcel.readParcelableList(SomeParcelableType.class);

// If untyped method was used when reading, list can contain non-SomeParcelableType
// items and they would be written without errors
targetParcel.writeParcelableList(myList, 0);

// However if List contains non-SomeParcelableType item, this would throw during item access
// (That however commonly didn't happen if we used Parcelable object only as container in gadget chain)
SomeParcelableType myItem = myList.get(0);

This time type erasure didn't work in our favor. First we had method which actually invoked constructor through reflection

private static <T extends IntentInfo> ArrayList<T> createIntentsList(Parcel in) {
    // ...
    final ArrayList<T> intentsList;
    // ...
    intentsList.add(cons.newInstance(in));
    // ...
    return intentsList;
}

This method has generic parameter T. It doesn't matter what parameter type was used by caller, however since in declaration of this method there's <T extends IntentInfo>, the line with newInstance() call becomes intentsList.add((IntentInfo) cons.newInstance(in));, even though newInstance() returns Object and ArrayList.add() accepts Object as argument. This introduced need to wrap call of that with some Parcelable that swallows Exception

Then we have the bundle.getParcelable() call

@Deprecated
@Nullable
public <T extends Parcelable> T getParcelable(@Nullable String key) {
    unparcel();
    Object o = getValue(key);
    if (o == null) {
        return null;
    }
    try {
        return (T) o;
    } catch (ClassCastException e) {
        typeWarning(key, o, "Parcelable", e);
        return null;
    }
}

The deserialization procedure is performed by getValue() call, which actually leads to createFromParcel() call. If ClassCastException happens there it won't be caught. getValue() now returns whatever value was deserialized for this key through parcel.readValue()

However if we put SemImageClipData as value, under try-catch we'd try to cast to T, which in this case is Parcelable as declared in methods generic declaration. Caller uses this method as generic with T being an Intent, however getParcelable() doesn't know that and cast to Intent happens in caller and therefore ClassCastException is thrown outside try

We can however wrap our SemImageClipData inside Parcelable[] array, then cast to T within getParcelable() will fail to cast Parcelable[] to Parcelable and will throw ClassCastException under try, that Exception will be logged and null will be returned and then accepted by checkKeyIntent()

The Layout

So now we need to align stuff within Bundle so after writeToParcel/createFromParcel cycle it's contents will be ones that we've prepared

But unlike typical "Bundle FengShui" where the trigger is having createFromParcel read more or less data than matching writeToParcel previously did, here we have writeInt(0) overwriting part of non-deserialized LazyValue

So here's how Bundle.mParcelledData looks when it is first unparcelled by AccountManagerService (Offsets taken by calling dataPosition() through debugger attached to system_server)

Offset	Value	Note
0	3	Number of key-value pairs
4	"intent"	First key in Bundle, the one that will be accessed by getParcelable(AccountManager.KEY_INTENT)
24	16	First LazyValue starts here, type is VAL_PARCELABLEARRAY
28	340	Declared length of LazyValue, used to find next key in Bundle. Our LazyValue won't actually have this size after being read, but LazyValue.apply reports that through Slog.wtfStack() which doesn't throw
32	1	Length of Parcelable[] array, array has only one item and is present so ClassCastException happens inside try block that is in bundle.getParcelable()
36	"com.samsung.android. content.clipboard.data. SemImageClipData"	Name of Parcelable class, this is wrapper class that will swallow Exception
160	2	Type tag used by createClipBoardData()
164		Items that are read by SemImageClipData superclass constructor (which include readParcelable() call, however that happens outside try block). Not really relevant, but we need to go through them before reaching interesting part of createFromParcel()
252		Data read by SemImageClipData.readFromSource()
272	"android.content.pm. PackageParser$Activity"	Name of Parcelable read by mExtraParcelFd = in.readParcelable(). Type doesn't match, however before cast happens an Exception will be thrown anyway
360		className & metadata fields of PackageParser$Component
368	1	Number of items in createIntentsList()
372	"android.os. PooledStringWriter"	Name of class that we'll be instantiated through Class.forName().getConstructor(Parcel.class).newInstance(). At this position first LazyValue ends, parsing of it however continues as readValue() didn't reach end. Also interpreted as second key in Bundle during initial unparcel()
436	4	Second LazyValue starts here, this 4 is VAL_PARCELABLE for which Parcel.isLengthPrefixed() will return true. This value is later overwritten by PooledStringWriter constructor, after which an Exception is thrown and getParcelable(AccountManager.KEY_INTENT) finishes
440	240	Length of LazyValue whose type was declared to be VAL_PARCELABLE, this is used to determine position of next entry and how much data needs to be copied to target Bundle during re-serialization. This LazyValue is not actually unparcelled and is used as raw data container
684	"1&y~pw"	Third key/value pair, key is randomly generated to have Java hashCode() above previously used ones (Items stored inside ArrayMap are sorted by ascending hashCode() of key and that is the order items from Bundle will be written to Parcel). This key-value pair is present here only to increase total number of pairs written, as that will be number of pairs read, even though this pair actually won't be read
704	-1 (VAL_NULL)

Then, when Bundle is serialized again, it looks like that:

Offset	Value	Note
0	3	Number of key-value pairs
4	"intent"	First key in Bundle
24	16	VAL_PARCELABLEARRAY, previously deserialized Parcelable[] array is now being serialized again
28	196	Length of LazyValue, that is our wrapped SemImageClipData object. This length is taken from execution with my mock SemImageClipData and therefore offsets presented from this point on won't match ones that would appear on actual Samsung device, however this LazyValue won't be deserialized again so that doesn't matter for exploit execution
224	"android.os. PooledStringWriter"	Second key in Bundle
288	0	Second LazyValue starts here, item under "android.os.PooledStringWriter" key wasn't accessed, so this LazyValue is being copied from original data, however type tag was overwritten by writeInt(0) call done by PooledStringWriter constructor and upon reaching target process this is no longer interpreted as a LazyValue
292	240	This was length of second LazyValue that was copied from original Bundle, however since type tag was overwritten with writeInt(0), which is VAL_STRING, this value is now being read through readString(). Previously, for LazyValue, length was expressed in bytes, but now, for String, this is expressed in two-byte characters. There isn't enough data in source Parcel for that, so native parcel->readString16Inplace() fails after reading length, that however doesn't cause Exception on Java side
296	"intent"	"Third" key in Bundle. Actually overwrites first key: since "intent" has smaller hashCode() than previously seen key, ArrayMap.append() method uses put() which allows replacing values, otherwise we'd have duplicate key which would be later rejected by validate()
316	4	VAL_PARCELABLE, here starts LazyValue containing actual Intent that will be started
536	"1&y~pw"	Padding item that was written but isn't read because all 3 key-value pairs were already read. Unlike AIDL interfaces, there is no enforceNoDataAvail() check done on Bundle (but even if there were, it could be bypassed by inserting dummy entry that specifies expected length)

How it happened twice

Lets now discuss four patches, two of which fix vulnerability this writeup is about

• CVE-2023-20944 (bulletin, patch): This is another vulnerability found by me. Similarly to this, patch doesn't make it obvious how it would be exploited, but looks like someone else has figured it out (blog post in Chinese)
• CVE-2023-21098 (bulletin, patch): This is first time I reported exploit presented here. That patch also introduces fix for checkKeyIntentParceledCorrectly() bypass that is applicable to Android versions before 13
• CVE-2023-35669 (bulletin, patch): This one isn't in response to my report, but I think it was made to fix same issue as CVE-2023-20944 was about, but for cases where AccountManager.KEY_INTENT is launched by Activities other than ChooseTypeAndAccountActivity (for example AddAccountSettings, which I've missed when reporting bug first time). This change replaced use of typed bundle.getParcelable() use of untyped one and manual getClass() != Intent.class check, which actually reverted fix for CVE-2023-21098
• CVE-2023-45777 (bulletin, patch): This is second time I reported this exploit. Patch kept manual getClass() != Intent.class check, but in addition to that brought back use of typed bundle.getParcelable(), which is good way to fix both issues

While same exploit works for both CVE-2023-21098 and CVE-2023-45777, way it happened to bypass checkKeyIntentParceledCorrectly() differs

In case of CVE-2023-21098, checkKeyIntent() wasn't actually called if checked Bundle didn't have an Intent. As checkKeyIntent() is what calls checkKeyIntentParceledCorrectly(), in case when original Bundle didn't appear to contain an Intent, the Bundle after re-serialization wasn't checked

In case of CVE-2023-45777, checkKeyIntentParceledCorrectly() was correctly called, however writeBundle() happened there before getParcelable() call without type argument (until which Bundle didn't change its contents)