Bug 1079554 (CVE-2015-0819)

UITour: onPageEvent doesn't ignore API calls from background tabs

VERIFIED FIXED in Firefox 36

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: MattN, Assigned: MattN)

Tracking

({csectype-spoof, sec-moderate})

unspecified
Firefox 38
Points:
3
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox35 wontfix, firefox36+ verified, firefox37+ fixed, firefox38+ fixed, firefox-esr31 wontfix)

Details

(Whiteboard: [adv-main36+])

Attachments

(5 attachments, 1 obsolete attachment)

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.
Flags: in-testsuite?
Flags: firefox-backlog+
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).
Keywords: sec-moderate
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.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
Attachment #8550811 - Flags: review?(bmcbride)
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.
Iteration: --- → 38.1 - 26 Jan
Points: --- → 3
Component: General → Tours
Flags: qe-verify+
Comment on attachment 8550811 [details] [diff] [review]
v.1 Only allow specific non-UI actions

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

Eep.
Attachment #8550811 - Flags: review?(bmcbride) → review+
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.
Attachment #8550811 - Flags: review+ → review-
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).
Attachment #8550811 - Flags: review- → review?(bmcbride)
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.
Attachment #8550811 - Flags: review?(bmcbride) → review+
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).
Attachment #8550811 - Flags: sec-approval?
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.
Attachment #8550811 - Flags: sec-approval? → sec-approval+
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
Attachment #8550811 - Attachment is obsolete: true
Attachment #8552872 - Flags: review+
Attachment #8552872 - Flags: approval-mozilla-aurora?
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
Attachment #8552873 - Flags: sec-approval+
Attachment #8552873 - Flags: review+
Attachment #8552873 - Flags: approval-mozilla-beta?
Reminder to check-in the tests after shipping the fix.
Flags: needinfo?(MattN+bmo)
Attachment #8552874 - Flags: review+
Attachment #8552874 - Flags: checkin-
(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.
https://hg.mozilla.org/mozilla-central/rev/872fa257eee2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attachment #8552872 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8552873 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Is sec-moderate correct for this? I somehow thought this was a sec-high.
(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
Whiteboard: [adv-main36+]
Alias: CVE-2015-0819
Confirmed broken on Fx35, fixed on Fx36.
Status: RESOLVED → VERIFIED
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?
Attachment #8552874 - Flags: sec-approval?
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.
Attachment #8552874 - Flags: sec-approval? → sec-approval+
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
Flags: needinfo?(MattN+bmo)
Attachment #8552874 - Flags: checkin- → checkin+
Flags: in-testsuite? → in-testsuite+

Updated

4 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.