From f741c3727383008b131cd3877cbdb3857e07fc9b Mon Sep 17 00:00:00 2001 From: Adam Lesinski Date: Fri, 7 Nov 2014 11:26:14 -0800 Subject: Fix ParceledListSlice to enforce the same concrete types among its elements. Bug:17671747 Change-Id: I896f75738e5b464ccb6c03290f139cc2fa72f966 (cherry picked from commit 3df1c38ee098872352086e03d6f1adb16796ee29) --- .../java/android/content/pm/ParceledListSlice.java | 47 +++- .../android/content/pm/ParceledListSliceTest.java | 263 +++++++++++++++++++++ 2 files changed, 306 insertions(+), 4 deletions(-) create mode 100644 core/tests/coretests/src/android/content/pm/ParceledListSliceTest.java diff --git a/core/java/android/content/pm/ParceledListSlice.java b/core/java/android/content/pm/ParceledListSlice.java index 8a43472..335a45e 100644 --- a/core/java/android/content/pm/ParceledListSlice.java +++ b/core/java/android/content/pm/ParceledListSlice.java @@ -30,6 +30,12 @@ import java.util.List; * Transfer a large list of Parcelable objects across an IPC. Splits into * multiple transactions if needed. * + * Caveat: for efficiency and security, all elements must be the same concrete type. + * In order to avoid writing the class name of each object, we must ensure that + * each object is the same type, or else unparceling then reparceling the data may yield + * a different result if the class name encoded in the Parcelable is a Base type. + * See b/17671747. + * * @hide */ public class ParceledListSlice implements Parcelable { @@ -56,13 +62,25 @@ public class ParceledListSlice implements Parcelable { if (N <= 0) { return; } + Parcelable.Creator creator = p.readParcelableCreator(loader); + Class listElementClass = null; + int i = 0; while (i < N) { if (p.readInt() == 0) { break; } - mList.add(p.readCreator(creator, loader)); + + final T parcelable = p.readCreator(creator, loader); + if (listElementClass == null) { + listElementClass = parcelable.getClass(); + } else { + verifySameType(listElementClass, parcelable.getClass()); + } + + mList.add(parcelable); + if (DEBUG) Log.d(TAG, "Read inline #" + i + ": " + mList.get(mList.size()-1)); i++; } @@ -82,7 +100,11 @@ public class ParceledListSlice implements Parcelable { return; } while (i < N && reply.readInt() != 0) { - mList.add(reply.readCreator(creator, loader)); + final T parcelable = reply.readCreator(creator, loader); + verifySameType(listElementClass, parcelable.getClass()); + + mList.add(parcelable); + if (DEBUG) Log.d(TAG, "Read extra #" + i + ": " + mList.get(mList.size()-1)); i++; } @@ -91,6 +113,14 @@ public class ParceledListSlice implements Parcelable { } } + private static void verifySameType(final Class expected, final Class actual) { + if (!actual.equals(expected)) { + throw new IllegalArgumentException("Can't unparcel type " + + actual.getName() + " in list of type " + + expected.getName()); + } + } + public List getList() { return mList; } @@ -116,11 +146,16 @@ public class ParceledListSlice implements Parcelable { dest.writeInt(N); if (DEBUG) Log.d(TAG, "Writing " + N + " items"); if (N > 0) { + final Class listElementClass = mList.get(0).getClass(); dest.writeParcelableCreator(mList.get(0)); int i = 0; while (i < N && dest.dataSize() < MAX_FIRST_IPC_SIZE) { dest.writeInt(1); - mList.get(i).writeToParcel(dest, callFlags); + + final T parcelable = mList.get(i); + verifySameType(listElementClass, parcelable.getClass()); + parcelable.writeToParcel(dest, callFlags); + if (DEBUG) Log.d(TAG, "Wrote inline #" + i + ": " + mList.get(i)); i++; } @@ -137,7 +172,11 @@ public class ParceledListSlice implements Parcelable { if (DEBUG) Log.d(TAG, "Writing more @" + i + " of " + N); while (i < N && reply.dataSize() < MAX_IPC_SIZE) { reply.writeInt(1); - mList.get(i).writeToParcel(reply, callFlags); + + final T parcelable = mList.get(i); + verifySameType(listElementClass, parcelable.getClass()); + parcelable.writeToParcel(reply, callFlags); + if (DEBUG) Log.d(TAG, "Wrote extra #" + i + ": " + mList.get(i)); i++; } diff --git a/core/tests/coretests/src/android/content/pm/ParceledListSliceTest.java b/core/tests/coretests/src/android/content/pm/ParceledListSliceTest.java new file mode 100644 index 0000000..e7f4bad --- /dev/null +++ b/core/tests/coretests/src/android/content/pm/ParceledListSliceTest.java @@ -0,0 +1,263 @@ +package android.content.pm; + +import android.os.Parcel; +import android.os.Parcelable; +import junit.framework.TestCase; + +import java.util.ArrayList; +import java.util.List; + +public class ParceledListSliceTest extends TestCase { + + public void testSmallList() throws Exception { + final int objectCount = 100; + List list = new ArrayList<>(); + for (int i = 0; i < objectCount; i++) { + list.add(new SmallObject(i * 2, (i * 2) + 1)); + } + + ParceledListSlice slice; + + Parcel parcel = Parcel.obtain(); + try { + parcel.writeParcelable(new ParceledListSlice<>(list), 0); + parcel.setDataPosition(0); + slice = parcel.readParcelable(getClass().getClassLoader()); + } finally { + parcel.recycle(); + } + + assertNotNull(slice); + assertNotNull(slice.getList()); + assertEquals(objectCount, slice.getList().size()); + + for (int i = 0; i < objectCount; i++) { + assertEquals(i * 2, slice.getList().get(i).mFieldA); + assertEquals((i * 2) + 1, slice.getList().get(i).mFieldB); + } + } + + private static int measureLargeObject() { + Parcel p = Parcel.obtain(); + try { + new LargeObject(0, 0, 0, 0, 0).writeToParcel(p, 0); + return p.dataPosition(); + } finally { + p.recycle(); + } + } + + /** + * Test that when the list is large, the data is successfully parceled + * and unparceled (the implementation will send pieces of the list in + * separate round-trips to avoid the IPC limit). + */ + public void testLargeList() throws Exception { + final int thresholdBytes = 256 * 1024; + final int objectCount = thresholdBytes / measureLargeObject(); + + List list = new ArrayList<>(); + for (int i = 0; i < objectCount; i++) { + list.add(new LargeObject( + i * 5, + (i * 5) + 1, + (i * 5) + 2, + (i * 5) + 3, + (i * 5) + 4 + )); + } + + ParceledListSlice slice; + + Parcel parcel = Parcel.obtain(); + try { + parcel.writeParcelable(new ParceledListSlice<>(list), 0); + parcel.setDataPosition(0); + slice = parcel.readParcelable(getClass().getClassLoader()); + } finally { + parcel.recycle(); + } + + assertNotNull(slice); + assertNotNull(slice.getList()); + assertEquals(objectCount, slice.getList().size()); + + for (int i = 0; i < objectCount; i++) { + assertEquals(i * 5, slice.getList().get(i).mFieldA); + assertEquals((i * 5) + 1, slice.getList().get(i).mFieldB); + assertEquals((i * 5) + 2, slice.getList().get(i).mFieldC); + assertEquals((i * 5) + 3, slice.getList().get(i).mFieldD); + assertEquals((i * 5) + 4, slice.getList().get(i).mFieldE); + } + } + + /** + * Test that only homogeneous elements may be unparceled. + */ + public void testHomogeneousElements() throws Exception { + List list = new ArrayList<>(); + list.add(new LargeObject(0, 1, 2, 3, 4)); + list.add(new SmallObject(5, 6)); + list.add(new SmallObject(7, 8)); + + Parcel parcel = Parcel.obtain(); + try { + writeEvilParceledListSlice(parcel, list); + parcel.setDataPosition(0); + try { + ParceledListSlice.CREATOR.createFromParcel(parcel, getClass().getClassLoader()); + assertTrue("Unparceled heterogeneous ParceledListSlice", false); + } catch (IllegalArgumentException e) { + // Success, we're not allowed to process heterogeneous + // elements in a ParceledListSlice. + } + } finally { + parcel.recycle(); + } + } + + /** + * Write a ParcelableListSlice that uses the BaseObject base class as the Creator. + * This is dangerous, as it may affect how the data is unparceled, then later parceled + * by the system, leading to a self-modifying data security vulnerability. + */ + private static void writeEvilParceledListSlice(Parcel dest, List list) { + final int listCount = list.size(); + + // Number of items. + dest.writeInt(listCount); + + // The type/creator to use when unparceling. Here we use the base class + // to simulate an attack on ParceledListSlice. + dest.writeString(BaseObject.class.getName()); + + for (int i = 0; i < listCount; i++) { + // 1 means the item is present. + dest.writeInt(1); + list.get(i).writeToParcel(dest, 0); + } + } + + public abstract static class BaseObject implements Parcelable { + protected static final int TYPE_SMALL = 0; + protected static final int TYPE_LARGE = 1; + + protected void writeToParcel(Parcel dest, int flags, int type) { + dest.writeInt(type); + } + + @Override + public int describeContents() { + return 0; + } + + /** + * This is *REALLY* bad, but we're doing it in the test to ensure that we handle + * the possible exploit when unparceling an object with the BaseObject written as + * Creator. + */ + public static final Creator CREATOR = new Creator() { + @Override + public BaseObject createFromParcel(Parcel source) { + switch (source.readInt()) { + case TYPE_SMALL: + return SmallObject.createFromParcelBody(source); + case TYPE_LARGE: + return LargeObject.createFromParcelBody(source); + default: + throw new IllegalArgumentException("Unknown type"); + } + } + + @Override + public BaseObject[] newArray(int size) { + return new BaseObject[size]; + } + }; + } + + public static class SmallObject extends BaseObject { + public int mFieldA; + public int mFieldB; + + public SmallObject(int a, int b) { + mFieldA = a; + mFieldB = b; + } + + @Override + public void writeToParcel(Parcel dest, int flags) { + super.writeToParcel(dest, flags, TYPE_SMALL); + dest.writeInt(mFieldA); + dest.writeInt(mFieldB); + } + + public static SmallObject createFromParcelBody(Parcel source) { + return new SmallObject(source.readInt(), source.readInt()); + } + + public static final Creator CREATOR = new Creator() { + @Override + public SmallObject createFromParcel(Parcel source) { + // Consume the type (as it is always written out). + source.readInt(); + return createFromParcelBody(source); + } + + @Override + public SmallObject[] newArray(int size) { + return new SmallObject[size]; + } + }; + } + + public static class LargeObject extends BaseObject { + public int mFieldA; + public int mFieldB; + public int mFieldC; + public int mFieldD; + public int mFieldE; + + public LargeObject(int a, int b, int c, int d, int e) { + mFieldA = a; + mFieldB = b; + mFieldC = c; + mFieldD = d; + mFieldE = e; + } + + @Override + public void writeToParcel(Parcel dest, int flags) { + super.writeToParcel(dest, flags, TYPE_LARGE); + dest.writeInt(mFieldA); + dest.writeInt(mFieldB); + dest.writeInt(mFieldC); + dest.writeInt(mFieldD); + dest.writeInt(mFieldE); + } + + public static LargeObject createFromParcelBody(Parcel source) { + return new LargeObject( + source.readInt(), + source.readInt(), + source.readInt(), + source.readInt(), + source.readInt() + ); + } + + public static final Creator CREATOR = new Creator() { + @Override + public LargeObject createFromParcel(Parcel source) { + // Consume the type (as it is always written out). + source.readInt(); + return createFromParcelBody(source); + } + + @Override + public LargeObject[] newArray(int size) { + return new LargeObject[size]; + } + }; + } +} -- cgit v1.1