Closed Bug 1228590 (CVE-2016-1943) Opened 4 years ago Closed 4 years ago

Location Bar Spoofing Risk - scrollto leads to that the location bar is hidden

Categories

(Firefox for Android :: General, defect)

42 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 46
Tracking Status
firefox43 --- wontfix
firefox44 + fixed
firefox45 + fixed
firefox46 + fixed
firefox-esr38 --- unaffected
b2g-v2.5 --- fixed
fennec 46+ ---

People

(Reporter: jordi.chancel, Assigned: ahunt)

References

(Blocks 1 open bug, )

Details

(Keywords: sec-high, Whiteboard: [adv-main44+][post-critsmash-triage])

Attachments

(6 files, 3 obsolete files)

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

Steps to reproduce:


(I will upload a video in the attachments quickly for show you the steps needed and the real result of this security bug/vulnerability)

Step1 : Go to https://www.alternativ-testing.fr/Research/Mozilla/android/R5fh-Firefox-Android-Risk/poc-spoofing-risk.html

Step2 : Go down into this webpage and go down on this webpage (the location bar normaly disapears) and in the down of this web page , a link named "Open-me in a new tab" .

Step3 : Let pressed on this link and choose the option "Open in a new tab".



Actual results:

--A fake location bar replaces the real location bar.--

Explanation :
The real Location which is normally always visible when you open a new webpage but in this case the Scrollto JavaScript Function makes the Location Bar not visible and leads to a Location Bar URL and SSL Spoofing Risk.



Expected results:

When the scrollto function is used on a webpage which is opened in a new tab , this function shouldn't affect the visibility of the real location Bar
Look this video for understand what steps are used and what is the result and the severity of this vulnerability.
Attached image Screenshot n°1.png
Screenshot of the first step.
Attached image Screenshot n°2.png
Screenshot of the 2nd step.
Screenshot of the 3rd step (final result): When the user views the Web page loaded into a new tab.
In the Description of this bug report ( https://bugzilla.mozilla.org/show_bug.cgi?id=1228590#c0 ), 
the text that I've writen in the "Expected results:" is in fact the problem described of Mozilla Firefox for Android which leads to this vulnerability, but not the expected result which shows the dangerous result of the Testcase.
Summary: (Location Bar URL and SSL Spoofing Risk) - Open a webpage into a new tab which uses the scrollto JavaScript Function leads to that the location bar which isn't visible but by default the Location Bar is always visible when a webpage is opened. → Location Bar Spoofing Risk - Open a webpage into a new tab which uses the JavaScript function «scrollto» leads to that the location bar is hidden - but by default, the Location Bar should be always visible when a webpage is opened.
Flags: sec-bounty?
tracking-fennec: --- → ?
Al, can you explain your sec-high rating?
Flags: needinfo?(abillings)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #6)
> Al, can you explain your sec-high rating?

Not really. It was a consensus during critsmash triage and I don't recall the exact reasoning. I just marked it for the group. Andrew?
Flags: needinfo?(abillings) → needinfo?(continuation)
sec-high includes "Spoofing of full URL bar or bypass of SSL integrity checks" and the video seemed fairly convincing. https://wiki.mozilla.org/Security_Severity_Ratings
Flags: needinfo?(continuation)
Discussed in triage - let's try always showing the URL bar when switching to a new tab.
Assignee: nobody → ahunt
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(alam)
Attached patch showlocationbar_1228590.patch (obsolete) — Splinter Review
ahunt, can you share a build with antlam to get feedback on the UX interaction here?
Flags: needinfo?(ahunt)
(In reply to :Margaret Leibovic from comment #12)
> ahunt, can you share a build with antlam to get feedback on the UX
> interaction here?

Here's the build:

http://people.mozilla.org/~ahunt/fennec/46_forceShowURLBar.apk
Flags: needinfo?(ahunt)
Comment on attachment 8699683 [details] [diff] [review]
showlocationbar_1228590.patch

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

I'll take a look at this.
Attachment #8699683 - Flags: review?(margaret.leibovic)
tracking-fennec: ? → 46+
Comment on attachment 8699683 [details] [diff] [review]
showlocationbar_1228590.patch

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

I just tried out the build, and I don't think this is what we want... with this patch, we show the URL bar as soon as a new tab is opened in the background, but the real risk here comes from the fact that the URL bar is hidden when we switch tabs. Showing the URL bar when a new tab is opened in the background is a pretty janky experience, so I don't think we should do this.

I think we should just show the URL bar when a new tab is selected.
Attachment #8699683 - Flags: review?(margaret.leibovic) → review-
Attached patch showlocationbar_1228590_v2.patch (obsolete) — Splinter Review
Here's a new patch and new build. The testing link above doesn't work for me anymore - I've tested with other links that open directly to anchors on separate pages instead (I'm not sure that's how the example page was implemented).

New build:
http://people.mozilla.org/~ahunt/fennec/46_forceShowURLBar_v2.apk

(In reply to :Margaret Leibovic from comment #15) 
> I just tried out the build, and I don't think this is what we want... with
> this patch, we show the URL bar as soon as a new tab is opened in the
> background, but the real risk here comes from the fact that the URL bar is
> hidden when we switch tabs. Showing the URL bar when a new tab is opened in
> the background is a pretty janky experience, so I don't think we should do
> this.
> 
> I think we should just show the URL bar when a new tab is selected.

Oops, I got a bit too caught up in ensuring we definitely show the url bar after switching and didn't pay attention to the behaviour change before the switch. We have a fall-through from LOCATION_CHANGE to SELECTED in our switch statement, which was causing the URL bar to show even when background tabs changed location (which happens when they're opened).
Attachment #8699683 - Attachment is obsolete: true
Attachment #8702366 - Flags: review?(margaret.leibovic)
Comment on attachment 8702366 [details] [diff] [review]
showlocationbar_1228590_v2.patch

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

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ +308,5 @@
>                  }
>                  if (Tabs.getInstance().isSelectedTab(tab)) {
>                      updateHomePagerForTab(tab);
> +                    if (mDynamicToolbar.isEnabled()) {
> +                        mDynamicToolbar.setVisible(true, VisibilityTransition.IMMEDIATE);

Currently we animate the toolbar in when you click a link to navigate within the same tab, and I want to make sure we don't break that transition.

Trying out the build you provided here, it does seem like we still animate the toolbar in when changing location on the same tab, but it seems a bit more janky than my Nightly build... maybe we should change this logic to explicitly only show the toolbar when a new tab is selected. Maybe we could change the order of the case statements and only make this call in the SELECTED case.
> Currently we animate the toolbar in when you click a link to navigate within
> the same tab, and I want to make sure we don't break that transition.
>
As far as I can tell, we animate the toolbar in START (which is what I think happens when we click a link), so we shouldn't be directly affecting the behaviour - however I'm still not sure I fully understand all the other interactions going on there yet though.

> Trying out the build you provided here, it does seem like we still animate
> the toolbar in when changing location on the same tab, but it seems a bit
> more janky than my Nightly build... maybe we should change this logic to
> explicitly only show the toolbar when a new tab is selected. Maybe we could
> change the order of the case statements and only make this call in the
> SELECTED case.

I've done the reordering, so we now only try to show the toolbar for SELECTED - this seems to behave correctly for all the cases I've tested, but I'm thinking it would useful to have a list of the various transitions where the URL bar should/shouldn't be shown (I'm not sure how easy it is to write tests for this)?

I've also changed the patch to animate the URL bar showing when switching tabs - this seems a bit nicer, but isn't always consistent - if the page has loaded fast enough then the URL bar appears immediately on switching tabs, whereas if the page is still loading the URL bar smoothly slides onto screen. Should I switch this back to showing immediately, or is this an acceptable solution?

New apk:
http://people.mozilla.org/~ahunt/fennec/46_forceShowURLBar_v3.apk
Attachment #8702366 - Attachment is obsolete: true
Attachment #8702366 - Flags: review?(margaret.leibovic)
Attachment #8702729 - Flags: review?(margaret.leibovic)
(In reply to Andrzej Hunt :ahunt from comment #18)
> Created attachment 8702729 [details] [diff] [review]
> showlocationbar_1228590_v3.patch
> 
> 
> > Currently we animate the toolbar in when you click a link to navigate within
> > the same tab, and I want to make sure we don't break that transition.
> >
> As far as I can tell, we animate the toolbar in START (which is what I think
> happens when we click a link), so we shouldn't be directly affecting the
> behaviour - however I'm still not sure I fully understand all the other
> interactions going on there yet though.
> 
> > Trying out the build you provided here, it does seem like we still animate
> > the toolbar in when changing location on the same tab, but it seems a bit
> > more janky than my Nightly build... maybe we should change this logic to
> > explicitly only show the toolbar when a new tab is selected. Maybe we could
> > change the order of the case statements and only make this call in the
> > SELECTED case.
> 
> I've done the reordering, so we now only try to show the toolbar for
> SELECTED - this seems to behave correctly for all the cases I've tested, but
> I'm thinking it would useful to have a list of the various transitions where
> the URL bar should/shouldn't be shown (I'm not sure how easy it is to write
> tests for this)?

We might be able to write a robocop test for this, but I'm worried it might be flaky in automation. But we could audit all the places we call mDynamicToolbar.setVisible to get an understanding of our current behavior.
 
> I've also changed the patch to animate the URL bar showing when switching
> tabs - this seems a bit nicer, but isn't always consistent - if the page has
> loaded fast enough then the URL bar appears immediately on switching tabs,
> whereas if the page is still loading the URL bar smoothly slides onto
> screen. Should I switch this back to showing immediately, or is this an
> acceptable solution?

Let's hear what Anthony has to say. I feel like having it always show immediately is fine, since we're also immediately showing the new page content.

However, this seems like it could be a bug with DynamicToolbarAnimator, since I don't see why we would avoid the animation if the page loaded quickly.
Comment on attachment 8702729 [details] [diff] [review]
showlocationbar_1228590_v3.patch

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

This seems fine to me, but we could also land with the immediate transition and then update the animation afterwards.
Attachment #8702729 - Flags: review?(margaret.leibovic) → review+
May related to bug 1227538. Needs to be examined to see how related.
Flags: needinfo?(dveditz)
(In reply to Al Billings [:abillings] from comment #21)
> May related to bug 1227538. Needs to be examined to see how related.

Bug 1227538 is not related with this bug and showlocationbar_1228590_v3.patch ( http://people.mozilla.org/~ahunt/fennec/46_forceShowURLBar_v3.apk ) doesn't fixe the bug 1227538 .

So, I think than these two security Bugs are not related at all.
Flags: needinfo?(abillings)
Sorry for the delay here!

(In reply to :Margaret Leibovic from comment #19)
> (In reply to Andrzej Hunt :ahunt from comment #18)
> > Created attachment 8702729 [details] [diff] [review]
> > showlocationbar_1228590_v3.patch

This UX works great. I am in favor of showing our Toolbar for new tabs regardless of scroll position. Nice job team!

> > > Currently we animate the toolbar in when you click a link to navigate within
> > > the same tab, and I want to make sure we don't break that transition.
> > >
> > As far as I can tell, we animate the toolbar in START (which is what I think
> > happens when we click a link), so we shouldn't be directly affecting the
> > behaviour - however I'm still not sure I fully understand all the other
> > interactions going on there yet though.
> > 
> > > Trying out the build you provided here, it does seem like we still animate
> > > the toolbar in when changing location on the same tab, but it seems a bit
> > > more janky than my Nightly build... maybe we should change this logic to
> > > explicitly only show the toolbar when a new tab is selected. Maybe we could
> > > change the order of the case statements and only make this call in the
> > > SELECTED case.
> > 
> > I've done the reordering, so we now only try to show the toolbar for
> > SELECTED - this seems to behave correctly for all the cases I've tested, but
> > I'm thinking it would useful to have a list of the various transitions where
> > the URL bar should/shouldn't be shown (I'm not sure how easy it is to write
> > tests for this)?
> 
> We might be able to write a robocop test for this, but I'm worried it might
> be flaky in automation. But we could audit all the places we call
> mDynamicToolbar.setVisible to get an understanding of our current behavior.
>  
> > I've also changed the patch to animate the URL bar showing when switching
> > tabs - this seems a bit nicer, but isn't always consistent - if the page has
> > loaded fast enough then the URL bar appears immediately on switching tabs,
> > whereas if the page is still loading the URL bar smoothly slides onto
> > screen. Should I switch this back to showing immediately, or is this an
> > acceptable solution?
> 
> Let's hear what Anthony has to say. I feel like having it always show
> immediately is fine, since we're also immediately showing the new page
> content.
> 
> However, this seems like it could be a bug with DynamicToolbarAnimator,
> since I don't see why we would avoid the animation if the page loaded
> quickly.

I didn't see the animation in the v3 build that :ahunt provided. But, I would like to preserve the animating-in if possible. How much work is it to get it to appear consistently all the time? as opposed to only for pages that have a lot of stuff to load?

Everything else looks good!
Flags: needinfo?(margaret.leibovic)
Flags: needinfo?(alam)
Flags: needinfo?(ahunt)
Flags: needinfo?(abillings)
(In reply to Anthony Lam (:antlam) from comment #23)
> I didn't see the animation in the v3 build that :ahunt provided. But, I
> would like to preserve the animating-in if possible. How much work is it to
> get it to appear consistently all the time? as opposed to only for pages
> that have a lot of stuff to load?
> 
> Everything else looks good!

The animation seems to be dependent on how fast the page loads (at least on the devices I'm testing on): If the page is still loading when we switch tabs it animates onto screen. If the page is partly or fully loaded, then the URL bar tends to appear immediately.

I've had a quick look at the animation code, but I don't think I understand enough of it to determine how much effort making it consistent would be.
Flags: needinfo?(ahunt)
Comment on attachment 8702729 [details] [diff] [review]
showlocationbar_1228590_v3.patch

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

Difficult: the patch is a minor UI behaviour change, it only affects behaviour in one limited case (switching tabs via the snackbar, that appears when a new tab is opened from the context menu that appears when long-pressing a link).

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

No

<Which older supported branches are affected by this flaw?>

Aurora, Beta, Release

<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?>

This patch applies cleanly to aurora.
A separate patch is needed for beta and release due to path changes - this has already been created (I will attach it to the next comment).

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

Unlikely to cause regressions, light testing required (simple UI behaviour change).
Attachment #8702729 - Flags: sec-approval?
This is a backport of the previous patch which applies to beta and release (this is a simple path change, the code change is identical).
sec-approval+ for trunk.

We'll want it on the branches as well so please nominate patches for them.
Attachment #8702729 - Flags: sec-approval? → sec-approval+
I trust ahunt that it's probably a decent amount of work to make sure this animation works consistently. 

ahunt, can you file a follow-up bug to track that work? Maybe it could be a mentor bug to investigate what's going on there.
Flags: needinfo?(margaret.leibovic) → needinfo?(ahunt)
Comment on attachment 8702729 [details] [diff] [review]
showlocationbar_1228590_v3.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: The user could be shown a fake locationbar without the firefox locationbar (showing the actual URL) being visible, fooling the user into thinking they are on a different website.
[Describe test coverage new/current, TreeHerder]: Manual testing by myself, patch when applied to fx-team has been tested by :margaret and :antlam.
[Risks and why]: Low risk
[String/UUID change made/needed]: n/a
Attachment #8702729 - Flags: approval-mozilla-aurora?
Comment on attachment 8704436 [details] [diff] [review]
showlocationbar_1228590_v3_beta_release.patch

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: The user could be shown a fake locationbar without the firefox locationbar (showing the actual URL) being visible, fooling the user into thinking they are on a different website.
[Describe test coverage new/current, TreeHerder]: Manual testing by myself, patch when applied to fx-team has been tested by :margaret and :antlam.
[Risks and why]: Low risk
[String/UUID change made/needed]: n/a
Attachment #8704436 - Flags: approval-mozilla-release?
Attachment #8704436 - Flags: approval-mozilla-beta?
(In reply to :Margaret Leibovic from comment #29)
> I trust ahunt that it's probably a decent amount of work to make sure this
> animation works consistently. 
> 
> ahunt, can you file a follow-up bug to track that work? Maybe it could be a
> mentor bug to investigate what's going on there.
Filed as Bug 1237384
Blocks: 1237384
Flags: needinfo?(ahunt)
Attachment #8702729 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8704436 - Flags: approval-mozilla-release?
Attachment #8704436 - Flags: approval-mozilla-beta?
Attachment #8704436 - Flags: approval-mozilla-beta+
Now that the patches are approved for all (In reply to Al Billings [:abillings] from comment #27)
> sec-approval+ for trunk.
> 
> We'll want it on the branches as well so please nominate patches for them.

Now that the patches are approved for trunk + branches (aurora/beta) am I allowed to land them myself, or should I checkin-needed'ify them? (Sorry, I'm still quite new to the processes here.)
Flags: needinfo?(abillings)
(In reply to Andrzej Hunt :ahunt from comment #33)

> Now that the patches are approved for trunk + branches (aurora/beta) am I
> allowed to land them myself, or should I checkin-needed'ify them? (Sorry,
> I'm still quite new to the processes here.)

I don't control the process and don't do checkins so I don't know. I usually just mark them "checkin-needed" but I'm not sure if anything blocks you from checking in yourself. I'm just a security program manager (and manager of the fuzzing team), not a developer.
Flags: needinfo?(abillings)
https://hg.mozilla.org/integration/fx-team/rev/c91d6036456490a3bb20eb63e0dd02a5dd063561
Bug 1228590 - Always show location bar when switching tabs r=margaret
https://hg.mozilla.org/mozilla-central/rev/c91d60364564
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Jordi, can you confirm that this has been fixed properly?
Flags: needinfo?(jordi.chancel)
Yes it is fixed. Very good job !
Flags: needinfo?(jordi.chancel)
Flags: sec-bounty? → sec-bounty+
Group: firefox-core-security → core-security-release
This vulnerability is fixed in Firefox 44 beta (44.0b9).
Whiteboard: [adv-main44+]
Alias: CVE-2016-1943
Summary: Location Bar Spoofing Risk - Open a webpage into a new tab which uses the JavaScript function «scrollto» leads to that the location bar is hidden - but by default, the Location Bar should be always visible when a webpage is opened. → Location Bar Spoofing Risk - scrollto leads to that the location bar is hidden
Flags: qe-verify-
Whiteboard: [adv-main44+] → [adv-main44+][post-critsmash-triage]
Group: core-security-release
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.