452762 - Make sure Geolocation callbacks run in the right JS context

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

I'm not sure if this is handled properly, but I couldn't find any code where
the right context is pushed to stack (or whatever is the right thing to do here).
Because I don't have a testcase or anything, state UNCONFIRMED.

Comment 1

[Doug Turner (:dougt)]

at the very least, we should confirm that the right thing does happen.


jst:

http://mxr.mozilla.org/mozilla-central/source/dom/public/idl/geolocation/nsIDOMGeoGeolocation.idl#53

a js function is passed into this function.  from c++, i hold a strong reference to it.  when I am ready to call it back, I simply do a c++ method call. 

I do verify that the owner of the script still exists:

http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeolocation.cpp#698

Am I safe?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Doug, when a callback, which is registered on a web page, is called, can
it use Components.stack or do you get a security error?

I don't have Maemo stuff installed, so I don't have a data provider, I think.
So can't really test this.

For event listeners we explicitly push the context of the page to stack before
calling them. (I'd actually like to do it a bit different way than it is currently done, but at least the current way is safe.)
If something similar is needed for geolocation, perhaps you could
use nsCxPusher and push nsGeolocation::mOwner. (I hope it is the inner window)

And another thing, not related to security - I believe nsGeolocation should be added to cycle collection.

Comment 3

[Doug Turner (:dougt)]

Components.stack cause an assertion:

Error: Permission denied for <http://people.mozilla.org> to get property XPCComponents.stack
Source File: http://people.mozilla.org/~dougt/geo_stack_available.html
Line: 15

Please verify that my test case is correct.


for the CC issue, i filed:  bug 452834

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Hmm maybe Components.stack doesn't work because there isn't privileged context
on stack, but whether the context is the safeContext or the context of the
mOwner, dunno.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/xpccallcontext.cpp&rev=1.25&mark=95-103#88

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

So if safeContext is always used, there isn't security problems, but
chrome listeners don't get all the privileges they might want, I think.

Comment 7

[Johnny Stenback (:jst)]

I looked into this a bit today and we'll given that we don't know what's on the JS context stack when these callbacks fire, we'll need to push a context before calling these callbacks. After talking with dougt about this earlier today I thought we'd actually need to ensure that the geolocation code would always have access to the right inner window to be able to call these methods safely, but it turns out after having looked at that more closely that it's not necessary.

After talking through this with mrbkap as well I think the safest thing we can do here is to make sure that the code that's calling the callbacks always push null onto the context stack, that way xpconnect will do the right thing in all cases, it will either use the context where the callback came from, or the safe context if it can't find where the callback came from, but never any other random context that might be on the stack when one of these callbacks happen.

So dougt, IOW, you'll want to make sure you push null onto the context stack (nsIJSContextStack) before calling these callbacks, and popping (unconditionally) once you're done.

Comment 8

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 337181 [details] [diff] [review]
patch v.1

Looks good. It'd be nice to have a C++ class that would do this for you so that any early returns that might creep in here wouldn't hurt us, but unfortunately we don't have such a class that I know of (that can deal with pushing null). Write one if you want to. Either way, r+sr=jst

Comment 10

[Doug Turner (:dougt)]

if we continue adding more calls out to content from geolocation (which I suspect we will when we start handling more error cases, and timeouts), i will surely refactor the js context pushing.

thanks for the review!

Comment 11

[Doug Turner (:dougt)]

changeset:   19106:851e9199e5f1
tag:         tip
user:        Doug Turner <[e-mail]>
date:        Wed Sep 10 08:52:46 2008 -0700
summary:     Bug 452762 - Make sure Geolocation callbacks run in the right JS context r/sr=jst


smaug, thanks for discovering this problem.