Last Comment Bug 352465 - "open a blocked popup" feature uses the global window as the opener for blocked popups (variant of bug 343175)
: "open a blocked popup" feature uses the global window as the opener for block...
Status: RESOLVED FIXED
[sg:moderate?]
: fixed-seamonkey1.0.8, fixed-seamonkey1.1.1
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: 1.8 Branch
: All All
: -- major (vote)
: ---
Assigned To: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
:
Mentors:
https://recluse.mozilla.org/attachmen...
: 370341 (view as bug list)
Depends on: 354973
Blocks:
  Show dependency treegraph
 
Reported: 2006-09-12 23:10 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2011-05-20 11:32 PDT (History)
9 users (show)
kairo: blocking‑seamonkey1.0.8+
kairo: blocking‑seamonkey1.1-
kairo: blocking‑seamonkey1.1.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
attacker (219 bytes, text/html)
2006-09-12 23:14 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
victim (177 bytes, text/html)
2006-09-12 23:15 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details
patch (1.44 KB, patch)
2006-12-15 20:23 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details | Diff | Review
patch v2 (2.43 KB, patch)
2006-12-15 21:16 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: superreview-
Details | Diff | Review
patch v3 (8.25 KB, patch)
2006-12-27 17:27 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review-
neil: superreview+
Details | Diff | Review
v4 (8.19 KB, patch)
2006-12-30 12:56 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
neil: review+
kairo: approval‑seamonkey1.0.8+
kairo: approval‑seamonkey1.1.1+
Details | Diff | Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2006-09-12 23:10:31 PDT
SeaMonkey is still vulnerable to the XSS exploit reported in bug 343175, because it always uses the topmost window as the opener for blocked popups, instead of the window that originally tried to open the popup.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/browser/resources/content/navigator.js&rev=1.596&mark=2442#2439

The testcase in bug 343175 does not work, since the frame injection bug that it uses (bug 343168) was also fixed. I will attach a new testcase.

I tested the 1.0.5 candidate build, 1.8 branch and trunk builds should be equally affected.
Comment 1 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-09-12 23:14:26 PDT
Created attachment 238153 [details]
attacker
Comment 2 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-09-12 23:15:10 PDT
Created attachment 238154 [details]
victim
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-09-12 23:17:39 PDT
To test this bug, load https://recluse.mozilla.org/attachment.cgi?id=238154 .

Expected results: "My opener was bugzilla.mozilla.org"
Actual results: "My opener was recluse.mozilla.org"
Comment 4 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-09-13 09:01:23 PDT
I'd ask for blocking-seamonkey1.0.6 if that flag existed - I don't really think this is critical enough to block the 1.0.5 release at this stage.
Comment 5 neil@parkwaycc.co.uk 2006-09-13 09:53:40 PDT
jst, any idea why nsGlobalWindow::FireAbuseEvents doesn't fire the events on the document of the window that tried to open the blocked popup? Instead the UI code has to grovel through the frameset looking for a window that it thinks matches..
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-11-06 15:01:35 PST
FireAbuseEvents fires on the window on which open() was called...  Generally, detecting which window _originated_ the load is Hard.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-12-15 09:55:30 PST
This should be easier to fix once bug 354973 is fixed.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-12-15 09:56:59 PST
Were the blocking-seamonkey-*? flags on this bug ignored because the people driving SeaMonkey releases can't see this bug?
Comment 9 Daniel Veditz [:dveditz] 2006-12-15 18:45:46 PST
Neil and biesi can see these bugs.
http://wiki.mozilla.org/SeaMonkey:Project_Organization
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-12-15 18:58:22 PST
I know that Neil and biesi are in the security group, but I made the (perhaps incorrect) assumption based on bugmail that KaiRo and CTho are the ones that deal with most SeaMonkey blocking requests, so I thought that might explain why this bug's blocking nominations weren't dealt with. I see now that they were made about 2 days before the associated releases, though, so I guess it probably wouldn't have mattered either way.
Comment 11 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-12-15 20:23:08 PST
Created attachment 248805 [details] [diff] [review]
patch

dveditz, neil, I don't really care who reviews and who superreviews.  neil, I'm not sure if dveditz actually wants to review this.
Comment 12 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-12-15 20:54:55 PST
Comment on attachment 248805 [details] [diff] [review]
patch

It's important to have prefs set the right way when testing patches :(
Comment 13 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-12-15 21:16:48 PST
Created attachment 248808 [details] [diff] [review]
patch v2

This depends on attachment 245720 [details] [diff] [review].  Unfortunately I can't test it, as that patch is in some garbage format that patch doesn't understand, and it's way too much to hack in by hand.
Comment 14 neil@parkwaycc.co.uk 2006-12-16 03:27:28 PST
(In reply to comment #8)
>Were the blocking-seamonkey-*? flags on this bug ignored because the people
>driving SeaMonkey releases can't see this bug?
Personally I was waiting on the fix to what turned out to be bug 354973.
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-12-16 03:42:49 PST
(In reply to comment #14)
> Personally I was waiting on the fix to what turned out to be bug 354973.

That's fine, I'm just asking because the blocking flag was neither denied nor granted, which made it seem as though it was being ignored. When I made that comment, I didn't realize the blocking request was made so close to the release.
Comment 16 Robert Kaiser (not working on stability any more) 2006-12-16 12:17:13 PST
Is it correct that this is not fixed in Firefox 1.5.0.9 and 2.0.0.1 yet? If so, we can probably ship the already tagged SeaMonkkey 1.0.7 unfixed and include a fix in 1.1 and 1.0.8
Comment 17 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-12-16 14:06:50 PST
(In reply to comment #16)
> Is it correct that this is not fixed in Firefox 1.5.0.9 and 2.0.0.1 yet?

Yes, that's correct. If I knew that the next releases were this close I probably wouldn't have asked for blocking.
Comment 18 neil@parkwaycc.co.uk 2006-12-16 15:36:14 PST
Comment on attachment 248808 [details] [diff] [review]
patch v2

Wrong sort of popup event!
You need to save the name etc. from onPopupBlocked() - it might be simpler to save the whole event (ask jst if that's reasonable).
Comment 19 Robert Kaiser (not working on stability any more) 2006-12-24 07:38:28 PST
As Firefox does not ship a fix yet, this should be pushed out to the next releases, but it really blocks those.
Comment 20 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-12-27 17:27:18 PST
Created attachment 249816 [details] [diff] [review]
patch v3

Neil, I'm not sure what erroneous code you wanted me to remove.  Sorry.  I cleaned up various whitespace stuff in nearby code.
Comment 21 neil@parkwaycc.co.uk 2006-12-29 14:51:16 PST
Comment on attachment 249816 [details] [diff] [review]
patch v3

>-    separator.hidden = !createShowPopupsMenu(event.target);
>+    separator.hidden = !createShowPopupsMenu(event);
> }
> 
>-function createShowPopupsMenu(parent) {
>+function createShowPopupsMenu(event)
>+{
>+  var parent = event.target;
This was the spurious change that you don't need.

>   while (parent.lastChild && parent.lastChild.hasAttribute("uri"))
>     parent.removeChild(parent.lastChild);
Now that you no longer set the uri attribute you need another way to detect your dynamically added menuitems (none of which have ids for instance).

>-  if  (popupUrls.length == 0)
>+  if (popups.length == 0)
This was the whitespace change that I was looking for :-)

>+  var popup = target.popup;
popup doesn't always exist (see above).

>+  var reqWin = popup.reqWin;
>+  var uri = popup.url.spec;
>+  if (reqWin && reqWin.document == popup.reqDoc && uri) {
reqWin and uri will always exist if popup does.

>+    reqWin.open(uri, popup.name, popup.features); /// trunk-only???? (for name... s/popup.name/""/ ?)
/// ????
Comment 22 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-12-30 12:56:21 PST
Created attachment 250015 [details] [diff] [review]
v4
Comment 23 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-01-12 17:47:01 PST
Fixed on trunk
Comment 24 Robert Kaiser (not working on stability any more) 2007-01-12 18:07:11 PST
Comment on attachment 250015 [details] [diff] [review]
v4

a=me for 1.0.8 and 1.1.1
Comment 25 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-01-27 09:45:39 PST
Landed on both branches.
Comment 26 Andrew Schultz 2007-01-27 23:49:18 PST
This broke popup unblocking on the 1.8.0 branch.  There are no menu items for unblocking and the context menu for the blocker icon is just a tiny square.
Comment 27 Boris Zbarsky [:bz] (Out June 25-July 6) 2007-01-28 08:39:41 PST
Is there a bug on that?  That should block upcoming releases...
Comment 28 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-01-28 10:57:19 PST
I may have merged incorrectly... 1.8.0 differed from trunk much more than 1.8 did.  I'll look into it in a few minutes.
Comment 29 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-01-28 11:21:04 PST
-    var popup = popups[i];
+    var popup = browser.popups[i];

<CTho> NeilAway: do i need r/a, or just land it as bustage fix?
<NeilAway> CTho: just land it

Fixed.
Comment 30 Philip Chee 2011-05-20 11:32:01 PDT
*** Bug 370341 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.