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.
This is a fixed version of ag/17005706, to address b/239513606.
Bug: 187702830
Test: atest AvatarPhotoControllerTest
Change-Id: I21f1b25154dc00a305bdadb96fdf22edff31d9b8
diff --git a/packages/SettingsLib/src/com/android/settingslib/users/AvatarPhotoController.java b/packages/SettingsLib/src/com/android/settingslib/users/AvatarPhotoController.java
index adfa39e..4ce88ee 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.ContentResolver;
import android.content.Context;
import android.content.Intent;
+import android.content.pm.PackageManager;
+import android.content.pm.ResolveInfo;
import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.graphics.Canvas;
@@ -46,6 +48,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.util.List;
import java.util.concurrent.ExecutionException;
class AvatarPhotoController {
@@ -57,9 +60,9 @@
void startActivityForResult(Intent intent, int resultCode);
- int getPhotoSize();
+ boolean startSystemActivityForResult(Intent intent, int resultCode);
- boolean canCropPhoto();
+ int getPhotoSize();
}
interface ContextInjector {
@@ -82,6 +85,7 @@
private static final long DELAY_BEFORE_CROP_MILLIS = 150;
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";
@@ -91,6 +95,7 @@
private final ContextInjector mContextInjector;
private final File mImagesDir;
+ private final Uri mPreCropPictureUri;
private final Uri mCropPictureUri;
private final Uri mTakePictureUri;
@@ -100,6 +105,8 @@
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 =
@@ -131,7 +138,7 @@
return true;
case REQUEST_CODE_TAKE_PHOTO:
if (mTakePictureUri.equals(pictureUri)) {
- cropPhoto();
+ cropPhoto(pictureUri);
} else {
copyAndCropPhoto(pictureUri, false);
}
@@ -160,7 +167,7 @@
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);
@@ -168,7 +175,7 @@
}
Runnable cropRunnable = () -> {
if (!mAvatarUi.isFinishing()) {
- cropPhoto();
+ cropPhoto(mPreCropPictureUri);
}
};
if (delayBeforeCrop) {
@@ -183,22 +190,21 @@
}
}
- 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) {
@@ -320,14 +326,22 @@
}
@Override
- public int getPhotoSize() {
- return mActivity.getResources()
- .getDimensionPixelSize(com.android.internal.R.dimen.user_icon_size);
+ public boolean startSystemActivityForResult(Intent intent, int code) {
+ List<ResolveInfo> resolveInfos = mActivity.getPackageManager()
+ .queryIntentActivities(intent, PackageManager.MATCH_SYSTEM_ONLY);
+ if (resolveInfos.isEmpty()) {
+ Log.w(TAG, "No system package activity could be found for code " + code);
+ return false;
+ }
+ intent.setPackage(resolveInfos.get(0).activityInfo.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 3dc2fab..d988111 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
@@ -34,6 +34,7 @@
import android.content.Context;
import android.content.Intent;
import android.graphics.Bitmap;
+import android.graphics.BitmapFactory;
import android.net.Uri;
import android.provider.MediaStore;
@@ -51,6 +52,7 @@
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
+import java.io.InputStream;
import java.io.OutputStream;
@RunWith(AndroidJUnit4.class)
@@ -73,6 +75,7 @@
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");
@@ -110,9 +113,7 @@
}
@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();
@@ -121,14 +122,12 @@
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();
@@ -138,12 +137,11 @@
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();
@@ -151,14 +149,12 @@
mController.onActivityResult(
REQUEST_CODE_TAKE_PHOTO, Activity.RESULT_OK, intent);
- verifyStartActivityForResult(
+ verifyStartSystemActivityForResult(
"com.android.camera.action.CROP", REQUEST_CODE_CROP_PHOTO);
}
@Test
public void choosePhotoIsFollowedByCrop() throws IOException {
- when(mMockAvatarUi.canCropPhoto()).thenReturn(true);
-
new File(mImagesDir, "file.txt").createNewFile();
Intent intent = new Intent();
@@ -167,14 +163,12 @@
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();
@@ -184,12 +178,11 @@
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();
@@ -197,28 +190,11 @@
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();
intent.setData(mCropPhotoUri);
@@ -242,11 +218,58 @@
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);
+
+ 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);
+ }
+
+ private Intent verifyStartActivityForResult(String action, int resultCode) {
ArgumentCaptor<Intent> 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<Intent> 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 {
