789298 - Warn about failed property accesses on COWs without __exposedProps__ present

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[Josh Matthews [:jdm]]

From bug 553102 comment 130:
 
> Bobby Holley (:bholley) 2012-09-03 16:03:37 PDT
>
>(In reply to Kris Maglione [:kmag] from comment #129)
>> Can we throw when people try to inject objects without __exposedProps__ into
>> content rather than just making them silently innocuous? Even with the two
>> releases of warnings, I think this is going to make issues a lot harder to
>> track down in older codebases.
>
>The only way to do this would be to make JS_WrapValue fail for non-exceptional >conditions, which I'd rather not do (we did it for e4x objects, and it was a >major pain).
>
>We could certainly warn, though. Probably the best thing to do would be to >WarnOnceAbout when we compute a ChromeObjectWrapper in WrapperFactory::Rewrap on >an object without __exposedProps__. This would add the slight overhead of looking >up the __exposedProps__ property at wrap time, but that's probably not such a big >deal.
>
>I'm pretty backlogged coming back from vacation, so I'm unlikely to get to it >soon. But I think it should be simple for a non-xpcninja to do, and am happy to >provide support. Bug 758563 is a good starting point for boilerplate code and >tests.

I'm willing to mentor somebody who takes this on. Rewrap is here: http://hg.mozilla.org/mozilla-central/file/ca630290d89d/js/xpconnect/wrappers/WrapperFactory.cpp#l309

Comment 1

[Peter Xiao]

My friend and I are new to this and would like to take this bug to work on. We're doing this for a class, but I'm interested in doing more in the future.

We'd appreciate having a mentor as well!

Peter Xiao

Comment 2

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #1)
> My friend and I are new to this and would like to take this bug to work on.
> We're doing this for a class, but I'm interested in doing more in the future.

Nice! Glad to have you on board. :-)

Comment 3

[Josh Matthews [:jdm]]

Peter, please feel free to speak up (or email, or check in #introduction on IRC) if anything is unclear.

Comment 4

[Peter Xiao]

Great! Can I get assigned to this bug? 

Also, what is the best way to contact you? Through email or on IRC? If IRC, what is your handle (mine is qqqqqqqq)?

Comment 5

[Josh Matthews [:jdm]]

Email is best right now; I'm not on IRC as much as usual this week.

Comment 6

[Bobby Holley (:bholley)]

So, I'm thinking about this again, and I'm starting to feel that warning when the object is exposed isn't really the right thing to do here. There are lots of ways these objects can end up in content, and silently using a secure wrapper is fine. It's only ever an issue if somebody tries to _access_ these objects. The problem is that, right now, these accesses silently fail and give no indication that __exposedProps__ was missing if they're a get (rather than a set).

So I think the smarter thing to do here is to warn about what's going on. Specifically, we could call nsContentUtils::ReportToConsole right before denying access in ExposedPropertiesOnly::check.

Kmag, do you think this would be sufficient?

Comment 7

[Kris Maglione [:kmag]]

I'm not sure how useful this would be to report when it's accessed. The old warnings were useful because they showed what code was doing the injection, so the problem was easy to track down. I also didn't see many false positives, and can't imagine why we wouldn't want to know when people were injecting useless objects into content, in any case. Can you think of a valid reason a chrome object would wind up in content where a warning would not be useful?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #7)
> The old
> warnings were useful because they showed what code was doing the injection,
> so the problem was easy to track down.

What old warnings are you referring to, here? AFAIK we've never warned at wrap time. The old warnings would only warn when a property was accessed on a COW without __exposedProps__. Really, the only thing that changed is that we starting throwing (instead of warning) in the SET case, and silently failing (instead of warning) in the GET case.

So if those warnings were useful to you before, it sounds like my proposal in comment 6 would be sufficient. Thoughts?

Comment 9

[Wesley Kim]

(In reply to Bobby Holley (:bholley) from comment #8)
> (In reply to Kris Maglione [:kmag] from comment #7)
> > The old
> > warnings were useful because they showed what code was doing the injection,
> > so the problem was easy to track down.
> 
> What old warnings are you referring to, here? AFAIK we've never warned at
> wrap time. The old warnings would only warn when a property was accessed on
> a COW without __exposedProps__. Really, the only thing that changed is that
> we starting throwing (instead of warning) in the SET case, and silently
> failing (instead of warning) in the GET case.
> 
> So if those warnings were useful to you before, it sounds like my proposal
> in comment 6 would be sufficient. Thoughts?

Hi Josh, Bobby,

We saw the discussion going on, and we have a few questions and have changed our approach!

If we end up using a nsContentUtils::ReportToConsole, then we think this should be placed right before the deny. 

In order to use that, we need the nsIDocument. The code to get this is already there, so we want to move the code to the proper scope.

As far as the nsContentUtils::ReportToConsole call goes, what parameters should we use for? 

Lastly, how do we test this?

Thanks,
Peter Xiao, Wesley Kim

Comment 10

[Bobby Holley (:bholley)]

From email:

(In reply to Wesley Kim from comment #9)
> Hi Josh, Bobby,
> 
> We saw the discussion going on, and we have a few questions and have changed
> our approach!
> 
> If we end up using a nsContentUtils::ReportToConsole, then we think this
> should be placed right before the deny. 

Yeah, that sounds right.

Also, we might only want to warn for GETs, since the SETs already throw. It's worth checking to make sure that the same information is available in both cases.


> In order to use that, we need the nsIDocument. The code to get this is
> already there, so we want to move the code to the proper scope.

Why do you need the nsIDocument?

> As far as the nsContentUtils::ReportToConsole call goes, what parameters
> should we use for

I don't understand this question.

> 
> Lastly, how do we test this?
> 

See the patch attached to bug 758563 for an example of a similar test.

Comment 11

[Wesley Kim]

(In reply to Bobby Holley (:bholley) from comment #10)
> > In order to use that, we need the nsIDocument. The code to get this is
> > already there, so we want to move the code to the proper scope.
> 
> Why do you need the nsIDocument?
> 
> > As far as the nsContentUtils::ReportToConsole call goes, what parameters
> > should we use for
> 
> I don't understand this question.

This question and the question relating to nsIDocument, pertain to figuring out the signature for nsContentUtils::ReportToConsole (http://dxr.mozilla.org/mozilla-central/content/base/src/nsContentUtils.cpp#3201).

We saw that one of the parameters was for an nsIDocument, so we thought we would need to pass the document into the warning. We're not sure what to do for the remaining required parameters.

Thanks,

Peter and Wesley

Comment 12

[Peter Xiao]

(In reply to Wesley Kim from comment #11)
> (In reply to Bobby Holley (:bholley) from comment #10)
> > > In order to use that, we need the nsIDocument. The code to get this is
> > > already there, so we want to move the code to the proper scope.
> > 
> > Why do you need the nsIDocument?
> > 
> > > As far as the nsContentUtils::ReportToConsole call goes, what parameters
> > > should we use for
> > 
> > I don't understand this question.
> 
> This question and the question relating to nsIDocument, pertain to figuring
> out the signature for nsContentUtils::ReportToConsole
> (http://dxr.mozilla.org/mozilla-central/content/base/src/nsContentUtils.
> cpp#3201).
> 
> We saw that one of the parameters was for an nsIDocument, so we thought we
> would need to pass the document into the warning. We're not sure what to do
> for the remaining required parameters.
> 
> Thanks,
> 
> Peter and Wesley

Here's what we think the call to ReportToConsole will look like:

nsContentUtils::ReportToConsole(nsIScriptError::warningFlag,
                                  "DOM Events", doc,
                                  nsContentUtils::eDOM_PROPERTIES,
                                  "NoExposedPropsWarning");

aErrorFlags should be set to a warning.
We're not sure about aCategory, what is the "module" reporting error? 
We still think we need to get the nsIDocument for aDocument?
The property file will be dom.properties, so we pass in nsContentUtils::eDOM_PROPERTIES.
The name of the warning is "NoExposedPropsWarning". Is there a constant defined for this somewhere?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #12)
> We're not sure about aCategory, what is the "module" reporting error?

Just use "DOM Events".

> We still think we need to get the nsIDocument for aDocument?

Ah, good point. See the gunk on AccessCheck.cpp for an example of how to get the document.

> The property file will be dom.properties, so we pass in
> nsContentUtils::eDOM_PROPERTIES.

Right. That gets localized into other languages.

> The name of the warning is "NoExposedPropsWarning".

I think we want a different warning here. "NoExposedPropsWarning" is what we report when we allow access via the exception for sandboxes, so the message says something to the effect of "this will work for now, but will not work later". The warning we want says "Hey, this doesn't work because you didn't have __exposedProps__". 

> Is there a constant defined for this somewhere?

It's a string in dom.properties.

Comment 14

[Peter Xiao]

Attachment: Bug 789298 Fix V1

Comment 15

[Peter Xiao]

Above is our fix. Please review and give feedback! Thanks!

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 676504 [details] [diff] [review]
Bug 789298 Fix V1

Hey guys,

This is great work! Looks like you have a solid understanding of what's going on here, which is awesome. You're definitely ahead of the curve. :-)

I've been talking to Blake about this, and he's been explaining a bit of the history as to why we don't throw for invalid GETs on ChromeObjectWrappers (that is to say, why we call |static bool Deny()| rather than AccessCheck::deny. The basic issue is that there are two cases when a chrome JS object ends up in content (and thus is wrapped in a ChromeObjectWrapper). First, there are chrome objects that are exposed to content by accident, and thus have no __exposedProps__ property. In this case, we're happy to throw whenever someone touches the object. However, the second case involves intentionally exposing chrome objects to content (by defining an explicit __exposedProps__). In particular, we're starting to implement DOM interfaces with chrome javascript and expose them via ChromeObjectWrappers, and the DOM spec requires that invalid GETs never throw. So this is why Blake did the conservative thing with static bool Deny();

So this all boils down to the following: if we know an object was not exposed intentionally, we want to complain as loudly as possible (that is to say, throw rather than warn). If there's a possibility that the object might be exposed intentionally, then we don't want to complain at all (not even warn), because that would violate the spec.

Now, how do we determine if an object was intentionally exposed? In my opinion, the best heuristic is the one you guys are using in this patch: if there's no __exposedProps__ property at all, then the situation is probably arising by some sort of accident, and we should complain. The one caveat is arrays: arrays allow access to indexed properties (myArray[3]) and .length even without __exposedProps__, so intentionally-exposed arrays rarely have an __exposedProps__ property. So we'll have to check for that.

So in the end, I think the final patch we want in Gecko is your guys' patch, with the following modifications:
1 - Replace the call to WarnToConsole with AccessCheck::deny, but do it only in the case where the object isn't an array.
2 - Remove the warning string and update the tests appropriately.

Now, I feel really bad because you guys are on a deadline and I'm (yet again) moving the target here. So if you want to just review and land this patch as-is, we can do that, and then I'll go in afterwards and fix it up. But if you'd like your patch to be the final version that lives in Gecko, then you can make the modifications. For what it's worth, it'll make the tests a lot simpler, because you don't need to listen for the console (you can just use a try{} catch{}).

Thanks for the great work, and let me know what you decide, and if you have any questions! :-)

Comment 17

[Peter Xiao]

No worries, the deadline is past already, and it's all good.

I still want to get this patch through, so I will take a look at this and make the modifications / ask questions.

Thanks for all the help!

Comment 18

[Peter Xiao]

It looks like there is already a check if the COW is an array here: http://dxr.mozilla.org/mozilla-central/js/xpconnect/wrappers/AccessCheck.cpp.html#l378. It then short circuits the check, returning true.

So, when we're at the deny here: http://dxr.mozilla.org/mozilla-central/js/xpconnect/wrappers/AccessCheck.cpp.html#l411, we should call AccessCheck::deny, and return false instead of returning Deny(). We know at this point that the COW is not an array, and there is no __exposedProps__ so this object was probably injected by accident, so we throw. Sounds correct?

As a side note: should I just edit the source, then update my patch with hg qref, and reupload the patch? Sorry, I'm new to mercurial and I read around on MDN, but I want to confirm this here.

Comment 19

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #18)
> It looks like there is already a check if the COW is an array here:
> http://dxr.mozilla.org/mozilla-central/js/xpconnect/wrappers/AccessCheck.cpp.
> html#l378. It then short circuits the check, returning true.

Yes, but that's only for indexed properties and .length. If somebody reads someArray.foopy on a COW without __exposedProps__, we want to fail silently.

> So, when we're at the deny here:
> http://dxr.mozilla.org/mozilla-central/js/xpconnect/wrappers/AccessCheck.cpp.
> html#l411, we should call AccessCheck::deny, and return false instead of
> returning Deny().

Right.

> We know at this point that the COW is not an array

Not quite - see above. 

> there is no __exposedProps__ so this object was probably injected by
> accident, so we throw. Sounds correct?

Modulo the above issue, yes.

> As a side note: should I just edit the source, then update my patch with hg
> qref, and reupload the patch? Sorry, I'm new to mercurial and I read around
> on MDN, but I want to confirm this here.

Yeah, that's probably best. Note that a lot of Mozilla developers (myself included) use Git for their personal workflow (though you still need hg for pushing). So if you prefer that, you can just clone https://github.com/mozilla/mozilla-central .

Comment 20

[Peter Xiao]

Ah I see, I missed the parentheses in that check. 

So, instead, I need to replace the above Deny() with if (!JS_IsArrayObject(cx, wrappedObject) AccessCheck::deny.

Comment 21

[Peter Xiao]

Oh, and if it's not a array, then just return true to fail silently.

Comment 22

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #21)
> Oh, and if it's not a array, then just return true to fail silently.

We need to do that if it _is_ an array.

Comment 23

[Peter Xiao]

Oh, sorry, that's what I meant. Essentially that's the else to the if statement above.

Comment 24

[Peter Xiao]

So I am starting to grasp a sense of this compartment business. Am I right to say that I need to be in wrappedObject's compartment if I want to examine wrappedObject's properties? But then we need to deny in wrapper's compartment because that is where the property access is made?

We need to call JS_IsArrayObject(cx, wrappedObject) in the wrappee's compartment. Why do we not need to be in the wrappee's compartment for the JS_ObjectIsFunction(cx, wrappedObject) call? That is to say, why does JS_ObjectIsFunction not assert cx and obj have the same compartment? In fact, cx doesn't look to be used at all in JS_ObjectIsFunction!

Comment 25

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #24)
> So I am starting to grasp a sense of this compartment business. Am I right
> to say that I need to be in wrappedObject's compartment if I want to examine
> wrappedObject's properties?

Yep, that's the contract. If you want to touch an object, you have to be in its compartment.

> But then we need to deny in wrapper's
> compartment because that is where the property access is made?

Precisely. When we throw, we create an exception object. And we want that exception object to be in the caller's compartment so that the caller can see it, regardless of the security relationship between the caller and the callee.

> We need to call JS_IsArrayObject(cx, wrappedObject) in the wrappee's
> compartment. Why do we not need to be in the wrappee's compartment for the
> JS_ObjectIsFunction(cx, wrappedObject) call? That is to say, why does
> JS_ObjectIsFunction not assert cx and obj have the same compartment? In
> fact, cx doesn't look to be used at all in JS_ObjectIsFunction!

Yeah, it's a bit arbitrary. We introduced these assertions back when we introduced compartments so that we could catch all the places where we mess this up. But there are some API functions where it doesn't really matter (it only matters if you create objects or manipulate references in a way that changes the object graph as seen by the GC). Whether those do or don't call assertSameCompartment is isn't really well-defined.

But yeah, you already seem to have a better grasp of compartments that most full-time Mozilla platform engineers. Any chance I can continue to keep working on XPConnect? ;-)

Comment 26

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #25)
> But yeah, you already seem to have a better grasp of compartments that most
> full-time Mozilla platform engineers. Any chance I can continue to keep
> working on XPConnect? ;-)

Haha, I think it just seems like I have a better grasp than I actually have. I do plan to keep contributing. :)

So I wrote up a fix and it turns out it breaks test_cows.xul, which makes sense I suppose, since we are now throwing for property accesses without __exposedProps__. I'm nervous about straight up "correcting" test_cows.xul, so I'm going to run these test case changes with you.

http://dxr.mozilla.org/mozilla-central/js/xpconnect/tests/chrome/test_cows.xul.html#l97 
Here, we "shouldn't throw when accessing exposed properties that doesn't exist".
Now, we throw because empty does not have __exposedProps__, regardless of the fact that the property foo doesn't exist. I will catch this throw and pass the test.

http://dxr.mozilla.org/mozilla-central/js/xpconnect/tests/chrome/test_cows.xul.html#l103
func does not have __exposedProps__ so we throw for property accesses on foo, bar, and prototype. I will also catch this throw and pass the test.

http://dxr.mozilla.org/mozilla-central/js/xpconnect/tests/chrome/test_cows.xul.html#l110
I'm fairly new to Javascript and I'm not familiar with this syntax. Google has failed me too, but it looks like it's constructing an array of all the property names. I think it fails because it tries to access each property which will throw. 
Does it not add name to the array if the property is undefined? 
I'm thinking of just deleting this test case. Catching and passing seems redundant because then it's essentially the same test as above?


Additionally, do I want to create new test files like I did in the current patch, or should I just add tests to this test_cows.xul file?

Comment 27

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #26)

> Now, we throw because empty does not have __exposedProps__, regardless of
> the fact that the property foo doesn't exist. I will catch this throw and
> pass the test.

Sounds good.

> http://dxr.mozilla.org/mozilla-central/js/xpconnect/tests/chrome/test_cows.
> xul.html#l103
> func does not have __exposedProps__ so we throw for property accesses on
> foo, bar, and prototype. I will also catch this throw and pass the test.

Yep.

> http://dxr.mozilla.org/mozilla-central/js/xpconnect/tests/chrome/test_cows.
> xul.html#l110
> I'm fairly new to Javascript and I'm not familiar with this syntax. Google
> has failed me too, but it looks like it's constructing an array of all the
> property names.

http://mzl.la/QNoSrg

I think it fails because it tries to access each property
> which will throw. 
> Does it not add name to the array if the property is undefined?
> I'm thinking of just deleting this test case. Catching and passing seems
> redundant because then it's essentially the same test as above?

Well, it's effectively testing the enumerate trap, which we want to do the right thing here. See the enumerate implementation (and Filter()) in FilteringWrapper.cpp.

Come to think of it, I think that throwing here might actually break Filter(), because Filter() is written in a crappy way. Thankfully though, I just landed some code to mozilla-inbound which should fix this for you. See this patch:

http://hg.mozilla.org/integration/mozilla-inbound/rev/23c9f61a243b

> Additionally, do I want to create new test files like I did in the current
> patch, or should I just add tests to this test_cows.xul file?

Just include checks for the new behavior (that we throw) in this patch. Do something like this:

try {
  operationThatShouldThrow();
  ok(false, "Didn't throw!");
} catch (e) { ok(/denied/.exec(e), "Threw correctly"); }

Comment 28

[Peter Xiao]

In your recent patch, ::check() now merely returns if the access is allowed or not, and does not throw.

So now, I do not want to throw in ::check() anymore right? Where do I throw now?

Comment 29

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #28)
> In your recent patch, ::check() now merely returns if the access is allowed
> or not, and does not throw.

Mostly yes, but check() is still allowed to throw along the way. Callers of check() examine JS_IsExceptionPending() before calling deny() (note the other remaining calls to JS_ReportError in ExposedPropertiesOnly::check).

Comment 30

[Bobby Holley (:bholley)]

Hmm, now that I think about it, it might make more sense actually to put this code in ExposedPropertiesOnly::deny(). It would involve a duplicate call to JS_HasPropertyById, but only in the failure case, and it would also consolidate our logic to decide whether we should throw or not, making our implementation there much easier to grok. That is to say, someone could examine ExposedPropertiesOnly::deny and see the exact heuristic that we've worked out in this bug.

It's ultimately your call though. What do you think?

Comment 31

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #30)
> Hmm, now that I think about it, it might make more sense actually to put
> this code in ExposedPropertiesOnly::deny(). It would involve a duplicate
> call to JS_HasPropertyById, but only in the failure case, and it would also
> consolidate our logic to decide whether we should throw or not, making our
> implementation there much easier to grok. That is to say, someone could
> examine ExposedPropertiesOnly::deny and see the exact heuristic that we've
> worked out in this bug.
> 
> It's ultimately your call though. What do you think?

Based on how I understand it right now, I feel that the failed access throw should belong in ::check. As you said, there are other throws in ::check. Moving just one of them to ::deny would not be intuitive. It may just generate confusion on why this throw is in deny, but the others are not.

So I think that if we move this throw to ::deny, we should move -all- throws to ::deny. However, I feel that this would generate performance overhead as well as duplicated code (this can probably be fixed, but it might be messy). We already figured out why we should threw, but we didn't do it, and now we need to go back and do the same logic to actually do it. 


Am I correct to say that ::check is called, then JS_IsExceptionPending() is checked. If there is an exception, propagate it up? Otherwise, the caller may decide to deny or not?

Also what sort of heuristic are we using to determine what throws in ::check, and what throws in ::deny?

Comment 32

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #31)
> Based on how I understand it right now, I feel that the failed access throw
> should belong in ::check. As you said, there are other throws in ::check.
> Moving just one of them to ::deny would not be intuitive. It may just
> generate confusion on why this throw is in deny, but the others are not.

The difference, in my mind, is that the currently errors thrown from check() all involve a malformed __exposedProps__, rather than a policy-level denial of permissions. That is to say, they all involve a failure to parse the security characteristics of the object rather than a denial based on those characteristics.

Whether a missing __exposedProps__ falls in the first or second category is kind of a judgement call which I'll leave up to you.

> So I think that if we move this throw to ::deny, we should move -all- throws
> to ::deny. However, I feel that this would generate performance overhead as
> well as duplicated code (this can probably be fixed, but it might be messy).
> We already figured out why we should threw, but we didn't do it, and now we
> need to go back and do the same logic to actually do it.

Yeah, totally.


> Am I correct to say that ::check is called, then JS_IsExceptionPending() is
> checked. If there is an exception, propagate it up? Otherwise, the caller
> may decide to deny or not?

Right. And this decision depends on the Policy's implementation of deny().

> Also what sort of heuristic are we using to determine what throws in
> ::check, and what throws in ::deny?

See above.

Comment 33

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #32)
> The difference, in my mind, is that the currently errors thrown from check()
> all involve a malformed __exposedProps__, rather than a policy-level denial
> of permissions. That is to say, they all involve a failure to parse the
> security characteristics of the object rather than a denial based on those
> characteristics.
> 
> Whether a missing __exposedProps__ falls in the first or second category is
> kind of a judgement call which I'll leave up to you.

I wanted to say that a missing __exposedProps__ is a malformed __exposedProps__. However, throwing via JS_ReportError in ::check will break Filter(). Filter()'s JS_IsExceptionPending will return true, and then Filter() will return false.

(In reply to Bobby Holley (:bholley) from comment #27)
> Well, it's effectively testing the enumerate trap, which we want to do the
> right thing here. See the enumerate implementation (and Filter()) in
> FilteringWrapper.cpp.

Based on this, we want Filter() to not throw merely on ::check, correct? Then maybe we should actually put the throw in ::deny after all.

Comment 34

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #33)
> (In reply to Bobby Holley (:bholley) from comment #32)
> > The difference, in my mind, is that the currently errors thrown from check()
> > all involve a malformed __exposedProps__, rather than a policy-level denial
> > of permissions. That is to say, they all involve a failure to parse the
> > security characteristics of the object rather than a denial based on those
> > characteristics.
> > 
> > Whether a missing __exposedProps__ falls in the first or second category is
> > kind of a judgement call which I'll leave up to you.
> 
> I wanted to say that a missing __exposedProps__ is a malformed
> __exposedProps__. However, throwing via JS_ReportError in ::check will break
> Filter(). Filter()'s JS_IsExceptionPending will return true, and then
> Filter() will return false.
> 
> (In reply to Bobby Holley (:bholley) from comment #27)
> > Well, it's effectively testing the enumerate trap, which we want to do the
> > right thing here. See the enumerate implementation (and Filter()) in
> > FilteringWrapper.cpp.
> 
> Based on this, we want Filter() to not throw merely on ::check, correct?
> Then maybe we should actually put the throw in ::deny after all.

Well, we'd already not be throwing for arrays, right? So effectively, Filter() would only throw for objects on which no properties would be accessible anyway, and we wouldn't artificially hamper the ability to enumerate privileged arrays without __exposedProps__. Or am I missing something?

Comment 35

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #34)
> Well, we'd already not be throwing for arrays, right? So effectively,
> Filter() would only throw for objects on which no properties would be
> accessible anyway, and we wouldn't artificially hamper the ability to
> enumerate privileged arrays without __exposedProps__. Or am I missing
> something?

What about enumerating the properties of a non-array object without __exposedProps__? Do we want to say there are no such properties, or throw?

Comment 36

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #35)
> What about enumerating the properties of a non-array object without
> __exposedProps__? Do we want to say there are no such properties, or throw?

Hm, good question. My initial reaction was that we should throw. But that's sort of inconsistent with how Filter() behaves for other objects (like cross-origin-accessible objects), where it only returns properties that are safe to access without throwing.

It's a judgement call, but not throwing is safe and less likely to cause regressions.

Comment 37

[Peter Xiao]

I will throw, mostly to avoid having to duplicate code in ::check to ::deny.

By the way, I'm only really testing with mochitest-chrome. Should I be running the other test suites as well?

If everything goes alright, I'll have my patch in tonight.

Comment 38

[Peter Xiao]

Attachment: Bug 789298 Fix V2

Comment 39

[Peter Xiao]

Above is try #2! Looking forward to feedback!

Comment 40

[Bobby Holley (:bholley)]

Comment on attachment 679053 [details] [diff] [review]
Bug 789298 Fix V2

This patch looks great! r=bholley with comments addressed. Let me know if you need help pushing the patch. Also, I'm quite impressed with your work on this stuff. Any interest in an internship? :-)

---

I'm not convinced the mochitest-chrome test is really necessary. Is it testing anything that isn't also tested in test_cows? In general, mochitests are kind of heavyweight (the time it takes to run them adds up, and we're stuck with that overhead forever), so there's no point in adding them willy-nilly. Mochitests are necessary for testing DOM objects, but with COWs we're only dealing with pure JS objects (the original test was a mochitest because it needed to test the console warning mechanism, but we're not doing that anymore). So an xpcshell-test with a sandbox would also suffice, and be much faster and smaller. See test_bug780370.js for an example.

But anyway, here are comments regardless:


>+  /** Test for warn when accessing COW without __exposedProps__ **/

This comment is out of date.


>+    // Warnings are dispatched async, so stick ourselves at the end of the event
>+    // queue.
>+    setTimeout(done, 0);

This is no longer necessary, because we're not listening for warnings anymore.

>+  </script>
>+  <iframe id="frame1" onload="frameLoaded();" type="content" src="http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/file_bug789298.html" />
>+  <iframe id="frame2" onload="frameLoaded();" type="content" src="http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/file_bug789298.html" />

There's not really a reason to have two iframes here. The origin test did that because it was testing console warnings, which are supposed to fire exactly once per document. But if we're throwing, it's irrelevant.


>diff --git a/js/xpconnect/tests/chrome/test_cows.xul b/js/xpconnect/tests/chrome/test_cows.xul
>index fe6dc69..dd1849a 100644
>     //var cow = getCOW({ foo: "fooval", __exposedProps__: {}});
>     //Math.sin(1);

Looks like Blake left this in back in the day (when hacking C++, it's a nice trick to add Math.sin at the script point you're interested in debugging and then gdb break in js::math_sin). Anyway, bonus points for removing it.


>diff --git a/js/xpconnect/wrappers/AccessCheck.cpp b/js/xpconnect/wrappers/AccessCheck.cpp
>index 5082046..f748ec5 100644
>+    // Save this, need it later

Nit - complete sentences.

Comment 41

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #40)
> Comment on attachment 679053 [details] [diff] [review]
> Bug 789298 Fix V2
> 
> This patch looks great! r=bholley with comments addressed. Let me know if
> you need help pushing the patch. 

Not completely sure how this part works. Do I need to get you to commit for me? Also maybe I should apply to get access to the try server.

>Also, I'm quite impressed with your work on
> this stuff. Any interest in an internship? :-)

Thanks! Unfortunately, some of my offer deadlines are coming up soon, so I will probably take one of those. Maybe a fulltime position next year? :)

In the mean time, I'd like to keep contributing. Any good suggestions for a second bug to take on?

> I'm not convinced the mochitest-chrome test is really necessary. Is it
> testing anything that isn't also tested in test_cows? In general, mochitests
> are kind of heavyweight (the time it takes to run them adds up, and we're
> stuck with that overhead forever), so there's no point in adding them
> willy-nilly. Mochitests are necessary for testing DOM objects, but with COWs
> we're only dealing with pure JS objects (the original test was a mochitest
> because it needed to test the console warning mechanism, but we're not doing
> that anymore). So an xpcshell-test with a sandbox would also suffice, and be
> much faster and smaller. See test_bug780370.js for an example.

I put in that mochitest mostly because my test_cows wasn't working. Once I got test_cows to work, I wasn't sure if I should just get rid of it. It's a redundant test and will just add to the mochitest overhead, so I will get rid of it.

> >diff --git a/js/xpconnect/tests/chrome/test_cows.xul b/js/xpconnect/tests/chrome/test_cows.xul
> >index fe6dc69..dd1849a 100644
> >     //var cow = getCOW({ foo: "fooval", __exposedProps__: {}});
> >     //Math.sin(1);
> 
> Looks like Blake left this in back in the day (when hacking C++, it's a nice
> trick to add Math.sin at the script point you're interested in debugging and
> then gdb break in js::math_sin). Anyway, bonus points for removing it.

I can always use more bonus points.

> >diff --git a/js/xpconnect/wrappers/AccessCheck.cpp b/js/xpconnect/wrappers/AccessCheck.cpp
> >index 5082046..f748ec5 100644
> >+    // Save this, need it later
> 
> Nit - complete sentences.

Oops, I'll fix this.

Comment 42

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #41)
> (In reply to Bobby Holley (:bholley) from comment #40)
> > Comment on attachment 679053 [details] [diff] [review]
> > Bug 789298 Fix V2
> > 
> > This patch looks great! r=bholley with comments addressed. Let me know if
> > you need help pushing the patch. 
> 
> Not completely sure how this part works. Do I need to get you to commit for
> me?

Me or someone else. Once you have the patch and it passes try, you can set the checkin-needed keyword on the bug and someone will do it for you, or just ping me and I'll do it.

> Also maybe I should apply to get access to the try server.

Good idea. File a bug (bug 653773 is an example) and I'll vouch for you.

> Thanks! Unfortunately, some of my offer deadlines are coming up soon, so I
> will probably take one of those.

Ok. Depending on what "soon" is it might be possible to expedite things, but it's up to you.

> Maybe a fulltime position next year? :)

Hiring some for core platform work is always easier if they're already involved in Mozilla stuff. Internships are an easy way to do this, but if you keep contributing, it's definitely a possibility. :-)

> 
> In the mean time, I'd like to keep contributing. Any good suggestions for a
> second bug to take on?

bug 794923 is another bug very similar to this one, but probably not challenging enough. bug 687612 and bug 743121 are self-contained but not very important to fix. I'll also CC you on a security bug that you might be interested in. In general though, XPConnect bugs are pretty hard for new contributors to fix. Ping jdm or Ms2ger for other ideas. 

> I put in that mochitest mostly because my test_cows wasn't working. Once I
> got test_cows to work, I wasn't sure if I should just get rid of it. It's a
> redundant test and will just add to the mochitest overhead, so I will get
> rid of it.

Sounds good.

Comment 43

[Bobby Holley (:bholley)]

bug 686989 and bug 710806 might also be interesting, but those are pretty big tasks.

Comment 44

[Bobby Holley (:bholley)]

Actually, an _awesome_ bug to fix would be bug 791605. It's a regression from one of my patches somewhere, but I've never _quite_ had the time to look at it. Interested in taking it on? Should be pretty straightforward.

Comment 45

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #44)
> Actually, an _awesome_ bug to fix would be bug 791605. It's a regression
> from one of my patches somewhere, but I've never _quite_ had the time to
> look at it. Interested in taking it on? Should be pretty straightforward.

I'll take a look at this one! It sounds like you're endorsing it more.

My request for commit access is here: bug 809738.

Comment 46

[Peter Xiao]

Attachment: Bug 789298 Fix V3

Updated the patch. Will commit it to try server when I get access, and let you know how it goes.

Comment 47

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=24eed21a2026

Comment 48

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #47)
> https://tbpl.mozilla.org/?tree=Try&rev=24eed21a2026

My patch failed mochitest-2 and reftest. I'll look into mochitest-2 first. I suspect we are just catching injections that lack __exposedProps__, exactly what this patch was for! (I hope.) 

First up: http://dxr.mozilla.org/mozilla-central/dom/browser-element/mochitest/browserElement_TopBarrier.js.html

It looks like this one is the culprit of 90% of the failures.

At line 55, we get a "missing __exposedProps_" exception. I'm a little fuzzy on what is chrome, and what is content. Here is my attempt to explain it: iframe is chrome. We get the BrowserFrameMessageManager of iframe, also chrome. We add a message listener, onRecvTestPass(msg), but onRecvTestPass is content, and msg is chrome. Therefore, we're injecting msg into content.

To confirm this, I commented out my throw in ::check and saw this msg printed out was undefined.

Am I missing any nuances? I feel that browserElementTestHelpers.addPermission() is important somehow. Does this just give us the proper permission to create chrome objects inside of content?

Also, do I fix this in this patch? That is to say, this is a bug in another component that uses ours, correct? In general, what kind of heuristics determines if we should split this up into a new patch/bug report?

Next: http://dxr.mozilla.org/mozilla-central/dom/browser-element/mochitest/browserElement_BadScreenshot.js.html

Line 19 also throws "missing __exposedProps__". iframe is chrome, and we're accessing the getScreenshot property without __exposedProps__? I'm less sure about this one.

I'll look in to the reftest later.

Comment 49

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #48)
> (In reply to Bobby Holley (:bholley) from comment #47)
> > https://tbpl.mozilla.org/?tree=Try&rev=24eed21a2026
> 
> My patch failed mochitest-2 and reftest. I'll look into mochitest-2 first. I
> suspect we are just catching injections that lack __exposedProps__, exactly
> what this patch was for! (I hope.) 

That's what it seems like to me!

> First up:
> http://dxr.mozilla.org/mozilla-central/dom/browser-element/mochitest/
> browserElement_TopBarrier.js.html
> 
> It looks like this one is the culprit of 90% of the failures.

CCing jlebar. Justin, can you help out Peter on this one? We're making GETs throw when there's no __exposedProps__ rather than failing silently. See his questions in comment 48.

> Also, do I fix this in this patch? That is to say, this is a bug in another
> component that uses ours, correct? In general, what kind of heuristics
> determines if we should split this up into a new patch/bug report?

In this case, I'd fix the test in a second patch (same bug number as this bug). We'll land the two patches together, with the test fix as the first patch and the behavior change as the second patch. That way, each revision passes tests, which is important for bisecting.

> I'll look in to the reftest later.

I'm pretty sure that one is just intermittent orange that's unrelated to your patch. I retriggered it to make sure. If the second (R) comes up green, you're good.

Comment 50

[Justin Lebar (not reading bugmail)]

Oh, these tests are so much fun.  This is a really old test which I think we can rewrite in a much simpler way now.  Rewriting it might be simpler than trying to fix it.

> At line 55, we get a "missing __exposedProps_" exception. I'm a little fuzzy
> on what is chrome, and what is content.

injectedScript runs with chrome privileges inside a content iframe.  Everything
else is content, although SpecialPowers.wrap() gives content chrome privileges.

> Here is my attempt to explain it: iframe is chrome. We get the
> BrowserFrameMessageManager of iframe, also chrome. We add a message listener,
> onRecvTestPass(msg), but onRecvTestPass is content, and msg is chrome.
> Therefore, we're injecting msg into content.

msg is chrome and onRecvTestPass is content, correct.  But both iframes are
also content.

> Am I missing any nuances? I feel that
> browserElementTestHelpers.addPermission() is important somehow. Does this
> just give us the proper permission to create chrome objects inside of
> content?

This gives content permission to create <iframe mozbrowser> (which is still content).

It would be pretty simple to rewrite this, if you wanted to.  What you'd do is remove all this message manager and loadFrameScript business.  Instead, the test file would load file_browserElement_TopBarrier.html into an iframe and listen for mozbrowsershowmodalprompt events from the file indicating test success or failure.

See browserElement_OpenWindow.js for an example of this idiom.

You definitely don't need to rewrite this; you should do whatever you're more comfortable with.

Comment 51

[Peter Xiao]

Attachment: TopBarrier Test Rewrite

I put injectedScript into a file named file_browserElement_TopBarrier.html. This way we can run injectedScript in an iframe with content privileges, avoiding any cross membrane security issues. Thanks Justin!

Comment 52

[Peter Xiao]

I found another broken test that I missed in dom/contacts/tests/test_contacts_basics.html. This one took a bit more digging, but I traced it to line 802. Accessing createResult2.bday.valueOf() will throw "missing __exposedProps__", but createResult2.bday will not (does the fact that I had to test this to find out means the error message isn't helpful enough?). 

The real issue is when a contact is created. mozContacts is chrome, as are all contacts and their properties. mozContact correctly adds __exposedProps__ into a contact, which is why createResult2.bday does not throw. However, mozContacts does not add __exposedProps__ to bday itself, which is still chrome and injected.

I assume we want readwrite access for all properties in bday, as well as other objects that lack __exposedProps__ in mozContacts. I'll add another patch for this soon.

Comment 53

[Peter Xiao]

Attachment: mozContacts fix

Contact properties bday and anniversary now have __exposedProps__, making it safe to inject. Was previously throwing "missing __exposedProps__".

Comment 54

[Peter Xiao]

In browserElement_BadScreenshot.js, Is there a reason why getScreenshot.apply is used rather than a straight up call to getScreenshot? I'm not very familiar with Javascript, but apply is used to give functions a specified 'this'. In this case, iframe is passed in, but I believe 'this' is iframe in the normal getScreenshot function call.

Perhaps the reason why apply is used is because getScreenshot is implemented using a generalized defineMethod(name, fn) that uses fn.apply? Maybe the reasoning is because it is implemented this way, we should test it this way? I feel that we should test based on the exposed method parameters and not the underlying implementation. 

Once again, I think I need some clarification. iframe is content, getScreenshot is chrome? So it was throwing because we were accessing apply of getScreenshot. Is there a good way to tell if an object is chrome or content?

Comment 55

[Peter Xiao]

Attachment: Bad Screenshot Test Patch

Uses getScreenshot() instead of getScreenshot.apply() to avoid security exception from chrome to content.

All tests in mochitest-2 should pass now.

(The battery test fails for me because it's reading my actual battery stats rather than the default stats. Does this happen to others?)

Comment 56

[Justin Lebar (not reading bugmail)]

Comment on attachment 680034 [details] [diff] [review]
Bad Screenshot Test Patch

> -    req = iframe.getScreenshot.apply(iframe, args);

getScreenshot.apply was used because we wanted to be able to pass three arguments to the getScreenshot function.

I'm not wild about breaking this; I can't vouch that nobody in Gaia uses fn.apply (or fn.bind, etc.) on a function provided by the browser API.  How hard would it be to fix this?

Note that the browser API is not the only chrome JS thing which returns functions to content.  We have lots of APIs implemented in JS -- e.g. the FM radio API, the DOM Applications API -- and it seems like you're preventing us from calling fn.apply/bind/etc. on any function any of these APIs return.

Comment 57

[Bobby Holley (:bholley)]

Aww crap. I'd hoped that this wouldn't be an issue, but it looks like it is.

So, we do this funny thing for COWs where we remap standard prototypes into the content compartment, and then (in ChromeObjectWrapper.cpp), override various traps so that, if the lookup fails, we try the lookup on the content prototype chain. This means that authors don't have to explicitly expose things like Array.prototype.forEach in order for them to behave correctly - they just act like real arrays.

So the problem here is that people are exposing functions without __exposedProps__ (since generally functions don't need __exposedProps__), and expecting things like fun.apply to work. The stuff in ChromeObjectWrapper propagates any exceptions thrown by the base class, so we see an exception here rather than having fun.apply just come off the content prototype chain. Yuck.

One solution would be to just add another exception for Functions (like we do with arrays). This is kind of lame, and starts to suggest that we're doing something wrong.

Another solution would be to rearchitect how we do this stuff. Perhaps instead of making ChromeObjectWrapper a subclass like we do now, we could make it an explicit template specialization of FilteringWrapper, meaning we'd have more control over the ordering of lookup and throwing. Thoughts?

Are you on IRC? We should talk about this some more.

(Also, not that flagging review? without an explicit reviewer generally causes your patch to get lost unless an angel like jdm is watching over you. I'd generally suggest figuring out who owns the code and flagging them for review. Looking at the blame log and determining who wrote the file is a good starting point.)

Comment 58

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #57)
> Another solution would be to rearchitect how we do this stuff. Perhaps
> instead of making ChromeObjectWrapper a subclass like we do now, we could
> make it an explicit template specialization of FilteringWrapper, meaning
> we'd have more control over the ordering of lookup and throwing. Thoughts?

Actually, maybe we don't need to mess with the hierarchy. We can probably get away with just overriding the traps like we do now.

Comment 59

[Bobby Holley (:bholley)]

Ok, so this is just getting unfortunately complicated - I'm really sorry it turned out that way. If it were me, I'd probably just give up and work on something else at this point (might be better bang for your buck). But if you want to press ahead, I think the following is probably the path of least resistance:

1 - Modify the signature of Policy::deny to accept an |obj| parameter.

2 - Add a utility function AccessCheck::hasExposedProps(cx, obj) that tells us whether an object has an __exposedProps__ property or not.

3 - Move your throwing logic for missing __exposedProps__ from ExposedPropertiesOnly::check to ExposedPropertiesOnly::deny, and have it query the utility function (I think we've proven this to be a nuanced enough issue that we can't categorically say "missing __exposedProps__ is malformed __exposedProps__").

4 - Create two variants of ExposedPropertiesOnly. StrictExposedPropertiesOnly and SilentExposedPropertiesOnly. One of them always throws in ::deny, the other one never does.

5 - Make ChromeObjectWrapperBase use StrictExposedPropertiesOnly.

6 - In ChromeObjectWrapper.cpp, have the base wrapper lookups use SilentExposedPropertiesOnly.

Thoughts? Criticisms? Other ideas?

Comment 60

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #59)
> Ok, so this is just getting unfortunately complicated - I'm really sorry it
> turned out that way. If it were me, I'd probably just give up and work on
> something else at this point (might be better bang for your buck). But if
> you want to press ahead, I think the following is probably the path of least
> resistance:
> 
> 1 - Modify the signature of Policy::deny to accept an |obj| parameter.
> 
> 2 - Add a utility function AccessCheck::hasExposedProps(cx, obj) that tells
> us whether an object has an __exposedProps__ property or not.
> 
> 3 - Move your throwing logic for missing __exposedProps__ from
> ExposedPropertiesOnly::check to ExposedPropertiesOnly::deny, and have it
> query the utility function (I think we've proven this to be a nuanced enough
> issue that we can't categorically say "missing __exposedProps__ is malformed
> __exposedProps__").
> 
> 4 - Create two variants of ExposedPropertiesOnly.
> StrictExposedPropertiesOnly and SilentExposedPropertiesOnly. One of them
> always throws in ::deny, the other one never does.
> 
> 5 - Make ChromeObjectWrapperBase use StrictExposedPropertiesOnly.
> 
> 6 - In ChromeObjectWrapper.cpp, have the base wrapper lookups use
> SilentExposedPropertiesOnly.
> 
> Thoughts? Criticisms? Other ideas?

I think I want to take a stab at this. I have a good idea of what I'm doing in 1-4 but I need to take a closer look at ChromeObjectWrapperBase and ChromeObjectWrapper. Once I get a better understanding, I'll give my thoughts.

I'll break this down into three patches.

Patch 1: Step 2. Refactor out ::hasExposedProps for future reuse.
Patch 2: Steps 1 and 3. Add throw for missing __exposedProps__ to deny().
Patch 3: Steps 4-6. Separate ExposedProperties into different levels. I'll come up with a good commit message for this later.

Unfortunately, I'm fairly busy this week so I won't be able to put too much time until maybe the week of Thanksgiving.

Comment 61

[Bobby Holley (:bholley)]

Sounds good! Feel free to ping me on irc (bholley on #content) if you ever want to talk synchronously.

Comment 62

[Peter Xiao]

I had some time to do some digging around in the code.

Wouldn't an easy fix just be to put the throw for missing __exposedProps__ in the ::deny? 

In ChromeObjectWrapper::get, the initial base wrapper look up would not throw.

If it's not in the standard prototype and we find something, we call the get trap which flows to the ::enter, which calls ::deny.

The catch is, I don't think we throw for a call to ChromeObjectWrapper::getPropertyDescriptor. Is this okay?

Comment 63

[Peter Xiao]

Attachment: Refactor out ::hasExposedProps

Step 2 patch.

Comment 64

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #62)
> Wouldn't an easy fix just be to put the throw for missing __exposedProps__
> in the ::deny? 
> 
> In ChromeObjectWrapper::get, the initial base wrapper look up would not
> throw.

But if we throw for missing __exposedProps__ in ::deny, then it _would_ throw, right? The call to ::getPropertyDescriptor on the base wrapper would invoke FilteringWrapper::getPropertyDescriptor, which in turn invokes CrossCompartmentWrapper::getPropertyDescriptor, which in turn invokes DirectWrapper::getPropertyDescriptor, which calls ::enter(), landing us back in ::deny(). Or am I confused?

> The catch is, I don't think we throw for a call to
> ChromeObjectWrapper::getPropertyDescriptor. Is this okay?

Can you clarify what you mean here? Not sure I understand.

Comment 65

[Bobby Holley (:bholley)]

Comment on attachment 682793 [details] [diff] [review]
Refactor out ::hasExposedProps

Review of attachment 682793 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=bholley with comments addressed. You can upload a second patch with 'r=bholley' in the title and set the bugzilla flag to '+' to indicate that it doesn't need further review.

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +310,5 @@
> +    jsid exposedPropsId = GetRTIdByIndex(cx, XPCJSRuntime::IDX_EXPOSEDPROPS);
> +
> +    JSBool found = false;
> +    if (!JS_HasPropertyById(cx, obj, exposedPropsId, &found))
> +        JS_ReportError(cx, "__exposedProps__ look up failed");

If JS_HasPropertyById returns false, then it will have already set an exception (unless the problem is OOM, which is signaled by returning false without an exception). So no need to set an exception here (though you could NS_WARNING() if you want). I'd write this as:

return JS_HasPropertyById(...) && found;

Comment 66

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #64)
> But if we throw for missing __exposedProps__ in ::deny, then it _would_
> throw, right? The call to ::getPropertyDescriptor on the base wrapper would
> invoke FilteringWrapper::getPropertyDescriptor, which in turn invokes
> CrossCompartmentWrapper::getPropertyDescriptor, which in turn invokes
> DirectWrapper::getPropertyDescriptor, which calls ::enter(), landing us back
> in ::deny(). Or am I confused?
I believe it goes from CrossCompartmentSecurityWrapper::getPropertyDescriptor to Wrapper::getPropertyDescriptor which uses the CHECKED macro, which calls enter(). So you are right, it would still throw there. 

> Can you clarify what you mean here? Not sure I understand.
Never mind! 

So we still need to do the StrictExposedPropertiesOnly and SilentExposedPropertiesOnly.

I'm a little confused. Are we supposed to use FilteringWrapper with SilentExposedPropertiesOnly? But if FilteringWrapper will call deny for us, this will throw anyways. Do we need a SilentFilteringWrapper too?

Comment 67

[Peter Xiao]

Attachment: Refactor out ::hasExposedProps r=bholley

Comments addressed.

Comment 68

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #66)
> I'm a little confused. Are we supposed to use FilteringWrapper with
> SilentExposedPropertiesOnly? But if FilteringWrapper will call deny for us,
> this will throw anyways. Do we need a SilentFilteringWrapper too?

The idea here is that we basically want two different versions of ChromeObjectWrapperBase, depending on what we're doing. We want to inherit from the one with the Strict behavior, but when we're doing our crazy munging in the overridden traps, we want to bounce to something that's identical _except_ that it doesn't throw for failed GETs.

So indeed, we want them both to be filtering wrappers, and in fact the only difference between them is between their filtering policies. One of them throws in ::deny, the other one doesn't. FilteringWrapper depends on the implementation of ::deny of the associated Policy, so by changing it there we can avoid making it throw.

Comment 69

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #68)
> (In reply to Peter Xiao from comment #66)
> > I'm a little confused. Are we supposed to use FilteringWrapper with
> > SilentExposedPropertiesOnly? But if FilteringWrapper will call deny for us,
> > this will throw anyways. Do we need a SilentFilteringWrapper too?
> 
> The idea here is that we basically want two different versions of
> ChromeObjectWrapperBase, depending on what we're doing. We want to inherit
> from the one with the Strict behavior, but when we're doing our crazy
> munging in the overridden traps, we want to bounce to something that's
> identical _except_ that it doesn't throw for failed GETs.
> 
> So indeed, we want them both to be filtering wrappers, and in fact the only
> difference between them is between their filtering policies. One of them
> throws in ::deny, the other one doesn't. FilteringWrapper depends on the
> implementation of ::deny of the associated Policy, so by changing it there
> we can avoid making it throw.

Oh, that's right we can just not throw in ::deny. Oops. Okay I think I get it. I'll try to get this out soon.

Comment 70

[Peter Xiao]

Attachment: Refactor out ::hasExposedProps r=bholley

Forgot to put in AccessCheck.

Comment 71

[Peter Xiao]

Attachment: Refactor out ::hasExposedProps r=bholley

Sorry, I goofed. This time I committed it right.

Comment 72

[Peter Xiao]

Attachment: Add throw for missing __exposedProps in ExposedPropertiesOnly::deny.

I don't exactly like the duplicated code in ::deny. I almost created a utility function to convert jsid to jschar in AccessCheck, but argued against it in my head. In my opinion, this would be a more general utility function that doesn't belong in the scope of AccessCheck because it's not exactly related.

There's a commented out test on line 133 in test_cows.xul. It throws when trying to access the property 'undefined'. Interestingly enough, when I run it during an instance of running all mochitest-chrome, I get a throw for accessing 'valueof'. 

It was also throwing for accessing the 'toString' property when I was doing /denied/.exec(e) on the caught exception e. I need to investigate this further.

I'm starting to think that maybe we can add in an exception in ::check for standard prototype calls. Any thoughts on this?

Comment 73

[Peter Xiao]

I wrote a patch on top of the old patches where I had the throw in ::check. It seems to work, but it requires calls to JS_GetPropertyDescriptorById, and JS_IdentifyClassPrototype. Like you said, it seems kinda lame to just add an exception, but isn't allowing prototypes without __exposedProps__ an exception? Is it bad to add an exception for an exception? Let me know if I need to clarify this point.

Comment 74

[Bobby Holley (:bholley)]

Comment on attachment 682848 [details] [diff] [review]
Add throw for missing __exposedProps in ExposedPropertiesOnly::deny.

Review of attachment 682848 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/wrappers/AccessCheck.h
@@ +119,5 @@
>          // For gets, silently fail.
> +        if (act == js::Wrapper::GET) {
> +            JSAutoCompartment ac(cx, obj);
> +            if (!AccessCheck::hasExposedProps(cx, obj) && 
> +                !(JS_IsArrayObject(cx, obj) || JS_IsTypedArrayObject(obj))) {

I guess we really should also make the distinction between indexed array accesses (and length) and other array accesses, just like ::check does. So maybe we should add AccessCheck::isArrayAccess(cx, obj, id) or something of the sort? Then we could use it in ::check as well.

@@ +128,5 @@
> +                if (!str)
> +                    return false;
> +                const jschar *chars = JS_GetStringCharsZ(cx, str);
> +                JS_ReportError(cx, "Permission denied to property '%hs', missing __exposedProps__",
> +                               chars);

I agree that the repeated code here seems lame. I think we should probably reuse AccessCheck::deny here. Maybe AccessCheck::deny could take an optional |const char *reason| that it would append to the end of the exception string? That's the only reason for not using AccessCheck::deny here, right?

Comment 75

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #73)
> I wrote a patch on top of the old patches where I had the throw in ::check.
> It seems to work, but it requires calls to JS_GetPropertyDescriptorById, and
> JS_IdentifyClassPrototype. 

Wait, why do we need to do that? The whole point of our Strict/Silent FilterWrapper setup is that we avoid that problem. ChromeObjectWrapper overrides various methods of ChromeObjectWrapperBase. So if we fix up the base wrapper lookups with |Silent|, this should Just Work, right?

Comment 76

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #75)
> Wait, why do we need to do that? The whole point of our Strict/Silent
> FilterWrapper setup is that we avoid that problem. ChromeObjectWrapper
> overrides various methods of ChromeObjectWrapperBase. So if we fix up the
> base wrapper lookups with |Silent|, this should Just Work, right?

Yup, this is not needed here, but the earlier set of patches. I was just exploring an alternate solution that would apply on top of my "mozContacts fix" patch, rather than going with the Strict/Silent setup.

What are the benefits of adding this exception in via Strict/Silent over directly adding in an exception to ::check?

Comment 77

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #76)
> What are the benefits of adding this exception in via Strict/Silent over
> directly adding in an exception to ::check?

We don't want ::check to unconditionally grant access to properties on standard prototypes, because we don't want to grant access to that stuff in the common case. Maybe the object being COWed redefined the property to something else, or even if it didn't, we still don't want properties from Chrome's Object.prototype to leak across. The _only_ time we want to not throw is when we're in a position to remap the lookup into the home compartment, which is the stuff in ChromeObjectWrapper.cpp.

I really see this bug as being more trouble than it's worth at this point. The problem we're trying to solve here isn't major enough to justify significantly cluttering up the code. And while I think the solution in comment 59 is probably elegant enough if done right, it probably won't survive any more unexpected complications. I can't promise to take the patch if it turns out to complicate this already-complicated situation much further. :\

Comment 78

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #74)
> I guess we really should also make the distinction between indexed array
> accesses (and length) and other array accesses, just like ::check does. So
> maybe we should add AccessCheck::isArrayAccess(cx, obj, id) or something of
> the sort? Then we could use it in ::check as well.

I don't believe we need to check that here because it returns true in check. ::deny is only called when ::check returns false, correct? So it should never be the case that we are calling ::deny on an array access. I guess I'm imagining some sort of contractual agreement on the context deny is called on.

I still feel that it would be good to extract this out into a function though. I feel that it would improve the readability (although the comment is already there).

> I agree that the repeated code here seems lame. I think we should probably
> reuse AccessCheck::deny here. Maybe AccessCheck::deny could take an optional
> |const char *reason| that it would append to the end of the exception
> string? That's the only reason for not using AccessCheck::deny here, right?

So the main reason I decided not to put this in AccessCheck::deny was that checking for __exposedProps__ is specific to ExposedPropertiesOnly rather than general to all policies like the rest of AccessCheck::deny. I felt that it was out of place to have these checks in every Policy::deny.

Comment 79

[Kris Maglione [:kmag]]

Argh! Un-CCing myself before I throw my phone through a window. Please email me if you need any further input. If only there were an "email me if this bug changes state" flag...

Comment 80

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #78)
> (In reply to Bobby Holley (:bholley) from comment #74)
> I don't believe we need to check that here because it returns true in check.
> ::deny is only called when ::check returns false, correct? So it should
> never be the case that we are calling ::deny on an array access. I guess I'm
> imagining some sort of contractual agreement on the context deny is called
> on.

Ah right! Clever point.

> So the main reason I decided not to put this in AccessCheck::deny was that
> checking for __exposedProps__ is specific to ExposedPropertiesOnly rather
> than general to all policies like the rest of AccessCheck::deny. I felt that
> it was out of place to have these checks in every Policy::deny.

Er, I'm not suggesting putting the checks in AccessCheck::deny. I'm just saying that we should be using AccessCheck::deny for the stringification and error reporting. Right now, the only difference between the stringification code here and the code in AccessCheck::deny appears to be that the error message is different. And I'm saying we could accomplish that with an optional argument.

Comment 81

[Peter Xiao]

(In reply to Bobby Holley (:bholley) from comment #80)
> Er, I'm not suggesting putting the checks in AccessCheck::deny. I'm just
> saying that we should be using AccessCheck::deny for the stringification and
> error reporting. Right now, the only difference between the stringification
> code here and the code in AccessCheck::deny appears to be that the error
> message is different. And I'm saying we could accomplish that with an
> optional argument.

Oh, I see! That would be a good way avoid of duplicating the code.

(In reply to Bobby Holley (:bholley) from comment #77)
> I really see this bug as being more trouble than it's worth at this point.
> The problem we're trying to solve here isn't major enough to justify
> significantly cluttering up the code. And while I think the solution in
> comment 59 is probably elegant enough if done right, it probably won't
> survive any more unexpected complications. I can't promise to take the patch
> if it turns out to complicate this already-complicated situation much
> further. :\

I'll think it over if I want to continue on this. I may just see this to completion if only to get some more experience with the process and to familiarize myself more with the codebase. 

Also I'm far away from my Linux box until Thanksgiving break is over, so I can only really prep for the other bug I took on.

Comment 82

[Bobby Holley (:bholley)]

(In reply to Peter Xiao from comment #81)
> Also I'm far away from my Linux box until Thanksgiving break is over, so I
> can only really prep for the other bug I took on.

Ok, what OS are you on? VirtualBox is a great VM, and I use it all the time whenever I have to do linux-specific stuff.

If you're looking for other bugs, bug 809652, bug 776497, and bug 812415 (time sensitive) are all on my list of bugs to actively work on. They're certainly not first-bugs, but you seem to be pretty phenomenal at digesting complicated stuff quickly. So if you're interested and willing to put some hours in over the break, let me know. :-)

Comment 83

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 84

[Bobby Holley (:bholley)]

I don't think he is. Also, given the deprecation of COWs in general, I don't think we want this anymore. :-(

Comment 85

[Peter Xiao]

I am not. Sorry for the disappearance, my classes are taking up all my time this semester.