Flip __exposedProps__ default for functions to default-safe

RESOLVED FIXED

Status

()

Core
XPConnect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mrbkap, Assigned: sicking)

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [softblocker], fixed-in-tracemonkey)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

7 years ago
+++ 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?
Created attachment 506842 [details] [diff] [review]
Patch to fix

Untested, but I think this is right. Needs tests.
No longer depends on: 553102
Created attachment 506901 [details] [diff] [review]
Patch to fix

Better code, and with tests.
Attachment #506842 - Attachment is obsolete: true

Comment 6

7 years ago
Created attachment 507785 [details] [diff] [review]
patch
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.

Updated

7 years ago
Attachment #507785 - Attachment is obsolete: true
Attachment #507785 - Flags: review?(mrbkap)

Comment 8

7 years ago
I fixed a bug in the patch. The latest version is in bug 594999.

Comment 9

7 years ago
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.

Comment 11

7 years ago
Would be nice to add tests that test that we don't leak information.
Created attachment 508066 [details] [diff] [review]
Latest version
Attachment #507568 - Attachment is obsolete: true
Attachment #508066 - Flags: review?(gal)

Updated

7 years ago
Attachment #508066 - Flags: review?(gal) → review+

Updated

7 years ago
Whiteboard: [softblocker], fixed-in-tracemonkey

Comment 14

7 years ago
Jonas, the test is failing with your patch because InstallTriger + "" does toString on InstallTrigger, but InstallTrigger.toString is undefined.

Comment 15

7 years ago
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

Updated

7 years ago
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?

Comment 19

7 years ago
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.

Comment 23

7 years ago
(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.

Comment 26

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
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.
Created attachment 509424 [details] [diff] [review]
one-liner switch back to default-permit

This fixes console.log.{bind,apply,etc} for me.

I'll work on the tail-recursive fix now.
Attachment #509424 - Flags: review?(gal)

Comment 35

7 years ago
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-

Updated

7 years ago
Depends on: 631225

Comment 36

7 years ago
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)

Comment 37

7 years ago
(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)
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!
Keywords: dev-doc-needed → dev-doc-complete

Updated

7 years ago
Blocks: 601768

Comment 39

7 years ago
(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.

Comment 41

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

Updated

6 years ago
Attachment #508189 - Flags: review?(mrbkap)
You need to log in before you can comment on or make changes to this bug.