1869250 - "File not found" when opening a PDF from Whatsapp, Dropbox and other apps

Status: VERIFIED FIXED

(GeckoView :: PDF Viewer, defect, P1)

Description

[Julien Wajsberg [:julienw]]

STR:

1. Tap a PDF from a Whatsapp discussion.
2. 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

Comment 1

[Marco Castelluccio [:marco]]

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

[Andrew Gaul]

Also not working for Dropbox.

Comment 3

[Olivia Hall [:olivia]]

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

[Titouan Thibaud [:titouan]]

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

[Marco Castelluccio [:marco]]

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").

Comment 6

[Olivia Hall [:olivia]]

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

[Jonathan Almeida [:jonalmeida]]

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, and exported 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

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 9

[Olivia Hall [:olivia]]

Thanks for the discussion, I'll put a patch up shortly!

Comment 10

[Olivia Hall [:olivia]]

Attachment: Bug 1869250 - Geckoview Adjust PDF Viewer Input Stream Requirements

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

[Daniel Veditz [:dveditz]]

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

[[:owlish] 🦉 PST]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d38bc6246a8
Geckoview Adjust PDF Viewer Input Stream Requirements r=geckoview-reviewers,owlish

Comment 14

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3d38bc6246a8

Comment 15

[Olivia Hall [:olivia]]

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

[miralobontiu]

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.