1273537 - (CVE-2017-5451) A click into the location bar can lead to a Location Bar Spoofing vulnerability (URL and SSL Spoofing) using the onblur event and change the URL size by a bigger URL size.

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Jordi Chancel]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160502172042

Steps to reproduce:


(Only tested on Mac OS X).

A simple click on the location bar can lead to a Location Bar Spoofing (URL and SSL Spoofing).

Explication:

If an user click on the location bar and the ONBLUR event defines that the actual URL changes its size by a bigger size, (eg: http://www.yyy.com/ becomes http://www.yyy.com/#wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwhttps://www.bank.com/ ) , the end of the URL is shown instead of the start of this URL (so the location bar shows https://www.bank.com instead of http://www.yyy.com/# ).

-A demonstration video will be uploaded.

1) Open with Firefox "PoC v1 Part1.html" and click on the button "Step 1: ClickMe" 
(a popup window is opened)

2) Click on the location bar



Actual results:

The Location Bar is Spoofed and if the Malicious WebSite is secure , this leads to URL And SSL Spoofing



Expected results:

If an user click on the location bar and the ONBLUR event defines that the actual URL changes its size by a bigger size, the end of the URL is shown instead of the start of this URL.

Possible way to patch this vulnerability :

When an user click on the location bar , Firefox should continue to show the start of the visited URL instead of the end of this URL.

Comment 1

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

Note: This reproduces on 46 (Release) but not nightly (49).

Comment 2

[Kamil Jozwiak [:kjozwiak]]

(In reply to Al Billings [:abillings] from comment #1)
> Note: This reproduces on 46 (Release) but not nightly (49).

Seems like e10s is the reason we couldn't reproduce this on fx49.0a1 (m-c). This is reproducible on all the channels once you either disable e10s or open a "non-e10s" window via the hamburger menu.

* fx49.0a1 buildid: 20160517030211 (m-c) - reproducible with a non-e10s window or with e10s disabled
* fx48.0a2 buildid: 20160517004009 (m-a) - reproducible with a non-e10s window or with e10s disabled
* fx47.0b6 buildid: 20160516123243 (m-b) - reproducible (e10s not available under m-b)
* fx46.0.1 buildid: 20160502172042 (m-r) - reproducible (e10s not available under m-b)

I also went as far back as "mozregression --launch 2015-08-01" and I could still reproduce this when e10s was disabled/non-e10s window:

0:19.71 INFO: application_buildid: 20150801030211
0:19.71 INFO: application_changeset: aeb85029c3b3
0:19.71 INFO: application_name: Firefox
0:19.71 INFO: application_repository: https://hg.mozilla.org/mozilla-central
0:19.71 INFO: application_version: 42.0a1

Comment 3

[Kamil Jozwiak [:kjozwiak]]

Felipe, as you're working on bug # 1272961 which is somewhat similar, would you mind taking a look at this one?

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Yeah. This similarly exploits the fact that blur() is synchronously dispatched to the page, but uses a different technique to trick the URL bar.

E10s mitigates the problem by making blur async from the point of view of the page, but it's unclear if some of these can also randomly work by lucky timing.

I'll leave the needinfo a little bit more to take a look soon and see if there's a simple solution to this. But the patch for bug 1272961 won't fix this one.

Comment 5

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

Felipe, you've had a ni? on you for seven months and this an outstanding sec-high. Is there anything we can do in order for this to move forward or be resolved?

Comment 6

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

Attachment: 0001-Bug-1273537-r-felipe.patch

A quick fix would be make to the XULBrowserWindow#onLocationChange async, given that the side effect of e10s conveniently fix this.

Alternatively, I can change the implementation of the onLocationChange in the object returned by tabbrowser.mTabProgressListener. That would make all the progress listeners it calls async, including the aforementioned one.

It would be better if we could keep the patch uplift-able by keeping it small and introduces a behavior that is similar to e10s so tests won't fail.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8839049 [details] [diff] [review]
0001-Bug-1273537-r-felipe.patch

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

Hmm this feels very risky. I'd expect tests to fail with this, did you send it to try yet?  Granted, in e10s this is async from the point of view of the content, but on the parent the callers are still used to this being sync. Like, I've seen code in the past that relies on some of these attributes being set/unset right after OnLocationChange ends. Specially things related to the URL bar mode (displaying url / inputting url).

I'm not opposed to giving this a try but I'd be happier with a more specific fix. Let's see a try run and then think about landing it. In any case, I'd not want this to skip trains..

Comment 8

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

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6)
> Alternatively, I can change the implementation of the onLocationChange in
> the object returned by tabbrowser.mTabProgressListener. That would make all
> the progress listeners it calls async, including the aforementioned one.

This is the alternative patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d046a293b1f05084d3c62f778273f96f73df748

Comment 9

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

(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> Comment on attachment 8839049 [details] [diff] [review]
> 0001-Bug-1273537-r-felipe.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45d522a63b9c418110d8d953ae6345be990cd18a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1534031e96cd76e2323cc15f989193cf4801e406

Comment 10

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

Attachment: URLBarSetURI.patch

You are right. Any changes that changes timing doesn't look pretty on test results. Here is another approach that only changes the timing of gURLBar attributes change, and PopupNotifications.anchorVisibilityChange().

It certainly breaks less tests. Do you agree with this approach? If so I will start fixing tests ...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97a66b01cca4c077d5d8c661419e2fb08c05aa96

Comment 11

[:Gijs (he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #10)
> Created attachment 8839347 [details] [diff] [review]
> URLBarSetURI.patch
> 
> You are right. Any changes that changes timing doesn't look pretty on test
> results. Here is another approach that only changes the timing of gURLBar
> attributes change, and PopupNotifications.anchorVisibilityChange().
> 
> It certainly breaks less tests. Do you agree with this approach? If so I
> will start fixing tests ...
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=97a66b01cca4c077d5d8c661419e2fb08c05aa96

This looks like it's almost certainly wrong because gBrowser.selectedBrowser can change (sometimes because of actions the website takes) inbetween when we call URLBarSetURI and when it resolves, and URLBarSetURI does rely on what the selected browser is.

It seems to me a much simpler fix would be to ensure we end up setting the start and end of the selection to the start of the URL when the URL changes in the codepath used in the testcase/poc. See bug 1114343, where (a) I was probably right that there are spoofing issues (viz. this bug) and (b) we probably put the selectionStart/End setting in the wrong place. It's tricky to get that right, though, as we don't want the website having control over selection in the navbar (cf. bug 1199934).

Comment 12

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

Idea: whenever the focus leaves the URL bar and whenever the URL updates, we reset the scroll position to where the user will always see the base domain?

(I was going to upload another patch that simply do `this.inputField.scrollLeft = 0` but I realized attacker can simply navigate to |www.google.com.login.evil.com| instead.)

Comment 13

[:Gijs (he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #12)
> Idea: whenever the focus leaves the URL bar and whenever the URL updates, we
> reset the scroll position to where the user will always see the base domain?

In this testcase, focus doesn't leave the URL bar. But yes, this is essentially what I suggested in comment #11.

> (I was going to upload another patch that simply do
> `this.inputField.scrollLeft = 0` but I realized attacker can simply navigate
> to |www.google.com.login.evil.com| instead.)

I don't think scrollLeft is involved here - nothing is scrolling - you should manipulate the selection.

Comment 14

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

(In reply to :Gijs from comment #13)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #12)
> > Idea: whenever the focus leaves the URL bar and whenever the URL updates, we
> > reset the scroll position to where the user will always see the base domain?
> 
> In this testcase, focus doesn't leave the URL bar. But yes, this is
> essentially what I suggested in comment #11.
> 

Actually it did. The fake login page calls alert() which takes the focus away.

Even if it didn't, user would need move the focus back to the page manually to start typing credentials...

> > (I was going to upload another patch that simply do
> > `this.inputField.scrollLeft = 0` but I realized attacker can simply navigate
> > to |www.google.com.login.evil.com| instead.)
> 
> I don't think scrollLeft is involved here - nothing is scrolling - you
> should manipulate the selection.

I don't understand this. I tried |this.inputField.scrollLeft = 0| and it did scroll the input. Are you saying I shouldn't be using it?

Also, do we need an UX decision here? Base domain is certainly important from security point of view, but for users being able to see the path changes between navigation might be more important...

Comment 15

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

Attachment: Make sure base domain is visible

This patch sets scrollLeft in formatValue() to ensure base domain is always visible. Specifically for the attack described here, the attacker's domain will be shown (instead of the fake domain in the hash) when the focus moves away from the url bar to the alert prompt.

This is not fool proof; we do not reset scroll position on other occasions, like when user manually scroll the url bar with mouse wheel without focus it, and/or when the window is resized.

If we want to dig deeper and ensure the url don't scroll when it's being focused, this is where it should take place: 

http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/toolkit/content/widgets/textbox.xml#199

Comment 16

[:Gijs (he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #14)
> (In reply to :Gijs from comment #13)
> > (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> > #12)
> > > Idea: whenever the focus leaves the URL bar and whenever the URL updates, we
> > > reset the scroll position to where the user will always see the base domain?
> > 
> > In this testcase, focus doesn't leave the URL bar. But yes, this is
> > essentially what I suggested in comment #11.
> > 
> 
> Actually it did. The fake login page calls alert() which takes the focus
> away.

But that's not a fundamental part of the poc/exploit here - it's not important and we shouldn't rely on it for 'fixing' things.

> Even if it didn't, user would need move the focus back to the page manually
> to start typing credentials...

By which time the user is already not looking at the url bar anymore so it's too late.
 
> > > (I was going to upload another patch that simply do
> > > `this.inputField.scrollLeft = 0` but I realized attacker can simply navigate
> > > to |www.google.com.login.evil.com| instead.)
> > 
> > I don't think scrollLeft is involved here - nothing is scrolling - you
> > should manipulate the selection.
> 
> I don't understand this. I tried |this.inputField.scrollLeft = 0| and it did
> scroll the input.

That's surprising. :-\

> Are you saying I shouldn't be using it?

Probably not, because it looks like it just scrolls the insertion/selection point out of view. So if focus is left in the url bar, text might still be selected but the user can't see it (or the cursor is hidden... which sounds like a bug if we allow it to happen at all, tbh).

> Also, do we need an UX decision here? Base domain is certainly important
> from security point of view, but for users being able to see the path
> changes between navigation might be more important...

We already do the 'scroll to the left' thing if you navigate in the URL bar by hitting enter, and anything else going through openLinkIn, IIRC, so I don't think so.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8840312 [details]
Make sure base domain is visible

This is overly complex, so f- for that. We should just set the main input's selectionStart and selectionEnd to 0 like we do elsewhere (and the other code should probably be moved to wherever we end up doing it instead).

Just showing the base domain is not enough anyway because the protocol should be visible too, otherwise things remain spoofable e.g. for javascript: or data: URLs which don't technically have domains but can be made to look like they have domains if we only show a substring of the domain.

The tricky thing is *where* to do it, and I can't tell if this is the right place without more information. It *looks* like this is the code that runs when we do highlighting of the base domain, ie when the location bar is blurred, in which case this is too late as I said in my previous comment. Anyway, the patch context doesn't even include the method name. So for the next patch, please can you explain where you are doing this and why is that the right place?

Comment 18

[:Gijs (he/him)]

(In reply to :Gijs from comment #17)
> So for the
> next patch, please can you explain where you are doing this and why is that
> the right place?

Ah, I now saw that you did this in the other comment, which I missed - sorry. But as I said, I don't think this is the right place.

Comment 19

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

Attachment: URLBarSetURI-selectionStart.patch

After giving it a thought on where to reset the caret position, I've come to the conclusion that the right place to put this would be simply URLBarSetURI() in browser.js. This the the only other place where we rewrite the entire URL by setting the .value (the first place being gURLBar#_loadURL). With this patch and bug 1114343 both call to setter of .value will reset the caret position.

There is nothing to check (e.g. if the bar is focused etc.) here when reset the caret position. The code that eventually calls URLBarSetURI() is responsible of making sure of that, given it rewrites the value (e.g. bug 1199934).

Comment 20

[:Gijs (he/him)]

Comment on attachment 8840767 [details] [diff] [review]
URLBarSetURI-selectionStart.patch

At a glance, this looks OK. I haven't tested this, though. Please remember to remove the other place I referenced where we do this if that makes sense (or maybe it doesn't?). Do tests in the browser/base/content/test/urlbar/ directory pass for you locally with this change?

I should have cycles to test this tomorrow, but I'm aware that because of the time difference early feedback might be useful.

Comment 21

[:Gijs (he/him)]

Comment on attachment 8840767 [details] [diff] [review]
URLBarSetURI-selectionStart.patch

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #19)
> Created attachment 8840767 [details] [diff] [review]
> URLBarSetURI-selectionStart.patch
> 
> After giving it a thought on where to reset the caret position, I've come to
> the conclusion that the right place to put this would be simply
> URLBarSetURI() in browser.js. This the the only other place where we rewrite
> the entire URL by setting the .value (the first place being
> gURLBar#_loadURL). With this patch and bug 1114343 both call to setter of
> .value will reset the caret position.
> 
> There is nothing to check (e.g. if the bar is focused etc.) here when reset
> the caret position. The code that eventually calls URLBarSetURI() is
> responsible of making sure of that, given it rewrites the value (e.g. bug
> 1199934).

This isn't quite true, because in some cases we will call URLBarSetURI multiple times with the same value. You can see this in action fairly easily when loading e.g. yahoo.com (which is slow to load if you're not using an adblocker or TP). If you turn off browser.urlbar.clickSelectsAll (if you're on a platform where that's true) and load yahoo.com, then click toward the end of the URL bar after "https://uk.yahoo.com/?p=us" appears as the URL, you can see focus is initially towards the end and then gets switched to index 0 (by the code you're adding).

We should avoid that.

I ran tests locally, and it looks like your changes break browser/base/content/test/urlbar/browser_bug304198.js  and browser_canonizeURL.js

I believe both tests can be fixed by changing the patch so it only updates the selection for cases where there's no typed value (and we use either currentURI or a passed-in URI) and where the value is different from the URL bar's current value. So change the selection inside the extant if statement and only do so if gURLBar.value is different from the value we're going to set (at the end of the if block).

Doing that seems to fix both tests for me. I'd still like to know if this means we can remove the other selection setting code as I asked earlier.

Comment 22

[:Gijs (he/him)]

Attachment: Patch that wfm

Comment 23

[:Gijs (he/him)]

(In reply to :Gijs from comment #22)
> Created attachment 8841740 [details] [diff] [review]
> Patch that wfm

... though that doesn't seem to fix the problem in the testcase completely, because it updates the selection before assigning to .value. Fixing up this patch so it doesn't do that reintroduces problems with the tests I mentioned in comment #21, but does seem to fix the manual yahoo testcase in my testing. I expect the tests can be fixed by manually fixing assumptions they make about the insertion point in the URL bar.

Comment 24

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

Attachment: URLBarSetURI-selectionStart-v2.patch

Incorporate your finding and update the tests.

Comment 25

[:Gijs (he/him)]

Comment on attachment 8842715 [details] [diff] [review]
URLBarSetURI-selectionStart-v2.patch

Please tick the 'patch' checkbox when you add patches to the bug. Otherwise I can't use splinter or the diff view to review the patch.

Comment 26

[:Gijs (he/him)]

Comment on attachment 8842715 [details] [diff] [review]
URLBarSetURI-selectionStart-v2.patch

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

::: browser/base/content/test/urlbar/browser_bug304198.js
@@ +48,5 @@
>          resolve();
>        }, {once: true});
>        gURLBar.focus();
> +      if (gURLBar.selectionStart == gURLBar.selectionEnd) {
> +        gURLBar.selectionStart = gURLBar.selectionEnd = gURLBar.textValue.length;

Why does this use gURLBar.textValue and the other test fix use gURLBar.value ? Which one is right, and can we make them match?

Comment 27

[:Gijs (he/him)]

Note that this will need sec-approval before landing given this is a sec-high.

Comment 28

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

(In reply to :Gijs from comment #26) 
> Why does this use gURLBar.textValue and the other test fix use gURLBar.value
> ? Which one is right, and can we make them match?

Both getter eventually gets the value of gURLBar.inputField.value. I simply want to make the change consistent of the setter in the test files.

I've update the line in browser_canonizeURL.js to |gURLBar.inputField.value.length| given the next line sets |gURLBar.inputField.value| directly*.

Comment 29

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

Attachment: bug1273537.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Should not be easy.  The patch changes the position of caret (thus, scroll position of the URL bar text) when the user first focused on the URL bar. It does not specifically workaround the attack (which rely on timing of URL rewrite and scroll position).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No tests was added in the patch. No commit comment.

Which older supported branches are affected by this flaw?

Yes, see flags.

If not all supported branches, which bug introduced the flaw?

N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backports would need to be created for test changes, but not the code change itself. It shouldn't be hard to do it.

How likely is this patch to cause regressions; how much testing does it need?

Tests in browser/base/content/test/urlbar/ all passes locally. The worse regression could happen would be unexpected moves of the URL bar caret.

Comment 30

[:Gijs (he/him)]

Comment on attachment 8843186 [details] [diff] [review]
bug1273537.patch

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

::: browser/base/content/browser.js
@@ +2560,5 @@
>    }
>  
>    gURLBar.value = value;
>    gURLBar.valueIsTyped = !valid;
> +  if (valid) {

Ah, wait, don't we need to also check that the value is changing (ie check gURLBar.value != value, before we set it of course...)? Otherwise I think we still hit the problem I mentioned in comment #21.

Comment 31

[:Gijs (he/him)]

Ni for comment #30.

Comment 32

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

(In reply to :Gijs from comment #30)
> Comment on attachment 8843186 [details] [diff] [review]
> bug1273537.patch
> 
> Review of attachment 8843186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +2560,5 @@
> >    }
> >  
> >    gURLBar.value = value;
> >    gURLBar.valueIsTyped = !valid;
> > +  if (valid) {
> 
> Ah, wait, don't we need to also check that the value is changing (ie check
> gURLBar.value != value, before we set it of course...)? Otherwise I think we
> still hit the problem I mentioned in comment #21.

I tested comment 21 manually and it didn't happen. I wonder if I overlooked the behavior you described, or if I couldn't test this because of my geoip.

Comment 33

[:Gijs (he/him)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #32)
> (In reply to :Gijs from comment #30)
> > Comment on attachment 8843186 [details] [diff] [review]
> > bug1273537.patch
> > 
> > Review of attachment 8843186 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/base/content/browser.js
> > @@ +2560,5 @@
> > >    }
> > >  
> > >    gURLBar.value = value;
> > >    gURLBar.valueIsTyped = !valid;
> > > +  if (valid) {
> > 
> > Ah, wait, don't we need to also check that the value is changing (ie check
> > gURLBar.value != value, before we set it of course...)? Otherwise I think we
> > still hit the problem I mentioned in comment #21.
> 
> I tested comment 21 manually and it didn't happen. I wonder if I overlooked
> the behavior you described, or if I couldn't test this because of my geoip.

Yeah, I just checked and I can still reproduce it. This should do to fix it:

diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -2552,23 +2552,24 @@ function URLBarSetURI(aURI) {
       try {
         value = losslessDecodeURI(uri);
       } catch (ex) {
         value = "about:blank";
       }
     }
 
     valid = !isBlankPageURL(uri.spec);
   }
 
+  let isDifferentValidValue = valid && value != gURLBar.value;
   gURLBar.value = value;
   gURLBar.valueIsTyped = !valid;
-  if (valid) {
+  if (isDifferentValidValue) {
     gURLBar.selectionStart = gURLBar.selectionEnd = 0;
   }
 
   SetPageProxyState(valid ? "valid" : "invalid");
 }
 
 function losslessDecodeURI(aURI) {
   let scheme = aURI.scheme;
   if (scheme == "moz-action")
     throw new Error("losslessDecodeURI should never get a moz-action URI");

Comment 34

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

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #29)

> Which older supported branches are affected by this flaw?
> 
> Yes, see flags.

"Yes" is an answer to "Which?" No... The status flags for affected versions are empty. How far back does this go?

Comment 35

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

Attachment: bug1273537-central-aurora.patch

Comment 36

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

(In reply to Al Billings [:abillings] from comment #34)
> "Yes" is an answer to "Which?" No... The status flags for affected versions
> are empty. How far back does this go?

Sorry, I should say "Version".
Given that it's being filed against 46 branch I would say this affects all the way to release, and maybe currently supported ESRs.

Comment 37

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

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #36)

> Given that it's being filed against 46 branch I would say this affects all
> the way to release, and maybe currently supported ESRs.

We need to determine whether this affects ESR45 or not. This is high enough rated that, if it does, we would take it on that branch.

Comment 38

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

I can verify 45.0 is affected by this bug.

Comment 39

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

Comment on attachment 8843804 [details] [diff] [review]
bug1273537-central-aurora.patch

sec-approval+ for trunk.
We'll want branch patches made and nominated as well for all affected branches.

Comment 40

[Daniel Veditz [:dveditz]]

Lowering security severity to moderate: This is now a non-default configuration (for most people) and it's a partial spoof (scheme not shown, interaction with the URL bar shows the real location).

Comment 41

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

I was busy on something else. I hope I can get to this by Friday!

Comment 42

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

Patch would apply to Central & Aurora:

$ hg update aurora -C && hg log -r . &&  patch --dry-run -p1 < .hg/patches/bug1273537.patch
0 files updated, 0 files merged, 0 files removed, 0 files unresolved
changeset:   396328:dff5f8de5eed
fxtree:      aurora
parent:      377218:445c4d9562b6
user:        Ehsan Akhgari <[e-mail]>
date:        Thu Mar 16 22:52:43 2017 -0400
summary:     Bug 1348168 - Disable Mozilla custom ImageBitmap extensions that didn't go through proper API review; r=bzbarsky a=dveditz

checking file browser/base/content/browser.js
Hunk #1 succeeded at 2551 (offset -3 lines).
checking file browser/base/content/test/urlbar/browser_bug304198.js
checking file browser/base/content/test/urlbar/browser_canonizeURL.js

Need a patch for Beta & esr52. Another patch for esr45.

Comment 43

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

Attachment: bug1273537-beta.patch

Rebased patch on beta. Locally built on Linux and tested all tests in browser/base/content/test/urlbar/

Comment 44

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

Attachment: bug1273537-esr52.patch

Patch for esr52. Built and tested locally.

Comment 45

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

Attachment: bug1273537-esr45.patch

Rebased to esr45. It's hard to get a full successful test run on browser/base/content/test/ but I can verify the two tests I modified passes locally.

Comment 46

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

Comment on attachment 8848416 [details] [diff] [review]
bug1273537-esr45.patch

You only need to ask for sec-approval for trunk patches. I suspect you want to ask for Aurora, Beta, and ESR45 or 52 approvals on these patches. That is what you need for checkins on those branches.

Comment 47

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

Comment on attachment 8848407 [details] [diff] [review]
bug1273537-beta.patch

Approval Request Comment
[Feature/Bug causing the regression]: Not sure, probably not a regression.
[User impact if declined]: See the POC phishing attack described in comment 0
[Is this code covered by automated tests?]: No test was added to avoid drawing a target.
[Has the fix been verified in Nightly?]: Not landed yet and only verified locally.
[Needs manual test from QE? If yes, steps to reproduce]: No need for manually test I think. The possible regression should not be critical.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The change is localized and possible regression should not be critical.
[String changes made/needed]: No.

Comment 48

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

Comment on attachment 8848407 [details] [diff] [review]
bug1273537-beta.patch

Though this got re-rated to sec-moderate, it would be nice to fix. This should land for beta 5 or 6 depending on when we go to build today.

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/333b172694da88d07e96ab17e09dcfc5d082a297

Comment 50

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

https://hg.mozilla.org/mozilla-central/rev/333b172694da

Comment 51

[Gerry Chang [:gchang]]

Comment on attachment 8843804 [details] [diff] [review]
bug1273537-central-aurora.patch

Fix a security issue. Aurora54+.

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/154ed63f43f6

Comment 53

[Carsten Book [:Tomcat]]

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

Comment 54

[Julien Cristau [:jcristau]]

Comment on attachment 8848416 [details] [diff] [review]
bug1273537-esr45.patch

sec-moderate, doesn't sound like a huge issue, esr45-

Comment 55

[Julien Cristau [:jcristau]]

Comment on attachment 8848410 [details] [diff] [review]
bug1273537-esr52.patch

fix partial url bar spoof on non-e10s, esr52+

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d91260f0069a

Comment 57

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Tim Guan-tin Chien's assessment on manual testing needs (see Comment 47).

Comment 58

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

Jordi, can you verify this has been fixed?

Comment 59

[Jordi Chancel]

Yes, this vulnerability is fixed in Firefox Beta 53 (Firefox 53.0b10).