Closed Bug 352465 Opened 15 years ago Closed 15 years ago

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

Categories

(SeaMonkey :: General, defect)

1.8 Branch
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Gavin, Assigned: csthomas)

References

()

Details

(Keywords: fixed-seamonkey1.0.8, fixed-seamonkey1.1.1, Whiteboard: [sg:moderate?])

Attachments

(3 files, 3 obsolete files)

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.
Attached file attacker
Attached file victim
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"
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.
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..
FireAbuseEvents fires on the window on which open() was called...  Generally, detecting which window _originated_ the load is Hard.
Flags: blocking-seamonkey1.5a?
Flags: blocking-seamonkey1.1b?
Flags: blocking-seamonkey1.0.6?
This should be easier to fix once bug 354973 is fixed.
Flags: blocking-seamonkey1.0.6? → blocking-seamonkey1.0.7?
Were the blocking-seamonkey-*? flags on this bug ignored because the people driving SeaMonkey releases can't see this bug?
Flags: blocking-seamonkey1.1b? → blocking-seamonkey1.1?
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.
Attached patch patch (obsolete) — Splinter Review
dveditz, neil, I don't really care who reviews and who superreviews.  neil, I'm not sure if dveditz actually wants to review this.
Assignee: general → cst
Status: NEW → ASSIGNED
Attachment #248805 - Flags: superreview?(neil)
Attachment #248805 - Flags: review?(dveditz)
Comment on attachment 248805 [details] [diff] [review]
patch

It's important to have prefs set the right way when testing patches :(
Attachment #248805 - Attachment is obsolete: true
Attachment #248805 - Flags: superreview?(neil)
Attachment #248805 - Flags: review?(dveditz)
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #248808 - Flags: superreview?(neil)
Attachment #248808 - Flags: review?(dveditz)
(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.
(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.
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
(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 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).
Attachment #248808 - Flags: superreview?(neil) → superreview-
As Firefox does not ship a fix yet, this should be pushed out to the next releases, but it really blocks those.
Flags: blocking-seamonkey1.1?
Flags: blocking-seamonkey1.1.1+
Flags: blocking-seamonkey1.1-
Flags: blocking-seamonkey1.0.8+
Flags: blocking-seamonkey1.0.7?
Attached patch patch v3 (obsolete) — Splinter Review
Neil, I'm not sure what erroneous code you wanted me to remove.  Sorry.  I cleaned up various whitespace stuff in nearby code.
Attachment #248808 - Attachment is obsolete: true
Attachment #249816 - Flags: superreview?(neil)
Attachment #249816 - Flags: review?(neil)
Attachment #248808 - Flags: review?(dveditz)
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/""/ ?)
/// ????
Attachment #249816 - Flags: superreview?(neil)
Attachment #249816 - Flags: superreview+
Attachment #249816 - Flags: review?(neil)
Attachment #249816 - Flags: review-
Attached patch v4Splinter Review
Attachment #249816 - Attachment is obsolete: true
Attachment #250015 - Flags: review?(neil)
Attachment #250015 - Flags: review?(neil) → review+
Fixed on trunk
Assignee: cst → general
Status: ASSIGNED → NEW
Attachment #250015 - Flags: approval-seamonkey1.1.1?
Attachment #250015 - Flags: approval-seamonkey1.0.8?
Assignee: general → cst
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 250015 [details] [diff] [review]
v4

a=me for 1.0.8 and 1.1.1
Attachment #250015 - Flags: approval-seamonkey1.1.1?
Attachment #250015 - Flags: approval-seamonkey1.1.1+
Attachment #250015 - Flags: approval-seamonkey1.0.8?
Attachment #250015 - Flags: approval-seamonkey1.0.8+
Flags: blocking-seamonkey1.5a?
Whiteboard: [sg:moderate?]
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.
Is there a bug on that?  That should block upcoming releases...
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.
-    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.
Group: security
Duplicate of this bug: 370341
You need to log in before you can comment on or make changes to this bug.