Closed
Bug 1353150
Opened 7 years ago
Closed 7 years ago
Disable named element access on the window object in Xray Vision for webextensions
Categories
(Core :: XPConnect, enhancement, P2)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: johannh, Assigned: kmag)
References
Details
(Keywords: sec-want, Whiteboard: [adv-main55-])
Attachments
(3 files, 1 obsolete file)
In his write-up of the latest Lastpass exploit[0], Tavis Ormandy uses DOM clobbering to define global variables that can pass through isolated worlds/xray vision, exploiting checks such as if ("undefined" != typeof base_url) { in extension content scripts. He states that both Firefox and Chrome extensions are vulnerable to this. To solve this, we could disable named element access in content scripts, which is the same as disabling it in Xray Vision AFAUI. I don't think there are a lot of content scripts that require window[id] to work and even if that was the case, with the switch to WebExtensions we're in a unique moment in time where we can easily break add-on compatibility without a big fallout. [0] https://bugs.chromium.org/p/project-zero/issues/detail?id=1225&desc=6
Assignee | ||
Updated•7 years ago
|
Component: Security → XPConnect
Comment 1•7 years ago
|
||
Um... Bareword property accesses in content scripts end up doing a property access on the untrusted window? You don't have to explicitly refer to the untrusted window when you want things from it? If so, then I agree that having the named property stuff on window over Xrays is a terrible idea, at least in this case...
Reporter | ||
Comment 2•7 years ago
|
||
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #1) > Um... Bareword property accesses in content scripts end up doing a property > access on the untrusted window? You don't have to explicitly refer to the > untrusted window when you want things from it? Yup, I just tried it and I can confirm there's bareword element access on the window in a simple WebExtension content script.
Comment 3•7 years ago
|
||
Yeah, seems like we should add an option to disable that Xray behavior for WebExtensions.
Assignee | ||
Comment 4•7 years ago
|
||
Is there a reason we can't just disable it altogether? I'm pretty sure element ID aliasing is generally agreed to be a Really Bad Idea that we're stuck with for web compat. I can't think of any reason we'd ever want it on security wrappers.
Comment 5•7 years ago
|
||
(In reply to Kris Maglione [:kmag] from comment #4) > Is there a reason we can't just disable it altogether? I'm pretty sure > element ID aliasing is generally agreed to be a Really Bad Idea that we're > stuck with for web compat. I can't think of any reason we'd ever want it on > security wrappers. Well, frontend and addon code might depend on it. We can fix the frontend code, and the addon code is a non-issue once we hit 57.
Comment 6•7 years ago
|
||
Given the conversation in this bug it seems somewhat easy, but that's just my impression. Can someone shed some light on the change that needs to happen? Is this just a boolean change, or is this a good second bug? How concerned are we about compat with Chrome Extensions that might use this and would have to switch to using getElementById() instead?
Keywords: sec-want
Comment 7•7 years ago
|
||
> Can someone shed some light on the change that needs to happen?
Most simply, the ResolveWindowNamedProperty and EnumerateWindowNamedProperties functions in dom/base/WindowNamedPropertiesHandler.cpp would need to check for whatever conditions we want to not expose these props in (based on the current compartment at entry into those functions), and early-return in those cases. That would allow us to restrict this to certain sorts of cases (e.g. just webextensions) or whatnot.
If we want to remove this altogether, we could probably remove those hooks and simply have the eNamedPropertiesObject cases in BindingUtils.cpp no-op.
We _might_ be able to remove more code if we completely remove this functionality, but I'm not sure how much exactly. Would need some thought.
Updated•7 years ago
|
Comment 8•7 years ago
|
||
Is anyone signing up to do this work? Are we going to wait until >= 57 to do this? Are we waiting on Chrome at all?
Priority: -- → P3
Assignee | ||
Comment 10•7 years ago
|
||
We should probably do it at least for WebExtensions before 57. The security aspects are pretty worrying.
Comment 11•7 years ago
|
||
I could use a hand with the tests here
Attachment #8855507 -
Flags: review?(bobbyholley)
Comment 12•7 years ago
|
||
Kris, are you willing to write a test? I'm not quite sure where to start on that in this case. ;)
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 13•7 years ago
|
||
Sure
Comment 14•7 years ago
|
||
Comment on attachment 8855507 [details] [diff] [review] Add a preference that controls whether named properties object properties are exposed on Xrays, with a default of "not exposed in web extension content scripts" Review of attachment 8855507 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/WindowNamedPropertiesHandler.cpp @@ +246,5 @@ > +} > + > +static int32_t sAlwaysAllowNamedPropertiesObject = 0; > +static int32_t sDisallowNamedPropertiesObjectForContentScripts = 1; > +static int32_t sDisallowNamedPropertiesObjectForXrays = 2; Make these const kFoo rather than static sFoo.
Attachment #8855507 -
Flags: review?(bobbyholley) → review+
Comment 15•7 years ago
|
||
I also renamed the pref to dom.allow_named_properties_object_for_xrays to make it clearer what it's about
Updated•7 years ago
|
Attachment #8855507 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(kmaglione+bmo)
Comment 18•7 years ago
|
||
mozreview-review |
Comment on attachment 8855553 [details] Bug 1353150: Add tests for WebExtension Xray named element property access. https://reviewboard.mozilla.org/r/127400/#review130094 ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_xrays.js:65 (Diff revision 1) > + }); > + > + equal(Preferences.get(XRAY_PREF), 1, "Should have pref=1 by default"); > + > + await extension.startup(); > + let contentPage = await ExtensionTestUtils.loadContentPage(`${BASE_URL}/file_sample.html`); file_sample.html is missing
Attachment #8855553 -
Flags: review?(mixedpuppy)
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8855553 [details] Bug 1353150: Add tests for WebExtension Xray named element property access. https://reviewboard.mozilla.org/r/127400/#review130094 > file_sample.html is missing It looks like I had the file in my local repo already, but not checked in. I can't switch branches to update the patch right now, but this is what the file looks like: https://gist.github.com/8d22c1a6a4f7a936dd9bdc7e6475e6c0
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8855553 [details] Bug 1353150: Add tests for WebExtension Xray named element property access. https://reviewboard.mozilla.org/r/127400/#review130114 r+ with file_sample included.
Attachment #8855553 -
Flags: review+
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8855552 [details] Bug 1353150: Add tests for chrome Xray named element property access. https://reviewboard.mozilla.org/r/127398/#review130128 Nice! r=me
Attachment #8855552 -
Flags: review?(bzbarsky) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8855553 [details] Bug 1353150: Add tests for WebExtension Xray named element property access. https://reviewboard.mozilla.org/r/127400/#review130132 r=me on the xray bits; I assume Shane is reviewing the scaffolding. Thank you very much for putting this together! ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_xrays.js:9 (Diff revision 1) > + > +const server = createHttpServer(); > +server.registerDirectory("/data/", do_get_file("data")); > + > +const BASE_URL = `http://localhost:${server.identity.primaryPort}/data`; > +const XRAY_PREF = "dom.allow_named_properties_object"; dom.allow_named_properties_object_for_xrays ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_xrays.js:48 (Diff revision 1) > + > + let extension = ExtensionTestUtils.loadExtension({ > + manifest: { > + content_scripts: [{ > + "matches": ["http://*/*/file_sample.html"], > + "js": ["content_script.js"], Does content_script.js need to exist?
Attachment #8855553 -
Flags: review?(bzbarsky) → review+
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8855552 [details] Bug 1353150: Add tests for chrome Xray named element property access. https://reviewboard.mozilla.org/r/127398/#review130130 ::: js/xpconnect/tests/unit/test_xray_named_element_access.js:9 (Diff revision 1) > +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > + > +Cu.import("resource://gre/modules/Preferences.jsm"); > +Cu.import("resource://gre/modules/Services.jsm"); > + > +const XRAY_PREF = "dom.allow_named_properties_object"; Need to update the pref name to dom.allow_named_properties_object_for_xrays, though.
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8855553 [details] Bug 1353150: Add tests for WebExtension Xray named element property access. https://reviewboard.mozilla.org/r/127400/#review130132 > Does content_script.js need to exist? Nope. We generate zip files for test extensions on the fly, so content_script.js gets created from the contentScript function.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 27•7 years ago
|
||
Let me know how you want to handle landing this. Feel free to land my patches with yours, or I can just land them afterwards.
Comment 28•7 years ago
|
||
I can just land all of it. Waiting for try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47cd87bddd125112a50f09638ee36ab63d73dc05
Comment 29•7 years ago
|
||
Resummarizing to reflect what we're really fixing. We can flip the pref to disable for all in bug 1354730.
Blocks: 1354730
Summary: Disable named element access on the window object in Xray Vision → Disable named element access on the window object in Xray Vision for webextensions
Comment 30•7 years ago
|
||
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/7950608b2f1f Add a preference that controls whether named properties object properties are exposed on Xrays, with a default of "not exposed in web extension content scripts". r=bholley https://hg.mozilla.org/integration/mozilla-inbound/rev/fbb4d6c9c01c Add tests for chrome Xray named element property access. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/9570de2c281a Add tests for WebExtension Xray named element property access. r=bz,mixedpuppy
Comment 31•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7950608b2f1f https://hg.mozilla.org/mozilla-central/rev/fbb4d6c9c01c https://hg.mozilla.org/mozilla-central/rev/9570de2c281a
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → kmaglione+bmo
Updated•7 years ago
|
Whiteboard: [adv-main55-]
You need to log in
before you can comment on or make changes to this bug.
Description
•