1079554 - (CVE-2015-0819) UITour: onPageEvent doesn't ignore API calls from background tabs

Status: VERIFIED FIXED

(Firefox :: Tours, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screen Shot of example exploit

UITour annotations/behaviour is torn down when a non-Tour tab is selected but we aren't preventing UITour API calls from background tabs on the whitelist.

If one of the whitelisted mozilla.org websites was compromised, users could be tricked into taking an action on some other tab using showInfo (an info panel anchored in the browser chrome). The info panel can cover the address bar and doesn't dismiss with outside clicks (the X would have be clicked).

Example code you can run on https://www.mozilla.org/en-US/firefox/35.0a1/tour/ in the Web Console:

window.onclick = function () {
  window.open('https://people.mozilla.org/~mnoorenberghe/bugs/evil.htm')
  setTimeout(function () {
    Mozilla.UITour.showInfo('selectedTabIcon',
                            'Enter your credit card to purchase Mozilla Firefox!',
                            'Three easy payments of $9.99',
                            '//mozorg.cdn.mozilla.net/media/img/firefox/australis/logo.png?2014-03')
  }, 400)
}

Once you have run this, click anywhere in the empty area of the page to have the spoofing tab and info panel open.

Comment 1

[Matthew N. [:MattN]]

Attachment: Screen Recording

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Nice catch. We were thinking of rating this sec-high as it is a troublesome spoof but we are going with moderate due to the extra step of the whitelisted website needing to be compromised (as pointed out above).

Comment 3

[Matthew N. [:MattN]]

Attachment: v.1 Only allow specific non-UI actions

To avoid breaking existing tours for cases where a tour tab is loaded in the background (e.g. session restore), I added a list of APIs that are okay to use in the background. Most, hopefully all, tours currently listen for visibilitychange to re-show any UI that needs to be shown already so at least the newer tours should continue to work.

Comment 4

[Matthew N. [:MattN]]

It would be useful to have QE test with existing tours (e.g. Help > …Tour, Loop Tour, etc.) to make sure this doesn't break them.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8550811 [details] [diff] [review]
v.1 Only allow specific non-UI actions

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

Eep.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8550811 [details] [diff] [review]
v.1 Only allow specific non-UI actions

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

Er, actually...

::: browser/components/uitour/UITour.jsm
@@ +358,5 @@
>        return false;
>      }
>  
> +    if ((aEvent.pageVisibilityState == "hidden" ||
> +         aEvent.pageVisibilityState == "unloaded") &&

This doesn't cover your original example, where the tour tab is still selected but another window has focus.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8550811 [details] [diff] [review]
v.1 Only allow specific non-UI actions

(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #6)

> ::: browser/components/uitour/UITour.jsm
> @@ +358,5 @@
> >        return false;
> >      }
> >  
> > +    if ((aEvent.pageVisibilityState == "hidden" ||
> > +         aEvent.pageVisibilityState == "unloaded") &&
> 
> This doesn't cover your original example, where the tour tab is still
> selected but another window has focus.

It does cover it. window.open with no arguments opens a new tab, not a new window by default AFAIK and I don't think there is a problem with annotations showing on a background window as long as we aren't using something like level=top on a panel (making it appear over all windows).

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8550811 [details] [diff] [review]
v.1 Only allow specific non-UI actions

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

Ok, fair enough.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8550811 [details] [diff] [review]
v.1 Only allow specific non-UI actions

Since this requires a whitelisted site I'm not sure if it's necessary to obfuscate the problem more e.g. not landing the test initially and/or changing the commit message.

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

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

Which older supported branches are affected by this flaw? Fx29+

If not all supported branches, which bug introduced the flaw? Original UITour landing (bug 862998)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? This should be easy to backport everwhere.

How likely is this patch to cause regressions; how much testing does it need? Low risk of regressions. There are a fixed number of existing tours to test (firstrun, whatsnew [a few], & Hello).

Comment 10

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

Comment on attachment 8550811 [details] [diff] [review]
v.1 Only allow specific non-UI actions

sec-approval+ for checkin on Jan 21 or later. We shouldn't land this with any tests. We can land the tests after we ship the fix public. 

We'll want Aurora and Beta patches as well.

Comment 11

[Matthew N. [:MattN]]

Attachment: v.1 m-c and m-a patch without tests r=Unfocused sec-approval=abillings

Approval Request Comment
[Feature/regressing bug #]: Initially UITour landing in Fx29
[User impact if declined]: A site on the allow list could trick the user into
[Describe test coverage new/current, TreeHerder]: automated tests soon to be running on m-c
[Risks and why]: Low risk of breaking an interactive tour when a user changes the visibility of a tour tab
[String/UUID change made/needed]: None

Comment 12

[Matthew N. [:MattN]]

Attachment: v.1 m-b patch without tests r=Unfocused sec-approval=abillings

Approval Request Comment
[Feature/regressing bug #]: Initially UITour landing in Fx29
[User impact if declined]: A site on the allow list could trick the user into
[Describe test coverage new/current, TreeHerder]: automated tests soon to be running on m-c
[Risks and why]: Low risk of breaking an interactive tour when a user changes the visibility of a tour tab
[String/UUID change made/needed]: None

Comment 13

[Matthew N. [:MattN]]

Attachment: v.1 Tests r=Unfocused (don't check-in yet)

Reminder to check-in the tests after shipping the fix.

Comment 14

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #11)
> [Describe test coverage new/current, TreeHerder]: automated tests soon to be

Sorry, this was a mistake since I'm not landing the tests now. The tests pass locally for me on 10.9. I think a QE spot-check of existing tours would be useful.

Comment 15

[Matthew N. [:MattN]]

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/872fa257eee2

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/872fa257eee2

Comment 17

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

Is sec-moderate correct for this? I somehow thought this was a sec-high.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aee871513001
https://hg.mozilla.org/releases/mozilla-beta/rev/e35e98044772

Comment 19

[Matthew N. [:MattN]]

(In reply to Al Billings [:abillings] from comment #17)
> Is sec-moderate correct for this? I somehow thought this was a sec-high.

See comment 2. It sounds like it would be high if we didn't have a list of allowed sites: https://mxr.mozilla.org/mozilla-central/source/browser/app/default_permissions#9

Comment 20

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

Confirmed broken on Fx35, fixed on Fx36.

Comment 21

[Matthew N. [:MattN]]

Comment on attachment 8552874 [details] [diff] [review]
v.1 Tests r=Unfocused (don't check-in yet)

[Security approval request comment for the tests]
This fix is shipping in 36. When can I land the tests?

Comment 22

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

Comment on attachment 8552874 [details] [diff] [review]
v.1 Tests r=Unfocused (don't check-in yet)

Yes, we can check in tests if this was fixed in 36.

Comment 23

[Matthew N. [:MattN]]

Comment on attachment 8552874 [details] [diff] [review]
v.1 Tests r=Unfocused (don't check-in yet)

Thanks for clarifying.

https://hg.mozilla.org/integration/fx-team/rev/c3edb8128b39

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c3edb8128b39