Should we prevent redefining valueOf on Location objects too?

VERIFIED FIXED in Firefox 16

Status

()

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: bzbarsky, Assigned: bholley)

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.
(Assignee)

Comment 1

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

Comment 2

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

Comment 4

6 years ago
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
(Assignee)

Comment 6

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

Comment 7

6 years ago
I don't know.  Ask Adam Barth?

Comment 8

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

Comment 9

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

Updated

6 years ago
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]
(Assignee)

Comment 14

6 years ago
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)
(Assignee)

Comment 15

6 years ago
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
(Assignee)

Comment 18

6 years ago
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
(Assignee)

Comment 21

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

Comment 22

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

Updated

6 years ago
tracking-firefox16: ? → +
tracking-firefox17: ? → +
tracking-firefox18: ? → +
tracking-firefox19: ? → +
(Assignee)

Comment 24

6 years ago
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?
(Assignee)

Comment 26

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

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

Updated

6 years ago
Whiteboard: [need review] [Must Land in 16.0] → [need review] [Must Land in 17.0]
(Assignee)

Comment 27

6 years ago
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)
(Assignee)

Comment 28

6 years ago
Created attachment 673148 [details] [diff] [review]
Tests. v1

Feel free to suggest other tests here, blake.
Attachment #673148 - Flags: review?(mrbkap)
(Assignee)

Comment 29

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

Comment 30

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

Comment 33

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

Updated

6 years ago
status-firefox-esr10: --- → affected
https://hg.mozilla.org/mozilla-central/rev/ebdaddbe9b7b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox19: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Some branch approval noms when you have time would be great here.
(Assignee)

Comment 37

6 years ago
Akeybl gave me blanket approval to land this bug on all branches on monday.

Updated

6 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
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
(Assignee)

Updated

6 years ago
status-firefox16: affected → fixed
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
(Assignee)

Comment 46

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