1380950 - (Leanplum) Deeplinks should not be triggered through content or other apps

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Metrics, defect)

Description

[Frederik Braun [:freddy]]

+++ This bug was initially created as a clone of Bug #1376690 +++

Deeplinking should not happen from content, as it exposes the risks of evil website or evil webs accessing / controlling Firefox settings.

STR
1) install firefox Nightly
2) visit <https://freddyb.dubhe.uberspace.de/eimer/deeplinking.html>
3) click the button (an attacker does not have to make this behind a button)



(I could not find a way to interact with the things behind deeplinks. But if that changes, this is a _really_ bad security bugs. Thus rated sec-moderate)

Comment 1

[Frederik Braun [:freddy]]

If a website victim.com has an open redirect (a vulnerability in itself, I concede), an evil app on the phone could link to victim.com/?to=firefox://save_as_pdf which will save the content of 'victim.com?to' as a PDF to the disk.
The evil app could then access this, if it has access to device storage.

I suppose that's a stretch.

Comment 2

[Daniel Veditz [:dveditz]]

We have invented a protocol (firefox:) and exposed it to web content -- that's almost always a bad idea. Note that with "about:" in the browser we've gone the opposite direction and made lots of formerly safe about: links unreachable because it's just too much hassle worrying about it.

Comment 3

[Nevin Chen(Not active on Bugzilla)]

An evil app with device storage permission could listen to every file added to the public storage and access it anyway:)
I didn't see the deep links as the culprit.

I agree any action that generates new content is dangerous, like save_as_pdf. Even about:reader is a target of spoofing so we disable it in Bug 1358946.

Maybe we shouldn't perform that feature for the user. We should just display the UI and let them do by themselves.

Comment 4

[Frederik Braun [:freddy]]

(In reply to Nevin Chen [:nechen] from comment #3)
> An evil app with device storage permission could listen to every file added
> to the public storage and access it anyway:)
> I didn't see the deep links as the culprit.

As I said, this isn't the main problem. But the PDF file might give access to confidential third-party website content.

> 
> I agree any action that generates new content is dangerous, like
> save_as_pdf. Even about:reader is a target of spoofing so we disable it in
> Bug 1358946.
> 
> Maybe we shouldn't perform that feature for the user. We should just display
> the UI and let them do by themselves.

Note that this bug is not about save_as_pdf, but about deeplinks in general.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 56 since so far we are aiming to ship the sdk in Fennec 56.

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Hi Liz.
As in comment 4,  this isn't the main problem.
I guess we don't need to set tracking-firefox56?

Comment 7

[Paul Theriault [:pauljt] (no longer reading bugmail)]

My understanding of the issue here is that we need a way for deep links to be available for Leanplum notifications, but NOT allowed for web content in general. That was what we discussed at SF, in any case.

Nevin, can you think of a way that we can implement this restriction?

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Hi Paul
As we discuss in SF, AFAIK there's no way to restrcit deep link only for Fennec, not allowed for web content.
Cause deep link is scheme+uri in Android Intent. There's nothing I know to prevent other app to send it.

Hi Liz
May I know is this bug the blocker for RM to ship Leanplum in 56?

Comment 9

[Frederik Braun [:freddy]]

I have two suggestion that would match our baseline.

1) We should require have all firefox:// URLs to include a suffix of ?uid={{User ID}}.
This way, our deeplink handler could check that the leanplum id is indeed what we have stored for this device and abort otherwise. This is something that's also easily possible for our campaigns in the leanplum backend.

2) Without further research, I know there must also be a way to mark this activity as not exported, so that it is prevented from use by other apps.

This is just our baseline as it would still allow other web content to trigger the deeplink code and open Firefox, but we would not activate any kind of functionality unless the ID is correct (which is unknown to web content and other apps).

Comment 10

[Nevin Chen(Not active on Bugzilla)]

(In reply to Frederik Braun [:freddyb] from comment #9)
> I have two suggestion that would match our baseline.
> 
> 1) We should require have all firefox:// URLs to include a suffix of
> ?uid={{User ID}}.
> This way, our deeplink handler could check that the leanplum id is indeed
> what we have stored for this device and abort otherwise. This is something
> that's also easily possible for our campaigns in the leanplum backend.
> 

This is an excellent idea!
I can submit a patch later.
Just one thing to be noted : FxA also use deeplink for quick sign in. We can restrict deep links used by Leanplum can only be trigger by Leanplum. But for other deep links we need to skip the check. Is that okay?

> 2) Without further research, I know there must also be a way to mark this
> activity as not exported, so that it is prevented from use by other apps.
> 
Make an activity not exported can't work with intent filters(ACTION_VIEW,BROWSABLE..etc)
We need those to accept deep link

Hi Grisha.
I remember you had a patch about deep link.
Could you please check if it will be affected if I add Leanplum uid check on firefox://sign_up deep link?

Thanks!

Comment 11

[:sdaswani]

FYI on how we handle deep links used by LeanPlum on iOS: When we process one of these deep links, we can check that 'source app' that fired the deep link is Firefox, and if it isn't, we don't process the deep link. Can this be done on Android too?

Nevin, regarding adding the uid - will this affect show the Marketing team (Jean) can utilize deep links in Leanplum Messages?

Frederik, you said "Deeplinking should not happen from content, as it exposes the risks of evil website or evil webs accessing / controlling Firefox settings."

How exactly are these evil actors 'accessing / controlling Firefox settings'? From testing your STR the Android deep links just put the user on the settings screen - they don't actually toggle any of the settings?

Comment 12

[Nevin Chen(Not active on Bugzilla)]

For (In reply to Susheel Daswani from comment #11)
> FYI on how we handle deep links used by LeanPlum on iOS: When we process one
> of these deep links, we can check that 'w that fired the deep link
> is Firefox, and if it isn't, we don't process the deep link. Can this be
> done on Android too?

For web content, Freddy's solution is perfect.
For intent from other app, its also hard to block...oh wait.. maybe we can use the permission on an activity?


> Nevin, regarding adding the uid - will this affect show the Marketing team
> (Jean) can utilize deep links in Leanplum Messages?
We'll need to add ?uid={{User Id}} in for every deep links in messages.

For shipping Leanplum in 56 Since
1. There are some clarification needed
2. Deep links is fennec feature already shipped in 54
3. There's no high rish in Leanplum. It's deep links has the concern

I suggest this security review result won't block Leanplum in 56

Comment 13

[:sdaswani]

Cool Nevin - please work with Jean to solidify using the uid.

Comment 14

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> How exactly are these evil actors 'accessing / controlling Firefox
> settings'? From testing your STR the Android deep links just put the user on
> the settings screen - they don't actually toggle any of the settings?

It's less about accessing or controlling settings, and more due to the history of privilege escalation bugs in Firefox.In the past, a key step of privilege escalation attacks from script running in malicious web content has been the web content getting a reference to a privilege context. The restriction which prevents web content from loading or linking to chrome privileged content is designed to break these class of attacks by making it impossible to get such a reference. Typically just the reference alone is not enough: it would usually be combined with some other bug, but making such a separation mitigates this class of issue entirely.

In the Android case, I think there is less risk than on desktop because from testing it appears that:
-  the link is not opened directly; an about:blank tab is spawned, which then spawns the tab which holds the privileged content
- this flow means the web content has a reference to the about:blank tab, not the privileged tab, so there appears to be still a degree of separation (how strong this is, I dont know)
- this "linking" only works for actual links (ie href or similar). You can not use firefox: uris for things like src attributes on things like iframes, so this limits the potential avenues for attack.

So in this scenario, it appears that web content would be limited to spawning a new window which contains the preferences page. In testing, we weren't able to exploit this. That isn't to say there is no risk here - there may well be a way to take advantage of this given the presence of other bugs -  but given the points above, my guess at this point is that is unlikely. 

But given the history here, and given that it seems from the previous comments that a fix is possible, I would advocate that we remove the ability for web content to trigger firefox: links. 

I'm less worried about the case were other apps trigger this issue (since apps are in theory more trusted) but again, if we can fix this, then we should.

Comment 15

[Paul Theriault [:pauljt] (no longer reading bugmail)]

> But given the history here, and given that it seems from the previous
> comments that a fix is possible, I would advocate that we remove the ability
> for web content to trigger firefox: links. 

To be clear, I mean 'remove the ability for web content to deep link into privileged content'. (I don't know if there are other use cases for the firefox: uri.

Comment 16

[Frederik Braun [:freddy]]

See Paul's replies in comment 14 and comment 15.

Comment 17

[Nevin Chen(Not active on Bugzilla)]

Attachment: add_uid_to_block_deeplink_from_web_content.patch

This patch is to apply Freddy's solution using uid to block deep links from web content. Max will submit a patch for activity permission later.

Comment 18

[Frederik Braun [:freddy]]

Comment on attachment 8888734 [details] [diff] [review]
add_uid_to_block_deeplink_from_web_content.patch

Review of attachment 8888734 [details] [diff] [review]:
-----------------------------------------------------------------

I have still not completely understood why we can not do this for Firefox Accounts sign-in.

If I were to accept that, I'd like you to consider my note below:

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java
@@ +201,5 @@
>          switch (host) {
>              case LINK_DEFAULT_BROWSER:
> +                if (!isMmaDeeplink) {
> +                    break;
> +                }

It seems odd to me to repeat these 3 lines all over the codebase. It's also risky in terms of deeplinks to be added in the future.

I'd suggest testing whether it is one of the excepted cases and if not return early, if the condition is not met.

Something like
> if (host !== LINK_FXA_SIGNIN && !isMmaDeeplink) { return }

Comment 19

[:Grisha Kruglov]

For the FxAccounts signin deeplink, it's not possible to use the same leanplum UID check, because the origin of it will be very different, and the context is quite different (we don't know what device will be processing the URL when it's generated).

IIUC, leanplum deviceId is locally generated, and set via leanplum's sdk, which I presume uploads it to leanplum's servers along with the rest of its config (GCM token, something else?). The part that I'm missing is that presumably we'd also need to modify how leanplum creates the deep-links on its servers before they're pushed to the device?

For the FxA deeplinks, there is no similar out-of-band communication mechanism that we can use.

The main use case here is that an Adjust URL is sent to the user via an SMS message (or an email?), when they sign-up from desktop Firefox. When opened via some means (Chrome, maybe another install of Firefox, whatever browser user chooses), an FxA "deferred deep-link" will be triggered. The "deferred" bit means that the Adjust page will either open a Google Play Firefox page if the app isn't installed, and if it is installed (or once it's installed), firefox://fxa-signin?token=magictoken deep-link will be triggered. The token alongside some utm_ tracking params will be passed-through to the about:accounts page. The page currently uses it to identify the user and pre-fill their email, but eventually the plan is to streamline login process further (auto-login?).

And so, since we don't know what device will be opening this deep-link, the proposed solution doesn't apply conceptually. Furthermore, this deeplink must be accessible from outside of our application (from web content served by another browser, IIUC how Adjust works).

Comment 20

[Nevin Chen(Not active on Bugzilla)]

Attachment: 1380950_add_uid_to_block_deeplink_from_web_content.patch

add check and return early

Comment 21

[Frederik Braun [:freddy]]

(In reply to :Grisha Kruglov from comment #19)
> The part that I'm missing is that
> presumably we'd also need to modify how leanplum creates the deep-links on
> its servers before they're pushed to the device?

Yes, LeanPlum allows parameters in things pushed to device. e.g., something like {deviceID} is turned into the actual device id for each targeted device


> For the FxA deeplinks, there is no similar out-of-band communication
> mechanism that we can use.
> 
Well, where do the FxA deeplinks come from?

> The main use case here is that an Adjust URL is sent to the user via an SMS
> message (or an email?), when they sign-up from desktop Firefox. When opened
> via some means (Chrome, maybe another install of Firefox, whatever browser
> user chooses), an FxA "deferred deep-link" will be triggered. The "deferred"
> bit means that the Adjust page will either open a Google Play Firefox page
> if the app isn't installed, and if it is installed (or once it's installed),
> firefox://fxa-signin?token=magictoken deep-link will be triggered. The token
> alongside some utm_ tracking params will be passed-through to the
> about:accounts page. The page currently uses it to identify the user and
> pre-fill their email, but eventually the plan is to streamline login process
> further (auto-login?).
> 
> And so, since we don't know what device will be opening this deep-link, the
> proposed solution doesn't apply conceptually. Furthermore, this deeplink
> must be accessible from outside of our application (from web content served
> by another browser, IIUC how Adjust works).

I see. Hmm.

Comment 22

[Nevin Chen(Not active on Bugzilla)]

Hi Grisha
Please help reply comment 21. Thanks!

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like this doesn't need to block the feature shipping in 56.

Comment 24

[:Grisha Kruglov]

For FxA deeplinks, if the goal is to verify identity of the origin of the deeplink, we could digitally sign the token (or a distinct identity verification parameter) with server-owned private key. If the client has server's public key, it will be able to verify that the deep link is authentic.

IIUC this will alleviate risks outlined in Comment 14 and others. Admitted, public-key cryptography is not my strong suite, so feel free to highlight why this won't work and if I'm missing something.

NI Shane to comment on how feasible this is from the FxA point of view.

Comment 25

[Ryan Kelly [:rfkelly]]

> we could digitally sign the token (or a distinct identity verification parameter)
> with server-owned private key. If the client has server's public key, it will
> be able to verify that the deep link is authentic

We could do some variant of this in order to assert that the deeplink came from the FxA servers, but that's not the same thing as asserting that it's "safe" in any sense.  If an attacker wanted to trigger this deeplink for some reason, then it's likely they could acquire a valid signed token from the FxA servers in order to include in the link.  So I don't think it would buy us anything overall.

Is the concern here just that web content can trigger deeplinks to open privileged content?  IIUC there's not a lot of difference between web content opening firefox://fxa-signin?foo=bar and web content opening https://accounts.firefox.com/signin?service=sync&foo=bar in terms of privileges.  The extra powers of the FxA signin page come from it being hosted on accounts.firefox.com and Android treating that domain specially, not from it being loaded into a privileged about:accounts page.

Comment 26

[Ryan Kelly [:rfkelly]]

Also just to head off any concern with the combination of this comment:

>  The extra powers of the FxA signin page come from it being hosted on accounts.firefox.com
> and Android treating that domain specially

And this earlier comment:

> eventually the plan is to streamline login process further (auto-login?)

There are no plans to allow accounts.firefox.com to log the user in without explicit user consent, regardless of whether it's opened in about:accounts or directly through the web.

Comment 27

[Alex Davis [:adavis]]

Deeplinking is a really useful feature that we hope to be able to use from SMS, emails and even our own web content.

On iOS, it allows us to work around the fact that Firefox can't be set as a default browser resulting in Safari always opening by default. This can break many of our flows. For example, you sign up to an account after freshly installing Firefox, you confirm your email address in your inbox, Safari opens by default and now we've already lost you within moments of installing our browser. I can list more examples of flows where this happens but I think you get the point.

On Android, it helps us work around if you are not set as default browser yet. (which is possible)

Since I don't have a great understanding of the threat, I have a few questions:
- If certain pages are high privilege (riskier), can we just prevent to deeplink to those pages rather than block all deeplinks?
- On Android, how is deeplinking to a malicious PDF different than linking to a malicious PDF when Android is set as the default browser?
  - If there is a big difference, can we prevent downloads to occur from deeplinks?

Again, deeplinks are REALLY useful to us and a big part of our efforts to drive mobile app engagement and increase user retention of our products from triggers outside of our product (not just from within). I'd like us to find a solution that allows us to easily use deeplinks and perhaps even allow others to open pages in our browsers while still securing any possible risks.

Comment 28

[Nevin Chen(Not active on Bugzilla)]

I got a release tracking alert today for 56. But I think this bug shouldn't be tracked for 56 because there are still some discussion going on.

Hi Frederik
About comment 27, I can answer this one
> - On Android, how is deeplinking to a malicious PDF different than linking to a malicious PDF when Android is set as the default browser?
Ans: firefox://save_as_pdf deeplink in Android will save the current opened page to pdf and download it. So we are not deeplinking to a malicious(or any) PDF.


Could you please help anwser comment 27? Do you think my patch is enough for Leanplum deeplinks? Or do you want a total solution for all deeplinks?

Thank you!

Comment 29

[Frederik Braun [:freddy]]

Apologies for the late reply. Busy days.

(In reply to Alex Davis [:adavis] [PM FxA+Sync] from comment #27)
> - On Android, how is deeplinking to a malicious PDF different than linking
> to a malicious PDF when Android is set as the default browser?
>   - If there is a big difference, can we prevent downloads to occur from
> deeplinks?

This is not about linking to PDFs or malicious PDFs.
The idea is that linking to save_as_pdf places the currently opened website (and thus potentially sensitive content) onto the sd card without any user intervention.

> Again, deeplinks are REALLY useful to us and a big part of our efforts to
> drive mobile app engagement and increase user retention of our products from
> triggers outside of our product (not just from within). I'd like us to find
> a solution that allows us to easily use deeplinks and perhaps even allow
> others to open pages in our browsers while still securing any possible risks.

I understand this and I think the best way to mitigate this is to limit the functionality of deeplinks to showing pages, preferences or highlighting specific switches within those preferences. It should not allow performing (e.g., privileged) actions.

Comment 30

[Frederik Braun [:freddy]]

Comment on attachment 8889255 [details] [diff] [review]
1380950_add_uid_to_block_deeplink_from_web_content.patch

Review of attachment 8889255 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits. No need to flag me for review again.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java
@@ +206,4 @@
>              case LINK_DEFAULT_BROWSER:
> +                if (!isMmaDeeplink) {
> +                    break;
> +                }

Looks like you implemented the check as I suggested in canSKipMmaUid but repeat the check again here. Not necessary

@@ +275,5 @@
>                  Log.w(TAG, "Unrecognized deep link");
>          }
>      }
>  
> +    private boolean canSkipMmaUid(@NonNull final String deepLink, boolean isMmaDeeplink) {

Yep, that's the check function I was suggesting.
Can you add a comment as to why LINK_FXA_SIGNING is whitelisted, e.g., because it doesnt come from leanplum/mma?

Nit: I'd give the function a different, more speaking, name, but that's your decision.

Comment 31

[Nevin Chen(Not active on Bugzilla)]

Attachment: 1380950_add_uid_to_block_deeplink_from_web_content_0816.patch

Fix as suggested.

There are some changes coming up in related code so I'll wait for that and then land this.

Comment 32

[Nevin Chen(Not active on Bugzilla)]

The related changes just landed yesterday. I think I can start to work on rebasing and testing. But I suggest this should be later (not 57)

Hi Wesly
Maybe we need to find some time to test this ( after I complete the rebase)

Comment 33

[Wesly Huang (EPM)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #23)
> Sounds like this doesn't need to block the feature shipping in 56.

Liz, maybe you can remove the 56 tracking flag as well? I also feel this shouldn't block 56 per the discussion above.

Comment 34

[Nevin Chen(Not active on Bugzilla)]

Attachment: 1380950_add_uid_to_block_deeplink_from_web_content_1025.patch

I finally have time to rebase and land this. Target 59 since I don't have time to test it in 58 (PTO before sign-off)

Comment 35

[Nevin Chen(Not active on Bugzilla)]

I don't want to uplift this patch since 
1. I want to test more scenarios in nightly and see if it breaks other features.
2. This is not sec-high according to previous discussion.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca46fd749dc5a830ee9c34bb840efa69e002a268

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ca46fd749dc5

Comment 38

[Wesly Huang (EPM)]

Jean, soft reminder here!

Since this patch has landed on 58 Nightly, since then you'll need to add ?uid={{User ID}} in your Leanplum dashboard setting, as previously discussed in comment#12.Otherwise it won't work as 

(In reply to Nevin Chen [:nechen] from comment #12)
> For (In reply to Susheel Daswani from comment #11)
> > Nevin, regarding adding the uid - will this affect show the Marketing team
> > (Jean) can utilize deep links in Leanplum Messages?

> We'll need to add ?uid={{User Id}} in for every deep links in messages.
>

Comment 39

[Jean Collings]

This is for 58 moving forward right? To clarify an example of how I would execute a deeplink:

firefox://default_browser?uid={{User ID}}

Is this correct?

Comment 40

[Nevin Chen(Not active on Bugzilla)]

yes. It's correct. 
Please note:
When 58 goes to release, 56, 57 users still need to work with old format (i.e firefox://default_browser)
Unfortunately, 56 & 57 users can't recognise firefox://default_browser?uid={{User ID}}

Comment 41

[Jean Collings]

So for the targets I can do:

App version is 57 and less: firefox://default_browser
App version is at least 58: firefox://default_browser?uid={{User ID}}

Let me know if that works.

Comment 42

[:sdaswani]

Matt and Sorina, I see a 'qe-verify-' from a month ago - does this mean this work wasn't verified?

Comment 43

[:sdaswani]

This is confirmed working.