Preventing deleting file that does not belong to parts directory. b/414388731#comment18. I'm adding this change to err on the side of caution, but it may be redundant given the recent sql injection protection changes. bug: 414388731 Flag: EXEMPT bugfix test: new test + existing coverage from testMmsPartDelete_canDeleteById. (cherry picked from commit fce8998a1770d05a768c30e5822c25db1b758208) Cherrypick-From: https://googleplex-android-review.googlesource.com/q/commit:9ce5c9d9bd63c7db1ba5ff3e3e834e03b00d0fff Merged-In: Ic2072a12bf93be7c17a86b372ae7e7057d7eb237 Change-Id: Ic2072a12bf93be7c17a86b372ae7e7057d7eb237
diff --git a/src/com/android/providers/telephony/MmsProvider.java b/src/com/android/providers/telephony/MmsProvider.java index 050826d..af7e44e 100644 --- a/src/com/android/providers/telephony/MmsProvider.java +++ b/src/com/android/providers/telephony/MmsProvider.java
@@ -805,6 +805,8 @@ } int deletedRows = 0; + Context context = getContext(); + final long token = Binder.clearCallingIdentity(); String selectionBySubIds; try { @@ -821,12 +823,12 @@ finalSelection = DatabaseUtils.concatenateWhere(finalSelection, selectionBySubIds); if (TABLE_PDU.equals(table)) { - deletedRows = deleteMessages(getContext(), db, finalSelection, + deletedRows = deleteMessages(context, db, finalSelection, selectionArgs, uri); } else if (TABLE_PART.equals(table)) { - deletedRows = deleteParts(db, finalSelection, selectionArgs); + deletedRows = deleteParts(context, db, finalSelection, selectionArgs); } else if (TABLE_DRM.equals(table)) { - deletedRows = deleteTempDrmData(db, finalSelection, selectionArgs); + deletedRows = deleteTempDrmData(context, db, finalSelection, selectionArgs); } else { deletedRows = db.delete(table, finalSelection, selectionArgs); } @@ -851,7 +853,7 @@ } while (cursor.moveToNext()) { - deleteParts(db, Part.MSG_ID + " = ?", + deleteParts(context, db, Part.MSG_ID + " = ?", new String[] { String.valueOf(cursor.getLong(0)) }); } } finally { @@ -870,17 +872,17 @@ return count; } - private static int deleteParts(SQLiteDatabase db, String selection, + private static int deleteParts(Context context, SQLiteDatabase db, String selection, String[] selectionArgs) { - return deleteDataRows(db, TABLE_PART, selection, selectionArgs); + return deleteDataRows(context, db, TABLE_PART, selection, selectionArgs); } - private static int deleteTempDrmData(SQLiteDatabase db, String selection, + private static int deleteTempDrmData(Context context, SQLiteDatabase db, String selection, String[] selectionArgs) { - return deleteDataRows(db, TABLE_DRM, selection, selectionArgs); + return deleteDataRows(context, db, TABLE_DRM, selection, selectionArgs); } - private static int deleteDataRows(SQLiteDatabase db, String table, + private static int deleteDataRows(Context context, SQLiteDatabase db, String table, String selection, String[] selectionArgs) { Cursor cursor = db.query(table, new String[] { "_data" }, selection, selectionArgs, null, null, null); @@ -899,9 +901,25 @@ try { // Delete the associated files saved on file-system. String path = cursor.getString(0); - if (path != null) { - new File(path).delete(); + if (path == null) { + continue; } + // Check if the path is canonical and belongs to the parts directory. + File file = new File(path); + boolean isPathCanonical = isFilePathCanonical(context, file); + if (!isPathCanonical) { + Log.e( + TAG, + "deleteFile: path " + + file.getCanonicalPath() + + " does not start with " + + context + .getDir(PARTS_DIR_NAME, 0) + .getCanonicalPath()); + continue; + } + + file.delete(); } catch (Throwable ex) { Log.e(TAG, ex.getMessage(), ex); } @@ -913,6 +931,20 @@ return db.delete(table, selection, selectionArgs); } + @VisibleForTesting + public static boolean isFilePathCanonical(Context context, File file) { + try { + return file.getCanonicalPath() + .startsWith( + context + .getDir(PARTS_DIR_NAME, 0) + .getCanonicalPath()); + } catch (IOException e) { + Log.e(TAG, "Exception in context#getDir#getCanonicalPath: " + e); + return false; + } + } + @Override 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 @@ -1087,14 +1119,13 @@ } File filePath = new File(path); - try { + try { // 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())) { + if (!isFilePathCanonical(getContext(), filePath)) { Log.e(TAG, "openFile: path " + filePath.getCanonicalPath() + " does not start with " @@ -1107,6 +1138,7 @@ return null; } + int modeBits = ParcelFileDescriptor.parseMode(mode); return ParcelFileDescriptor.open(filePath, modeBits); }
diff --git a/tests/src/com/android/providers/telephony/MmsProviderTest.java b/tests/src/com/android/providers/telephony/MmsProviderTest.java index 2e618b0..db8fc51 100644 --- a/tests/src/com/android/providers/telephony/MmsProviderTest.java +++ b/tests/src/com/android/providers/telephony/MmsProviderTest.java
@@ -35,6 +35,11 @@ import android.telephony.TelephonyManager; import android.test.mock.MockContentResolver; import android.util.Log; +import java.io.File; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import junit.framework.TestCase; @@ -42,12 +47,17 @@ public class MmsProviderTest extends TestCase { private static final String TAG = "MmsProviderTest"; + private static final String PARTS_DIR_NAME = "parts"; + private File testDir; + private File partsDir; private MockContentResolver mContentResolver; private MmsProviderTestable mMmsProviderTestable; private int notifyChangeCount; + private Context context; + @Override protected void setUp() throws Exception { super.setUp(); @@ -55,7 +65,7 @@ mMmsProviderTestable = new MmsProviderTestable(); // setup mocks - Context context = mock(Context.class); + context = mock(Context.class); PackageManager packageManager = mock(PackageManager.class); Resources resources = mock(Resources.class); when(context.getSystemService(eq(Context.APP_OPS_SERVICE))) @@ -98,12 +108,27 @@ mContentResolver.addProvider("mms", mMmsProviderTestable); Log.d(TAG, "MockContextWithProvider: Add MmsProvider to mResolver"); notifyChangeCount = 0; + + //Setup test directory + testDir = Files.createTempDirectory("testDir").toFile(); + partsDir = new File(testDir, PARTS_DIR_NAME); + partsDir.mkdirs(); // Create the parts directory } @Override protected void tearDown() throws Exception { super.tearDown(); mMmsProviderTestable.closeDatabase(); + + if (testDir != null && testDir.exists()) { + File[] files = testDir.listFiles(); + if (files != null) { + for (File file : files) { + file.delete(); + } + } + testDir.delete(); + } } @Test @@ -118,6 +143,32 @@ } @Test + public void testDeleteDataRows_pathStartsWithPartsDirectory_canDeleteFile() throws Exception { + // Create a file in the parts directory. + when(context.getDir(eq(PARTS_DIR_NAME), anyInt())).thenReturn(partsDir); + File validFile = new File(partsDir, "mms_part.txt"); + validFile.createNewFile(); + + boolean canDeleteFile = MmsProviderTestable.isFilePathCanonical( + context, validFile); + + assertTrue(canDeleteFile); + } + + @Test + public void testDeleteDataRows_invalidPath_cannotDeleteFile() throws Exception { + // Create a file in the test directory, which is not under the parts directory. + when(context.getDir(eq(PARTS_DIR_NAME), anyInt())).thenReturn(partsDir); + File invalidFile = new File(testDir, "not_mms.txt"); + invalidFile.createNewFile(); + + boolean canDeleteFile = MmsProviderTestable.isFilePathCanonical( + context, invalidFile); + + assertFalse(canDeleteFile); + } + + @Test public void testInsertMmsWithoutNotify() { MmsProvider.ProviderUtilWrapper providerUtilWrapper =