Skip to content

Commit

Permalink
Fix issue #16794553: Duplicate ArrayMap entries in Bundle...
Browse files Browse the repository at this point in the history
...can lead to launching of un-exported activities

We now validate the array map after unparcelling to make sure there
are no duplicate keys.

And to make up for the performance overhead this introduces, I switched
the parcelling/unparcelling code to write keys as explicit string
objects rather than generic values.  There was no reason to use generic
values since the write method itself only accepts an array map with
String keys.

Change-Id: I57bda9eb79ceaaa9c1b94ad49d9e462b52102149
  • Loading branch information
Dianne Hackborn authored and andi34 committed Jun 7, 2016
1 parent f702e7c commit f415bf5
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 4 deletions.
27 changes: 23 additions & 4 deletions core/java/android/os/Parcel.java
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -599,7 +599,7 @@ public final void writeMap(Map val) {
* Flatten an ArrayMap into the parcel at the current dataPosition(), * Flatten an ArrayMap into the parcel at the current dataPosition(),
* growing dataCapacity() if needed. The Map keys must be String objects. * growing dataCapacity() if needed. The Map keys must be String objects.
*/ */
/* package */ void writeArrayMapInternal(ArrayMap<String,Object> val) { /* package */ void writeArrayMapInternal(ArrayMap<String, Object> val) {
if (val == null) { if (val == null) {
writeInt(-1); writeInt(-1);
return; return;
Expand All @@ -614,7 +614,7 @@ public final void writeMap(Map val) {
int startPos; int startPos;
for (int i=0; i<N; i++) { for (int i=0; i<N; i++) {
if (DEBUG_ARRAY_MAP) startPos = dataPosition(); if (DEBUG_ARRAY_MAP) startPos = dataPosition();
writeValue(val.keyAt(i)); writeString(val.keyAt(i));
writeValue(val.valueAt(i)); writeValue(val.valueAt(i));
if (DEBUG_ARRAY_MAP) Log.d(TAG, " Write #" + i + " " if (DEBUG_ARRAY_MAP) Log.d(TAG, " Write #" + i + " "
+ (dataPosition()-startPos) + " bytes: key=0x" + (dataPosition()-startPos) + " bytes: key=0x"
Expand All @@ -623,6 +623,13 @@ public final void writeMap(Map val) {
} }
} }


/**
* @hide For testing only.
*/
public void writeArrayMap(ArrayMap<String, Object> val) {
writeArrayMapInternal(val);
}

/** /**
* Flatten a Bundle into the parcel at the current dataPosition(), * Flatten a Bundle into the parcel at the current dataPosition(),
* growing dataCapacity() if needed. * growing dataCapacity() if needed.
Expand Down Expand Up @@ -2310,14 +2317,15 @@ protected void finalize() throws Throwable {
int startPos; int startPos;
while (N > 0) { while (N > 0) {
if (DEBUG_ARRAY_MAP) startPos = dataPosition(); if (DEBUG_ARRAY_MAP) startPos = dataPosition();
Object key = readValue(loader); String key = readString();
Object value = readValue(loader); Object value = readValue(loader);
if (DEBUG_ARRAY_MAP) Log.d(TAG, " Read #" + (N-1) + " " if (DEBUG_ARRAY_MAP) Log.d(TAG, " Read #" + (N-1) + " "
+ (dataPosition()-startPos) + " bytes: key=0x" + (dataPosition()-startPos) + " bytes: key=0x"
+ Integer.toHexString((key != null ? key.hashCode() : 0)) + " " + key); + Integer.toHexString((key != null ? key.hashCode() : 0)) + " " + key);
outVal.append(key, value); outVal.append(key, value);
N--; N--;
} }
outVal.validate();
} }


/* package */ void readArrayMapSafelyInternal(ArrayMap outVal, int N, /* package */ void readArrayMapSafelyInternal(ArrayMap outVal, int N,
Expand All @@ -2328,7 +2336,7 @@ protected void finalize() throws Throwable {
Log.d(TAG, "Reading safely " + N + " ArrayMap entries", here); Log.d(TAG, "Reading safely " + N + " ArrayMap entries", here);
} }
while (N > 0) { while (N > 0) {
Object key = readValue(loader); String key = readString();
if (DEBUG_ARRAY_MAP) Log.d(TAG, " Read safe #" + (N-1) + ": key=0x" if (DEBUG_ARRAY_MAP) Log.d(TAG, " Read safe #" + (N-1) + ": key=0x"
+ (key != null ? key.hashCode() : 0) + " " + key); + (key != null ? key.hashCode() : 0) + " " + key);
Object value = readValue(loader); Object value = readValue(loader);
Expand All @@ -2337,6 +2345,17 @@ protected void finalize() throws Throwable {
} }
} }


/**
* @hide For testing only.
*/
public void readArrayMap(ArrayMap outVal, ClassLoader loader) {
final int N = readInt();
if (N < 0) {
return;
}
readArrayMapInternal(outVal, N, loader);
}

private void readListInternal(List outVal, int N, private void readListInternal(List outVal, int N,
ClassLoader loader) { ClassLoader loader) {
while (N > 0) { while (N > 0) {
Expand Down
38 changes: 38 additions & 0 deletions core/java/android/util/ArrayMap.java
Original file line number Original file line Diff line number Diff line change
Expand Up @@ -493,6 +493,44 @@ public void append(K key, V value) {
mArray[index+1] = value; mArray[index+1] = value;
} }


/**
* The use of the {@link #append} function can result in invalid array maps, in particular
* an array map where the same key appears multiple times. This function verifies that
* the array map is valid, throwing IllegalArgumentException if a problem is found. The
* main use for this method is validating an array map after unpacking from an IPC, to
* protect against malicious callers.
* @hide
*/
public void validate() {
final int N = mSize;
if (N <= 1) {
// There can't be dups.
return;
}
int basehash = mHashes[0];
int basei = 0;
for (int i=1; i<N; i++) {
int hash = mHashes[i];
if (hash != basehash) {
basehash = hash;
basei = i;
continue;
}
// We are in a run of entries with the same hash code. Go backwards through
// the array to see if any keys are the same.
final Object cur = mArray[i<<1];
for (int j=i-1; j>=basei; j--) {
final Object prev = mArray[j<<1];
if (cur == prev) {
throw new IllegalArgumentException("Duplicate key in ArrayMap: " + cur);
}
if (cur != null && prev != null && cur.equals(prev)) {
throw new IllegalArgumentException("Duplicate key in ArrayMap: " + cur);
}
}
}
}

/** /**
* Perform a {@link #put(Object, Object)} of all key/value pairs in <var>array</var> * Perform a {@link #put(Object, Object)} of all key/value pairs in <var>array</var>
* @param array The array whose contents are to be retrieved. * @param array The array whose contents are to be retrieved.
Expand Down

0 comments on commit f415bf5

Please sign in to comment.