Bug 1338867 (CVE-2017-5463)

Fennec allows address bar spoofing in reader view with userinfo field in front of hostname of url

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sdna.muneaki.nishimura, Assigned: maliu)

Tracking

({csectype-spoof, sec-moderate})

Trunk
Firefox 55
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr45 unaffected, fennec53+, firefox52 wontfix, firefox-esr52 unaffected, firefox53+ fixed, firefox54+ fixed, firefox55+ fixed)

Details

(Whiteboard: [adv-main53+])

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

2 years ago
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%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20@blog.mozilla.org/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.
tracking-fennec: --- → ?
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?
Flags: needinfo?(s.kaspari)
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.
Flags: needinfo?(s.kaspari)
Tim this needs some frontend help.
Flags: needinfo?(timdream)
Let's pick this up on Fennec triage.
Flags: needinfo?(timdream)
Flags: sec-bounty?
Assignee

Updated

2 years ago
Assignee: nobody → max
Assignee

Comment 5

2 years ago
Attachment #8847512 - Flags: review?(s.kaspari)
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.
Attachment #8847512 - Flags: review?(s.kaspari) → review+
Assignee

Updated

2 years ago
Keywords: checkin-needed
This needs sec-approval before landing, no?
https://wiki.mozilla.org/Security/Bug_Approval_Process
Flags: needinfo?(max)
Keywords: checkin-needed
Assignee

Comment 8

2 years ago
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.
Attachment #8847512 - Flags: sec-approval?
Assignee

Updated

2 years ago
Flags: needinfo?(max)
Assignee

Updated

2 years ago
Keywords: checkin-needed
Assignee

Updated

2 years ago
Keywords: checkin-needed
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.
Assignee

Comment 10

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

commit 66d655ee8a350ae0ad7d9f04b9775a3059c4f5d5
Author: maliu <max@mxli.us>
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,
Assignee

Comment 11

2 years ago
Rebase after another sec Bug 1325955 landed.
Attachment #8851901 - Flags: review?(s.kaspari)
Assignee

Updated

2 years ago
Attachment #8847512 - Attachment is obsolete: true
Attachment #8847512 - Flags: sec-approval?
Assignee

Comment 12

2 years ago
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.
Attachment #8851901 - Flags: sec-approval?
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.
Attachment #8851901 - Flags: review?(s.kaspari) → review+
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.
Attachment #8851901 - Flags: sec-approval? → sec-approval+
Assignee

Comment 15

2 years ago
Attachment #8851901 - Attachment is obsolete: true
Attachment #8852751 - Flags: review?(s.kaspari)
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.
Attachment #8852751 - Flags: review?(s.kaspari) → review+
tracking-fennec: ? → 53+
Assignee

Comment 17

2 years ago
(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? :)
Attachment #8852751 - Attachment is obsolete: true
Attachment #8853279 - Flags: review?(s.kaspari)
Attachment #8853279 - Flags: review?(s.kaspari) → review+
Should we land this?
Flags: needinfo?(max)
Assignee

Comment 19

2 years ago
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.
Flags: needinfo?(max)
Attachment #8853279 - Flags: sec-approval?
Assignee

Comment 20

2 years ago
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
Flags: needinfo?(abillings)
Attachment #8853279 - Flags: approval-mozilla-beta?
Attachment #8853279 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Flags: needinfo?(jcristau)
Flags: needinfo?(abillings)
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.
Flags: needinfo?(rkothari)
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?
Flags: needinfo?(s.kaspari)
Attachment #8853279 - Flags: sec-approval? → sec-approval+
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.
Keywords: sec-highsec-moderate
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.
Flags: needinfo?(lhenry)
Attachment #8853279 - Flags: approval-mozilla-beta?
Attachment #8853279 - Flags: approval-mozilla-beta+
Attachment #8853279 - Flags: approval-mozilla-aurora?
Attachment #8853279 - Flags: approval-mozilla-aurora+
Assignee

Updated

2 years ago
Keywords: checkin-needed
fennec issue, so doesn't apply to esr
Flags: needinfo?(jcristau)
https://hg.mozilla.org/mozilla-central/rev/344527734950
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Group: firefox-core-security → core-security-release
(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
Flags: needinfo?(s.kaspari)
Flags: needinfo?(dveditz)
(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.
Alias: CVE-2017-5463
Whiteboard: [adv-main53+]
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.
Flags: needinfo?(dveditz)
Flags: sec-bounty? → sec-bounty+
Follow-up bug for the intent filters / whitelisting: bug 1357377
(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
See Also: → CVE-2017-7759

Updated

2 years ago
Depends on: CVE-2017-7762
Duplicate of this bug: 1269742
Depends on: 1358946
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.