See bug 652317
This is extra fun because the callbacks might come from different origins, so we need to keep track of where each one came from... I guess I get to own this one. :(
Assignee: nobody → bzbarsky
Priority: -- → P1
Isn't this just the same what event handlers have had for years (longer than I've been hacking gecko) - use the context from the relevant window. If the callback is coming from chrome, it gets chrome privileges anyway, *IIRC*.
Or, hmm, do we handle all the callbacks related to a top level window in a same place? Then when adding a new callback, take reference to the window on which the method was called.
> Or, hmm, do we handle all the callbacks related to a top level window in a same > place? Refresh drivers are per-tab, effectively. We do know the document the callback comes, from, sort of. If we try a bit. I'll try to post a patch tomorrow.
So... Does this mean that use of nsIDOMNodeFilter in the treewalker is also unsafe?
And also geolocation, the xpath nsresolver.... Can we seriously not do the right thing somehow in the xpconnect wrappedjs code?
nsIDOMNodeFilter and xpath resolver are always called back synchronously as a result from other js-calls. So the right context should already be on the stack. geolocation is a different story though. As are desktop notifications (implemented on fennec).
> nsIDOMNodeFilter and xpath resolver are always called back synchronously Ah, good point. Sounds like we need another bug on geolocation and desktop notifications? :(
IIRC, either geolocation or desktop notifications should behave ok already.
or both...but better to make sure.
OK, dunno about desktop notifications (can't find where it calls its callbacks offhand), but in bug 452762 geolocation was changed to push null precisely to address this sort of issue. And from my reading of the XPCWrappedJSClass code, that looks correct in terms of the obvious "what else might be on the stack" issue: if there's nothing on the stack or the top thing on the stack is null, XPCWrappedJSClass will find the right JSContext for the callee object and then init an XPCCallContext with it, which will push it on the stack. In the case of refresh driver, we can only enter Notify() in the following ways: 1) AdvanceTimeAndRefresh called. 2) RestoreNormalRefresh called. 3) Timer fires. 4) DoRefresh called. #3 and #4 can only happen directly from the event loop, and when the event loop is spinning we're guaranteed a null on the JSContext stack (see bug 326777). We should probably push null for #1 and #2, but that doesn't involve branches. Are there things we need to protect against here other than "something is already on the stack"?
In the case of sync-xhr and alert()s, can't there be things on the JS stack even when called from the event loop?
OK, I talked to smaug. He's worried about bfcache interactions when a callback spins the event loop to put one of the documents with a pending callback into bfcache, but I think the last part of bug 607529 comment 8 applies. I don't see a security issue there.
Jonas, that's what bug 326777 was about. Any time nsThread proceses the event loop it will push a null JSContext on the stack before actually calling Run() on the runnable. Thus there will be things on the JS stack, but the _top_ thing on the stack will be null.
So I would like to claim there is no 4.0 or earlier issue here, and that the issue in 5.0 is limited to the APIs bug 435442 added. Testing a patch for those now, but since they're not web-exposed I don't think this bug needs to stay security-sensitive (though maybe we should make sure about comment 10 before we open it up?).
Try says this is green on our tests in a debug build, so I think we're good.
Attachment #529202 - Flags: review?(Olli.Pettay)
(In reply to comment #15) > issue in 5.0 is limited to the APIs bug 435442 added. Testing a patch for > those now, but since they're not web-exposed [...] They're not? I thought CSS Animations were one of the new web features for Firefox 5? If it's truly not webexposed then I suppose we can just pick it up in fx6
CSS Animations are exposed. The two functions in question that live on refresh driver are only exposed via nsIDOMWindowUtils; they were added to allow testing of animations and enforce the caller having UniversalXPConnect privileges before calling into the refresh driver. You can see the exact code at https://bug435442.bugzilla.mozilla.org/attachment.cgi?id=523829
Comment on attachment 529202 [details] [diff] [review] I think this is a sufficient fix for this bug. Review of attachment 529202 [details] [diff] [review]:
Attachment #529202 - Flags: review?(Olli.Pettay) → review+
Whiteboard: [need review][sg:moderate] dangerous if web exposed or misused by addons. → [need landing][sg:moderate] dangerous if web exposed or misused by addons.
Pushed http://hg.mozilla.org/mozilla-central/rev/285a5ec88501 I think we should probably open this bug up sometime in the near future.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [need landing][sg:moderate] dangerous if web exposed or misused by addons. → [sg:moderate] dangerous if web exposed or misused by addons.
Target Milestone: --- → mozilla6
Comment on attachment 529202 [details] [diff] [review] I think this is a sufficient fix for this bug. We probably should fix this on aurora, though. This is a safe fix that would just make sure extensions can't shoot themselves in the foot.
Attachment #529202 - Flags: approval-mozilla-aurora?
Attachment #529202 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.