1338867 - (CVE-2017-5463) Fennec allows address bar spoofing in reader view with userinfo field in front of hostname of url

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Muneaki Nishimura]

Attachment: Screenshot of Firefox when the bug reproduced

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/602.4.8 (KHTML, like Gecko) Version/10.0.3 Safari/602.4.8

Steps to reproduce:

Connect ADB on your Android device that installed Firefox/Fennec and type following commands on the terminal.
1) adb shell
2) am start -a android.intent.action.VIEW -d about:reader?url=https://accounts.google.com%20%20%20%20%20%20%20%20%20%20%20%2[e-mail]/security/2017/01/29/mozilla-security-bytes-episode-1-csp/


Actual results:

Firefox/Fennec launches Mozilla security blog in reader view but the address bar shows "https://accounts.google.com".


Expected results:

Userinfo field in front of hostname, i.e., https://{userinfo}@hostname, should be stripped before showing it on the address bar.

Comment 1

[Daniel Veditz [:dveditz]]

Does Fennec no longer run the URI-fixup code to strip out the userinfo section?! what other clean ups are we getting wrong if so?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

We still do that.

This here seems to be only reproducible if an explicit Intent launching an about:reader URL directly is executed (See step 2 in comment 0). This could be done by an app installed on the device or maybe even from an intent link on a website.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Tim this needs some frontend help.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Let's pick this up on Fennec triage.

Comment 5

[Max Liu [:maliu]]

Attachment: 0001-Bug-1338867-Strip-username-password-after-strip-read.patch

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8847512 [details] [diff] [review]
0001-Bug-1338867-Strip-username-password-after-strip-read.patch

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

LGTM. Although I think we could do this in _stripAboutReaderURL() and only if needed.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This needs sec-approval before landing, no?
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 8

[Max Liu [:maliu]]

Comment on attachment 8847512 [details] [diff] [review]
0001-Bug-1338867-Strip-username-password-after-strip-read.patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Very easy, attackers can understand the username/password are not stripped when URL is prefixed with reader mode("about:reader?url=").

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not exactly, check-in comment does not disclose the spoofing of URL.

> Which older supported branches are affected by this flaw?
Firefox for Android 52

> If not all supported branches, which bug introduced the flaw?
N/A, since reader mode initial supported.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should be easy enough to uplift. Though it's easy to reproduce, the web page looks different in reader mode, so the risk should be low when the spoofing only happen on displayed URL.

> How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause regression. A similar code block is few lines above. No further testing required because it's easy to reproduce and test locally.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

I'm going to delay sec-approval until we're closer to the release since you say the issue is pretty obvious with the patch.

Comment 10

[Max Liu [:maliu]]

Comment on attachment 8847512 [details] [diff] [review]
0001-Bug-1338867-Strip-username-password-after-strip-read.patch

commit 66d655ee8a350ae0ad7d9f04b9775a3059c4f5d5
Author: maliu <[e-mail]>
Date:   Tue Mar 14 14:07:57 2017 +0800

    Bug 1338867 - Strip username/password after strip reader mode url prefix, r=sebastian

    MozReview-Commit-ID: KCr7cBdetq7

diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js
index 6976f8907064..c4b79d15aee8 100644
--- a/mobile/android/chrome/content/browser.js
+++ b/mobile/android/chrome/content/browser.js
@@ -4472,6 +4472,10 @@ Tab.prototype = {
       ExternalApps.updatePageActionUri(fixedURI);
     }

+    try {
+      fixedURI = URIFixup.createExposableURI(Services.io.newURI(this._stripAboutReaderURL(fixedURI.spec)));
+    } catch (ex) { }
+
     let message = {
       type: "Content:LocationChange",
       tabID: this.id,

Comment 11

[Max Liu [:maliu]]

Attachment: 0001-Bug-1338867-Strip-username-password-after-strip-read.patch

Rebase after another sec Bug 1325955 landed.

Comment 12

[Max Liu [:maliu]]

Comment on attachment 8851901 [details] [diff] [review]
0001-Bug-1338867-Strip-username-password-after-strip-read.patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Very easy, attackers can understand the username/password are not stripped when URL is prefixed with reader mode("about:reader?url=").

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not exactly, check-in comment does not disclose the spoofing of URL.

> Which older supported branches are affected by this flaw?
Firefox for Android 52

> If not all supported branches, which bug introduced the flaw?
N/A, since reader mode initial supported.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should be easy enough to uplift. Though it's easy to reproduce, the web page looks different in reader mode, so the risk should be low when the spoofing only happen on displayed URL.

> How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause regression. A similar code block is few lines above. No further testing required because it's easy to reproduce and test locally.

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8851901 [details] [diff] [review]
0001-Bug-1338867-Strip-username-password-after-strip-read.patch

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

No need to re-request review after rebasing.

However if there's still time left before this can land: Can we move this inside _stripAboutReaderURL to only do this if ReaderMode.getOriginalUrl() returned a new URL? This will avoid that we call createExposableURI() multiple times always.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8851901 [details] [diff] [review]
0001-Bug-1338867-Strip-username-password-after-strip-read.patch

sec-approval+ for trunk. We'll want Aurora and Beta patches for this.
I don't *think* we have ESR52 on Android so we shouldn't need it there.

Comment 15

[Max Liu [:maliu]]

Attachment: 0003-Bug-1338867-Strip-username-password-after-strip-read.patch

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8852751 [details] [diff] [review]
0003-Bug-1338867-Strip-username-password-after-strip-read.patch

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

::: mobile/android/chrome/content/browser.js
@@ +4470,5 @@
>      if (BrowserApp.selectedTab == this) {
>        ExternalApps.updatePageActionUri(fixedURI);
>      }
> +    // Strip reader mode URI and also make it exposable if needed
> +    fixedURI = this._stripAboutReaderURL(fixedURI);

This is already called a couple of lines above. I assume it doesn't need to be called again? At least this was my intention by moving the code inside stripReaderURL().

@@ +4498,5 @@
>        this.hasTouchListener = false;
>        Services.obs.notifyObservers(this.browser, "Session:NotifyLocationChange", null);
>      }
>    },
> +  _stripAboutReaderURL: function (nsURI) {

nit: Why the renaming of the parameter to nsURI? In the code below it is a bit confusing - I first though you are calling methods on the XPCOM interface somehow.

Comment 17

[Max Liu [:maliu]]

Attachment: 0004-Bug-1338867-Strip-username-password-after-strip-read.patch

(In reply to Sebastian Kaspari (:sebastian) from comment #16)
> ::: mobile/android/chrome/content/browser.js
> > + // Strip reader mode URI and also make it exposable if needed
> > + fixedURI = this._stripAboutReaderURL(fixedURI);
> 
> This is already called a couple of lines above. I assume it doesn't need to
> be called again? At least this was my intention by moving the code inside
> stripReaderURL().

The other invocation you mention was removed by another security Bug 1325955. So I guess both of these two patches should be uplifted to release/beta/aurora at the same time?


> @@ +4498,5 @@
> > this.hasTouchListener = false;
> > Services.obs.notifyObservers(this.browser, "Session:NotifyLocationChange", null);
> > }
> > },
> > + _stripAboutReaderURL: function (nsURI) {
> 
> nit: Why the renaming of the parameter to nsURI? In the code below it is a
> bit confusing - I first though you are calling methods on the XPCOM
> interface somehow.

My intention was trying to apply a type naming convention on the parameter, telling people remember to pass URI object not a string as it used to be. Obviously, I misused the "ns" prefix, so I guess "originalURI" is better? :)

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Should we land this?

Comment 19

[Max Liu [:maliu]]

Comment on attachment 8853279 [details] [diff] [review]
0004-Bug-1338867-Strip-username-password-after-strip-read.patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Very easy, attackers can understand the username/password are not stripped when URL is prefixed with reader mode("about:reader?url=").

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not exactly, check-in comment does not disclose the spoofing of URL.

> Which older supported branches are affected by this flaw?
Firefox for Android 52

> If not all supported branches, which bug introduced the flaw?
N/A, since reader mode initial supported.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should be easy enough to uplift. Though it's easy to reproduce, the web page looks different in reader mode, so the risk should be low when the spoofing only happen on displayed URL.

> How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause regression. A similar code block is few lines above. No further testing required because it's easy to reproduce and test locally.

Comment 20

[Max Liu [:maliu]]

Comment on attachment 8853279 [details] [diff] [review]
0004-Bug-1338867-Strip-username-password-after-strip-read.patch

Approval Request Comment
[Feature/Bug causing the regression]: Reader Mode
[User impact if declined]: Url spoofing could impact security in reader mode
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes, STR: refer to adb command in description
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Reuse Url fixup logic.
[String changes made/needed]: None

Comment 21

[Al Billings [:abillings - ex-MoCo]]

I can't give approvals this late in the cycle unless you make a good case for it and Release Management approves it. We've mostly run out of room here given how close the release is.

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that we still have an entire week till we go live and that this maybe easily exploitable, I would lean towards taking it in the release. But Liz owns 53 and should make a final decision.

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 8853279 [details] [diff] [review]
0004-Bug-1338867-Strip-username-password-after-strip-read.patch

This is a fine fix so sec-approval=dveditz and I'll let release drivers do the usual branch approvals.

While I wouldn't want to skip what this patch adds, I think it misses the underlying problem that we may be accepting too many intents. On desktop firefox we have a list of intentionally exposed schemes, and if someone tries to call us with something other than that we reject it. There's no legit reason other apps are calling about:reader, or really any about: url that I can think of. Do we have such a whitelist?

Comment 24

[Daniel Veditz [:dveditz]]

Although I'd call this spoof sec-high if it could be launched from web content inside Fennec, the fact that it has to be launched from another app lowers the risk somewhat. A straight out malicious app could do it, but more likely might be 1) user has Firefox as the default browser and 2) malicious ads in an otherwise OK app trigger this intent.

Comment 25

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

Comment on attachment 8853279 [details] [diff] [review]
0004-Bug-1338867-Strip-username-password-after-strip-read.patch

The risk here seems to be for reader mode, the exploit might be obvious from the patch that we've already landed.  So let's take this on beta.

Comment 26

[Julien Cristau [:jcristau]]

fennec issue, so doesn't apply to esr

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/344527734950caddf7279c5ee5c7be78532362bb

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee6e9f3833ea
https://hg.mozilla.org/releases/mozilla-beta/rev/62e3e4ef5e6f

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Daniel Veditz [:dveditz] from comment #23)
> While I wouldn't want to skip what this patch adds, I think it misses the
> underlying problem that we may be accepting too many intents. On desktop
> firefox we have a list of intentionally exposed schemes, and if someone
> tries to call us with something other than that we reject it. There's no
> legit reason other apps are calling about:reader, or really any about: url
> that I can think of. Do we have such a whitelist?

That's a good question. We have explicitly exposed the "about" scheme in our manifest [1]. This means whenever the user clicks an about:// link anywhere in the Android system Firefox will offer to open it. I'm not sure why this was done in the past. We do the same for javascript which is a little bit scarier.

However Chrome seems to do the same [3].

Anyhow, our code handling the intent doesn't actually care about the URL and is happy to forward just basically anything to Gecko [4][5][6]. This means an app that is sending an explicit intent to us (not going through the intent resolution system which uses the intent-filters).

So there might be two issues:
* (1) Should we allow schemes like about:// and javascript:// to be routed to our app by the Android system?
* (2) Should we implement a whitelist for incoming URLs that basically matches our intent-filter?

(2) seems to be a good idea. I'm not sure about (1). Do you consider any of those two a security issue (for filling follow-ups)?

[1] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#99
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#100
[3] https://cs.chromium.org/chromium/src/chrome/android/java/AndroidManifest.xml?l=187
[4] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#2166-2175
[5] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#937
[6] https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#966

Comment 31

[Frederik Braun [:freddy]]

(In reply to Sebastian Kaspari (:sebastian) from comment #30)
> * (1) Should we allow schemes like about:// and javascript:// to be routed
> to our app by the Android system?
> * (2) Should we implement a whitelist for incoming URLs that basically
> matches our intent-filter?
> 
> (2) seems to be a good idea. I'm not sure about (1). Do you consider any of
> those two a security issue (for filling follow-ups)?

I consider (1) a security issue, especially for javascript URLs.
All I can see is executing code on someone's behalf, where the someone could also be an attacker.
Where do you expect to get those from and what's the desired outcome? Maybe I'm missing something.

Comment 32

[Daniel Veditz [:dveditz]]

javascript: urls are either useless or dangerous--we shouldn't allow those. If we open a new tab for them then script will run in an empty context and can't do much of anything useful. At best it could trigger a known browser exploit? If we open it against the "active" tab as we used to do in Desktop then it's an XSS attack. In Firefox Desktop we no longer accept javascript: urls from outside the browser.

Why do we want to accept about: urls? I worry that there's some internal feature I'm not seeing in about:about that maybe we rely on. I can't think of any good reason to allow external apps to link to about: urls, and it adds attack surface because many of these can be parameterized. Note that we recently went through and made most about: urls unlinkable from web content, even the supposedly "safe" unprivileged ones.

data: urls can also cause problems, but we should generally allow them. The problems would arise if we open a data: url in a window that already has content because we'd inherit the principal. If we open a new tab for them then it's fine.

If "intents" aren't the only way an external app can trigger a content load then this filtering has to be applied somewhere upstream so it applies to both. Or, if there's one specific about: url we need to allow for some reason I guess you could leave it available in the INTENT filter and then fail the load for any but the acceptable one.

In general it's unsafe to allow external apps to load content that web content isn't allowed to load. There may be exceptions for specific things we want to expose to a platform, but those exceptions should be narrow and need a security review.

Comment 33

[Sebastian Kaspari (:sebastian; :pocmo)]

Follow-up bug for the intent filters / whitelisting: bug 1357377

Comment 34

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Daniel Veditz [:dveditz] from comment #32)
> javascript: urls are either useless or dangerous--we shouldn't allow those.
> If we open a new tab for them then script will run in an empty context and
> can't do much of anything useful. At best it could trigger a known browser
> exploit? If we open it against the "active" tab as we used to do in Desktop
> then it's an XSS attack. In Firefox Desktop we no longer accept javascript:
> urls from outside the browser.
> 
> Why do we want to accept about: urls? I worry that there's some internal
> feature I'm not seeing in about:about that maybe we rely on. I can't think
> of any good reason to allow external apps to link to about: urls, and it
> adds attack surface because many of these can be parameterized. Note that we
> recently went through and made most about: urls unlinkable from web content,
> even the supposedly "safe" unprivileged ones.

We pass about: URLs to Fennec to start the Firefox Accounts sign up/sign in flow, potentially when Fennec has not yet started.  See [1] [2] [3].

This behaviour is not required -- it's just convenient -- and I can't think of a reason that it has to be the "regular URL" flow via an Intent.  I expect we could add replace this with a custom IntentFilter just for about:accounts if we so desired.

See also Bug 1356893, which feels similar.

[1] https://dxr.mozilla.org/mozilla-central/rev/a374c35469935a874fefe64d3e07003fc5bc8884/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountWebFlowActivity.java#69
[2] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusFragment.java#235
[3] https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/activities/FxAccountStatusFragment.java#240