1693664 - Title of popups without location bar can fully be spoofed - title does not include the URL or extension name

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Rob Wu [:robwu]]

bug 1621328 moved Services.uriFixup.createExposableURI to Services.io.createExposableURI, but a consumer in tabbrowser.js at getWindowTitleForBrowser still references the old method: https://searchfox.org/mozilla-central/rev/e84baf5c730abd453be5d50dcb0aba1e743bac47/browser/base/content/tabbrowser.js#950

The relevant logic is wrapped in a try-catch that blindly ignores errors. The impact of this is that if a page can open a popup without location bar (e.g. extensions), then an extension can spoof the full popup title, and the user wouldn't know that the popup comes from an extension.

There is a unit test that is supposedly verifying that the extension name is in the title (toolkit/components/extensions/test/browser/browser_ext_windows_popup_title.js), but that doesn't work because of a bug in it (see two comments at https://bugzilla.mozilla.org/show_bug.cgi?id=1690611#c14).

So, to fix this bug, uriFixup should be replaced with io, and the unit test (browser_ext_windows_popup_title.js) should be fixed.

Comment 1

[Rob Wu [:robwu]]

Attachment: Bug 1693664 - Correctly run prefix logic in getWindowTitleForBrowser

Comment 2

[Rob Wu [:robwu]]

Comment on attachment 9204070 [details]
Bug 1693664 - Correctly run prefix logic in getWindowTitleForBrowser

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Easy. But if concerning, the unit test can be added to a different patch that is already in development, see bug report.
• 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?: esr78, all release
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Patch includes unit test that passed locally. No further testing is needed.

Comment 3

[Daniel Veditz [:dveditz]]

Comment on attachment 9204070 [details]
Bug 1693664 - Correctly run prefix logic in getWindowTitleForBrowser

I called this sec-moderate because it's limited to malicious extensions; technically doesn't need sec-approval now. Since I'm here: sec-approval+

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Correctly run prefix logic in getWindowTitleForBrowser r=Gijs
https://hg.mozilla.org/integration/autoland/rev/d031e888c05e0dca05eb6c6a3ada116cef2e48f8

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta and ESR78 approval when you get a chance.

Comment 7

[Rob Wu [:robwu]]

Comment on attachment 9204070 [details]
Bug 1693664 - Correctly run prefix logic in getWindowTitleForBrowser

Beta/Release Uplift Approval Request

• User impact if declined: Scripts that can open popups without location bar (e.g. extensions) can fully spoof the title of the window.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple fix covered by unit tests.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes a UI spoofing issue.
• User impact if declined: Scripts that can open popups without location bar (e.g. extensions) can fully spoof the title of the window.
• Fix Landed on Version: 88 pending uplift to 87
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple fix covered by unit tests.
• String or UUID changes made by this patch:

Comment 8

[Rob Wu [:robwu]]

I'm linking bug 1296365 because it had a (partially working) unit test meant to detect this kind of bug. If the test was working as intended, then this regression wouldn't have happened because the test failure would prevent the patch for bug 1621328 from landing.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9204070 [details]
Bug 1693664 - Correctly run prefix logic in getWindowTitleForBrowser

Approved for 87.0b3 and 78.9esr.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0cac59b82f83

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/e0491358343d

Comment 12

[Tom Ritter [:tjr]]

Attachment: advisory.txt