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

Make sure Geolocation callbacks run in the right JS context

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: smaug, Assigned: dougt)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate])

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

9 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

9 years ago
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?
Assignee: nobody → doug.turner
(Reporter)

Comment 2

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

Comment 3

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

Comment 4

9 years ago
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.
Whiteboard: [sg:investigate]
(Reporter)

Comment 5

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

Comment 6

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

Comment 8

9 years ago
Created attachment 337181 [details] [diff] [review]
patch v.1
Attachment #337181 - Flags: review?(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
Attachment #337181 - Flags: superreview+
Attachment #337181 - Flags: review?(jst)
Attachment #337181 - Flags: review+
(Assignee)

Comment 10

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

Comment 11

9 years ago
changeset:   19106:851e9199e5f1
tag:         tip
user:        Doug Turner <dougt@meer.net>
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.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Group: core-security
Component: DOM: Other → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.