Closed Bug 628410 Opened 14 years ago Closed 14 years ago

Flip __exposedProps__ default for functions to default-safe

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: mrbkap, Assigned: sicking)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [softblocker], fixed-in-tracemonkey)

Attachments

(3 files, 5 obsolete files)

+++ This bug was initially created as a clone of Bug #553102 +++ Right now, things are default-unsafe. We should make an object that has no __exposedProps__ not expose anything by default.
That should be "We should make a *function* object that has...", right?
Attached patch Patch to fix (obsolete) — Splinter Review
Untested, but I think this is right. Needs tests.
Attached patch Patch to fix (obsolete) — Splinter Review
Better code, and with tests.
Attachment #506842 - Attachment is obsolete: true
Attached patch Better patch (obsolete) — Splinter Review
Attachment #506901 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attached patch patch (obsolete) — Splinter Review
Attachment #507672 - Attachment is obsolete: true
Andreas: What's different about this second patch? I ran the first one through tryserver and it passed fine (with exception of the COW tests which tests for the old behavior). I think this patch is ready for review.
Attachment #507785 - Attachment is obsolete: true
Attachment #507785 - Flags: review?(mrbkap)
I fixed a bug in the patch. The latest version is in bug 594999.
And the fact that this passes try proves that we don't have sufficient test coverage. The first patch permits InstallTrigger.foo = 42 from content (writes are not rejected).
Well, it's possible that the COWs tests cover that. Since I knew they would fail since they are still checking for the throwing behavior, I didn't check explicitly what's failing on them.
Would be nice to add tests that test that we don't leak information.
Attached patch Latest versionSplinter Review
Attachment #507568 - Attachment is obsolete: true
Attachment #508066 - Flags: review?(gal)
Attachment #508066 - Flags: review?(gal) → review+
Whiteboard: [softblocker], fixed-in-tracemonkey
Jonas, the test is failing with your patch because InstallTriger + "" does toString on InstallTrigger, but InstallTrigger.toString is undefined.
I added toString to the list of exposed props on InstallTrigger. toSource is already allowed and that reveals much more. Still, mrbkap should review this change. http://hg.mozilla.org/tracemonkey/rev/771d5cb2c5a3
Attached patch patchSplinter Review
Attachment #508189 - Flags: review?(mrbkap)
What is |foo + ""| supposed to do if foo doesn't have a toString? I was under the impression that it wouldn't throw at least? One fix here might be to simply remove the test that's failing if throwing is indeed the expected behavior.
I think it will throw, per ECMA -- seems the tests showed that too. Virtually all objects chain back to Object.prototype.toString, but InstallTrigger seems not to, for some reason. Or is the inaccessible property fatally shadowing an accessible one farther up the prototype chain?
foo + "" does foo.toString and since foo is a COW and toString is not in exposedProps we return undefined and the VM tries to call that. Wrappers hide their proto chain during lookup. Nothing from the proto chain is visible if its not in exposed props. That would be super dangerous. If someone sticks something on O.p it would shine through all chrome objects visible to content. The current approach seems food to me. You have to explicitly enable toString.
I think the question here is if we should change the test or expose toString. Either is fine with me though the former seems safer.
(In reply to comment #20) > I think the question here is if we should change the test or expose toString. > Either is fine with me though the former seems safer. This misses the question in comment 19. Are we returning undefined for toString on the wrapper when we should find nothing and therefore not shadow a standard toString on a prototype object (presumably Object.prototype)? /be
I didn't interpret comment 19 as a question, but rather as support of the current design. Andreas, please clarify. A few options: 1. Only expose properties which are listed in __exposedProps__. Doesn't matter if the property comes from the object itself, or from something on the prototype chain. Also doesn't matter if __exposedProps__ comes from the object itself or from something on the prototype chain. Normal shadowing rules apply. This is what we do now on the trace-monkey branch. 2. Assuming a prototype chain like A->B->C (A.__proto__ === B, B.__proto__ === C). Properties from A are only exposed if A.__exposedProps__ expose them, and if A.hasOwnProperty("__exposedProps__") is true. Properties from B are only exposed if B.__exposedProps__ exposes the property and B.hasOwnProperty("__exposedProps__") returns true. And so on. 3. Assuming the same prototype chain as in 2. Expose all properties on A.__exposedProps__ (no matter where in the proto-chain that __exposedProps__ comes from), and expose all properties on B and C no matter what. I think 2 is an interesting idea and allows to keep more of the ECMAScript prototype design in exposed objects. But I think it requires that we default __exposedProps__ to "safe". I.e. if an object doesn't have __exposedProps__ we expose nothing. I.e. I think it requires fixing bug 553102, something we have deemed is too likely to break addons for Firefox 4. But 2 might be an option for next release. I'll leave to others to determine how well it fits with ECMAScript though. I think 3 is too likely to cause security leakage. It means that setting something on Object.prototype is very risky for any chrome code to do since if you do that then those functions are automatically exposed as soon as you expose any objects to content. This leaves 1 as the only option for Firefox 4 that I can think of. I don't think this will be a big problem for web authors or addon authors.
(3) is what we do right now and I think that makes the most sense. Anything that is not in __exposedProps__ is censored, no matter whether it exists or not.
(In reply to comment #22) > I didn't interpret comment 19 as a question, but rather as support of the > current design. Andreas, please clarify. I'm just trying to figure out why the string conversion threw. It's rare for that to happen (clearly, you can befoul your own nest as Mark Miller put it by setting Object.prototype.toString = "haha" or whatever). Were we shadowing with a censoring value of undefined? > I think 2 is an interesting idea and allows to keep more of the ECMAScript > prototype design in exposed objects. Blech. That's JS prototype design (it predates ECMA-262 by several years). > I think 3 is too likely to cause security leakage. It means that setting > something on Object.prototype is very risky for any chrome code to do since if > you do that then those functions are automatically exposed as soon as you > expose any objects to content. If their ids are among __exposedProps__, and only if. Right? I agree with Andreas, we're talking about JS property lookup so "in" (the operator), not "hasOwnProperty". /be
(In reply to comment #23) We're currently doing (1). (3) is exposing all properties on the prototype chain, no matter what __exposedProps__ says. I don't think that's safe, and get the impression that you agree.
I meant 1. sicking is right.
Ugh, still misunderstandings all around :( I'll try to explain my three options from comment 22 more formally. Say that we are getting property "foo" from object A: Option 1. (what we do now, on tracemonkey, as of yesterday) var exposed = A.__exposedProps__; if (!exposed || (exposed.foo && /r/.match(exposed.foo))) return A.foo; return undefined; Option 2. let obj = A; while (obj) { let expDesc = Object.getOwnPropertyDescriptor(obj, "__exposedProps__"); if (expDesc) { let expProp = expDesc.value; if ("foo" in expProp) return /r/.match(expProp.foo) ? obj.foo : undefined; } obj = obj.__proto__; } Option 3. var exposed = A.__exposedProps__; if (!exposed || (exposed.foo && /r/.match(exposed.foo))) return A.foo; return A.__proto__.foo; Note that there is one important difference here, which is that option 2 defaults to not exposing things when there are no __exposedProps__ at all. Options 1 and 3 defaults to exposing everything in that case. This makes the comparison somewhat apples-to-oranges, but it doesn't really make sense to me to rewrite option 2 as defaulting to exposing everything. Anyhow. With these options hopefully established, here is why we currently throw: In option 1, when __exposedProps__ exists but doesn't whitelist toString, the property is completely hidden. Note that we in that case always return undefined, no matter what exists on the prototype chain. Hence stringifying throws. Option 2 could be used to fix this. But option 2 defaults to not exposing anything if __exposedProps__ doesn't exist. This has been deemed to risky of a change for Firefox 4. But it could be an option for next release. Though we'd still have to explicitly whitelist toString on Object.prototype in the scope that installs InstallTrigger in order to make toString available. Ok, I hope that that explains things. There are definitely lots of ways we can solve this, the three above are just some examples. If someone think we should do something else, either for FF4 or later, please feel free to come up with additional options.
Those aren't the only options. 4) Wrappers should only expose the items in __exposedProps__, and things that are not there should be treated as not existing. Writing to a read-only property should behave as it does when setting a getter-only object (silently ignored, unless strict). Object.prototype is not a wrapper, and so its toString should be visible and used. This is maximally compatible, as far as I can tell, permits wrappers to exist safely in the prototype chain, and reduces the differences in behaviour among the wrapper / native object / host object triad.
Shaver: is that option 3 from sicking's last comment? Kind of, but it's written there only in terms of exposedProps on the direct object and no wrappers needing the exposed property magic beyond that on the proto chain. Anway, your comment 18 and my comment 21 wanted what you say in comment 28. The lack of exposed property should not be shadowing a property of the same name that is higher up the prototype chain (and exposed). Whew! /be
Sorry, I misread option 3 -- it's what I want, since it's inductive over other wrappers in the proto chain I assume. I misread the .proto thing as being .prototype, which is silly.
Yes, option 3 -- tail recursion FTW! /be
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
This landed on m-c, breaking console.log.bind or console.log.apply, at least the former of which is used by firebug and the add-on builder editor. It may need to be one-line reverted to save b11.
This fixes console.log.{bind,apply,etc} for me. I'll work on the tail-recursive fix now.
Attachment #509424 - Flags: review?(gal)
Comment on attachment 509424 [details] [diff] [review] one-liner switch back to default-permit You have to set perm as well. Also sicking should review this. Its his baby.
Attachment #509424 - Flags: review?(gal) → review-
Depends on: 631225
I backed out http://hg.mozilla.org/mozilla-central/rev/6d5c859c452d and http://hg.mozilla.org/mozilla-central/rev/771d5cb2c5a3, but only on the beta 11 relbranch (GECKO20b11_2011020209_RELBRANCH). Please back this out of trunk if need be (not sure if we are keeping this in, going with a different approach, or backing out due to bug 631225)
(also note I had to disable the test on the relbranch with http://hg.mozilla.org/mozilla-central/rev/f9d66f4d17bf as the test wasn't backed out when backing out the changesets in comment 32)
Depends on: 631725
I've added a note in the blue box in this section: https://developer.mozilla.org/en/XPConnect_wrappers#Other_security_wrappers Is this adequate? The entire area of security wrappers is slightly outside the area of things I understand well, so it may not be. Feel free to add more as appropriate!
Blocks: 601768
(In reply to comment #38) > Is this adequate? The entire area of security wrappers is slightly outside the > area of things I understand well, so it may not be. Feel free to add more as > appropriate! Sorry Eric, I'd say "not adequate". Our introduction to the __exposedProps__ property is in the section "Other security wrappers". That may be ok for the implementers, but I guess they already know the subject. For extension authors we don't have "Other security wrappers". We have objects returned by API, which may be references to wrappers. The XPConnect wrappers page is the wrong point of view. A more promising page is https://developer.mozilla.org/en/Safely_accessing_content_DOM_from_chrome but it is mostly about old versions. But I've said all this before. So I'll play 20 Questions on moz.dev.platform with Boris again. How could we have a different outcome? Eric should the two of us try to write new doc page from the perspective of an extension dev? Is it worth it?
Yes, there needs to be significant work done on wrappers documentation. I continue not to have any real understanding of what they are or how they work, and now we have XrayWrappers, which also need documenting.
(In reply to comment #40) > Yes, there needs to be significant work done on wrappers documentation. I > continue not to have any real understanding of what they are or how they work, > and now we have XrayWrappers, which also need documenting. My point is we don't need improvements in documentation of wrappers. Perhaps you can ask a few extension authors: do you need more documentation on what wrappers are and how they work? Or do you need more documentation on how to safely and correctly interact with content objects?
Comment on attachment 508066 [details] [diff] [review] Latest version >+ }, >+ >+ _getTopChromeWindow: function(window) { >+ var Ci = Components.interfaces; >+ return window.QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIWebNavigation) >+ .QueryInterface(Ci.nsIDocShellTreeItem) >+ .rootTreeItem >+ .QueryInterface(Ci.nsIInterfaceRequestor) >+ .getInterface(Ci.nsIDOMWindow) >+ .QueryInterface(Ci.nsIDOMChromeWindow); >+ }, >+ _getAutoCompletePopup: function(window) { >+ return this._getTopChromeWindow(window).document >+ .getElementById("PopupAutoComplete"); >+ }, >+ addAutoCompletePopupEventListener: function(window, listener) { >+ this._getAutoCompletePopup(window).addEventListener("popupshowing", >+ listener, >+ false); >+ }, >+ removeAutoCompletePopupEventListener: function(window, listener) { >+ this._getAutoCompletePopup(window).removeEventListener("popupshowing", >+ listener, >+ false); >+ }, >+ isBackButtonEnabled: function(window) { >+ return !this._getTopChromeWindow(window).document >+ .getElementById("Browser:Back") >+ .hasAttribute("disabled") >+ }, > } > So this added code to SpecialPowers which doesn't work in E10s :( Could you perhaps file a followup to fix that.
Attachment #508189 - Flags: review?(mrbkap)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: