Closed Bug 693733 Opened 13 years ago Closed 12 years ago

Components.lookupMethod should not be accessible to page JS

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: zwol, Unassigned)

References

Details

(Keywords: sec-want, Whiteboard: [sg:want?])

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.
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?]
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.
|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.
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.
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.
Components.lookupMethod was intentionally exposed to web pages in bug 286629 for – drumroll – security reasons. I wonder if Flashblock still relies on it.
Blocks: 286629
"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.
(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).
> 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?
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.
Please don't turn it off for content unless there is some content equivalent we can use in Flashblock (our XBL runs in content).
No longer blocks: CVE-2012-1967
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.
Oh thanks muchly Bobby!
Depends on: 790732
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
Blocks: 856437
I split that into bug 856437. Morphing this bug doesn't make sense given its dependencies and keywords.
Status: NEW → RESOLVED
Closed: 12 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.