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

RESOLVED FIXED

Status

()

Core
General
P1
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com], Assigned: Neil Deakin (mostly unavailable until September))

Tracking

({verified1.8.1.13})

unspecified
x86
Linux
verified1.8.1.13
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.12 -
blocking1.8.1.13 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high] qa: verify comment 9 testcase also, URL)

Attachments

(6 attachments, 3 obsolete attachments)

378 bytes, application/x-javascript
Details
610 bytes, application/vnd.mozilla.xul+xml
Details
13.73 KB, patch
roc
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
13.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.63 KB, patch
roc
: review+
dveditz
: superreview+
Details | Diff | Splinter Review
1.42 KB, patch
Neil Deakin (mostly unavailable until September)
: review+
Details | Diff | Splinter Review
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.
Created attachment 291348 [details]
testcase

Simple testcase
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.
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).
Flags: wanted1.8.1.x+
Flags: blocking1.9?
Flags: blocking1.8.1.12?
Whiteboard: [sg:high]

Updated

10 years ago
Summary: I can still steal your bank login → I can still steal your bank login (spoofing using <xul:popup>, take 2)
OK.  +'ing this and making this a P2.  We should fix this before final. 
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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?
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
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.
Attachment #291931 - Flags: superreview?(roc)
Attachment #291931 - Flags: review?(roc)
Attachment #291931 - Flags: superreview?(roc)
Attachment #291931 - Flags: superreview+
Attachment #291931 - Flags: review?(roc)
Attachment #291931 - Flags: review+
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?
(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.


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).
Flags: blocking1.8.1.12? → blocking1.8.1.12+

Comment 10

10 years ago
if we are going to take something for this it should be b3.  p1.
Priority: P2 → P1
Whiteboard: [sg:high] → [sg:high] need patch.
Created attachment 298261 [details] [diff] [review]
This patch hides popups in child frames as well
Attachment #291931 - Attachment is obsolete: true
Attachment #298261 - Flags: superreview?(dveditz)
Attachment #298261 - Flags: review?(roc)
 #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?
And maybe the null-check should be after the do_QueryInterface, unless something guarantees that the result of nsIDocument::GetContainer always QIs to nsIDocShellTreeItem.
Attachment #298261 - Flags: review?(roc) → review-
Created attachment 299782 [details] [diff] [review]
address review comments
Attachment #298261 - Attachment is obsolete: true
Attachment #299782 - Flags: superreview?(dveditz)
Attachment #299782 - Flags: review?(roc)
Attachment #298261 - Flags: superreview?(dveditz)
+  nsIDocShellTreeItem* docShellItem;
+  CallQueryInterface(doc, &docShellItem);

Aren't we leaking here?
Created attachment 299802 [details] [diff] [review]
Fix up last CallQueryInterface
Attachment #299782 - Attachment is obsolete: true
Attachment #299802 - Flags: superreview?(dveditz)
Attachment #299802 - Flags: review?(roc)
Attachment #299782 - Flags: superreview?(dveditz)
Attachment #299782 - Flags: review?(roc)
No time for trunk baking before branch landing.
Flags: blocking1.8.1.13+
Flags: blocking1.8.1.12-
Flags: blocking1.8.1.12+
Attachment #299802 - Flags: review?(roc) → review+
Whiteboard: [sg:high] need patch. → [sg:high][need sr=dveditz]
Priority: P1 → P2
Priority: P2 → P1
Target Milestone: --- → mozilla1.9beta4
Comment on attachment 299802 [details] [diff] [review]
Fix up last CallQueryInterface

thanks for fixing the frame bit.
sr=dveditz
Attachment #299802 - Flags: superreview?(dveditz) → superreview+
Whiteboard: [sg:high][need sr=dveditz] → [sg:high]
Check in, still need to make a test for this
Flags: in-testsuite?
I backed this out now the second time to figure out whether it's the cause for the orange.
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.
Attachment #301939 - Flags: superreview?(roc)
Attachment #301939 - Flags: review?(roc)
Created attachment 301955 [details] [diff] [review]
the branch version is simpler
Attachment #301955 - Flags: superreview?(dveditz)
Attachment #301955 - Flags: review?(roc)
Attachment #301955 - Flags: review?(roc) → review+
Attachment #301939 - Flags: superreview?(roc)
Attachment #301939 - Flags: superreview+
Attachment #301939 - Flags: review?(roc)
Attachment #301939 - Flags: review+
OK, checked in the patch with the nullcheck.
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
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
Whiteboard: [sg:high] → [sg:high][needs sr=dveditz]
Target Milestone: mozilla1.9beta4 → ---
Comment on attachment 301955 [details] [diff] [review]
the branch version is simpler

works great on branch, both the original and framed cases.
sr=dveditz
Attachment #301955 - Flags: superreview?(dveditz) → superreview+
Whiteboard: [sg:high][needs sr=dveditz] → [sg:high] qa: verify comment 9 testcase also
Attachment #301955 - Flags: approval1.8.1.13?
Comment on attachment 301955 [details] [diff] [review]
the branch version is simpler

approved for 1.8.1.13, a=dveditz for release-drivers
Attachment #301955 - Flags: approval1.8.1.13? → approval1.8.1.13+
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.13
Resolution: --- → FIXED
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.
Keywords: fixed1.8.1.13 → verified1.8.1.13

Comment 29

10 years ago
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.
Attachment #311591 - Flags: review?(enndeakin)

Updated

10 years ago
Flags: blocking1.8.0.15+
Group: security
Comment on attachment 311591 [details] [diff] [review]
1.8.0 version of the patch

Looks ok
Attachment #311591 - Flags: review?(enndeakin) → review+
You need to log in before you can comment on or make changes to this bug.