diff options
author | Andre Furtado <afurtado@google.com> | 2016-09-09 14:45:20 -0700 |
---|---|---|
committer | Jessica Wagantall <jwagantall@cyngn.com> | 2016-10-04 16:13:04 -0700 |
commit | 24837c13e01562518767957810b2f0ec3b74618d (patch) | |
tree | a8d9d3da6e72800529fd5f78b6bd08a8bf1ae9c7 | |
parent | ce96f1c4a3bf2253736ac5438095b79860a3e8dc (diff) | |
download | android_packages_providers_TelephonyProvider-stable/cm-13.0-ZNH2KB.tar.gz android_packages_providers_TelephonyProvider-stable/cm-13.0-ZNH2KB.tar.bz2 android_packages_providers_TelephonyProvider-stable/cm-13.0-ZNH2KB.zip |
30481342: Security Vulnerability - TOCTOU in MmsProviderstable/cm-13.0-ZNH2KB
allows access to files as phone (radio) uid
Problem: MmsProvider.openFile validated the current _data column
in the DB and then called ContentProvider.openFileHelper which was again
reading from the DB. A race condition could cause the second DB read to
read an updated, malicious value.
Fix: instead of doing the first DB check and calling
ContentProvider.openFileHelper, we're now just calling
MmsProvider.safeOpenFileHelper which does a single check.
Test: used the POC provided for this incident.
CYNGNOS-3286
b/30481342
Change-Id: I653129359130b9fae59d4c355320b266c158a698
(cherry picked from commit 5bc7f9682d72c89ba252be6471b2db9b7e7815e3)
(cherry picked from commit ae6fcd48cf70318c38655f5dbecc2b6d1398ea99)
(cherry picked from commit d68da723f73658838df9036c0959a0969455b675)
-rw-r--r-- | src/com/android/providers/telephony/MmsProvider.java | 37 |
1 files changed, 28 insertions, 9 deletions
diff --git a/src/com/android/providers/telephony/MmsProvider.java b/src/com/android/providers/telephony/MmsProvider.java index 9bb94eb..ccd9ab1 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.ContentUris; @@ -493,7 +494,11 @@ public class MmsProvider extends ContentProvider { @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; } @@ -939,9 +944,12 @@ public class MmsProvider extends ContentProvider { } @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; } @@ -1047,7 +1055,12 @@ public class MmsProvider extends ContentProvider { return null; } - // 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) { @@ -1068,10 +1081,16 @@ public class MmsProvider extends ContentProvider { 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).getCanonicalPath())) { Log.e(TAG, "openFile: path " @@ -1079,7 +1098,6 @@ public class MmsProvider extends ContentProvider { + " does not start with " + getContext().getDir(PARTS_DIR_NAME, 0).getCanonicalPath()); // Don't care return value - filePath.delete(); return null; } } catch (IOException e) { @@ -1087,7 +1105,8 @@ public class MmsProvider extends ContentProvider { return null; } - return openFileHelper(uri, mode); + int modeBits = ParcelFileDescriptor.parseMode(mode); + return ParcelFileDescriptor.open(filePath, modeBits); } private void filterUnsupportedKeys(ContentValues values) { |