406686 - I can still steal your bank login (spoofing using <xul:popup>, take 2)

Status: RESOLVED FIXED

(Core :: General, defect, P1)

Description

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

Attachment: script for following testcase

Bug 326877 comment 82 (open a new tab and show a popup in front of it from a different tab) still works just fine for me on trunk.  I can still swipe your login info just fine with the URL bar showing your bank's domain.  Simplified testcase in a moment.

Comment 1

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

Attachment: testcase

Simple testcase

Comment 2

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

The testcase doesn't look right in 1.8.x but it looks fine in latest-trunk.  If it looks wrong, the button is unlabeled, but the testcase still works.

Comment 3

[Daniel Veditz [:dveditz]]

testcase could have had a text field in the popup like the ones in the old bug, and thus steal your password (OK, username in this particular page, but could be adapted).

Comment 4

[Damon Sicore (:damons)]

OK.  +'ing this and making this a P2.  We should fix this before final. 

Comment 5

[Neil Deakin]

Attachment: one possibility is to just hide popups when switching tabs

Is this an effective solution, or is more necessary here?

Comment 6

[Neil Deakin]

Comment on attachment 291931 [details] [diff] [review]
one possibility is to just hide popups when switching tabs

OK, no answer, so let's try just this then. Ignore the change to Hakan's name there.

Comment 7

[Daniel Veditz [:dveditz]]

appears to fix the problem on trunk. Patch doesn't apply to the 1.8 branch (no nsXULPopupManager.h), hopefully it won't be too hard to duplicate what the XULPopupManager does.

Should the #include also be limited to #ifdef MOZ_XUL or is nsXULPopupManager.h exported regardless?

Comment 8

[Neil Deakin]

(In reply to comment #7)
> appears to fix the problem on trunk. Patch doesn't apply to the 1.8 branch (no
> nsXULPopupManager.h), hopefully it won't be too hard to duplicate what the
> XULPopupManager does.

For the branch, nsPresShell:HidePopups should be called instead.

> Should the #include also be limited to #ifdef MOZ_XUL or is nsXULPopupManager.h
> exported regardless?

Yes, I'll make that change.

Comment 9

[Daniel Veditz [:dveditz]]

I take that back, it only fixes the problem if the evil page is the top-level document. If you put it in a frame the attack still works

data:text/html,<iframe%20width=100%25%20height=100%25%20src="https://bugzilla.mozilla.org/attachment.cgi?id=291348"></iframe>

(note: for some reason bugzilla doesn't think I'm logged in to see the attachment when I do the above. If you see the same thing go ahead and manually click the "login" link, and when the bug itself shows up click the original testcase. Annoying, but doesn't invalidate my claim).

Comment 10

[chris hofmann]

if we are going to take something for this it should be b3.  p1.

Comment 11

[Neil Deakin]

Attachment: This patch hides popups in child frames as well

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

 #ifdef MOZ_XUL
     // if attempting to move the window, hide any open popups
     nsXULPopupManager* pm = nsXULPopupManager::GetInstance();
     nsCOMPtr<nsIDocument> doc(do_QueryInterface(mDocument));
-    if (pm && doc)
-      pm->HidePopupsInDocument(doc);
+    if (pm && doc) {
+      nsCOMPtr<nsISupports> container = doc->GetContainer();
+      if (container) {
+        nsIDocShellTreeItem* docShellToHide;
+        CallQueryInterface(container, &docShellToHide);
+        pm->HidePopupsInDocShell(docShellToHide);
+      }
+    }
 #endif

Could you share this code? With nsDocumentViewer too? Maybe nsContentUtils, although I don't know if you can call that from dom/

Why CallQueryInterface and not do_QueryInterface? Aren't we leaking?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And maybe the null-check should be after the do_QueryInterface, unless something guarantees that the result of nsIDocument::GetContainer always QIs to nsIDocShellTreeItem.

Comment 14

[Neil Deakin]

Attachment: address review comments

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  nsIDocShellTreeItem* docShellItem;
+  CallQueryInterface(doc, &docShellItem);

Aren't we leaking here?

Comment 16

[Neil Deakin]

Attachment: Fix up last CallQueryInterface

Comment 17

[Daniel Veditz [:dveditz]]

No time for trunk baking before branch landing.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 299802 [details] [diff] [review]
Fix up last CallQueryInterface

thanks for fixing the frame bit.
sr=dveditz

Comment 19

[Neil Deakin]

Check in, still need to make a test for this

Comment 20

[Asaf Romano (gone)]

I backed this out now the second time to figure out whether it's the cause for the orange.

Comment 21

[Neil Deakin]

Attachment: add null check

One of tests is crashing. The fix is to add a nullcheck for mDocument in nsDocumentViewer::PageHide. Otherwise, the patch is the same.

Comment 22

[Neil Deakin]

Attachment: the branch version is simpler

Comment 23

[Neil Deakin]

OK, checked in the patch with the nullcheck.

Comment 24

[Justin Dolske [:Dolske]]

Backed out on trunk. Mochitests were failing on qm-win2k3-01, and this looked like a possible cause (still investigating).

*** 46346 ERROR FAIL | mouse click on trigger after_start position |  | /tests/toolkit/content/tests/widgets/test_popup_button.xul
*** 46398 ERROR FAIL | open popup anchored after_start position |  | /tests/toolkit/content/tests/widgets/test_popup_button.xul
*** 46405 ERROR FAIL | open popup anchored after_end position |  | /tests/toolkit/content/tests/widgets/test_popup_button.xul
*** 46461 ERROR FAIL | open popup anchored with attribute after_start position |  | /tests/toolkit/content/tests/widgets/test_popup_button.xul
*** 46468 ERROR FAIL | open popup anchored with attribute after_end position |  | /tests/toolkit/content/tests/widgets/test_popup_button.xul
*** 46631 ERROR FAIL | open popup with open property after_start position |  | /tests/toolkit/content/tests/widgets/test_popup_button.xul

Backed out with:

cvs update -j1.47 -j1.46 layout/xul/base/src/nsXULPopupManager.cpp
cvs update -j1.26 -j1.25 layout/xul/base/public/nsXULPopupManager.h
cvs update -j3.332 -j3.331 layout/generic/nsFrameFrame.cpp
cvs update -j1.571 -j1.570 layout/base/nsDocumentViewer.cpp
cvs update -j1.985 -j1.984 dom/src/base/nsGlobalWindow.cpp
cvs update -j1.275 -j1.274 content/base/src/nsContentUtils.cpp
cvs update -j1.161 -j1.160 content/base/public/nsContentUtils.h

Comment 25

[Justin Dolske [:Dolske]]

So... The qm-win2k3-01 box went green, but in the cycle before it picked up the backout. The other suspect for the orange was QA (logged in to make a change for bug 414720), but they say they left the box in a good state and didn't change anything when it went green. It's not clear exactly how things started working, but since the tests were able to pass without the backout it should be ok to reland this.

Relanded with:

cvs update -j1.162 -j1.161 content/base/public/nsContentUtils.h
cvs update -j1.276 -j1.275 content/base/src/nsContentUtils.cpp
cvs update -j1.986 -j1.985 dom/src/base/nsGlobalWindow.cpp
cvs update -j1.48 -j1.47 layout/xul/base/src/nsXULPopupManager.cpp
cvs update -j1.27 -j1.26 layout/xul/base/public/nsXULPopupManager.h
cvs update -j3.333 -j3.332 layout/generic/nsFrameFrame.cpp
cvs update -j1.572 -j1.571 layout/base/nsDocumentViewer.cpp

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 301955 [details] [diff] [review]
the branch version is simpler

works great on branch, both the original and framed cases.
sr=dveditz

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 301955 [details] [diff] [review]
the branch version is simpler

approved for 1.8.1.13, a=dveditz for release-drivers

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Fixed on branch with both the original testcase and Dan's iframe version with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.13) Gecko/2008031114 Firefox/2.0.0.13 and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/2008031115 Firefox/2.0.0.13.

Comment 29

[Alexander Sack]

Attachment: 1.8.0 version of the patch

neil, please verify that this shuffled patch makes sense on 1.8.0 branch.

Comment 30

[Neil Deakin]

Comment on attachment 311591 [details] [diff] [review]
1.8.0 version of the patch

Looks ok