Last Comment Bug 406686 - I can still steal your bank login (spoofing using <xul:popup>, take 2)
: I can still steal your bank login (spoofing using <xul:popup>, take 2)
Status: RESOLVED FIXED
[sg:high] qa: verify comment 9 testca...
: verified1.8.1.13
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Linux
: P1 critical (vote)
: ---
Assigned To: Neil Deakin
:
:
Mentors:
data:text/html,<iframe%20width=100%25...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-03 17:51 PST by Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
Modified: 2008-03-28 14:47 PDT (History)
16 users (show)
dsicore: blocking1.9+
dveditz: blocking1.8.1.12-
dveditz: blocking1.8.1.13+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
enndeakin: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
script for following testcase (378 bytes, application/x-javascript)
2007-12-03 17:51 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details
testcase (610 bytes, application/vnd.mozilla.xul+xml)
2007-12-03 17:53 PST, Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com]
no flags Details
one possibility is to just hide popups when switching tabs (2.86 KB, patch)
2007-12-06 12:47 PST, Neil Deakin
roc: review+
roc: superreview+
Details | Diff | Splinter Review
This patch hides popups in child frames as well (10.61 KB, patch)
2008-01-21 06:40 PST, Neil Deakin
roc: review-
Details | Diff | Splinter Review
address review comments (12.88 KB, patch)
2008-01-28 10:05 PST, Neil Deakin
no flags Details | Diff | Splinter Review
Fix up last CallQueryInterface (13.73 KB, patch)
2008-01-28 11:10 PST, Neil Deakin
roc: review+
dveditz: superreview+
Details | Diff | Splinter Review
add null check (13.71 KB, patch)
2008-02-07 10:07 PST, Neil Deakin
roc: review+
roc: superreview+
Details | Diff | Splinter Review
the branch version is simpler (1.63 KB, patch)
2008-02-07 11:25 PST, Neil Deakin
roc: review+
dveditz: superreview+
dveditz: approval1.8.1.13+
Details | Diff | Splinter Review
1.8.0 version of the patch (1.42 KB, patch)
2008-03-25 09:14 PDT, Alexander Sack
enndeakin: review+
Details | Diff | Splinter Review

Description Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-12-03 17:51:36 PST
Created attachment 291347 [details]
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 cst@andrew.cmu.edu cst@yecc.com] 2007-12-03 17:53:03 PST
Created attachment 291348 [details]
testcase

Simple testcase
Comment 2 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2007-12-03 17:56:14 PST
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] 2007-12-03 18:34:46 PST
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) 2007-12-05 17:15:26 PST
OK.  +'ing this and making this a P2.  We should fix this before final. 
Comment 5 Neil Deakin 2007-12-06 12:47:22 PST
Created attachment 291931 [details] [diff] [review]
one possibility is to just hide popups when switching tabs

Is this an effective solution, or is more necessary here?
Comment 6 Neil Deakin 2007-12-17 12:35:09 PST
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] 2007-12-20 10:58:53 PST
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 2007-12-20 11:07:13 PST
(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] 2007-12-20 11:08:18 PST
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 2008-01-15 15:06:26 PST
if we are going to take something for this it should be b3.  p1.
Comment 11 Neil Deakin 2008-01-21 06:40:57 PST
Created attachment 298261 [details] [diff] [review]
This patch hides popups in child frames as well
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-01-21 13:02:27 PST
 #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-10 2008-01-21 14:18:12 PST
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 2008-01-28 10:05:05 PST
Created attachment 299782 [details] [diff] [review]
address review comments
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-01-28 10:41:48 PST
+  nsIDocShellTreeItem* docShellItem;
+  CallQueryInterface(doc, &docShellItem);

Aren't we leaking here?
Comment 16 Neil Deakin 2008-01-28 11:10:47 PST
Created attachment 299802 [details] [diff] [review]
Fix up last CallQueryInterface
Comment 17 Daniel Veditz [:dveditz] 2008-01-28 11:19:55 PST
No time for trunk baking before branch landing.
Comment 18 Daniel Veditz [:dveditz] 2008-01-31 00:45:58 PST
Comment on attachment 299802 [details] [diff] [review]
Fix up last CallQueryInterface

thanks for fixing the frame bit.
sr=dveditz
Comment 19 Neil Deakin 2008-02-06 13:12:29 PST
Check in, still need to make a test for this
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2008-02-06 18:19:50 PST
I backed this out now the second time to figure out whether it's the cause for the orange.
Comment 21 Neil Deakin 2008-02-07 10:07:31 PST
Created attachment 301939 [details] [diff] [review]
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 2008-02-07 11:25:02 PST
Created attachment 301955 [details] [diff] [review]
the branch version is simpler
Comment 23 Neil Deakin 2008-02-08 06:37:12 PST
OK, checked in the patch with the nullcheck.
Comment 24 Justin Dolske [:Dolske] 2008-02-08 11:09:35 PST
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] 2008-02-08 12:25:43 PST
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] 2008-02-13 18:38:46 PST
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] 2008-02-15 11:18:42 PST
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] 2008-03-13 17:37:07 PDT
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 2008-03-25 09:14:49 PDT
Created attachment 311591 [details] [diff] [review]
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 2008-03-28 14:47:05 PDT
Comment on attachment 311591 [details] [diff] [review]
1.8.0 version of the patch

Looks ok

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