From f7667a655bc603fa996c9b4a4c8f7f525d52eef7 Mon Sep 17 00:00:00 2001 From: Gray Mackall <34871572+gmackall@users.noreply.github.com> Date: Wed, 27 Nov 2024 13:29:15 -0800 Subject: [PATCH] [file_selector_android] Refactor interactions with `ContentProvider` provided filenames (#8184) https://developer.android.com/privacy-and-security/risks/untrustworthy-contentprovider-provided-filename#don%27t-trust-user-input Adapted from this option https://developer.android.com/privacy-and-security/risks/untrustworthy-contentprovider-provided-filename#sanitize-provided-filenames --- .../file_selector_android/CHANGELOG.md | 4 + .../FileSelectorApiImpl.java | 31 ++- .../file_selector_android/FileUtils.java | 75 +++++-- .../GeneratedFileSelectorApi.java | 163 +++++++++++++- .../FileSelectorAndroidPluginTest.java | 202 ++++++++++++++++++ .../file_selector_android/FileUtilsTest.java | 86 ++++++-- .../lib/file_selector_android.dart | 1 + .../lib/src/file_selector_android.dart | 19 ++ .../lib/src/file_selector_api.g.dart | 57 ++++- .../native_illegal_argument_exception.dart | 17 ++ .../pigeons/file_selector_api.dart | 13 ++ .../file_selector_android/pubspec.yaml | 2 +- 12 files changed, 616 insertions(+), 54 deletions(-) create mode 100644 packages/file_selector/file_selector_android/lib/src/types/native_illegal_argument_exception.dart diff --git a/packages/file_selector/file_selector_android/CHANGELOG.md b/packages/file_selector/file_selector_android/CHANGELOG.md index ca3067927f53..534c75f58bb3 100644 --- a/packages/file_selector/file_selector_android/CHANGELOG.md +++ b/packages/file_selector/file_selector_android/CHANGELOG.md @@ -1,3 +1,7 @@ +## 0.5.1+12 + +* Fixes a security issue related to improperly trusting filenames provided by a `ContentProvider`. + ## 0.5.1+11 * Bumps androidx.annotation:annotation from 1.9.0 to 1.9.1. diff --git a/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileSelectorApiImpl.java b/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileSelectorApiImpl.java index 555318b29959..37d38c2f5083 100644 --- a/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileSelectorApiImpl.java +++ b/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileSelectorApiImpl.java @@ -4,6 +4,8 @@ package dev.flutter.packages.file_selector_android; +import static dev.flutter.packages.file_selector_android.FileUtils.FILE_SELECTOR_EXCEPTION_PLACEHOLDER_PATH; + import android.annotation.TargetApi; import android.app.Activity; import android.content.ClipData; @@ -357,8 +359,32 @@ GeneratedFileSelectorApi.FileResponse toFileResponse(@NonNull Uri uri) { return null; } - final String uriPath = - FileUtils.getPathFromCopyOfFileFromUri(activityPluginBinding.getActivity(), uri); + String uriPath; + GeneratedFileSelectorApi.FileSelectorNativeException nativeError = null; + + try { + uriPath = FileUtils.getPathFromCopyOfFileFromUri(activityPluginBinding.getActivity(), uri); + } catch (IOException e) { + // If closing the output stream fails, we cannot be sure that the + // target file was written in full. Flushing the stream merely moves + // the bytes into the OS, not necessarily to the file. + uriPath = null; + } catch (SecurityException e) { + // Calling `ContentResolver#openInputStream()` has been reported to throw a + // `SecurityException` on some devices in certain circumstances. Instead of crashing, we + // return `null`. + // + // See https://github.com/flutter/flutter/issues/100025 for more details. + uriPath = null; + } catch (IllegalArgumentException e) { + uriPath = FILE_SELECTOR_EXCEPTION_PLACEHOLDER_PATH; + nativeError = + new GeneratedFileSelectorApi.FileSelectorNativeException.Builder() + .setMessage(e.getMessage() == null ? "" : e.getMessage()) + .setFileSelectorExceptionCode( + GeneratedFileSelectorApi.FileSelectorExceptionCode.ILLEGAL_ARGUMENT_EXCEPTION) + .build(); + } return new GeneratedFileSelectorApi.FileResponse.Builder() .setName(name) @@ -366,6 +392,7 @@ GeneratedFileSelectorApi.FileResponse toFileResponse(@NonNull Uri uri) { .setPath(uriPath) .setMimeType(contentResolver.getType(uri)) .setSize(size.longValue()) + .setFileSelectorNativeException(nativeError) .build(); } } diff --git a/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java b/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java index 5d0b61312b36..e3cd81239eb6 100644 --- a/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java +++ b/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java @@ -45,6 +45,8 @@ public class FileUtils { /** URI authority that represents access to external storage providers. */ public static final String EXTERNAL_DOCUMENT_AUTHORITY = "com.android.externalstorage.documents"; + public static final String FILE_SELECTOR_EXCEPTION_PLACEHOLDER_PATH = "FILE_SELECTOR_EXCEPTION"; + /** * Retrieves path of directory represented by the specified {@code Uri}. * @@ -98,6 +100,12 @@ public static String getPathFromUri(@NonNull Context context, @NonNull Uri uri) * Copies the file from the given content URI to a temporary directory, retaining the original * file name if possible. * + *

If the filename contains path indirection or separators (.. or /), the end file name will be + * the segment after the final separator, with indirection replaced by underscores. E.g. + * "example/../..file.png" -> "_file.png". See: Improperly + * trusting ContentProvider-provided filename. + * *

Each file is placed in its own directory to avoid conflicts according to the following * scheme: {cacheDir}/{randomUuid}/{fileName} * @@ -111,7 +119,8 @@ public static String getPathFromUri(@NonNull Context context, @NonNull Uri uri) * or if a security exception is encountered when opening the input stream to start the copying. */ @Nullable - public static String getPathFromCopyOfFileFromUri(@NonNull Context context, @NonNull Uri uri) { + public static String getPathFromCopyOfFileFromUri(@NonNull Context context, @NonNull Uri uri) + throws IOException, SecurityException, IllegalArgumentException { try (InputStream inputStream = context.getContentResolver().openInputStream(uri)) { String uuid = UUID.nameUUIDFromBytes(uri.toString().getBytes()).toString(); File targetDirectory = new File(context.getCacheDir(), uuid); @@ -122,7 +131,7 @@ public static String getPathFromCopyOfFileFromUri(@NonNull Context context, @Non if (fileName == null) { if (extension == null) { - throw new IllegalArgumentException("No name nor extension found for file."); + throw new IllegalStateException("No name nor extension found for file."); } else { fileName = "file_selector" + extension; } @@ -130,24 +139,13 @@ public static String getPathFromCopyOfFileFromUri(@NonNull Context context, @Non fileName = getBaseName(fileName) + extension; } - File file = new File(targetDirectory, fileName); + String filePath = new File(targetDirectory, fileName).getPath(); + File outputFile = saferOpenFile(filePath, targetDirectory.getCanonicalPath()); - try (OutputStream outputStream = new FileOutputStream(file)) { + try (OutputStream outputStream = new FileOutputStream(outputFile)) { copy(inputStream, outputStream); - return file.getPath(); + return outputFile.getPath(); } - } catch (IOException e) { - // If closing the output stream fails, we cannot be sure that the - // target file was written in full. Flushing the stream merely moves - // the bytes into the OS, not necessarily to the file. - return null; - } catch (SecurityException e) { - // Calling `ContentResolver#openInputStream()` has been reported to throw a - // `SecurityException` on some devices in certain circumstances. Instead of crashing, we - // return `null`. - // - // See https://github.com/flutter/flutter/issues/100025 for more details. - return null; } } @@ -172,14 +170,17 @@ private static String getFileExtension(Context context, Uri uriFile) { return null; } - return "." + extension; + return "." + sanitizeFilename(extension); } /** Returns the name of the file provided by ContentResolver; this may be null. */ private static String getFileName(Context context, Uri uriFile) { try (Cursor cursor = queryFileName(context, uriFile)) { - if (cursor == null || !cursor.moveToFirst() || cursor.getColumnCount() < 1) return null; - return cursor.getString(0); + if (cursor == null || !cursor.moveToFirst() || cursor.getColumnCount() < 1) { + return null; + } + String unsanitizedFileName = cursor.getString(0); + return sanitizeFilename(unsanitizedFileName); } } @@ -206,4 +207,38 @@ private static String getBaseName(String fileName) { // Basename is everything before the last '.'. return fileName.substring(0, lastDotIndex); } + + // From https://developer.android.com/privacy-and-security/risks/untrustworthy-contentprovider-provided-filename#sanitize-provided-filenames. + protected static @Nullable String sanitizeFilename(@Nullable String displayName) { + if (displayName == null) { + return null; + } + + String[] badCharacters = new String[] {"..", "/"}; + String[] segments = displayName.split("/"); + String fileName = segments[segments.length - 1]; + for (String suspString : badCharacters) { + fileName = fileName.replace(suspString, "_"); + } + return fileName; + } + + /** + * Use with file name sanatization and an non-guessable directory. From + * https://developer.android.com/privacy-and-security/risks/path-traversal#path-traversal-mitigations. + */ + protected static @NonNull File saferOpenFile(@NonNull String path, @NonNull String expectedDir) + throws IllegalArgumentException, IOException { + File f = new File(path); + String canonicalPath = f.getCanonicalPath(); + if (!canonicalPath.startsWith(expectedDir)) { + throw new IllegalArgumentException( + "Trying to open path outside of the expected directory. File: " + + f.getCanonicalPath() + + " was expected to be within directory: " + + expectedDir + + "."); + } + return f; + } } diff --git a/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/GeneratedFileSelectorApi.java b/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/GeneratedFileSelectorApi.java index de595ef85847..ee744f5ec8d5 100644 --- a/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/GeneratedFileSelectorApi.java +++ b/packages/file_selector/file_selector_android/android/src/main/java/dev/flutter/packages/file_selector_android/GeneratedFileSelectorApi.java @@ -1,7 +1,7 @@ // Copyright 2013 The Flutter Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Autogenerated from Pigeon (v22.4.2), do not edit directly. +// Autogenerated from Pigeon (v22.6.2), do not edit directly. // See also: https://pub.dev/packages/pigeon package dev.flutter.packages.file_selector_android; @@ -66,6 +66,115 @@ protected static ArrayList wrapError(@NonNull Throwable exception) { @Retention(CLASS) @interface CanIgnoreReturnValue {} + public enum FileSelectorExceptionCode { + SECURITY_EXCEPTION(0), + IO_EXCEPTION(1), + ILLEGAL_ARGUMENT_EXCEPTION(2), + ILLEGAL_STATE_EXCEPTION(3); + + final int index; + + FileSelectorExceptionCode(final int index) { + this.index = index; + } + } + + /** Generated class from Pigeon that represents data sent in messages. */ + public static final class FileSelectorNativeException { + private @NonNull FileSelectorExceptionCode fileSelectorExceptionCode; + + public @NonNull FileSelectorExceptionCode getFileSelectorExceptionCode() { + return fileSelectorExceptionCode; + } + + public void setFileSelectorExceptionCode(@NonNull FileSelectorExceptionCode setterArg) { + if (setterArg == null) { + throw new IllegalStateException("Nonnull field \"fileSelectorExceptionCode\" is null."); + } + this.fileSelectorExceptionCode = setterArg; + } + + private @NonNull String message; + + public @NonNull String getMessage() { + return message; + } + + public void setMessage(@NonNull String setterArg) { + if (setterArg == null) { + throw new IllegalStateException("Nonnull field \"message\" is null."); + } + this.message = setterArg; + } + + /** Constructor is non-public to enforce null safety; use Builder. */ + FileSelectorNativeException() {} + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + FileSelectorNativeException that = (FileSelectorNativeException) o; + return fileSelectorExceptionCode.equals(that.fileSelectorExceptionCode) + && message.equals(that.message); + } + + @Override + public int hashCode() { + return Objects.hash(fileSelectorExceptionCode, message); + } + + public static final class Builder { + + private @Nullable FileSelectorExceptionCode fileSelectorExceptionCode; + + @CanIgnoreReturnValue + public @NonNull Builder setFileSelectorExceptionCode( + @NonNull FileSelectorExceptionCode setterArg) { + this.fileSelectorExceptionCode = setterArg; + return this; + } + + private @Nullable String message; + + @CanIgnoreReturnValue + public @NonNull Builder setMessage(@NonNull String setterArg) { + this.message = setterArg; + return this; + } + + public @NonNull FileSelectorNativeException build() { + FileSelectorNativeException pigeonReturn = new FileSelectorNativeException(); + pigeonReturn.setFileSelectorExceptionCode(fileSelectorExceptionCode); + pigeonReturn.setMessage(message); + return pigeonReturn; + } + } + + @NonNull + ArrayList toList() { + ArrayList toListResult = new ArrayList<>(2); + toListResult.add(fileSelectorExceptionCode); + toListResult.add(message); + return toListResult; + } + + static @NonNull FileSelectorNativeException fromList( + @NonNull ArrayList pigeonVar_list) { + FileSelectorNativeException pigeonResult = new FileSelectorNativeException(); + Object fileSelectorExceptionCode = pigeonVar_list.get(0); + pigeonResult.setFileSelectorExceptionCode( + (FileSelectorExceptionCode) fileSelectorExceptionCode); + Object message = pigeonVar_list.get(1); + pigeonResult.setMessage((String) message); + return pigeonResult; + } + } + /** Generated class from Pigeon that represents data sent in messages. */ public static final class FileResponse { private @NonNull String path; @@ -127,6 +236,16 @@ public void setBytes(@NonNull byte[] setterArg) { this.bytes = setterArg; } + private @Nullable FileSelectorNativeException fileSelectorNativeException; + + public @Nullable FileSelectorNativeException getFileSelectorNativeException() { + return fileSelectorNativeException; + } + + public void setFileSelectorNativeException(@Nullable FileSelectorNativeException setterArg) { + this.fileSelectorNativeException = setterArg; + } + /** Constructor is non-public to enforce null safety; use Builder. */ FileResponse() {} @@ -143,12 +262,13 @@ public boolean equals(Object o) { && Objects.equals(mimeType, that.mimeType) && Objects.equals(name, that.name) && size.equals(that.size) - && Arrays.equals(bytes, that.bytes); + && Arrays.equals(bytes, that.bytes) + && Objects.equals(fileSelectorNativeException, that.fileSelectorNativeException); } @Override public int hashCode() { - int pigeonVar_result = Objects.hash(path, mimeType, name, size); + int pigeonVar_result = Objects.hash(path, mimeType, name, size, fileSelectorNativeException); pigeonVar_result = 31 * pigeonVar_result + Arrays.hashCode(bytes); return pigeonVar_result; } @@ -195,6 +315,15 @@ public static final class Builder { return this; } + private @Nullable FileSelectorNativeException fileSelectorNativeException; + + @CanIgnoreReturnValue + public @NonNull Builder setFileSelectorNativeException( + @Nullable FileSelectorNativeException setterArg) { + this.fileSelectorNativeException = setterArg; + return this; + } + public @NonNull FileResponse build() { FileResponse pigeonReturn = new FileResponse(); pigeonReturn.setPath(path); @@ -202,18 +331,20 @@ public static final class Builder { pigeonReturn.setName(name); pigeonReturn.setSize(size); pigeonReturn.setBytes(bytes); + pigeonReturn.setFileSelectorNativeException(fileSelectorNativeException); return pigeonReturn; } } @NonNull ArrayList toList() { - ArrayList toListResult = new ArrayList<>(5); + ArrayList toListResult = new ArrayList<>(6); toListResult.add(path); toListResult.add(mimeType); toListResult.add(name); toListResult.add(size); toListResult.add(bytes); + toListResult.add(fileSelectorNativeException); return toListResult; } @@ -229,6 +360,9 @@ ArrayList toList() { pigeonResult.setSize((Long) size); Object bytes = pigeonVar_list.get(4); pigeonResult.setBytes((byte[]) bytes); + Object fileSelectorNativeException = pigeonVar_list.get(5); + pigeonResult.setFileSelectorNativeException( + (FileSelectorNativeException) fileSelectorNativeException); return pigeonResult; } } @@ -334,8 +468,17 @@ private PigeonCodec() {} protected Object readValueOfType(byte type, @NonNull ByteBuffer buffer) { switch (type) { case (byte) 129: - return FileResponse.fromList((ArrayList) readValue(buffer)); + { + Object value = readValue(buffer); + return value == null + ? null + : FileSelectorExceptionCode.values()[((Long) value).intValue()]; + } case (byte) 130: + return FileSelectorNativeException.fromList((ArrayList) readValue(buffer)); + case (byte) 131: + return FileResponse.fromList((ArrayList) readValue(buffer)); + case (byte) 132: return FileTypes.fromList((ArrayList) readValue(buffer)); default: return super.readValueOfType(type, buffer); @@ -344,11 +487,17 @@ protected Object readValueOfType(byte type, @NonNull ByteBuffer buffer) { @Override protected void writeValue(@NonNull ByteArrayOutputStream stream, Object value) { - if (value instanceof FileResponse) { + if (value instanceof FileSelectorExceptionCode) { stream.write(129); + writeValue(stream, value == null ? null : ((FileSelectorExceptionCode) value).index); + } else if (value instanceof FileSelectorNativeException) { + stream.write(130); + writeValue(stream, ((FileSelectorNativeException) value).toList()); + } else if (value instanceof FileResponse) { + stream.write(131); writeValue(stream, ((FileResponse) value).toList()); } else if (value instanceof FileTypes) { - stream.write(130); + stream.write(132); writeValue(stream, ((FileTypes) value).toList()); } else { super.writeValue(stream, value); diff --git a/packages/file_selector/file_selector_android/android/src/test/java/dev/flutter/packages/file_selector_android/FileSelectorAndroidPluginTest.java b/packages/file_selector/file_selector_android/android/src/test/java/dev/flutter/packages/file_selector_android/FileSelectorAndroidPluginTest.java index 00533c282640..b213e85f05cf 100644 --- a/packages/file_selector/file_selector_android/android/src/test/java/dev/flutter/packages/file_selector_android/FileSelectorAndroidPluginTest.java +++ b/packages/file_selector/file_selector_android/android/src/test/java/dev/flutter/packages/file_selector_android/FileSelectorAndroidPluginTest.java @@ -5,6 +5,8 @@ package dev.flutter.packages.file_selector_android; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; @@ -212,6 +214,206 @@ public void openFilesReturnsSuccessfully() throws FileNotFoundException { } } + // This test was created when error handling was moved from FileUtils.java to FileSelectorApiImpl.java + // in https://github.com/flutter/packages/pull/8184, so as to maintain the existing test. + // The behavior is actually an error case and should be fixed, + // see: https://github.com/flutter/flutter/issues/159568. + // Remove when fixed! + @SuppressWarnings({"rawtypes", "unchecked"}) + @Test + public void + openFileThrowsIllegalStateException_whenSecurityExceptionInGetPathFromCopyOfFileFromUri() + throws FileNotFoundException { + + try (MockedStatic mockedFileUtils = mockStatic(FileUtils.class)) { + + final ContentResolver mockContentResolver = mock(ContentResolver.class); + + final Uri mockUri = mock(Uri.class); + final String mockUriPath = "some/path/"; + mockedFileUtils + .when(() -> FileUtils.getPathFromCopyOfFileFromUri(any(Context.class), eq(mockUri))) + .thenThrow(SecurityException.class); + mockContentResolver(mockContentResolver, mockUri, "filename", 30, "text/plain"); + + final Uri mockUri2 = mock(Uri.class); + final String mockUri2Path = "some/other/path/"; + mockedFileUtils + .when(() -> FileUtils.getPathFromCopyOfFileFromUri(any(Context.class), eq(mockUri2))) + .thenAnswer((Answer) invocation -> mockUri2Path); + mockContentResolver(mockContentResolver, mockUri2, "filename2", 40, "image/jpg"); + + when(mockObjectFactory.newIntent(Intent.ACTION_OPEN_DOCUMENT)).thenReturn(mockIntent); + when(mockObjectFactory.newDataInputStream(any())).thenReturn(mock(DataInputStream.class)); + when(mockActivity.getContentResolver()).thenReturn(mockContentResolver); + when(mockActivityBinding.getActivity()).thenReturn(mockActivity); + final FileSelectorApiImpl fileSelectorApi = + new FileSelectorApiImpl( + mockActivityBinding, + mockObjectFactory, + (version) -> Build.VERSION.SDK_INT >= version); + + final GeneratedFileSelectorApi.Result mockResult = + mock(GeneratedFileSelectorApi.Result.class); + fileSelectorApi.openFiles( + null, + new GeneratedFileSelectorApi.FileTypes.Builder() + .setMimeTypes(Collections.emptyList()) + .setExtensions(Collections.emptyList()) + .build(), + mockResult); + verify(mockIntent).addCategory(Intent.CATEGORY_OPENABLE); + verify(mockIntent).putExtra(Intent.EXTRA_ALLOW_MULTIPLE, true); + + verify(mockActivity).startActivityForResult(mockIntent, 222); + + final ArgumentCaptor listenerArgumentCaptor = + ArgumentCaptor.forClass(PluginRegistry.ActivityResultListener.class); + verify(mockActivityBinding).addActivityResultListener(listenerArgumentCaptor.capture()); + + final Intent resultMockIntent = mock(Intent.class); + final ClipData mockClipData = mock(ClipData.class); + when(mockClipData.getItemCount()).thenReturn(2); + + final ClipData.Item mockClipDataItem = mock(ClipData.Item.class); + when(mockClipDataItem.getUri()).thenReturn(mockUri); + when(mockClipData.getItemAt(0)).thenReturn(mockClipDataItem); + + final ClipData.Item mockClipDataItem2 = mock(ClipData.Item.class); + when(mockClipDataItem2.getUri()).thenReturn(mockUri2); + when(mockClipData.getItemAt(1)).thenReturn(mockClipDataItem2); + + when(resultMockIntent.getClipData()).thenReturn(mockClipData); + + assertThrows( + IllegalStateException.class, + () -> + listenerArgumentCaptor + .getValue() + .onActivityResult(222, Activity.RESULT_OK, resultMockIntent)); + } + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + @Test + public void + openFileReturnsNativeException_whenIllegalArgumentExceptionInGetPathFromCopyOfFileFromUri() + throws FileNotFoundException { + try (MockedStatic mockedFileUtils = mockStatic(FileUtils.class)) { + final ContentResolver mockContentResolver = mock(ContentResolver.class); + + final Uri mockUri = mock(Uri.class); + mockedFileUtils + .when(() -> FileUtils.getPathFromCopyOfFileFromUri(any(Context.class), eq(mockUri))) + .thenThrow(IllegalArgumentException.class); + mockContentResolver(mockContentResolver, mockUri, "filename", 30, "text/plain"); + + when(mockObjectFactory.newIntent(Intent.ACTION_OPEN_DOCUMENT)).thenReturn(mockIntent); + when(mockObjectFactory.newDataInputStream(any())).thenReturn(mock(DataInputStream.class)); + when(mockActivity.getContentResolver()).thenReturn(mockContentResolver); + when(mockActivityBinding.getActivity()).thenReturn(mockActivity); + final FileSelectorApiImpl fileSelectorApi = + new FileSelectorApiImpl( + mockActivityBinding, + mockObjectFactory, + (version) -> Build.VERSION.SDK_INT >= version); + + final GeneratedFileSelectorApi.NullableResult mockResult = + mock(GeneratedFileSelectorApi.NullableResult.class); + fileSelectorApi.openFile( + null, + new GeneratedFileSelectorApi.FileTypes.Builder() + .setMimeTypes(Collections.emptyList()) + .setExtensions(Collections.emptyList()) + .build(), + mockResult); + verify(mockIntent).addCategory(Intent.CATEGORY_OPENABLE); + + verify(mockActivity).startActivityForResult(mockIntent, 221); + + final ArgumentCaptor listenerArgumentCaptor = + ArgumentCaptor.forClass(PluginRegistry.ActivityResultListener.class); + verify(mockActivityBinding).addActivityResultListener(listenerArgumentCaptor.capture()); + + final Intent resultMockIntent = mock(Intent.class); + when(resultMockIntent.getData()).thenReturn(mockUri); + listenerArgumentCaptor.getValue().onActivityResult(221, Activity.RESULT_OK, resultMockIntent); + + final ArgumentCaptor fileCaptor = + ArgumentCaptor.forClass(GeneratedFileSelectorApi.FileResponse.class); + verify(mockResult).success(fileCaptor.capture()); + + final GeneratedFileSelectorApi.FileResponse file = fileCaptor.getValue(); + assertNotNull(file.getFileSelectorNativeException()); + assertEquals(file.getPath(), FileUtils.FILE_SELECTOR_EXCEPTION_PLACEHOLDER_PATH); + } + } + + @SuppressWarnings({"rawtypes", "unchecked"}) + @Test + public void + openFilesReturnsNativeException_whenIllegalArgumentExceptionInGetPathFromCopyOfFileFromUri() + throws FileNotFoundException { + try (MockedStatic mockedFileUtils = mockStatic(FileUtils.class)) { + + final ContentResolver mockContentResolver = mock(ContentResolver.class); + + final Uri mockUri = mock(Uri.class); + final String mockUriPath = "some/path/"; + mockedFileUtils + .when(() -> FileUtils.getPathFromCopyOfFileFromUri(any(Context.class), eq(mockUri))) + .thenThrow(IllegalArgumentException.class); + mockContentResolver(mockContentResolver, mockUri, "filename", 30, "text/plain"); + + when(mockObjectFactory.newIntent(Intent.ACTION_OPEN_DOCUMENT)).thenReturn(mockIntent); + when(mockObjectFactory.newDataInputStream(any())).thenReturn(mock(DataInputStream.class)); + when(mockActivity.getContentResolver()).thenReturn(mockContentResolver); + when(mockActivityBinding.getActivity()).thenReturn(mockActivity); + final FileSelectorApiImpl fileSelectorApi = + new FileSelectorApiImpl( + mockActivityBinding, + mockObjectFactory, + (version) -> Build.VERSION.SDK_INT >= version); + + final GeneratedFileSelectorApi.Result mockResult = + mock(GeneratedFileSelectorApi.Result.class); + fileSelectorApi.openFiles( + null, + new GeneratedFileSelectorApi.FileTypes.Builder() + .setMimeTypes(Collections.emptyList()) + .setExtensions(Collections.emptyList()) + .build(), + mockResult); + verify(mockIntent).addCategory(Intent.CATEGORY_OPENABLE); + verify(mockIntent).putExtra(Intent.EXTRA_ALLOW_MULTIPLE, true); + + verify(mockActivity).startActivityForResult(mockIntent, 222); + + final ArgumentCaptor listenerArgumentCaptor = + ArgumentCaptor.forClass(PluginRegistry.ActivityResultListener.class); + verify(mockActivityBinding).addActivityResultListener(listenerArgumentCaptor.capture()); + + final Intent resultMockIntent = mock(Intent.class); + final ClipData mockClipData = mock(ClipData.class); + when(mockClipData.getItemCount()).thenReturn(1); + + final ClipData.Item mockClipDataItem = mock(ClipData.Item.class); + when(mockClipDataItem.getUri()).thenReturn(mockUri); + when(mockClipData.getItemAt(0)).thenReturn(mockClipDataItem); + + when(resultMockIntent.getClipData()).thenReturn(mockClipData); + + listenerArgumentCaptor.getValue().onActivityResult(222, Activity.RESULT_OK, resultMockIntent); + + final ArgumentCaptor fileListCaptor = ArgumentCaptor.forClass(List.class); + verify(mockResult).success(fileListCaptor.capture()); + + final List fileList = fileListCaptor.getValue(); + assertEquals(fileList.get(0).getPath(), FileUtils.FILE_SELECTOR_EXCEPTION_PLACEHOLDER_PATH); + assertNotNull(fileList.get(0).getFileSelectorNativeException()); + } + } + @SuppressWarnings({"rawtypes", "unchecked"}) @Test public void getDirectoryPathReturnsSuccessfully() { diff --git a/packages/file_selector/file_selector_android/android/src/test/java/dev/flutter/packages/file_selector_android/FileUtilsTest.java b/packages/file_selector/file_selector_android/android/src/test/java/dev/flutter/packages/file_selector_android/FileUtilsTest.java index 760874317efb..a6d0f573a536 100644 --- a/packages/file_selector/file_selector_android/android/src/test/java/dev/flutter/packages/file_selector_android/FileUtilsTest.java +++ b/packages/file_selector/file_selector_android/android/src/test/java/dev/flutter/packages/file_selector_android/FileUtilsTest.java @@ -6,14 +6,12 @@ import static java.nio.charset.StandardCharsets.UTF_8; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.when; import static org.robolectric.Shadows.shadowOf; import android.content.ContentProvider; @@ -125,22 +123,6 @@ public void getPathFromCopyOfFileFromUri_returnsPathWithContent() throws IOExcep assertEquals("fileStream", fileStream); } - @Test - public void getPathFromCopyOfFileFromUri_returnsNullPathWhenSecurityExceptionThrown() - throws IOException { - Uri uri = Uri.parse("content://dummy/dummy.png"); - - ContentResolver mockContentResolver = mock(ContentResolver.class); - when(mockContentResolver.openInputStream(any(Uri.class))).thenThrow(SecurityException.class); - - Context mockContext = mock(Context.class); - when(mockContext.getContentResolver()).thenReturn(mockContentResolver); - - String path = FileUtils.getPathFromCopyOfFileFromUri(mockContext, uri); - - assertNull(path); - } - @Test public void getFileExtension_returnsExpectedFileExtension() throws IOException { Uri uri = MockContentProvider.TXT_URI; @@ -196,6 +178,18 @@ public void getPathFromCopyOfFileFromUri_returnsExpectedPathForUriWithUnknownTyp assertTrue(path.endsWith("e.f.g")); } + @Test + public void getPathFromCopyOfFileFromUri_sanitizesPathIndirection() throws IOException { + Uri uri = Uri.parse(MockMaliciousContentProvider.PNG_URI); + Robolectric.buildContentProvider(MockMaliciousContentProvider.class).create("dummy"); + shadowContentResolver.registerInputStream( + uri, new ByteArrayInputStream("fileStream".getBytes(UTF_8))); + String path = FileUtils.getPathFromCopyOfFileFromUri(context, uri); + assertNotNull(path); + assertTrue(path.endsWith("_bar.png")); + assertFalse(path.contains("..")); + } + private static class MockContentProvider extends ContentProvider { public static final Uri TXT_URI = Uri.parse("content://dummy/dummydocument"); public static final Uri PNG_URI = Uri.parse("content://dummy/a.b.png"); @@ -252,4 +246,56 @@ public int update( return 0; } } + + // Mocks a malicious content provider attempting to use path indirection to modify files outside + // of the intended directory. + // See https://developer.android.com/privacy-and-security/risks/untrustworthy-contentprovider-provided-filename#don%27t-trust-user-input. + private static class MockMaliciousContentProvider extends ContentProvider { + public static String PNG_URI = "content://dummy/a.png"; + + @Override + public boolean onCreate() { + return true; + } + + @Nullable + @Override + public Cursor query( + @NonNull Uri uri, + @Nullable String[] projection, + @Nullable String selection, + @Nullable String[] selectionArgs, + @Nullable String sortOrder) { + MatrixCursor cursor = new MatrixCursor(new String[] {MediaStore.MediaColumns.DISPLAY_NAME}); + cursor.addRow(new Object[] {"foo/../..bar.png"}); + return cursor; + } + + @Nullable + @Override + public String getType(@NonNull Uri uri) { + return "image/png"; + } + + @Nullable + @Override + public Uri insert(@NonNull Uri uri, @Nullable ContentValues values) { + return null; + } + + @Override + public int delete( + @NonNull Uri uri, @Nullable String selection, @Nullable String[] selectionArgs) { + return 0; + } + + @Override + public int update( + @NonNull Uri uri, + @Nullable ContentValues values, + @Nullable String selection, + @Nullable String[] selectionArgs) { + return 0; + } + } } diff --git a/packages/file_selector/file_selector_android/lib/file_selector_android.dart b/packages/file_selector/file_selector_android/lib/file_selector_android.dart index c86f2da2fa13..6775131236e2 100644 --- a/packages/file_selector/file_selector_android/lib/file_selector_android.dart +++ b/packages/file_selector/file_selector_android/lib/file_selector_android.dart @@ -3,3 +3,4 @@ // found in the LICENSE file. export 'src/file_selector_android.dart'; +export 'src/types/native_illegal_argument_exception.dart'; diff --git a/packages/file_selector/file_selector_android/lib/src/file_selector_android.dart b/packages/file_selector/file_selector_android/lib/src/file_selector_android.dart index bc11695265c2..8f9e46be017b 100644 --- a/packages/file_selector/file_selector_android/lib/src/file_selector_android.dart +++ b/packages/file_selector/file_selector_android/lib/src/file_selector_android.dart @@ -8,6 +8,7 @@ import 'package:file_selector_platform_interface/file_selector_platform_interfac import 'package:flutter/cupertino.dart'; import 'file_selector_api.g.dart'; +import 'types/native_illegal_argument_exception.dart'; /// An implementation of [FileSelectorPlatform] for Android. class FileSelectorAndroid extends FileSelectorPlatform { @@ -56,6 +57,9 @@ class FileSelectorAndroid extends FileSelectorPlatform { } XFile _xFileFromFileResponse(FileResponse file) { + if (file.fileSelectorNativeException != null) { + _resolveErrorCodeAndMaybeThrow(file.fileSelectorNativeException!); + } return XFile.fromData( file.bytes, // Note: The name parameter is not used by XFile. The XFile.name returns @@ -95,4 +99,19 @@ class FileSelectorAndroid extends FileSelectorPlatform { extensions: extensions.toList(), ); } + + /// Translates a [FileSelectorExceptionCode] to its corresponding error and + /// handles throwing. + void _resolveErrorCodeAndMaybeThrow( + FileSelectorNativeException fileSelectorNativeException) { + switch (fileSelectorNativeException.fileSelectorExceptionCode) { + case FileSelectorExceptionCode.illegalArgumentException: + throw NativeIllegalArgumentException( + fileSelectorNativeException.message); + case (FileSelectorExceptionCode.illegalStateException || + FileSelectorExceptionCode.ioException || + FileSelectorExceptionCode.securityException): + // unused for now + } + } } diff --git a/packages/file_selector/file_selector_android/lib/src/file_selector_api.g.dart b/packages/file_selector/file_selector_android/lib/src/file_selector_api.g.dart index 558ab5f557f4..73341425bf5d 100644 --- a/packages/file_selector/file_selector_android/lib/src/file_selector_api.g.dart +++ b/packages/file_selector/file_selector_android/lib/src/file_selector_api.g.dart @@ -1,7 +1,7 @@ // Copyright 2013 The Flutter Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -// Autogenerated from Pigeon (v22.4.2), do not edit directly. +// Autogenerated from Pigeon (v22.6.2), do not edit directly. // See also: https://pub.dev/packages/pigeon // ignore_for_file: public_member_api_docs, non_constant_identifier_names, avoid_as, unused_import, unnecessary_parenthesis, prefer_null_aware_operators, omit_local_variable_types, unused_shown_name, unnecessary_import, no_leading_underscores_for_local_identifiers @@ -18,6 +18,39 @@ PlatformException _createConnectionError(String channelName) { ); } +enum FileSelectorExceptionCode { + securityException, + ioException, + illegalArgumentException, + illegalStateException, +} + +class FileSelectorNativeException { + FileSelectorNativeException({ + required this.fileSelectorExceptionCode, + required this.message, + }); + + FileSelectorExceptionCode fileSelectorExceptionCode; + + String message; + + Object encode() { + return [ + fileSelectorExceptionCode, + message, + ]; + } + + static FileSelectorNativeException decode(Object result) { + result as List; + return FileSelectorNativeException( + fileSelectorExceptionCode: result[0]! as FileSelectorExceptionCode, + message: result[1]! as String, + ); + } +} + class FileResponse { FileResponse({ required this.path, @@ -25,6 +58,7 @@ class FileResponse { this.name, required this.size, required this.bytes, + this.fileSelectorNativeException, }); String path; @@ -37,6 +71,8 @@ class FileResponse { Uint8List bytes; + FileSelectorNativeException? fileSelectorNativeException; + Object encode() { return [ path, @@ -44,6 +80,7 @@ class FileResponse { name, size, bytes, + fileSelectorNativeException, ]; } @@ -55,6 +92,7 @@ class FileResponse { name: result[2] as String?, size: result[3]! as int, bytes: result[4]! as Uint8List, + fileSelectorNativeException: result[5] as FileSelectorNativeException?, ); } } @@ -92,11 +130,17 @@ class _PigeonCodec extends StandardMessageCodec { if (value is int) { buffer.putUint8(4); buffer.putInt64(value); - } else if (value is FileResponse) { + } else if (value is FileSelectorExceptionCode) { buffer.putUint8(129); + writeValue(buffer, value.index); + } else if (value is FileSelectorNativeException) { + buffer.putUint8(130); + writeValue(buffer, value.encode()); + } else if (value is FileResponse) { + buffer.putUint8(131); writeValue(buffer, value.encode()); } else if (value is FileTypes) { - buffer.putUint8(130); + buffer.putUint8(132); writeValue(buffer, value.encode()); } else { super.writeValue(buffer, value); @@ -107,8 +151,13 @@ class _PigeonCodec extends StandardMessageCodec { Object? readValueOfType(int type, ReadBuffer buffer) { switch (type) { case 129: - return FileResponse.decode(readValue(buffer)!); + final int? value = readValue(buffer) as int?; + return value == null ? null : FileSelectorExceptionCode.values[value]; case 130: + return FileSelectorNativeException.decode(readValue(buffer)!); + case 131: + return FileResponse.decode(readValue(buffer)!); + case 132: return FileTypes.decode(readValue(buffer)!); default: return super.readValueOfType(type, buffer); diff --git a/packages/file_selector/file_selector_android/lib/src/types/native_illegal_argument_exception.dart b/packages/file_selector/file_selector_android/lib/src/types/native_illegal_argument_exception.dart new file mode 100644 index 000000000000..b22be8042b6e --- /dev/null +++ b/packages/file_selector/file_selector_android/lib/src/types/native_illegal_argument_exception.dart @@ -0,0 +1,17 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +/// A representation of a Java IllegalArgumentException in dart. +class NativeIllegalArgumentException implements Exception { + /// Creates a [NativeIllegalArgumentException]. + NativeIllegalArgumentException(this.message); + + /// The message provided by the native error. + final String message; + + @override + String toString() { + return 'NativeIllegalArgumentException($message)'; + } +} diff --git a/packages/file_selector/file_selector_android/pigeons/file_selector_api.dart b/packages/file_selector/file_selector_android/pigeons/file_selector_api.dart index 3a6c5228a4b4..409eec5959ca 100644 --- a/packages/file_selector/file_selector_android/pigeons/file_selector_api.dart +++ b/packages/file_selector/file_selector_android/pigeons/file_selector_api.dart @@ -16,12 +16,25 @@ import 'package:pigeon/pigeon.dart'; copyrightHeader: 'pigeons/copyright.txt', ), ) +enum FileSelectorExceptionCode { + securityException, // unused + ioException, // unused + illegalArgumentException, + illegalStateException, //unused +} + +class FileSelectorNativeException implements Exception { + late final FileSelectorExceptionCode fileSelectorExceptionCode; + late final String message; +} + class FileResponse { late final String path; late final String? mimeType; late final String? name; late final int size; late final Uint8List bytes; + late final FileSelectorNativeException? fileSelectorNativeException; } class FileTypes { diff --git a/packages/file_selector/file_selector_android/pubspec.yaml b/packages/file_selector/file_selector_android/pubspec.yaml index 76087f641971..e70379fc114d 100644 --- a/packages/file_selector/file_selector_android/pubspec.yaml +++ b/packages/file_selector/file_selector_android/pubspec.yaml @@ -2,7 +2,7 @@ name: file_selector_android description: Android implementation of the file_selector package. repository: https://github.com/flutter/packages/tree/main/packages/file_selector/file_selector_android issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+file_selector%22 -version: 0.5.1+11 +version: 0.5.1+12 environment: sdk: ^3.5.0