352465 - "open a blocked popup" feature uses the global window as the opener for blocked popups (variant of bug 343175)

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

Attachment: attacker

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Attachment: victim

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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: [e-mail]]]

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

[[e-mail]]

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 [:bzbarsky]]

FireAbuseEvents fires on the window on which open() was called...  Generally, detecting which window _originated_ the load is Hard.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

This should be easier to fix once bug 354973 is fixed.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

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]]

Neil and biesi can see these bugs.
http://wiki.mozilla.org/SeaMonkey:Project_Organization

Comment 10

[:Gavin Sharp [email: [e-mail]]]

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 [e-mail] [e-mail]]]

Attachment: 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 [e-mail] [e-mail]]]

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 [e-mail] [e-mail]]]

Attachment: 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

[[e-mail]]

(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: [e-mail]]]

(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]

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: [e-mail]]]

(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

[[e-mail]]

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]

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 [e-mail] [e-mail]]]

Attachment: 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

[[e-mail]]

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 [e-mail] [e-mail]]]

Attachment: v4

Comment 23

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Fixed on trunk

Comment 24

[Robert Kaiser]

Comment on attachment 250015 [details] [diff] [review]
v4

a=me for 1.0.8 and 1.1.1

Comment 25

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Landed on both branches.

Comment 26

[Andrew Schultz]

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 [:bzbarsky]]

Is there a bug on that?  That should block upcoming releases...

Comment 28

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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 [e-mail] [e-mail]]]

-    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.