FPII-2379 : Elevation of privilege vulnerability in Telephony CVE-2016-3914
A-30481342
MmsProvider.openFile validates the current_data column in the database (DB)
and then calls ContentProvider.openFileHelper, which reads from the DB again.
A race condition could cause the second DB read to read an updated, malicious value.
The fix is designed to call MmsProvider.safeOpenFileHelper instead of conducting
the first DB check and calling ContentProvider.openFileHelper.
Change-Id: Iee7dc3d8da17439be8c1c12a45b58cdb13fa71e0
diff --git a/src/com/android/providers/telephony/MmsProvider.java b/src/com/android/providers/telephony/MmsProvider.java
index a311788..da8f193 100644
--- a/src/com/android/providers/telephony/MmsProvider.java
+++ b/src/com/android/providers/telephony/MmsProvider.java
@@ -16,6 +16,7 @@
package com.android.providers.telephony;
+import android.annotation.NonNull;
import android.app.AppOpsManager;
import android.content.ContentProvider;
import android.content.ContentValues;
@@ -278,6 +279,11 @@
@Override
public Uri insert(Uri uri, ContentValues values) {
// Don't let anyone insert anything with the _data column
+ // The _data column is filled internally in MmsProvider, so this check is just to avoid
+ // it from being inadvertently set. This is not supposed to be a protection against
+ // malicious attack, since sql injection could still be attempted to bypass the check. On
+ // the other hand, the MmsProvider does verify that the _data column has an allowed value
+ // before opening any uri/files.
if (values != null && values.containsKey(Part._DATA)) {
return null;
}
@@ -702,9 +708,12 @@
}
@Override
- public int update(Uri uri, ContentValues values,
- String selection, String[] selectionArgs) {
- // Don't let anyone update the _data column
+ public int update(Uri uri, ContentValues values, String selection, String[] selectionArgs) {
+ // The _data column is filled internally in MmsProvider, so this check is just to avoid
+ // it from being inadvertently set. This is not supposed to be a protection against
+ // malicious attack, since sql injection could still be attempted to bypass the check. On
+ // the other hand, the MmsProvider does verify that the _data column has an allowed value
+ // before opening any uri/files.
if (values != null && values.containsKey(Part._DATA)) {
return 0;
}
@@ -811,6 +820,12 @@
}
// Verify that the _data path points to mms data
+ return safeOpenFileHelper(uri, mode);
+ }
+
+ @NonNull
+ private ParcelFileDescriptor safeOpenFileHelper(
+ @NonNull Uri uri, @NonNull String mode) throws FileNotFoundException {
Cursor c = query(uri, new String[]{"_data"}, null, null, null);
int count = (c != null) ? c.getCount() : 0;
if (count != 1) {
@@ -831,10 +846,15 @@
c.close();
if (path == null) {
- return null;
+ throw new FileNotFoundException("Column _data not found.");
}
+ File filePath = new File(path);
try {
- File filePath = new File(path);
+ // The MmsProvider shouldn't open a file that isn't MMS data, so we verify that the
+ // _data path actually points to MMS data. That safeguards ourselves from callers who
+ // inserted or updated a URI (more specifically the _data column) with disallowed paths.
+ // TODO(afurtado): provide a more robust mechanism to avoid disallowed _data paths to
+ // be inserted/updated in the first place, including via SQL injection.
if (!filePath.getCanonicalPath()
.startsWith(getContext().getDir(PARTS_DIR_NAME, 0).getPath())) {
Log.e(TAG, "openFile: path "
@@ -842,7 +862,6 @@
+ " does not start with "
+ getContext().getDir(PARTS_DIR_NAME, 0).getPath());
// Don't care return value
- filePath.delete();
return null;
}
} catch (IOException e) {
@@ -850,7 +869,8 @@
return null;
}
- return openFileHelper(uri, mode);
+ int modeBits = ParcelFileDescriptor.parseMode(mode);
+ return ParcelFileDescriptor.open(filePath, modeBits);
}
private void filterUnsupportedKeys(ContentValues values) {