Boris: Have you had a chance to look at this yet? Do you want me to take it? If so, can you give me a hint where to start looking (if this works along the same lines as the other bug, I assume it is in browser code, not spidermonkey)?
FF2.0/188.8.131.52 are fine, FF184.108.40.206 is affected (I'll assume 220.127.116.11 as well for now, but will check in a bit). -> crowder based purely on enthusiasm, but maybe should be part of Boris's fix for bug 363594.
Assignee: dveditz → crowder
Whiteboard: [sg:critical] → [sg:critical] needs patch
I'm not having a lot of luck with this, it's in code that I have not looked at much before, if at all. Might be better owned by bz or someone else?
Adding mrbkap and Brendan. If we want this done today, I am going to need help.
So finally had time to sit down and read this bug and more importantly the testcase. Sounds to me like we just need to land the patch for bug 363594 on the branches, no? Unfortunately, it's been a while since I built branch, so I won't be able to test for a bit. But I'll post a patch if someone is willing to test.
I'll gladly test a patch.
Actually, it's not quite so simple, and I'm not as convinced that the fix for bug 363594 is correct. The reason bug 363594 is trunk-only is that in that case we were evaluating the JS async, so the caller cx was no longer on the context stack. All well and good, but fixing that by pushing the _target_ cx on the context stack (as we did) only works as long as the target is not itself privileged in some way. If push comes to shove, we should just land the patch I'll attach (port of bug 363594 to branches) because it's better than nothing, but it's not perfect. Ideally, we would push on the stack the same JSContext that the sandbox used for evaluation. Failing that, we should probably create a vanilla context with the sandbox object as its global, etc, just like EvalInSandboxObject() ends up doing.
Created attachment 254063 [details] [diff] [review] Branch port of bug 363594 This will fix this specific testcase.... At least with this, getting UniversalXPConnect involves getting your hands on a window whose global has UniversalXPConnect and then doing the load in some object connected with that window....
So I considered just doing a JS_NewContext here. But then we'd either need a bunch of code to make sure that bug 368714 stays fixed or we'd allow infinite loops in toString/valueOf to hang the app. :( I suppose we could more or less copy/paste the ContextHolder class into this code...
With this patch applied, neither of these testcases yields an alert window for me.
(that was testing with the 1.8 branch, btw)
Applied and built on 1.8.0 cleanly. If we need this fixed today, I recommend we land Boris' patch.
Comment on attachment 254063 [details] [diff] [review] Branch port of bug 363594 Nominating for branches since I can't find bz at the moment.
Comment on attachment 254063 [details] [diff] [review] Branch port of bug 363594 Who's a good sr?
Attachment #254063 - Flags: review?(jst)
Comment on attachment 254063 [details] [diff] [review] Branch port of bug 363594 sr=dveditz
Attachment #254063 - Flags: review?(jst) → review+
Comment on attachment 254063 [details] [diff] [review] Branch port of bug 363594 Approved for both branches, a=jay (Thanks for the quick turnaround on the patch bz/crowder!)
Whiteboard: [sg:critical] needs patch → [sg:critical] needs landing
Checking in dom/src/jsurl/nsJSProtocolHandler.cpp; /cvsroot/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,v <-- nsJSProtocolHandler.cpp new revision: 18.104.22.168; previous revision: 22.214.171.124 done brian-crowders-computer:~/mozcvs/mozilla crowder$ cvs ci dom/src/jsurl/nsJSProtocolHandler.cppChecking in dom/src/jsurl/nsJSProtocolHandler.cpp; /cvsroot/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp,v <-- nsJSProtocolHandler.cpp new revision: 126.96.36.199.2.4; previous revision: 188.8.131.52.2.3 done
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Keywords: regression → fixed184.108.40.206, fixed220.127.116.11
Resolution: --- → FIXED
Whiteboard: [sg:critical] needs landing → [sg:critical]
Verified fixed on branches, with a 2007-01-31 build of the 1.8.1 branch, I see the alerts in the testcases. With a 2007-02-07 build of the 1.8.1 branch, I don't get any alerts anymore, instead I get these error messages in the js console: Error: uncaught exception: Permission denied to get property Function.__proto__ Error: uncaught exception: Permission denied to get property String.__parent__ Also verified fixed with the latest 1.8.0.x branch build.
Status: RESOLVED → VERIFIED
Keywords: fixed18.104.22.168, fixed22.214.171.124 → verified126.96.36.199, verified188.8.131.52
regression between 184.108.40.206/2.0 and 220.127.116.11/18.104.22.168. Although fixed by the patch for trunk bug 363594, the blame for that regression was laid on trunk-only bug 351633. Would be nice to be able to identify the branch regressor.
On Firefox2, this seems to have regressed between 2006-12-01 and 2006-12-03: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-12-01+03&maxdate=2006-12-03+06&cvsroot=%2Fcvsroot There is a check-in by jst that looks like the likely cause, it doesn't have a bug number associated with it, but I think it's bug 351370. So I think the branch patch for bug 351370 was the branch regressor.
Oh, yeah. Absolutely. That said, did anyone file a bug to fix this correctly? See comment 9. The bandaid we checked in is sort of ok, but we should really fix this correctly.
Whiteboard: [sg:critical] → [sg:critical] hold for tbird release
I filed bug 372075 on comment 9. I think we should fix it for the next security release, but I'm not sure I'll be able to work on it... :(
By the way, is this a bugzilla's bug? I couldn't see comment #24 until after comment #27 has been added. And now I cannot see comment #25.
Does bugzilla keep track of private comment flags? I see nothing about them in the bug history, but comment 24 used to be private. Now comment 25 is. If someone twiddled the flags, that would make sense...
pvnick is doing a bit of research on XSS and also gathering up bugs with security related test cases to help add to the regression/certification test suites. adding him to the cc list in these...
You need to log in before you can comment on or make changes to this bug.