1325955 - (CVE-2017-5450) Address bar spoofing on Android

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[gnehsoah]

Attachment: Screenshot_20161227-221200.png

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36

Steps to reproduce:

User Agent: "Mozilla/5.0 (Android 6.0.1; Mobile; rv:50.1.0) Gecko/50.1.0 Firefox/50.1.0"

POC:

http://192.243.113.21/spoof/firefox/url1.html



Actual results:

The fake top level domain has been highlighted and the site attribute is wrong(see screenshot).


Expected results:

The site attribute should display the correct domain.

Comment 1

[gnehsoah]

Attachment: Screenshot_20161227-221242.png

Comment 2

[gnehsoah]

Attachment: testcase

Comment 3

[gnehsoah]

Any follow up on this issue?

Comment 4

[Daniel Veditz [:dveditz]]

Tim: is there anyone on your team who could look into this?

Note: the Desktop UI _also_ shows the location as "javascript:mail.google.com//" which is still a bit spoofy but at least isn't highlighted to lend more authenticity to it (see bug 1228719 for example). But on Desktop the permission dialog has the correct domain in the door hanger.

Is it just the doorhanger UI that's wrong (spoof) or will the permission be saved to the wrong domain? On Desktop the permission is saved for the real domain--expected since it's not spoofed. On Fennec the permission is not saved for the real asking domain (which can be checked with the site identity doorhanger), but I'm not easily able to check the spoofed domain. Doesn't seem to be saved for the real mail.google.com, but maybe it's saved internally for the fake "javascript:mail.google.com"

Comment 5

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

Thanks for ni me. Will find someone to address this quickly.

Do we want to do a chemspell release on this?

Comment 6

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

(* chemspill, obviously.)

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Hi Julian 
Could you please help me with this? Thank you!

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

This is not a chemspill bug. It is a normal security bug and should be handled as described at https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 9

[Julian Chu [:walkingice]]

So far what I found is here[1], it parses location regardless protocol (in this case, "javascript:"), then provide "gmail.com" as baseDomain to Android Notification. I am still trying to understand how desktop-firefox deal with this situation.

[1] https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/mobile/android/chrome/content/browser.js#4265

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Julian Chu [:walkingice] from comment #9)
> So far what I found is here[1], it parses location regardless protocol (in
> this case, "javascript:"), then provide "gmail.com" as baseDomain to Android
> Notification. I am still trying to understand how desktop-firefox deal with
> this situation.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 8eaf154b385bbe0ff06155294ccf7962aa2d3324/mobile/android/chrome/content/
> browser.js#4265

We parse the URL "properly" using nsIURI, if possible, and if not we don't highlight. See the code in https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/browser/base/content/urlbarBindings.xml#236 , specifically https://dxr.mozilla.org/mozilla-central/source/browser/base/content/urlbarBindings.xml#256-270

Comment 11

[Julian Chu [:walkingice]]

Gijs, thanks you for reminding me about that. So today I see desktop-firefox use uri[1] to present notification. The uri is assigned from a request[2]. Back to the problem, I think I can use `contentWin.document.documentURIObject.hostPort` as baseDomain if its protocol doesn't match http/https/ftp.

[1] https://dxr.mozilla.org/mozilla-central/rev/88030580b14bb253a55bc174c987a9fa43c3fb55/toolkit/modules/PopupNotifications.jsm#747
[2] https://dxr.mozilla.org/mozilla-central/rev/88030580b14bb253a55bc174c987a9fa43c3fb55/browser/modules/PermissionUI.jsm#342

Comment 12

[Julian Chu [:walkingice]]

When I try to access `hostPort` in this way

    baseDomain = contentWin.document.documentURIObject.hostPort;
    // or
    baseDomain = aLocationURI.hostPort;

I got an error

    NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort]

Jim, do you have any idea for that? Or I did anything wrong?

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Julian Chu [:walkingice] from comment #11)
> Gijs, thanks you for reminding me about that. So today I see desktop-firefox
> use uri[1] to present notification. The uri is assigned from a request[2].
> Back to the problem, I think I can use
> `contentWin.document.documentURIObject.hostPort` as baseDomain if its
> protocol doesn't match http/https/ftp.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 88030580b14bb253a55bc174c987a9fa43c3fb55/toolkit/modules/PopupNotifications.
> jsm#747
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> 88030580b14bb253a55bc174c987a9fa43c3fb55/browser/modules/PermissionUI.jsm#342

Oh, sorry, I didn't realize there were two bugs reported here. My answer was specific to the location bar highlighting. We just shouldn't highlight non-http/https/ftp URLs there.

For the permission prompt, you shouldn't use the documentURIObject's hostPort (which is going to be broken for e.g. data: URIs controlled by some domain), you should use the document's nodePrincipal's URI. Note that the request might come from a (cross-origin) frame, so make sure you use the right window/document, which is not necessarily the toplevel one.

Comment 14

[Julian Chu [:walkingice]]

I asked Kanru, and he told me that I cannot get hostPort due to the protocol is not recognized. (in this case, "javascript:gmail.google.com"). I need another way to get domain properly but no idea so far.

Comment 15

[:Gijs (out for now; he/him)]

(In reply to Julian Chu [:walkingice] from comment #14)
> I asked Kanru, and he told me that I cannot get hostPort due to the protocol
> is not recognized. (in this case, "javascript:gmail.google.com"). I need
> another way to get domain properly but no idea so far.

Does document.nodePrincipal.URI.hostPort not work?

Comment 16

[Julian Chu [:walkingice]]

Attachment: 0001-Bug-1325955-To-use-hostPort-as-default-base-domain.patch

Comment 17

[Julian Chu [:walkingice]]

Gijs: Yes it works, thanks! then I created a patch base on that value.

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827850 [details] [diff] [review]
0001-Bug-1325955-To-use-hostPort-as-default-base-domain.patch

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

I would have implemented this in ToolbarDisplayLayout where we use the baseDomain:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java#295-306

But not extracting the baseDomain if this is not a http/https URL makes sense too. I case in this case we could remove one of those checks in ToolbarDisplayLayout.

I'll flag Gijs to have another look too.

Comment 19

[:Gijs (out for now; he/him)]

Comment on attachment 8827850 [details] [diff] [review]
0001-Bug-1325955-To-use-hostPort-as-default-base-domain.patch

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

I don't think this is quite right. The eTLD service turns "mail.google.com" into "google.com", and "foo.gov.uk" into "gov.uk" but "foo.bar.ac.uk" into "bar.ac.uk".

This patch has almost no context (please configure hg/git to use 8 or 10 lines of context...), but as I understand it the baseDomain is used for two things:
- the thing in the permissions popup that indicates who's asking for permission
- the highlighted bit of the URL in the location bar

With this patch, we process the base domain differently in the case of an https URI than in the case of e.g. a data: or javascript: URI that inherits the principal from an http URI. So the permissions popup for either case might show the base domain (without subdomains) for one case, and the full domain for another. That's 1 bug.

Separately, AIUI the code as-is would still let us highlight a domain in a javascript: or data: uri if the domain from the inherited principal's URI occurs as a substring of the javascript or data URI. That also seems wrong, because such a substring has no special meaning in those URI schemes. We just shouldn't be highlighting the domain in the URI at all if we're displaying the full javascript: or data: URI (though it's probably sensible in the https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java#295-306 code for cases where we only show a domain anyway, to still only show a domain in the JS/data: case).

::: mobile/android/chrome/content/browser.js
@@ +4254,5 @@
>      // If reader mode, get the base domain for the original url.
>      let strippedURI = this._stripAboutReaderURL(documentURI);
>  
> +    // Regular expression is modified from urlbarBindings.xml. To use hostPort as default base-domain,
> +    // and to give advance process only if its scheme match http/https/ftp

Can we clarify this comment by writing the comment in the order the code acts, and to use a better phrase than "give advance process"?

@@ +4255,5 @@
>      let strippedURI = this._stripAboutReaderURL(documentURI);
>  
> +    // Regular expression is modified from urlbarBindings.xml. To use hostPort as default base-domain,
> +    // and to give advance process only if its scheme match http/https/ftp
> +    let baseDomain = contentWin.document.nodePrincipal.URI.hostPort || "";

Getting hostPort throws exceptions for non-standard URIs (e.g. null principal URIs or data: codebase URIs or about: URIs), which this code should probably deal with.

Comment 20

[:Gijs (out for now; he/him)]

Comment on attachment 8827850 [details] [diff] [review]
0001-Bug-1325955-To-use-hostPort-as-default-base-domain.patch

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

::: mobile/android/chrome/content/browser.js
@@ +4256,5 @@
>  
> +    // Regular expression is modified from urlbarBindings.xml. To use hostPort as default base-domain,
> +    // and to give advance process only if its scheme match http/https/ftp
> +    let baseDomain = contentWin.document.nodePrincipal.URI.hostPort || "";
> +    let matchedURL = strippedURI.match(/^((?:(http|https|ftp):\/\/)(?:[^\/]+@)?)(.+?)(?::\d+)?(?:\/|$)/);

In fact, this also looks wrong because the ?: annotation is outside the grouping which you're trying to not get included as a group... So you're effectively adding a group, for no reason - please include the ?: annotation inside (both) the groups, and revert the change to the array deconstruction lower down, if this code stays the same at all...

Finally, you can write: (http|https|ftp) as (https?|ftp).

Comment 21

[gnehsoah]

Any follow-up?

Comment 22

[Julian Chu [:walkingice]]

Gijs, thanks for your detailed explaining.:)

Yes, as you said, the baseDomain will be used in that 2 things. In the attached test case, we will got "gmail.com" as baseDomain, and it is incorrect. That's the reason I want to modify browser.js, rather than Java file.

So my idea is that, if the protocol is http/https/ftp, then it works like before. Otherwise, use host-port if exists. In desktop, the permission prompt use host-port as title in this test case, and I try to provide same behavior.

Let me update the patch as well as its regular expression.

Comment 23

[Julian Chu [:walkingice]]

Attachment: 0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-pr.patch

Changed regular expression, comments and add checking for nodePrincipal.URI

Comment 24

[:Gijs (out for now; he/him)]

Comment on attachment 8848048 [details] [diff] [review]
0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-pr.patch

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

This patch still has almost no context. Please configure hg/git to provide more context.

As noted below, I think we should just get rid of the regexp matching and only use the principal URL.

::: mobile/android/chrome/content/browser.js
@@ +4451,5 @@
>  
>      // Borrowed from desktop Firefox: https://hg.mozilla.org/mozilla-central/annotate/72835344333f/browser/base/content/urlbarBindings.xml#l236
> +    // For recognized protocol http/https/ftp, parse base domain from its URI.
> +    // If the protocol is not recognized, try to use hostPort to be base domain.
> +    let matchedURL = strippedURI.match(/((?:https?|ftp):\/\/(?:[^\/]+@)?)(.+?)(?::\d+)?(?:\/|$)/);

There should be a ^ to ensure this matches at the beginning of the URL, because now this will still do the wrong thing for e.g.:

javascript:/*https://google.com/*/mycodegoeshere().

@@ +4466,5 @@
>            baseDomain = IDNService.convertACEtoUTF8(baseDomain);
>          }
>        } catch (e) {}
> +    } else {
> +        if (contentWin.document.nodePrincipal.URI && contentWin.document.nodePrincipal.URI.hostPort) {

It's good to check for principal.URI being null, but this will still throw sometimes (cf. in the desktop browser console, evaluate: makeURI("about:home").hostPort ), and you're still not getting the base domain for the hostname, as this code is after we call getBaseDomainFromHost. So this isn't right.



I think more generally, we should default to using the base domain off the principal URI if available. In all other cases we should just set it to "". I don't think there are any practical cases where that would produce the wrong result, and it's a lot safer than regexp matching.

Comment 25

[Julian Chu [:walkingice]]

Attachment: 0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-sc.patch

Use URI.host to get base domain, instead of regex. If the URI is not available or the scheme does not match, leave base domain as empty.

Comment 26

[:Gijs (out for now; he/him)]

Comment on attachment 8848488 [details] [diff] [review]
0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-sc.patch

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

::: mobile/android/chrome/content/browser.js
@@ +4452,3 @@
>      let baseDomain = "";
> +    // For recognized scheme, parse base domain from host.
> +    let recognized = ["http", "https", "ftp"].some(scheme => strippedURI.startsWith(scheme));

Why not use the principal URI for data: and javascript: URIs which have inherited the principal? I don't understand why we're making the scheme determination based on the stripped (document) URI, rather than that of the principal.

Also, to check if an array contains an item, use array.includes().

@@ +4454,5 @@
> +    let recognized = ["http", "https", "ftp"].some(scheme => strippedURI.startsWith(scheme));
> +    if (recognized
> +        && contentWin.document.nodePrincipal.URI
> +        && contentWin.document.nodePrincipal.URI.host) {
> +      let host = contentWin.document.nodePrincipal.URI.host;

This looks very repetitive, I would use a temp variable, e.g.:

let principalURI = contentWin.document.nodePrincipal.URI;

if (principalURI && ["http", "https", "ftp"].includes(principalURI.scheme) &&
    principalURI.host) {
  try {
    baseDomain = Services.eTLD.getBaseDomainFromHost(principalURI.host);
    ...
  } catch (e) {}
}

Comment 27

[:Gijs (out for now; he/him)]

Also, why the switch from hostPort to host?

Comment 28

[Julian Chu [:walkingice]]

I use strippedURI because it existed, so I think it should be reliable to check its scheme. As your comment, just get scheme from `nodePrincipal.URI.scheme` would be better then I will do in this way.

Switch from hostPort to host due to I would like to keep original implementation which use `Services.eTLD.getBaseDomainFromHost`

Comment 29

[Julian Chu [:walkingice]]

Attachment: 0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-sc.patch

* Use principal.URI.scheme instead of strippedURI
* Use array.includes
* Use temporary variable principalURI

Comment 30

[:Gijs (out for now; he/him)]

Comment on attachment 8849839 [details] [diff] [review]
0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-sc.patch

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

This looks good to me, but it'd be a good idea for a mobile peer to have a look, too.

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8849839 [details] [diff] [review]
0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-sc.patch

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

LGTM

Comment 32

[:Gijs (out for now; he/him)]

Note that this bug will now need a sec rating and/or sec-approval before landing.

Comment 33

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

Since this is a sec-moderate rated issue (out of our triage meeting), this no longer needs sec-approval (only high and critical issues or unrated ones do).

Comment 34

[Julian Chu [:walkingice]]

Attachment: 0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-sc.patch

Only update comment of the patch

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/969b5b7a564671710a230fa2885024847d45e2f4

Comment 36

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

https://hg.mozilla.org/mozilla-central/rev/969b5b7a5646

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 38

[Julian Chu [:walkingice]]

Comment on attachment 8850452 [details] [diff] [review]
0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-sc.patch

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Notification might display incorrect base-domain in some scheme
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Is the change risky?]:Low
[Why is the change not risky?]: It checks NULL and handled exception. In worst case it just assign empty string to base-domain.

Comment 39

[Gerry Chang [:gchang]]

Comment on attachment 8850452 [details] [diff] [review]
0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-sc.patch

Fix a security issue. Aurora54+ & Beta53+.

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/578e94e40a70

Comment 41

[Carsten Book [:Tomcat]]

has problems to apply to beta:

grafting 407777:578e94e40a70 "Bug 1325955 - Prevent providing wrong baseDomain if scheme is not recognized r=sebastian a=gchang"
merging mobile/android/chrome/content/browser.js
warning: conflicts while merging mobile/android/chrome/content/browser.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a9e49ecff889

Comment 43

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi, gnehsoah, can you verify that this has been fixed?

Comment 44

[gnehsoah]

Yes, looks like it's fixed.

Comment 45

[Matt Wobensmith [:mwobensmith][:matt:]]

Thank you very much.