From 0206268a1224e60b4740ae44bafd0efc82775844 Mon Sep 17 00:00:00 2001 From: Bernardo Rufino Date: Wed, 9 Feb 2022 09:53:13 +0000 Subject: [PATCH] Safer @hide Bundle.getParcelable() with explicit type Provider a safer API getParcelable() that takes a Class parameter just like the safer Parcel APIs introduced, so we check the type before deserializing, preventing unexpected and potentially vulnerable code being executed (technique used in the bugs). Making it @hide since ASA requested this in T (more details on bugs) and the urgent usage is inside the platform, we can flesh out a public API for U. Test: App code gpaste/6130483466338304 logs gpaste/5148052949041152 Bug: 213169612 Bug: 212804042 Bug: 212803946 Bug: 210885162 Change-Id: Ieebc044043e0776e71d35c1cc11be9299f972c45 --- core/java/android/app/ActivityOptions.java | 2 +- core/java/android/os/BaseBundle.java | 52 ++++++++++++++-------- core/java/android/os/Bundle.java | 30 +++++++++++++ core/java/android/os/Parcel.java | 23 +++++----- 4 files changed, 77 insertions(+), 30 deletions(-) diff --git a/core/java/android/app/ActivityOptions.java b/core/java/android/app/ActivityOptions.java index e405b60c8daaf..e0b31d7bd84c6 100644 --- a/core/java/android/app/ActivityOptions.java +++ b/core/java/android/app/ActivityOptions.java @@ -1121,7 +1121,7 @@ public class ActivityOptions extends ComponentOptions { mPackageName = opts.getString(KEY_PACKAGE_NAME); try { - mUsageTimeReport = opts.getParcelable(KEY_USAGE_TIME_REPORT); + mUsageTimeReport = opts.getParcelable(KEY_USAGE_TIME_REPORT, PendingIntent.class); } catch (RuntimeException e) { Slog.w(TAG, e); } diff --git a/core/java/android/os/BaseBundle.java b/core/java/android/os/BaseBundle.java index ad3de25fecc23..244335d3aa067 100644 --- a/core/java/android/os/BaseBundle.java +++ b/core/java/android/os/BaseBundle.java @@ -31,7 +31,7 @@ import com.android.internal.util.IndentingPrintWriter; import java.io.Serializable; import java.util.ArrayList; import java.util.Set; -import java.util.function.Supplier; +import java.util.function.Function; /** * A mapping from String keys to values of various types. In most cases, you @@ -252,11 +252,10 @@ public class BaseBundle { if (size == 0) { return null; } - Object o = getValueAt(0); try { - return (String) o; - } catch (ClassCastException e) { - typeWarning("getPairValue()", o, "String", e); + return getValueAt(0, String.class); + } catch (ClassCastException | BadParcelableException e) { + typeWarning("getPairValue()", /* value */ null, "String", e); return null; } } @@ -309,7 +308,7 @@ public class BaseBundle { } for (int i = 0, n = mMap.size(); i < n; i++) { // Triggers deserialization of i-th item, if needed - getValueAt(i); + getValueAt(i, /* clazz */ null); } } } @@ -324,8 +323,21 @@ public class BaseBundle { * @hide */ final Object getValue(String key) { + return getValue(key, /* clazz */ null); + } + + /** + * Returns the value for key {@code key} for expected return type {@param clazz} (or {@code + * null} for no type check). + * + * This call should always be made after {@link #unparcel()} or inside a lock after making sure + * {@code mMap} is not null. + * + * @hide + */ + final T getValue(String key, @Nullable Class clazz) { int i = mMap.indexOfKey(key); - return (i >= 0) ? getValueAt(i) : null; + return (i >= 0) ? getValueAt(i, clazz) : null; } /** @@ -336,11 +348,12 @@ public class BaseBundle { * * @hide */ - final Object getValueAt(int i) { + @SuppressWarnings("unchecked") + final T getValueAt(int i, @Nullable Class clazz) { Object object = mMap.valueAt(i); - if (object instanceof Supplier) { + if (object instanceof Function) { try { - object = ((Supplier) object).get(); + object = ((Function, ?>) object).apply(clazz); } catch (BadParcelableException e) { if (sShouldDefuse) { Log.w(TAG, "Failed to parse item " + mMap.keyAt(i) + ", returning null.", e); @@ -351,7 +364,7 @@ public class BaseBundle { } mMap.setValueAt(i, object); } - return object; + return (clazz != null) ? clazz.cast(object) : (T) object; } private void initializeFromParcelLocked(@NonNull Parcel parcelledData, boolean recycleParcel, @@ -528,7 +541,7 @@ public class BaseBundle { } else { // Following semantic above of failing in case we get a serialized value vs a // deserialized one, we'll compare the map. If a certain element hasn't been - // deserialized yet, it's a Supplier (or more specifically a LazyValue, but let's + // deserialized yet, it's a function object (or more specifically a LazyValue, but let's // pretend we don't know that here :P), we'll use that element's equality comparison as // map naturally does. That will takes care of comparing the payload if needed (see // Parcel.readLazyValue() for details). @@ -982,15 +995,19 @@ public class BaseBundle { } // Log a message if the value was non-null but not of the expected type - void typeWarning(String key, Object value, String className, - Object defaultValue, ClassCastException e) { + void typeWarning(String key, @Nullable Object value, String className, + Object defaultValue, RuntimeException e) { StringBuilder sb = new StringBuilder(); sb.append("Key "); sb.append(key); sb.append(" expected "); sb.append(className); - sb.append(" but value was a "); - sb.append(value.getClass().getName()); + if (value != null) { + sb.append(" but value was a "); + sb.append(value.getClass().getName()); + } else { + sb.append(" but value was of a different type "); + } sb.append(". The default value "); sb.append(defaultValue); sb.append(" was returned."); @@ -998,8 +1015,7 @@ public class BaseBundle { Log.w(TAG, "Attempt to cast generated internal exception:", e); } - void typeWarning(String key, Object value, String className, - ClassCastException e) { + void typeWarning(String key, @Nullable Object value, String className, RuntimeException e) { typeWarning(key, value, className, "", e); } diff --git a/core/java/android/os/Bundle.java b/core/java/android/os/Bundle.java index b2bbfd6c163de..2b13f20ffe6dc 100644 --- a/core/java/android/os/Bundle.java +++ b/core/java/android/os/Bundle.java @@ -16,6 +16,9 @@ package android.os; +import static java.util.Objects.requireNonNull; + +import android.annotation.NonNull; import android.annotation.Nullable; import android.compat.annotation.UnsupportedAppUsage; import android.util.ArrayMap; @@ -912,6 +915,33 @@ public final class Bundle extends BaseBundle implements Cloneable, Parcelable { } } + /** + * Returns the value associated with the given key, or {@code null} if + * no mapping of the desired type exists for the given key or a {@code null} + * value is explicitly associated with the key. + * + *

Note: if the expected value is not a class provided by the Android platform, + * you must call {@link #setClassLoader(ClassLoader)} with the proper {@link ClassLoader} first. + * Otherwise, this method might throw an exception or return {@code null}. + * + * @param key a String, or {@code null} + * @param clazz The type of the object expected or {@code null} for performing no checks. + * @return a Parcelable value, or {@code null} + * + * @hide + */ + @SuppressWarnings("unchecked") + @Nullable + public T getParcelable(@Nullable String key, @NonNull Class clazz) { + unparcel(); + try { + return getValue(key, requireNonNull(clazz)); + } catch (ClassCastException | BadParcelableException e) { + typeWarning(key, /* value */ null, "Parcelable", e); + return null; + } + } + /** * Returns the value associated with the given key, or {@code null} if * no mapping of the desired type exists for the given key or a null diff --git a/core/java/android/os/Parcel.java b/core/java/android/os/Parcel.java index 9998e12066022..ae923530b6518 100644 --- a/core/java/android/os/Parcel.java +++ b/core/java/android/os/Parcel.java @@ -4319,18 +4319,19 @@ public final class Parcel { } /** - * This will return a {@link Supplier} for length-prefixed types that deserializes the object - * when {@link Supplier#get()} is called, for other types it will return the object itself. + * This will return a {@link Function} for length-prefixed types that deserializes the object + * when {@link Function#apply} is called with the expected class of the return object (or {@code + * null} for no type check), for other types it will return the object itself. * - *

After calling {@link Supplier#get()} the parcel cursor will not change. Note that you - * shouldn't recycle the parcel, not at least until all objects have been retrieved. No + *

After calling {@link Function#apply(Object)} the parcel cursor will not change. Note that + * you shouldn't recycle the parcel, not at least until all objects have been retrieved. No * synchronization attempts are made. * - *

The supplier returned implements {@link #equals(Object)} and {@link #hashCode()}. Two - * suppliers are equal if either of the following is true: + *

The function returned implements {@link #equals(Object)} and {@link #hashCode()}. Two + * function objects are equal if either of the following is true: *
    - *
  • {@link Supplier#get()} has been called on both and both objects returned are equal. - *
  • {@link Supplier#get()} hasn't been called on either one and everything below is true: + *
  • {@link Function#apply} has been called on both and both objects returned are equal. + *
  • {@link Function#apply} hasn't been called on either one and everything below is true: *
      *
    • The {@code loader} parameters used to retrieve each are equal. *
    • They both have the same type. @@ -4357,7 +4358,7 @@ public final class Parcel { } - private static final class LazyValue implements Supplier { + private static final class LazyValue implements Function, Object> { /** * | 4B | 4B | * mSource = Parcel{... | type | length | object | ...} @@ -4389,7 +4390,7 @@ public final class Parcel { } @Override - public Object get() { + public Object apply(@Nullable Class clazz) { Parcel source = mSource; if (source != null) { synchronized (source) { @@ -4398,7 +4399,7 @@ public final class Parcel { int restore = source.dataPosition(); try { source.setDataPosition(mPosition); - mObject = source.readValue(mLoader); + mObject = source.readValue(mLoader, clazz); } finally { source.setDataPosition(restore); }