Should we prevent redefining valueOf on Location objects too?

VERIFIED FIXED in Firefox 16

Status

()

Core
DOM
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: Bobby Holley (parental leave - send mail for anything urgent))

Tracking

({sec-high})

unspecified
mozilla19
x86
Mac OS X
sec-high
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16+ verified, firefox17+ verified, firefox18+ verified, firefox19+ verified, firefox-esr1016+ verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Adam Barth suggested that in the spec thread I started.  Opera, Safari, and WebKit certainly do that.

I haven't tested IE to see what it does here.
We certainly could. Our native property resolution code doesn't check things on Object.prototype because it doesn't need to, in general - the cross-compartment callers always just get something clean when looking things up on their holder objects. But we could just special-case either valueOf or all the non-own holder properties when determining whether we're shadowing or not.
Or could we just stick a valueOf in the Location IDL and implement it in C++?  Not sure what it should do, exactly...
Given the current wrapper situation, would it even be called?
I don't know.  I also don't know whether the current wrapper situation will persist...

Feel free to follow up on the whatwg/public-script-coord thread!
so sec-high because we (or plugins) get confused about some other window's origin?
Keywords: sec-high
We (browser chrome) will never be confused thanks to cross-compartment Xrays. As for plugins, do we have any reason to believe any plugins end up invoking valueOf() rather than toString()?
I don't know.  Ask Adam Barth?

Comment 8

5 years ago
@bholley: We did a survey of NPAPI plugins, and a whole bunch of them try to determine the security context of their container by evaling JavaScript.  It's scary stuff.  I don't remember which all variations we saw, but I remember we tried to be a bit conservative.
> As for plugins, do we have any reason to believe any plugins end up invoking valueOf()

They didn't use to, but now they do, yay.
Duplicate of this bug: 800666
So the obvious fix I know for this involves an IID rev on nsIDOMLocation.  How happy are we with that on releases and betas and such?  :(

Bobby, if you can think of a better idea Friday morning, please do.
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
Created attachment 670663 [details] [diff] [review]
Add a valueOf method to Location.
Attachment #670663 - Flags: review?(bobbyholley+bmo)
Alex, we need to decide what we're doing with 16 here....  :(
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Created attachment 670711 [details] [diff] [review]
Redirect Location valueOf() calls to toString() at the Xray layer. v1

This lets as avoid an IID rev.
Attachment #670711 - Flags: review?(bzbarsky)
Comment on attachment 670663 [details] [diff] [review]
Add a valueOf method to Location.

This patch is probably the way to go for branches where we don't care about the IID rev btw.
Attachment #670663 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 670711 [details] [diff] [review]
Redirect Location valueOf() calls to toString() at the Xray layer. v1

r=me, assuming it fixes the bug.
Attachment #670711 - Flags: review?(bzbarsky) → review+
Over to Bobby, since he's got a better handle on this.
Assignee: bzbarsky → bobbyholley+bmo
Awaiting instructions on how to proceed. Should I go ahead and push this to try  and / or inbound?
Blocks: 800666
two patches here, which were you going to push? Do we care about changing the IID for such an internal implementation detail as Location? bz's patch looks cleaner if we can take it everywhere.
For testcases for historical problems we've had with Location see (in part)

bug 765527
bug 756719
bug 735073
bug 665548
bug 541530
(In reply to Daniel Veditz [:dveditz] from comment #19)
> two patches here, which were you going to push? Do we care about changing
> the IID for such an internal implementation detail as Location? bz's patch
> looks cleaner if we can take it everywhere.

Yes, but I don't think we want to rev the IID on beta. It's against policy (especially for DOM interfaces) and only likely to draw attention to the issue.

If we want this anywhere outside of Nightly and Aurora, I think we should push my patch, and possibly follow up with bz's patch on Nightly.
Ok, I've been told to push to try only. I've got it ready to go, but try is closed.

http://sadtrombone.vorb.is/
There we go:

https://tbpl.mozilla.org/?tree=Try&rev=4b2fc0ba7fdc

Updated

5 years ago
tracking-firefox16: ? → +
tracking-firefox17: ? → +
tracking-firefox18: ? → +
tracking-firefox19: ? → +
That looks green, but there's a bunch of infra red cluttering things up. Let's try this again:

https://tbpl.mozilla.org/?tree=Try&rev=9e26cac83f2d
Can we get a testcase patch ready to land after we've shipped this fix?
Flags: in-testsuite?
(In reply to Daniel Veditz [:dveditz] from comment #25)
> Can we get a testcase patch ready to land after we've shipped this fix?

I can write such a testcase when the time comes. Please ping me whenever that is. I'd like to avoid juggling too many patches whose landing is deferred for security reasons.

Updated

5 years ago
Whiteboard: [need review] → [need review] [Must Land in 16.0]

Updated

5 years ago
Whiteboard: [need review] [Must Land in 16.0] → [need review] [Must Land in 17.0]
Created attachment 673146 [details] [diff] [review]
Define an identity transformation at the Xray layer. v2

This is the safer approach we discussed over vidyo - just make valueOf an identity transformation.
Attachment #670663 - Attachment is obsolete: true
Attachment #670711 - Attachment is obsolete: true
Attachment #673146 - Flags: review?(mrbkap)
Created attachment 673148 [details] [diff] [review]
Tests. v1

Feel free to suggest other tests here, blake.
Attachment #673148 - Flags: review?(mrbkap)
https://tbpl.mozilla.org/?tree=Try&rev=0ea992ee533f

Once this all blows over, I'm going to back out the Xray stuff and just define this as a property on nsIDOMWindow so that this all just works naturally.
All green. Given how this week has been, I'm going to go spend the next couple of hours outside. Blake, if you don't have any significant review comments, feel free to push the fix (but not the tests) to inbound. Otherwise, I can push it this evening.
Comment on attachment 673146 [details] [diff] [review]
Define an identity transformation at the Xray layer. v2

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

One question, but this looks good.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +769,5 @@
>  
> +static JSBool
> +IdentityValueOf(JSContext *cx, unsigned argc, jsval *vp)
> +{
> +    JS_SET_RVAL(cx, vp, JS_THIS(cx, vp));

Should we insist on |this| being an object (as opposed to undefined, which can happen in strict mode) so that this follows the implementation of Object.prototype.valueOf?
Attachment #673146 - Flags: review?(mrbkap) → review+
Comment on attachment 673146 [details] [diff] [review]
Define an identity transformation at the Xray layer. v2

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

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +811,5 @@
> +    {
> +        JSFunction *fun = JS_NewFunctionById(cx, &IdentityValueOf, 0, 0, NULL, id);
> +        if (!fun)
> +            return false;
> +        desc->obj = wrapper;

This patch makes valueOf an own property on location. I'm OK with that (given how weird the location object is in general, one more oddity won't doesn't seem like it makes a difference) but we should probably add a comment on that here.
mrbkap and I discussed these comments on IRC. comment 32 was based on mistaken premises, and we decided to skip comment 31 for lack of an easy API to do that in XPConnect.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebdaddbe9b7b

Updated

5 years ago
status-firefox-esr10: --- → affected
https://hg.mozilla.org/mozilla-central/rev/ebdaddbe9b7b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox19: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Some branch approval noms when you have time would be great here.
Akeybl gave me blanket approval to land this bug on all branches on monday.

Updated

5 years ago
Attachment #673148 - Flags: review?(mrbkap) → review+
Confirmed broken on build 2012-9-24, Firefox 18.0a1.
Verified fixed on build 2012-10-22, Firefox 19.0a1. 
Mac 10.8.2, Windows 7 and Ubuntu 11.10
https://hg.mozilla.org/releases/mozilla-aurora/rev/dbdae3188556
https://hg.mozilla.org/releases/mozilla-beta/rev/2dc642d4daef
https://hg.mozilla.org/releases/mozilla-esr10/rev/d8a766d176fd
status-firefox-esr10: affected → fixed
status-firefox17: --- → fixed
status-firefox18: --- → fixed
Comment on attachment 673146 [details] [diff] [review]
Define an identity transformation at the Xray layer. v2

This still needs to land on the FIREFOX_10_0_9esr_RELEASE branch of mozilla-esr10 as well as mozilla-release.
Attachment #673146 - Flags: approval-mozilla-release?
Attachment #673146 - Flags: approval-mozilla-beta+
Attachment #673146 - Flags: approval-mozilla-aurora+
Matt, can you please verify if this is fixed for...

latest-mozilla-aurora:
ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora

17.0b3 (should be available by tomorrow morning):
ftp://ftp.mozilla.org/pub/firefox/nightly/17.0b3-candidates

Same test as you did in comment 38, thanks.
Status: RESOLVED → VERIFIED
status-firefox19: fixed → verified
QA Contact: mwobensmith
Whiteboard: [need review] [Must Land in 17.0]
status-firefox16: --- → affected
https://hg.mozilla.org/releases/mozilla-release/rev/7440f6c1b07e
status-firefox16: affected → fixed
Pushed to esr10 relbranch:
https://hg.mozilla.org/releases/mozilla-esr10/rev/246c665f50d3
Verified fixed on build 2012-10-24, 17.0b3
Verified fixed on build 2012-10-24, Aurora 18.0a2
Mac 10.8.2, Windows 7 and Ubuntu 11.10
Verified fixed on build 2012-10-24, 10.0.10esr
Verified fixed on build 2012-10-24, 16.0.2
Mac 10.8.2, Windows 7 and Ubuntu 11.10
status-firefox-esr10: fixed → verified
status-firefox16: fixed → verified
status-firefox17: fixed → verified
status-firefox18: fixed → verified
Comment on attachment 673148 [details] [diff] [review]
Tests. v1

This should be fixed everywhere, so requesting approval to land the mochitest on m-i.
Attachment #673148 - Flags: sec-approval?
Comment on attachment 673148 [details] [diff] [review]
Tests. v1

sec-approval+
Attachment #673148 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5165ee49c7db
(In reply to Daniel Veditz [:dveditz] from comment #47)
> Comment on attachment 673148 [details] [diff] [review]
> Tests. v1

https://hg.mozilla.org/mozilla-central/rev/5165ee49c7db
Flags: in-testsuite? → in-testsuite+
Group: core-security
tracking-firefox-esr10: --- → 16+
Attachment #673146 - Flags: approval-mozilla-release?
You need to log in before you can comment on or make changes to this bug.