[file_selector_android] Refactor interactions with ContentProvider provided filenames
#8184
Conversation
|
Still need to write tests but any thoughts re: this specific implementation or using sanitation in general are welcome. The sanitization function is directly lifted from the android docs, applied it to both the file name and file extension. We could alternatively regress to not using a filename that is a function of the original file name, which is option one here (and what android recommends) |
flutter/flutter#64685 had a fair number of votes; I would strongly prefer not to regress it. If we want to go back to generated filenames, I think we would need to do that as a second, longer-term step with a new API that returns a data structure that contains the file as well as other metadata like the original name. |
...ctor_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java
Show resolved
Hide resolved
...ctor_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java
Show resolved
Hide resolved
gmackall
changed the title
ContentProvider provided filenamesContentProvider provided filenames
| // 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 { |
There was a problem hiding this comment.
I believe I could have set up mocks to get the new test to work with the existing MockContentProvider, but this felt closer to testing the actual attack to me, so I made it its own content provider.
Open to changing if it is too verbose to have this in addition to the existing MockContentProvider
There was a problem hiding this comment.
I like this and I don't think it is too verbose given the value. If someone did then I would still prefer this pattern but with the content providers moved to another file.
|
|
...ctor_android/android/src/main/java/dev/flutter/packages/file_selector_android/FileUtils.java
Show resolved
Hide resolved
..._android/android/src/test/java/dev/flutter/packages/file_selector_android/FileUtilsTest.java
Show resolved
Hide resolved
| // 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 { |
There was a problem hiding this comment.
I like this and I don't think it is too verbose given the value. If someone did then I would still prefer this pattern but with the content providers moved to another file.
| 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)) { |
There was a problem hiding this comment.
In theory, this check would allow a path traversal into a directory that has expectedDir as a prefix.
String expectedDir = "/data/data/com.app/cache/123";
String path = "/data/data/com.app/cache/123/../12345/traversal.png";
String canonicalPath = "/data/data/com.app/cache/12345/traversal.png";
canonicalPath.startsWith(expectedDir); // trueHowever, because of the filename sanitization earlier, path traversal sequences are already prevented and this cannot occur. Additionally, the expected directory is an unknown UUID. So, this is not an issue here; I just wanted to mention it for next time. :)
There was a problem hiding this comment.
I think this would be an interesting comment to convey to the authors of that method/maintainers of the page, I can try to bring it to their attention. Presumably this would be fixed by making the check instead be
canonicalPath.startsWith(expectedDir + "/")(after optionally stripping a / char at the very end so that we don't end up expecting "some/path/" + "/")
There was a problem hiding this comment.
I found a specific GitHub CodeQL docs page about the Partial Path Traversal issue that explains a better mitigation using Path objects
https://codeql.github.com/codeql-query-help/java/java-partial-path-traversal-from-remote/
gmackall
added
the
autosubmit
…vided filenames (#8188) 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 Based on the PR for `file_selector_android` #8184.
…Provider` provided filenames (flutter/packages#8184)
flutter/packages@e6932b7...bc0c22d 2024-11-27 [email protected] [image_picker_android] Refactor interactions with ContentProvider provided filenames (flutter/packages#8188) 2024-11-27 [email protected] [file_selector_android] Refactor interactions with `ContentProvider` provided filenames (flutter/packages#8184) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
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
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.