1656747 - Fenix allows navigating from http: to file: URLs

Status: VERIFIED FIXED

(Firefox for Android :: General, task)

Description

[Muneaki Nishimura]

Similar to Bug 1356893 on Firefox for Android, fenix://open?url=file:///sdcard on Fenix allows navigation from http: to file: URLs (also possible to resource:// about: URLs etc.).

1. Launch http://csrf.jp/2020/fenix_file_bypass.html on Fenix
2. Tap any links then file:///sdcard/Download, resource://android and about:networking is shown in new tab

Note that the page in the 1) has the following links.

<h1>For Fenix Beta</h1>
<a href=fenix-beta://open?url=file:///sdcard/Download>file:///sdcard/Download</a><br>
<a href=fenix-beta://open?url=resource://android>resource://android</a><br>
<a href=fenix-beta://open?url=about:networking>about:networking</a><br>
<h1>For Fenix Nightly</h1>
<a href=fenix-nightly://open?url=file:///sdcard/Download>file:///sdcard/Download</a><br>
<a href=fenix-nightly://open?url=resource://android>resource://android</a><br>
<a href=fenix-nightly://open?url=about:networking>about:networking</a><br>

Comment 1

[Frederik Braun [:freddy]]

Sebastian, can you take a look?

Comment 2

[Frederik Braun [:freddy]]

Sebastian asked elsewhere, posting reasoning why this is sec-high for posterity:

1. attacker triggers a download (e.g., through an <a href=evil.html download> and javascript .click() method) of evil.html on the filesystem, most preferable the sdcard's root directory
2. navigate to the evil.html file using the techniques from comment 0.
3. evil.html is considered same-origin with all files in the same directory (and subdirectory). Use fetch()/XMLHttpRequest/iframe to read those files or send them to an attacker-controlled server (using fetch/XMLHttpRequest/form, etc.)

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

This is caused by a mix of two features:

• We introduced the fenix(-beta/nightly) scheme that is used by Leanplum push messages that marketing sends out. They can open URL, but also do other things (1, 2).
• The "app links" feature (on by default) launches third-party apps for links where a third-party app has a registered intent filter (In this case it is Fenix itself).

We had a similar issue in Fennec before:
https://bugzilla.mozilla.org/show_bug.cgi?id=1380950

This is the patch we landed in Fennec:
https://hg.mozilla.org/mozilla-central/rev/ca46fd749dc5

In Fennec we require deep links to contain "&uid={uid}" (the UID of the leanplum user/device). Only if the UID matches the local UID we follow the deeplink. Apparently that was something that was possible to do for the marketing team.

I started looking into writing a similar patch for Fenix.. but on first sight it appears like we generate a randomized devide ID for leanplum on every app start (need to verify at runtime, maybe I can later ask the marketing team to verify with their data too)?
https://github.com/mozilla-mobile/fenix/blob/master/app/src/main/java/org/mozilla/fenix/components/metrics/LeanplumMetricsService.kt#L81

Fennec on the other hand seems to only generate a device ID if there's none persisted yet. Otherwise it reuses the existing one:
https://searchfox.org/mozilla-esr68/source/mobile/android/base/java/org/mozilla/gecko/mma/MmaDelegate.java#110-115

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(Pinged some folks to figure out if the regeneration of the devide ID is a mistake or done on purpose in Fenix)

Comment 5

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

We should include the same &uid={User ID} mechanism in Fenix.

Sebastian can you verify if we really randomize the device ID at every start? That sounds like a serious bug that will completely mess with our Leanplum metrics and usage. Looping in :liuche about this, the Fenix team can address that ASAP.

Comment 6

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Unless of course the random ID at every start was intentional - but I find that hard to believe as it will make it impossible to properly trigger campaigns based on past events.

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Sebastian can you verify if we really randomize the device ID at every start? That sounds like a serious bug that will completely mess with our Leanplum metrics and usage.

CC'ed Jeff. We looked at this and yeah, it seems like we generate a new ID on every start. We will have to fix that first before we can do the same check as Fennec.

Comment 8

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Not a blocker for the next Fenix rollout stage. But let's include at the next opportunity, before we hit tier 2 countries.

Comment 9

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddy] from comment #2)

3. evil.html is considered same-origin with all files in the same directory (and subdirectory). Use fetch()/XMLHttpRequest/iframe to read those files or send them to an attacker-controlled server (using fetch/XMLHttpRequest/form, etc.)

Have we not restricted inter-file: access on fenix as we have on desktop? If not, is there a tracking bug for this / what prevents us from doing this?

(This bug would be problematic either way but perhaps less severe...)

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 0001-Validate-deep-links.patch

This patch does multiple things:

• (The primary fix here) Only process a deep link that has a "uid" query parameter that matches the local Leanplum device ID. (Same validation we added to Fennec in Bug 1380950)
• Mitigating risk: For deep links that load a URL (instead of performing an action inside the app) limit them to only load "https://" URLs.
• Mitigating risk: For deep links that load a URL pass the "EXTERNAL" flag to the load request to let Gecko(View) know that this is an external URL and additional checks may need to happen.

This patch should also address bug 1656746.

Comment 11

[Christian Sadilek [:csadilek]]

Comment on attachment 9168107 [details] [diff] [review]
0001-Validate-deep-links.patch

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

Looks good! One question below re: openBrowserToLoad (with forceSearch = true).

::: app/src/main/java/org/mozilla/fenix/HomeActivity.kt
@@ +619,4 @@
>          }
>  
>          if (!forceSearch && searchTermOrURL.isUrl()) {
> +            loadUrlUseCase.invoke(searchTermOrURL.toNormalizedUrl(), flags)

nit: the load url flags are only used if we're not searching. The url loaded by the search use cases will not have the flags (e.x. .external()) set. Are we OK with that? We should probably add a comment to the method param then.

::: app/src/main/java/org/mozilla/fenix/home/intent/DeepLinkIntentProcessor.kt
@@ +137,4 @@
>                  }
>              }
>          }
> +

nit: Can we add some docs here describing the format of the deep link and what the verifier does :). Basically just explaining the uid and url query parameters and what we're verifying.

Comment 12

[Chenxia Liu [:liuche]]

We'll will want this on the Beta channel too, which is 80.0.0, so that it won't regress when 80 goes out.

When do you think you'll want to land this? We plan to release 80.0.0 to beta 8/6, but we could do a dot release afterwards with this fix. When you open the PR against the 79 branch, could you also open a PR against the releases/v80.0.0 branch? Then we'll tag an 80.0.0 beta dot release.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Christian Sadilek [:csadilek] from comment #11)

nit: the load url flags are only used if we're not searching. The url loaded
by the search use cases will not have the flags (e.x. .external()) set. Are
we OK with that? We should probably add a comment to the method param then.

I think that is fine since the search URL is not really external, we craft that ourselves. But yeah, I'll add the comment to make this more explicit.

nit: Can we add some docs here describing the format of the deep link and
what the verifier does :). Basically just explaining the uid and url query
parameters and what we're verifying.

Oh, yeah, absolutely. Will update the patch.(In reply to Chenxia Liu [:liuche] from comment #12)

When do you think you'll want to land this? We plan to release 80.0.0 to beta 8/6, but we could do a dot release afterwards with this fix. When you open the PR against the 79 branch, could you also open a PR against the releases/v80.0.0 branch? Then we'll tag an 80.0.0 beta dot release.

There are some other security patches (at least one more sec-high) that I would like to get in. I assume we need to coordinate the landing so that it goes out to all channels more or less at once (including 79 which is in rollout). I'll ping you and some others about that. Other than that my plan is to address the review nits, get a final r+ and request sec approval.

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 0001-Validate-deep-links.patch

Updated patch addressing review comments.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

For some reason I cannot request sec-approval on this patch (the field is just missing in Bugzilla)?

So I'll try to do that manually:

Security Approval Request

• How easily could an exploit be constructed based on the patch?: The patch reveals that we do not validate deep links. From that you may guess that you can launch them from web content (although that is not something the patch shows and it could also "just" be possible for third-party apps to do that). Since there is a (now public) Fennec bug about exactly the same problem (bug 1380950), it may be obvious.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
• Which older supported branches are affected by this flaw?: All Fenix releases (Nightly = master, Beta = releases/v80*, Release=releases/v79*)
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: From testing it seems unlikely to cause regressions. In the worst case we may break deep links from marketing push messages, which seems like something that is not critical and could be fixed in a follow-up release.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 9168419 [details] [diff] [review]
0001-Validate-deep-links.patch

sec-approval+

Fixed the sec-approval flag so it shows up in Fenix.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Patch for this landed in AC 48/52 and was part of Fenix build 79.0.3 and 80.0.0 Beta 5.

Comment 18

[Diana Rus]

Verified as fixed on Firefox RC 79.0.3 and 80.0.0 beta 5 with:

• Sony Xperia Z5 (Android 7)
• Samsung Galaxy S9 (Android 8)
• Google Pixel 3XL (Android 9)

Comment 19

[Diana Rus]

Verified as fixed on Nightly 8/11 with:

• Sony Xperia Z5 (Android 7)
• Samsung Galaxy S9 (Android 8)
• Google Pixel 3XL (Android 9)

Comment 20

[Frederik Braun [:freddy]]

(In reply to :Gijs (he/him) from comment #9)

(In reply to Frederik Braun [:freddy] from comment #2)

3. evil.html is considered same-origin with all files in the same directory (and subdirectory). Use fetch()/XMLHttpRequest/iframe to read those files or send them to an attacker-controlled server (using fetch/XMLHttpRequest/form, etc.)

Have we not restricted inter-file: access on fenix as we have on desktop? If not, is there a tracking bug for this / what prevents us from doing this?

(This bug would be problematic either way but perhaps less severe...)

I hope you don't mind me redirecting this. ckerschb, can you add to that? I couldn't find any bugs.

Comment 21

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Frederik Braun [:freddy] from comment #20)

(In reply to :Gijs (he/him) from comment #9)

(In reply to Frederik Braun [:freddy] from comment #2)

3. evil.html is considered same-origin with all files in the same directory (and subdirectory). Use fetch()/XMLHttpRequest/iframe to read those files or send them to an attacker-controlled server (using fetch/XMLHttpRequest/form, etc.)

Have we not restricted inter-file: access on fenix as we have on desktop? If not, is there a tracking bug for this / what prevents us from doing this?

(This bug would be problematic either way but perhaps less severe...)

I hope you don't mind me redirecting this. ckerschb, can you add to that? I couldn't find any bugs.

Is that different in Fenix than in Firefox Desktop? On Desktop all file: URIs are treated as unique origins (see Bug 1558299). I would hope that is also the case on Fenix. If not, we should instantly fix that.

Comment 22

[Frederik Braun [:freddy]]

oh. I was not aware of the work in bug 1558299. I think this bug is a bit less severe then..Not sure if that makes it sec-moderate already.

Comment 23

[Frederik Braun [:freddy]]

(In reply to Frederik Braun [:freddy] from comment #22)

oh. I was not aware of the work in bug 1558299. I think this bug is a bit less severe then..Not sure if that makes it sec-moderate already.