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)

enhancement

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
Component: Security → XPConnect
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...
(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.
Yeah, seems like we should add an option to disable that Xray behavior for WebExtensions.
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.
(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.
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
> 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.
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
Oops, I missed that this also affects Web Extensions.
Priority: P3 → P2
We should probably do it at least for WebExtensions before 57. The security aspects are pretty worrying.
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)
Sure
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+
I also renamed the pref to dom.allow_named_properties_object_for_xrays to make it clearer what it's about
Attachment #8855507 - Attachment is obsolete: true
Flags: needinfo?(kmaglione+bmo)
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)
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 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 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 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 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.
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.
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.
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
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
Assignee: nobody → kmaglione+bmo
Whiteboard: [adv-main55-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: