Disable named element access on the window object in Xray Vision for webextensions

RESOLVED FIXED in Firefox 55

Status

()

Core
XPConnect
P2
normal
RESOLVED FIXED
3 months ago
19 days ago

People

(Reporter: johannh, Assigned: kmag)

Tracking

(Blocks: 1 bug, {sec-want})

unspecified
mozilla55
sec-want
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 months ago
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

3 months ago
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...
(Reporter)

Comment 2

3 months 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.
Yeah, seems like we should add an option to disable that Xray behavior for WebExtensions.
(Assignee)

Comment 4

3 months 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.
(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
(Assignee)

Comment 10

3 months ago
We should probably do it at least for WebExtensions before 57. The security aspects are pretty worrying.
Created 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"

I could use a hand with the tests here
Attachment #8855507 - Flags: review?(bobbyholley)
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

3 months ago
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+
Created attachment 8855519 [details] [diff] [review]
Updated to review comments

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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Flags: needinfo?(kmaglione+bmo)

Comment 18

3 months 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

3 months 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

3 months 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 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.
(Assignee)

Comment 24

3 months 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

3 months 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.
I can just land all of it.  Waiting for try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=47cd87bddd125112a50f09638ee36ab63d73dc05
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

3 months 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
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
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

19 days ago
Assignee: nobody → kmaglione+bmo
You need to log in before you can comment on or make changes to this bug.