Components.lookupMethod should not be accessible to page JS

RESOLVED FIXED

Status

()

Core
XPConnect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: zwol, Unassigned)

Tracking

({sec-want})

Trunk
sec-want
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want?])

(Reporter)

Description

7 years ago
Per http://blog.kotowicz.net/2011/10/sad-state-of-dom-security-or-how-we-all.html Components.lookupMethod is accessible to page JS and can be used to bypass sandboxing.

I don't think there's any legitimate use for this in page JS - it should just be made inaccessible.  Actually I don't think there's any legitimate use for Components in page JS, period, except maybe Components.results for DOM exception matching (and people shouldn't need that one).

This is a security problem, but it's already been disclosed and is only relevant for a style of sandboxing that is not the usual thing, so I am not marking it secret.

Comment 1

7 years ago
I don't agree with your argument that this is a security problem. |window| is not a sandbox.

Components.lookupMethod is just a quirky version of Object.getOwnPropertyDescriptor Once bug 560072 is fixed, you should be able to pull off the same "attack" with standard ES5 features:

Object.getOwnPropertyDescriptor(document, "cookie").get.call(document)

The differences I'm aware of are that lookupMethod: doesn't suffer from bug 560072; returns a bound function; and can return a "native". Maybe that last one is an argument for hiding it from web pages (I think it was relevant in bug 650273.)
Whiteboard: [sg:want?]
(Reporter)

Comment 2

7 years ago
To be clear, I have not personally assessed this report in detail, I'm just passing along the claim that |C.lookupMethod| was an unresolvable hole in an otherwise tight sandbox.  They appear to have gone to some length to remove other ways to get at "true" object contents.

Comment 3

7 years ago
|window| may not be a sandbox but right now this feature is keeping it from being turned into one by providing a non-standard interface with completely atypical behavior.

Assuming I understand |O.getOwnPropertyDescriptor| correctly it doesn't raise the same concerns that |C.lookupMethod| would -- as far as I can see it rather enhances the possibility to contain specific DOM properties and manage the access to their values and child properties.

Comment 4

7 years ago
Oh, I see, the testcase starts with

Object.defineProperty(document, 'cookie', {value: null, configurable: false}); // nullify and seal the original cookie

So the problem is that lookupMethod looks at the "native" document object rather than the current one. I can buy that argument.

But you'll still be able to do call Object.getOwnPropertyDescriptor on document.__proto__ and other document objects (e.g. ones returned by DOMParser). I believe that preventing the "attacker" from getting their hands on the built-in cookie getter will prove difficult.

Comment 5

7 years ago
Tor would also appreciate this change, judging from
https://trac.torproject.org/projects/tor/ticket/2873

http://www.google.com/search?q=site:userscripts.org+Components.lookupMethod shows no hits, suggesting that even sneaky Mozilla-specific website code is unlikely to break.

Comment 6

7 years ago
Components.lookupMethod was intentionally exposed to web pages in bug 286629 for – drumroll – security reasons. I wonder if Flashblock still relies on it.

Updated

7 years ago
Blocks: 286629

Comment 7

7 years ago
"But you'll still be able to do call Object.getOwnPropertyDescriptor on document.__proto__"

The MDN page for __proto__ tells me it's either "Non-Standard" and "Deprecated". So - same as with the old setter/getter syntax I kinda suspect it to disappear at some point? 

https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/Proto
Yes, Flashblock still uses it:
http://www.mozdev.org/source/browse/flashblock/source/content/flashblock/flashblock.xml?rev=1.56

It wasn't a security issue, per se, just that Flashblock uses an XBL binding that runs in content scope. If content pages redefine certain DOM methods, they can easily break the Flashblock binding. lookupMethod is used to ensure that we get the real implementation of DOM methods.
It looks like Flashblock is careful about using this, with try/catch fallback, so you wouldn't directly break Flashblock by making this change. You would open it back up to the original problem, however.

Granted, relying on content-applied XBL to do anything sane is probably not a great strategy here, and bug 549697 would be a better long-term solution.

Comment 10

7 years ago
(In reply to Mario Heiderich from comment #7)
> "But you'll still be able to do call Object.getOwnPropertyDescriptor on
> document.__proto__"
> 
> The MDN page for __proto__ tells me it's "Non-Standard"...

I mean Object.getPrototypeOf(document).

Comment 11

7 years ago
> It looks like Flashblock is careful about using this, with try/catch fallback

Only for the SeaMonkey version. The fallback has long been removed in the Firefox version. I should remove the try/catch block as it's obsolete code to support Mozilla Suite which doesn't have Components.lookupMethod.

With increasing use of Javascript frameworks that redefine DOM methods in an attempt at cross browser compatibility, the probability of breaking Flashblock increases. And IIRC I've come across one or two websites that do this deliberately in order to defeat Content blockers (Flashblock) and NoScript like extensions.

Is there anyway for XBL defined in chrome to (safely) execute with chrome privileges?

Comment 12

7 years ago
I see - but I think it cannot be compared that easily. Unlike |C.lookupMethod| the mentioned |O.getPrototypeOf| is standards conform and most importantly can be overwritten or wrapped. It might return the same data but 

Ex.:

Object.getPrototypeOf=null;
Object.getPrototypeOf(window) 

// versus

Components.lookupMethod=null;
Components.lookupMethod(top,'window')

In any way - |C.lookupMethod| is a non-standard Gecko internal hindering the ES5 Object extensions from delivering - and additionally refuses to be disabled by standard conform methods. Gareth Heyes also mentioned that |C.lookupMethod| can be disabled by settings its |__proto__| to null - which moves it even more into the shady non-standards corner.

In terms of breaking extensions - do we know of any other case where this is being used improperly (the "improperly" is personal opinion of mine - no offense ;))?
Components.lookupMethod does the same thing we use XPCNativeWrapper/XrayWrapper for. So if we turn it off for content, we should just kill it entirely, since we get it automatically for chrome.

Comment 14

7 years ago
Please don't turn it off for content unless there is some content equivalent we can use in Flashblock (our XBL runs in content).
Blocks: 758344
No longer blocks: 758344
I'm re-implementing this method over in bug 774245 in a more sane way. It will also disappear from page JS once we can make the Components object visible only to XBL.

Comment 16

6 years ago
Oh thanks muchly Bobby!

Updated

6 years ago
Depends on: 808457
Now that bug 790732 landed, Components doesn't exist in content, so this is effectively fixed as-filed. But I don't really want to close this bug, because the Components stuff is behind a pref that we may end up flipping in on different channels.

So I'm morphing this bug into a removal of lookupMethod itself, which will be possible once Components-in-content is no longer behind a pref.
Depends on: 856424
Summary: Components.lookupMethod should not be accessible to page JS → Remove Components.lookupMethod

Updated

5 years ago
Blocks: 856437

Comment 18

5 years ago
I split that into bug 856437.  Morphing this bug doesn't make sense given its dependencies and keywords.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
No longer depends on: 856424
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Summary: Remove Components.lookupMethod → Components.lookupMethod should not be accessible to page JS
You need to log in before you can comment on or make changes to this bug.