"File not found" when opening a PDF from Whatsapp, Dropbox and other apps
Categories
(GeckoView :: PDF Viewer, defect, P1)
Tracking
(firefox120 unaffected, firefox121 unaffected, firefox122+ verified)
Tracking | Status | |
---|---|---|
firefox120 | --- | unaffected |
firefox121 | --- | unaffected |
firefox122 | + | verified |
People
(Reporter: julienw, Assigned: olivia)
References
Details
Attachments
(1 file)
STR:
- Tap a PDF from a Whatsapp discussion.
- Choose "Firefox Nightly" when the choice is offered
=> I get a "File not found" error
The URL looks like so: content://com.whatsapp.provider.media/item/844e823f-146a-43cc-acdb-1af952007dee
Updated•10 months ago
|
Comment 1•10 months ago
|
||
Bug 1868382 added an allow list, but that's not enough for all apps.
Titouan, Calixte mentioned you'd be OK with removing the allow list and just allowing all apps?
[Tracking Requested - why for this release]: Marking as Tracked for 122 because we can't ship Fenix as default PDF reader (bug 1864956) without fixing this.
Comment 2•10 months ago
|
||
Also not working for Dropbox.
Updated•10 months ago
|
Updated•10 months ago
|
Assignee | ||
Comment 3•10 months ago
|
||
I suggest we might need some additional discussion on the best approach here. We might need to take the time to curate an allow-list v. allowing all apps. I'm not sure which is most appropriate based on the other mitigations. Could we have some further discussion on the pros and cons of both approaches?
Some of the original discussion happened on this patch in bug 1815739, for additional context. The allow-list in bug 1868382 was to allow an additional path forward when exported
was false.
(ni?: I'm just trying to facilitate the discussion, please jump in if you have an opinion or more information.)
Comment 4•10 months ago
•
|
||
I haven't had time to dive into this yet, so my answer may be imcomplete.
After talking with Calixte, I'd say maintaining the allow-list can be complicated and expensive, as we would have to keep adding new apps as people file bugs (I guess). Each country can have specific apps people are used to, etc.
But I'm not sure of the possible impacts of removing the allow-list in terms of security.
Comment 5•10 months ago
|
||
My only thought is what Calixte said in bug 1868382 comment 0: the mitigations described in https://developer.android.com/privacy-and-security/risks/content-resolver#mitigations_2 are connected with "OR" and not "AND", so it should be OK that only one of them applies (and so it should be OK to ignore "exported").
Assignee | ||
Comment 6•10 months ago
|
||
One item that keeps giving me pause whenever I read the list of mitigations is the first item in the general section: Validate incoming URIs. For example, using an allowlist of expected authorities is considered good practice.
, but it doesn't really give more information.
That being mentioned, the "ORs" do make it seem okay to remove the isExported
check based on the ORs
orgaization under the main section as long as we check the other criteria, but I haven't done a deep dive for more information.
Comment 7•10 months ago
|
||
Since we landed a solution for bug 1868382 that used an allow list, we looked at the apk for the Files app and found this on that specific content provider:
<provider
android:name="com.google.android.libraries.storage.storagelib.FileProvider"
android:exported="false"
android:authorities="com.google.android.apps.nbu.files.provider"
android:grantUriPermissions="true" />
There is no exported
attribute there, but there is a grantUriPermissions
which seems to have been enough for other consumer applications and Google Files as a provider.
Based on what we read in the android:grantUriPermissions
documentation, it seems like our checks are fairly correct, but we should probably change the logic (as olivia hinted above):
Whether those who ordinarily don't have permission to access the content provider's data can be granted permission to do so, temporarily overcoming the restriction imposed by the
readPermission
,writePermission
,permission
, andexported
attributes.
So instead of including the isExported
AND wasGrantedPermission
as we currently have:
(isExported(context, uri) || isInAllowList(context, uri))
&& wasGrantedPermission(context, uri)
We probably need something that uses an OR (while also removing the allow list code):
(isExported(context, uri) || wasGrantedPermission(context, uri)
The problem with the allow list solution is also that there is no way to verify that com.google
app IDs are safe. Any one can register a new app with com.google.fake.app
tomorrow and that would be published. We have to use the fully qualified authority ID in that case, which then leads to the problems mentioned about around maintaining an ever growing list.
An allow list would make more sense for applications that are all related (e.g. under the same owner), but for our use case as a PDF reader, we need to be more open which means a bit more vulnerable too.
Leaving dveditz NI on in case they want to add something else.
Comment 8•10 months ago
|
||
The bug is marked as tracked for firefox122 (nightly). We have limited time to fix this, the soft freeze is in 2 days. However, the bug still isn't assigned.
:olivia, could you please find an assignee for this tracked bug? If you disagree with the tracking decision, please talk with the release managers.
For more information, please visit BugBot documentation.
Assignee | ||
Updated•10 months ago
|
Assignee | ||
Comment 9•10 months ago
|
||
Thanks for the discussion, I'll put a patch up shortly!
Assignee | ||
Comment 10•10 months ago
|
||
This bug adjusts the requirements for opening a PDF to fully use "OR"
statements when resolving and removes the allowlist.
See Android Documentation
for more details.
Comment 11•10 months ago
|
||
If the goal of the feature is to become the "default PDF Displayer" on the device then you can't use an allow-list. How can we know ahead of time all the apps that could exist in the future on someone's phone? But to be sure, this safe because of the limitations on PDFs that wouldn't apply if we implemented a more generic content:
handler that dealt with a wider variety of formats.
- we don't return any values, so even if an app tried to trick us into loading content that it couldn't read itself we'd just display it
- we already have to handle arbitrary maliciously malformed PDFs from the web
- the PDF is self-contained. it's not combined with other content that could trick the user about the source of the data or trigger actions in some other app or service through some special URL
Comment 12•10 months ago
|
||
Thinking about it now, and reading the discussion - I come to agree that allow list is not the best solution here. I agree with dveditz.
Comment 13•10 months ago
|
||
Comment 14•10 months ago
|
||
bugherder |
Assignee | ||
Comment 15•10 months ago
•
|
||
Hi QA,
Could you test opening a PDF from Whatsapp, Google Files, Dropbox, or any other similar app using Firefox Fenix for Android as the PDF viewer?
The first Fenix build with the change present will be GV: 122.0a1
20231214095424
in the Settings.
Comment 16•10 months ago
|
||
Hi Olivia,
I can confirm that this is verified as fixed on the latest Fenix Nightly 122.0a1 from 12/15 with a Samsung Galaxy A53 (Android 13) device.
I've opened a PDF file from Whatsapp, and Google Drive, and the PDFs were opened in Fenix Nightly in new tabs without issues.
Description
•