From ab985cdbc7ed847feecd0bdbb8d81b2249c7da6a Mon Sep 17 00:00:00 2001
From: Andre Furtado <afurtado@google.com>
Date: Fri, 19 Aug 2016 16:50:02 -0700
Subject: 30481342: Security Vulnerability - TOCTOU in MmsProvider allows
 access to files as phone (radio) uid - DO NOT MERGE

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.

b/30481342

Change-Id: I643ad76bdbbbc68c4b7dbd18f7e76021396d5ed8
(cherry picked from commit fd9be3377ffeb967601985a195ab9fe9cfd94872)
---
 .../android/providers/telephony/MmsProvider.java   | 32 ++++++++++++++++++----
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/src/com/android/providers/telephony/MmsProvider.java b/src/com/android/providers/telephony/MmsProvider.java
index 2a6784a..492278f 100644
--- a/src/com/android/providers/telephony/MmsProvider.java
+++ b/src/com/android/providers/telephony/MmsProvider.java
@@ -271,7 +271,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;
         }
@@ -727,7 +731,11 @@ 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
+        // 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;
         }
@@ -826,7 +834,11 @@ public class MmsProvider extends ContentProvider {
             return null;
         }
 
-        // Verify that the _data path points to mms data
+            return safeOpenFileHelper(uri, mode);
+        }
+
+    private ParcelFileDescriptor safeOpenFileHelper(
+            Uri uri, String mode) throws FileNotFoundException {
         Cursor c = query(uri, new String[]{"_data"}, null, null, null);
         int count = (c != null) ? c.getCount() : 0;
         if (count != 1) {
@@ -847,10 +859,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.
             File appDataDirPath = new File(getContext().getApplicationInfo().dataDir
                     + "/app_parts/");
             // use canonical path to determin whether two paths point to the
@@ -862,10 +880,12 @@ public class MmsProvider extends ContentProvider {
                 return null;
             }
         } catch (IOException e) {
+            Log.e(TAG, "openFile: create path failed " + e, e);
             return null;
         }
 
-        return openFileHelper(uri, mode);
+        int modeBits = ParcelFileDescriptor.parseMode(mode);
+        return ParcelFileDescriptor.open(filePath, modeBits);
     }
 
     private void filterUnsupportedKeys(ContentValues values) {
-- 
cgit v1.2.3