If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Enumerating cross-origin objects shouldn't throw

RESOLVED FIXED in mozilla24

Status

()

Core
XPConnect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla24
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

By my reading of the spec, enumerating cross-origin objects shouldn't throw, and we should instead return the appropriate properties, with the non-cross-origin-accessible properties filtered out. We have the capability to do that in FilteringWrapper, but currently just throw in any situations where we try to invoke traps like |enumerate| or |keys|.

If this is the spec behavior, I'm happy to try to align with it. Hixie, can you confirm that this is the case?
For Window, Document, and Location (I think those are the only objects you can access cross-origin without having once been same-origin), that sounds right. I don't think the spec actually documents this right now, though. What do browsers do?
(In reply to Hixie (not reading bugmail) from comment #1)
> For Window, Document, and Location (I think those are the only objects you
> can access cross-origin without having once been same-origin), that sounds
> right. I don't think the spec actually documents this right now, though.
> What do browsers do?

It appears that Safari and Chrome don't throw, whereas IE9 and Gecko do. I haven't tested IE10.
Do they enumerate anything?

I really don't mind what we spec here, so long as we're consistent. Throwing seems unhelpful, but an argument could be made that enumerating frame names is a security risk, so we probably shouldn't do that even if we allow access to the names if you need them? I don't know.
(In reply to Hixie (not reading bugmail) from comment #3)
> Do they enumerate anything?

It looks like they don't, and Object.getOwnPropertyNames and such provide arrays of length zero.
> I really don't mind what we spec here, so long as we're consistent. Throwing
> seems unhelpful, but an argument could be made that enumerating frame names
> is a security risk, so we probably shouldn't do that even if we allow access
> to the names if you need them?

Yeah, I think it's safest not to enumerate them. I'm going to upload some patches to align with Chrome/Safari here, and file a spec bug.
Created attachment 751995 [details] [diff] [review]
Part 1 - Pass the entered id in addition to the wrapper action to Policy::deny. v1
Attachment #751995 - Flags: review?(mrbkap)
Created attachment 751996 [details] [diff] [review]
Part 2 - Be more explicit about rejecting JSID_VOID for XOWs. v1

We already do this, but it's helpful to be clear about it.
Attachment #751996 - Flags: review?(mrbkap)
Created attachment 751997 [details] [diff] [review]
Part 3 - Silently fail for enumerate-like operations on XOWs. v1
Attachment #751997 - Flags: review?(mrbkap)
Created attachment 751998 [details] [diff] [review]
Part 4 - Tests. v1
Attachment #751998 - Flags: review?(mrbkap)
https://tbpl.mozilla.org/?tree=Try&rev=4f832df35e6e

Spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22102
Looks green.

Updated

4 years ago
Attachment #751995 - Flags: review?(mrbkap) → review+
Comment on attachment 751996 [details] [diff] [review]
Part 2 - Be more explicit about rejecting JSID_VOID for XOWs. v1

Ironically, I had to look up the meaning of EIBTI.
Attachment #751996 - Flags: review?(mrbkap) → review+

Updated

4 years ago
Attachment #751997 - Flags: review?(mrbkap) → review+
Comment on attachment 751998 [details] [diff] [review]
Part 4 - Tests. v1

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

::: js/xpconnect/tests/mochitest/test_bug862380.html
@@ +23,5 @@
> +      is(Object.getOwnPropertyNames(obj).length, 0, "Object.getOwnPropertyNames gives empty array");
> +      is(Object.keys(obj).length, 0, "Object.keys gives empty array");
> +      for (var i in obj)
> +        ok(false, "Enumerated something: " + i);
> +      ok(!((obj instanceof Window) || (obj instanceof Location)), "Instanceof doesn't do anything");

I was with you until here. Why should instanceof lie? It's pretty easy to figure out what kind of cross-origin object you have your hands on anyway.
Attachment #751998 - Flags: review?(mrbkap)
(In reply to Blake Kaplan (:mrbkap) from comment #12)
> I was with you until here. Why should instanceof lie? It's pretty easy to
> figure out what kind of cross-origin object you have your hands on anyway.

Ok, sure. I was just treating this as documenting our existing behavior, but you're right that said behavior is wrong. I'll remove that check and fix this in bug 870423.
Created attachment 753095 [details] [diff] [review]
Part 4 - Tests. v2
Attachment #751998 - Attachment is obsolete: true
Attachment #753095 - Flags: review?(mrbkap)

Updated

4 years ago
Attachment #753095 - Flags: review?(mrbkap) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/690f5224fb99
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/83ba168c4d7e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc3c9b632aa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/464c47889efe
https://hg.mozilla.org/mozilla-central/rev/690f5224fb99
https://hg.mozilla.org/mozilla-central/rev/83ba168c4d7e
https://hg.mozilla.org/mozilla-central/rev/2bc3c9b632aa
https://hg.mozilla.org/mozilla-central/rev/464c47889efe
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.