Bug 1325955 (CVE-2017-5450)

Address bar spoofing on Android

VERIFIED FIXED in Firefox 53

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: gnehsoah, Assigned: walkingice)

Tracking

({sec-moderate})

50 Branch
Firefox 55
Points:
---
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

(fennec+, firefox50 wontfix, firefox51 wontfix, firefox52 wontfix, firefox-esr52 unaffected, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [adv-main53+][post-critsmash-triage])

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
(Reporter)

Comment 2

2 years ago
Posted file testcase
Flags: sec-bounty?
(Reporter)

Comment 3

2 years ago
Any follow up on this issue?
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"
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(timdream)
Thanks for ni me. Will find someone to address this quickly.

Do we want to do a chemspell release on this?
tracking-fennec: --- → +
Flags: needinfo?(timdream)
Assignee: nobody → cnevinchen
Hi Julian 
Could you please help me with this? Thank you!
Assignee: cnevinchen → walkingice0204
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
(Assignee)

Comment 9

2 years ago
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

2 years ago
(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
(Assignee)

Comment 11

2 years ago
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
(Assignee)

Comment 12

2 years ago
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?
Flags: needinfo?(nchen)

Comment 13

2 years ago
(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.
(Assignee)

Comment 14

2 years ago
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.
Flags: needinfo?(nchen)

Comment 15

2 years ago
(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?
(Assignee)

Comment 17

2 years ago
Gijs: Yes it works, thanks! then I created a patch base on that value.
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.
Attachment #8827850 - Flags: review?(s.kaspari)
Attachment #8827850 - Flags: review?(gijskruitbosch+bugs)
Attachment #8827850 - Flags: review+

Comment 19

2 years ago
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.
Attachment #8827850 - Flags: review?(gijskruitbosch+bugs) → review-

Comment 20

2 years ago
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 hidden (off-topic)
Priority: -- → P1
(Assignee)

Comment 22

2 years ago
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.
(Assignee)

Comment 23

2 years ago
Changed regular expression, comments and add checking for nodePrincipal.URI
Attachment #8827850 - Attachment is obsolete: true
Attachment #8848048 - Flags: review?(gijskruitbosch+bugs)

Comment 24

2 years ago
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.
Attachment #8848048 - Flags: review?(gijskruitbosch+bugs) → review-
(Assignee)

Comment 25

2 years ago
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.
Attachment #8848048 - Attachment is obsolete: true
Attachment #8848488 - Flags: review?(gijskruitbosch+bugs)

Comment 26

2 years ago
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) {}
}
Attachment #8848488 - Flags: review?(gijskruitbosch+bugs) → review-

Comment 27

2 years ago
Also, why the switch from hostPort to host?
(Assignee)

Comment 28

2 years ago
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`
(Assignee)

Comment 29

2 years ago
* Use principal.URI.scheme instead of strippedURI
* Use array.includes
* Use temporary variable principalURI
Attachment #8848488 - Attachment is obsolete: true
Attachment #8849839 - Flags: review?(gijskruitbosch+bugs)

Comment 30

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

Comment 32

2 years ago
Note that this bug will now need a sec rating and/or sec-approval before landing.
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).
(Assignee)

Comment 34

2 years ago
Only update comment of the patch
Attachment #8849839 - Attachment is obsolete: true
Attachment #8850452 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/969b5b7a5646
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Please request Aurora/Beta approval on this when you get a chance.
(Assignee)

Comment 38

2 years ago
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.
Flags: needinfo?(walkingice0204)
Attachment #8850452 - Flags: approval-mozilla-beta?
Attachment #8850452 - Flags: approval-mozilla-aurora?
Comment on attachment 8850452 [details] [diff] [review]
0001-Bug-1325955-Prevent-providing-wrong-baseDomain-if-sc.patch

Fix a security issue. Aurora54+ & Beta53+.
Attachment #8850452 - Flags: approval-mozilla-beta?
Attachment #8850452 - Flags: approval-mozilla-beta+
Attachment #8850452 - Flags: approval-mozilla-aurora?
Attachment #8850452 - Flags: approval-mozilla-aurora+
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')
Flags: needinfo?(walkingice0204)
Group: firefox-core-security → core-security-release
Flags: sec-bounty? → sec-bounty+
Whiteboard: [adv-main53+]
Alias: CVE-2017-5450
Hi, gnehsoah, can you verify that this has been fixed?
Flags: needinfo?(gnehsoah)
Flags: qe-verify-
Whiteboard: [adv-main53+] → [adv-main53+][post-critsmash-triage]
(Reporter)

Comment 44

2 years ago
Yes, looks like it's fixed.
Flags: needinfo?(gnehsoah)
Thank you very much.
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.