Closed Bug 793969 Opened 7 years ago Closed 7 years ago

Should we prevent redefining valueOf on Location objects too?

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla19
Tracking Status
firefox16 + verified
firefox17 + verified
firefox18 + verified
firefox19 + verified
firefox-esr10 16+ verified

People

(Reporter: bzbarsky, Assigned: bholley)

References

Details

(Keywords: sec-high)

Attachments

(2 files, 2 obsolete files)

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?
@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: CVE-2012-4194
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.
Attachment #670663 - Flags: review?(bobbyholley+bmo)
Alex, we need to decide what we're doing with 16 here....  :(
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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?
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/
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.
Whiteboard: [need review] → [need review] [Must Land in 16.0]
Whiteboard: [need review] [Must Land in 16.0] → [need review] [Must Land in 17.0]
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)
Attached patch Tests. v1Splinter Review
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/mozilla-central/rev/ebdaddbe9b7b
Status: NEW → RESOLVED
Closed: 7 years ago
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.
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
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
QA Contact: mwobensmith
Whiteboard: [need review] [Must Land in 17.0]
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
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+
(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
Attachment #673146 - Flags: approval-mozilla-release?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.