From 1325574d3a72fe2f8dba234ef04045565ab1aacb Mon Sep 17 00:00:00 2001 From: Oli Lan Date: Wed, 23 Feb 2022 13:12:20 +0000 Subject: [PATCH] Prevent exfiltration of system files via avatar picker. This adds mitigations to prevent system files being exfiltrated via the settings content provider when a content URI is provided as a chosen user image. The mitigations are: 1) Copy the image to a new URI rather than the existing takePictureUri prior to cropping. 2) Only allow a system handler to respond to the CROP intent. Bug: 187702830 Test: atest AvatarPhotoControllerTest Change-Id: Idf1ab60878d619ee30505d71e8afe31d8b0c0ebe --- .../users/AvatarPhotoController.java | 61 +++++---- .../users/AvatarPhotoControllerTest.java | 122 +++++++++--------- 2 files changed, 100 insertions(+), 83 deletions(-) diff --git a/packages/SettingsLib/src/com/android/settingslib/users/AvatarPhotoController.java b/packages/SettingsLib/src/com/android/settingslib/users/AvatarPhotoController.java index e662776ed2bd6..0cb2c0b22a4c5 100644 --- a/packages/SettingsLib/src/com/android/settingslib/users/AvatarPhotoController.java +++ b/packages/SettingsLib/src/com/android/settingslib/users/AvatarPhotoController.java @@ -21,6 +21,8 @@ import android.content.ClipData; import android.content.ContentResolver; import android.content.Context; import android.content.Intent; +import android.content.pm.ActivityInfo; +import android.content.pm.PackageManager; import android.graphics.Bitmap; import android.graphics.BitmapFactory; import android.graphics.Canvas; @@ -57,9 +59,9 @@ class AvatarPhotoController { void startActivityForResult(Intent intent, int resultCode); - int getPhotoSize(); + boolean startSystemActivityForResult(Intent intent, int resultCode); - boolean canCropPhoto(); + int getPhotoSize(); } interface ContextInjector { @@ -77,6 +79,7 @@ class AvatarPhotoController { static final int REQUEST_CODE_CROP_PHOTO = 1003; private static final String IMAGES_DIR = "multi_user"; + private static final String PRE_CROP_PICTURE_FILE_NAME = "PreCropEditUserPhoto.jpg"; private static final String CROP_PICTURE_FILE_NAME = "CropEditUserPhoto.jpg"; private static final String TAKE_PICTURE_FILE_NAME = "TakeEditUserPhoto.jpg"; @@ -86,6 +89,7 @@ class AvatarPhotoController { private final ContextInjector mContextInjector; private final File mImagesDir; + private final Uri mPreCropPictureUri; private final Uri mCropPictureUri; private final Uri mTakePictureUri; @@ -95,6 +99,8 @@ class AvatarPhotoController { mImagesDir = new File(mContextInjector.getCacheDir(), IMAGES_DIR); mImagesDir.mkdir(); + mPreCropPictureUri = mContextInjector + .createTempImageUri(mImagesDir, PRE_CROP_PICTURE_FILE_NAME, !waiting); mCropPictureUri = mContextInjector.createTempImageUri(mImagesDir, CROP_PICTURE_FILE_NAME, !waiting); mTakePictureUri = @@ -127,7 +133,7 @@ class AvatarPhotoController { case REQUEST_CODE_TAKE_PHOTO: case REQUEST_CODE_CHOOSE_PHOTO: if (mTakePictureUri.equals(pictureUri)) { - cropPhoto(); + cropPhoto(pictureUri); } else { copyAndCropPhoto(pictureUri); } @@ -153,7 +159,7 @@ class AvatarPhotoController { ThreadUtils.postOnBackgroundThread(() -> { final ContentResolver cr = mContextInjector.getContentResolver(); try (InputStream in = cr.openInputStream(pictureUri); - OutputStream out = cr.openOutputStream(mTakePictureUri)) { + OutputStream out = cr.openOutputStream(mPreCropPictureUri)) { Streams.copy(in, out); } catch (IOException e) { Log.w(TAG, "Failed to copy photo", e); @@ -161,7 +167,7 @@ class AvatarPhotoController { } ThreadUtils.postOnMainThread(() -> { if (!mAvatarUi.isFinishing()) { - cropPhoto(); + cropPhoto(mPreCropPictureUri); } }); }).get(); @@ -170,22 +176,21 @@ class AvatarPhotoController { } } - private void cropPhoto() { - if (mAvatarUi.canCropPhoto()) { - // TODO: Use a public intent, when there is one. - Intent intent = new Intent("com.android.camera.action.CROP"); - intent.setDataAndType(mTakePictureUri, "image/*"); - appendOutputExtra(intent, mCropPictureUri); - appendCropExtras(intent); - try { - StrictMode.disableDeathOnFileUriExposure(); - mAvatarUi.startActivityForResult(intent, REQUEST_CODE_CROP_PHOTO); - } finally { - StrictMode.enableDeathOnFileUriExposure(); + private void cropPhoto(final Uri pictureUri) { + // TODO: Use a public intent, when there is one. + Intent intent = new Intent("com.android.camera.action.CROP"); + intent.setDataAndType(pictureUri, "image/*"); + appendOutputExtra(intent, mCropPictureUri); + appendCropExtras(intent); + try { + StrictMode.disableDeathOnFileUriExposure(); + if (mAvatarUi.startSystemActivityForResult(intent, REQUEST_CODE_CROP_PHOTO)) { + return; } - } else { - onPhotoNotCropped(mTakePictureUri); + } finally { + StrictMode.enableDeathOnFileUriExposure(); } + onPhotoNotCropped(pictureUri); } private void appendOutputExtra(Intent intent, Uri pictureUri) { @@ -307,14 +312,22 @@ class AvatarPhotoController { } @Override - public int getPhotoSize() { - return mActivity.getResources() - .getDimensionPixelSize(com.android.internal.R.dimen.user_icon_size); + public boolean startSystemActivityForResult(Intent intent, int code) { + ActivityInfo info = intent.resolveActivityInfo(mActivity.getPackageManager(), + PackageManager.MATCH_SYSTEM_ONLY); + if (info == null) { + Log.w(TAG, "No system package activity could be found for code " + code); + return false; + } + intent.setPackage(info.packageName); + mActivity.startActivityForResult(intent, code); + return true; } @Override - public boolean canCropPhoto() { - return PhotoCapabilityUtils.canCropPhoto(mActivity); + public int getPhotoSize() { + return mActivity.getResources() + .getDimensionPixelSize(com.android.internal.R.dimen.user_icon_size); } } diff --git a/packages/SettingsLib/tests/integ/src/com/android/settingslib/users/AvatarPhotoControllerTest.java b/packages/SettingsLib/tests/integ/src/com/android/settingslib/users/AvatarPhotoControllerTest.java index f705884bb8888..9ebdba3002660 100644 --- a/packages/SettingsLib/tests/integ/src/com/android/settingslib/users/AvatarPhotoControllerTest.java +++ b/packages/SettingsLib/tests/integ/src/com/android/settingslib/users/AvatarPhotoControllerTest.java @@ -75,6 +75,7 @@ public class AvatarPhotoControllerTest { public void setUp() { MockitoAnnotations.initMocks(this); when(mMockAvatarUi.getPhotoSize()).thenReturn(PHOTO_SIZE); + when(mMockAvatarUi.startSystemActivityForResult(any(), anyInt())).thenReturn(true); mImagesDir = new File( InstrumentationRegistry.getTargetContext().getCacheDir(), "multi_user"); @@ -108,9 +109,7 @@ public class AvatarPhotoControllerTest { } @Test - public void takePhotoIsFollowedByCropWhenSupported() throws IOException { - when(mMockAvatarUi.canCropPhoto()).thenReturn(true); - + public void takePhotoIsFollowedByCrop() throws IOException { new File(mImagesDir, "file.txt").createNewFile(); Intent intent = new Intent(); @@ -119,14 +118,12 @@ public class AvatarPhotoControllerTest { mController.onActivityResult( REQUEST_CODE_TAKE_PHOTO, Activity.RESULT_OK, intent); - verifyStartActivityForResult( + verifyStartSystemActivityForResult( "com.android.camera.action.CROP", REQUEST_CODE_CROP_PHOTO); } @Test public void takePhotoIsNotFollowedByCropWhenResultCodeNotOk() throws IOException { - when(mMockAvatarUi.canCropPhoto()).thenReturn(true); - new File(mImagesDir, "file.txt").createNewFile(); Intent intent = new Intent(); @@ -136,12 +133,11 @@ public class AvatarPhotoControllerTest { REQUEST_CODE_TAKE_PHOTO, Activity.RESULT_CANCELED, intent); verify(mMockAvatarUi, never()).startActivityForResult(any(), anyInt()); + verify(mMockAvatarUi, never()).startSystemActivityForResult(any(), anyInt()); } @Test public void takePhotoIsFollowedByCropWhenTakePhotoUriReturned() throws IOException { - when(mMockAvatarUi.canCropPhoto()).thenReturn(true); - new File(mImagesDir, "TakeEditUserPhoto.jpg").createNewFile(); Intent intent = new Intent(); @@ -149,13 +145,13 @@ public class AvatarPhotoControllerTest { mController.onActivityResult( REQUEST_CODE_TAKE_PHOTO, Activity.RESULT_OK, intent); - verifyStartActivityForResult( + verifyStartSystemActivityForResult( "com.android.camera.action.CROP", REQUEST_CODE_CROP_PHOTO); } @Test public void takePhotoIsNotFollowedByCropIntentWhenCropNotSupported() throws IOException { - when(mMockAvatarUi.canCropPhoto()).thenReturn(false); + when(mMockAvatarUi.startSystemActivityForResult(any(), anyInt())).thenReturn(false); File file = new File(mImagesDir, "file.txt"); saveBitmapToFile(file); @@ -167,33 +163,11 @@ public class AvatarPhotoControllerTest { REQUEST_CODE_TAKE_PHOTO, Activity.RESULT_OK, intent); verify(mMockAvatarUi, never()).startActivityForResult(any(), anyInt()); + verify(mMockAvatarUi, never()).startSystemActivityForResult(any(), anyInt()); } @Test - public void takePhotoIsFollowedByInternalCropWhenCropNotSupported() throws IOException { - when(mMockAvatarUi.canCropPhoto()).thenReturn(false); - - File file = new File(mImagesDir, "file.txt"); - saveBitmapToFile(file); - - Intent intent = new Intent(); - intent.setData(Uri.parse( - "content://com.android.settingslib.test/my_cache/multi_user/file.txt")); - mController.onActivityResult( - REQUEST_CODE_TAKE_PHOTO, Activity.RESULT_OK, intent); - - verify(mMockAvatarUi, timeout(TIMEOUT_MILLIS)).returnUriResult(mCropPhotoUri); - - InputStream imageStream = mContext.getContentResolver().openInputStream(mCropPhotoUri); - Bitmap bitmap = BitmapFactory.decodeStream(imageStream); - assertThat(bitmap.getWidth()).isEqualTo(PHOTO_SIZE); - assertThat(bitmap.getHeight()).isEqualTo(PHOTO_SIZE); - } - - @Test - public void choosePhotoIsFollowedByCropWhenSupported() throws IOException { - when(mMockAvatarUi.canCropPhoto()).thenReturn(true); - + public void choosePhotoIsFollowedByCrop() throws IOException { new File(mImagesDir, "file.txt").createNewFile(); Intent intent = new Intent(); @@ -202,14 +176,12 @@ public class AvatarPhotoControllerTest { mController.onActivityResult( REQUEST_CODE_CHOOSE_PHOTO, Activity.RESULT_OK, intent); - verifyStartActivityForResult( + verifyStartSystemActivityForResult( "com.android.camera.action.CROP", REQUEST_CODE_CROP_PHOTO); } @Test public void choosePhotoIsNotFollowedByCropWhenResultCodeNotOk() throws IOException { - when(mMockAvatarUi.canCropPhoto()).thenReturn(true); - new File(mImagesDir, "file.txt").createNewFile(); Intent intent = new Intent(); @@ -219,12 +191,11 @@ public class AvatarPhotoControllerTest { REQUEST_CODE_CHOOSE_PHOTO, Activity.RESULT_CANCELED, intent); verify(mMockAvatarUi, never()).startActivityForResult(any(), anyInt()); + verify(mMockAvatarUi, never()).startSystemActivityForResult(any(), anyInt()); } @Test public void choosePhotoIsFollowedByCropWhenTakePhotoUriReturned() throws IOException { - when(mMockAvatarUi.canCropPhoto()).thenReturn(true); - new File(mImagesDir, "TakeEditUserPhoto.jpg").createNewFile(); Intent intent = new Intent(); @@ -232,27 +203,10 @@ public class AvatarPhotoControllerTest { mController.onActivityResult( REQUEST_CODE_CHOOSE_PHOTO, Activity.RESULT_OK, intent); - verifyStartActivityForResult( + verifyStartSystemActivityForResult( "com.android.camera.action.CROP", REQUEST_CODE_CROP_PHOTO); } - @Test - public void choosePhotoIsNotFollowedByCropIntentWhenCropNotSupported() throws IOException { - when(mMockAvatarUi.canCropPhoto()).thenReturn(false); - - File file = new File(mImagesDir, "file.txt"); - saveBitmapToFile(file); - - Intent intent = new Intent(); - intent.setData(Uri.parse( - "content://com.android.settingslib.test/my_cache/multi_user/file.txt")); - mController.onActivityResult( - REQUEST_CODE_CHOOSE_PHOTO, Activity.RESULT_OK, intent); - - verify(mMockAvatarUi, never()).startActivityForResult(any(), anyInt()); - verify(mMockAvatarUi, timeout(TIMEOUT_MILLIS)).returnUriResult(mCropPhotoUri); - } - @Test public void cropPhotoResultIsReturnedIfResultOkAndContent() { Intent intent = new Intent(); @@ -277,11 +231,61 @@ public class AvatarPhotoControllerTest { verify(mMockAvatarUi, timeout(TIMEOUT_MILLIS).times(0)).returnUriResult(mCropPhotoUri); } - private void verifyStartActivityForResult(String action, int resultCode) { + @Test + public void cropDoesNotUseTakePhotoUri() throws IOException { + new File(mImagesDir, "file.txt").createNewFile(); + + Intent intent = new Intent(); + intent.setData(Uri.parse( + "content://com.android.settingslib.test/my_cache/multi_user/file.txt")); + mController.onActivityResult( + REQUEST_CODE_TAKE_PHOTO, Activity.RESULT_OK, intent); + + Intent startIntent = verifyStartSystemActivityForResult( + "com.android.camera.action.CROP", REQUEST_CODE_CROP_PHOTO); + assertThat(startIntent.getData()).isNotEqualTo(mTakePhotoUri); + } + + @Test + public void internalCropUsedIfNoSystemCropperFound() throws IOException { + when(mMockAvatarUi.startSystemActivityForResult(any(), anyInt())).thenReturn(false); + + new File(mImagesDir, "file.txt").createNewFile(); + + Intent intent = new Intent(); + intent.setData(Uri.parse( + "content://com.android.settingslib.test/my_cache/multi_user/file.txt")); + mController.onActivityResult( + REQUEST_CODE_TAKE_PHOTO, Activity.RESULT_OK, intent); + + Intent startIntent = verifyStartSystemActivityForResult( + "com.android.camera.action.CROP", REQUEST_CODE_CROP_PHOTO); + assertThat(startIntent.getData()).isNotEqualTo(mTakePhotoUri); + + verify(mMockAvatarUi, timeout(TIMEOUT_MILLIS)).returnUriResult(mCropPhotoUri); + + InputStream imageStream = mContext.getContentResolver().openInputStream(mCropPhotoUri); + Bitmap bitmap = BitmapFactory.decodeStream(imageStream); + assertThat(bitmap.getWidth()).isEqualTo(PHOTO_SIZE); + assertThat(bitmap.getHeight()).isEqualTo(PHOTO_SIZE); + } + + private Intent verifyStartActivityForResult(String action, int resultCode) { ArgumentCaptor captor = ArgumentCaptor.forClass(Intent.class); verify(mMockAvatarUi, timeout(TIMEOUT_MILLIS)) .startActivityForResult(captor.capture(), eq(resultCode)); - assertThat(captor.getValue().getAction()).isEqualTo(action); + Intent intent = captor.getValue(); + assertThat(intent.getAction()).isEqualTo(action); + return intent; + } + + private Intent verifyStartSystemActivityForResult(String action, int resultCode) { + ArgumentCaptor captor = ArgumentCaptor.forClass(Intent.class); + verify(mMockAvatarUi, timeout(TIMEOUT_MILLIS)) + .startSystemActivityForResult(captor.capture(), eq(resultCode)); + Intent intent = captor.getValue(); + assertThat(intent.getAction()).isEqualTo(action); + return intent; } private void saveBitmapToFile(File file) throws IOException {