Last Comment Bug 118657 - Bypassing same-origin by brute-force find()
: Bypassing same-origin by brute-force find()
Status: RESOLVED FIXED
[sg:mustfix]
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.5alpha
Assigned To: Simon Fraser
: bsharma
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-01-07 16:15 PST by Mitchell Stoltz (not reading bugmail)
Modified: 2004-07-20 04:49 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
findfrsearch2.html - part of the demonstration (943 bytes, text/html)
2002-01-07 16:17 PST, Mitchell Stoltz (not reading bugmail)
no flags Details
findfr2.html - load this to see the demonstration (caution - very slow) (169 bytes, text/html)
2002-01-07 16:18 PST, Mitchell Stoltz (not reading bugmail)
no flags Details
Untested patch (3.50 KB, patch)
2003-03-17 19:02 PST, Simon Fraser
security-bugs: review+
jst: superreview+
Details | Diff | Splinter Review
Patch to nsGlobalWindow to set the current search frame (2.23 KB, patch)
2003-04-16 12:17 PDT, Simon Fraser
security-bugs: review+
jst: superreview+
Details | Diff | Splinter Review
Sample file for testcase (277 bytes, text/html)
2003-04-16 12:18 PDT, Simon Fraser
no flags Details
Testcase frame, with JS that calls window.parent.find() (769 bytes, text/html)
2003-04-16 12:19 PDT, Simon Fraser
no flags Details
Testcase frameset -- lood this to run the test (202 bytes, text/html)
2003-04-16 12:21 PDT, Simon Fraser
no flags Details
testcase frameset with remote left frame (171 bytes, text/html)
2003-04-16 14:47 PDT, Simon Fraser
no flags Details
second try for frameset testcase (210 bytes, text/html)
2003-04-16 14:48 PDT, Simon Fraser
no flags Details

Description Mitchell Stoltz (not reading bugmail) 2002-01-07 16:15:33 PST
With the help of window.find(), frames and brute forcing it is
possible to read the source of arbitrary documents - especially these
on intranet servers.
A disadvantage of this is because of the bruteforcing it is *really* slow,
but anyway it is a bug.
Examine the attached file for demonstration against www.mozilla.org

Suggest disallowing window.find() across frames from scripts.
Comment 1 Mitchell Stoltz (not reading bugmail) 2002-01-07 16:17:06 PST
Created attachment 63892 [details]
findfrsearch2.html - part of the demonstration
Comment 2 Mitchell Stoltz (not reading bugmail) 2002-01-07 16:18:57 PST
Created attachment 63894 [details]
findfr2.html - load this to see the demonstration (caution - very slow)
Comment 3 Mitchell Stoltz (not reading bugmail) 2002-01-07 17:17:30 PST
fun fun fun. Looks like we'll have to change the find function (implemented in
docshell I think) so that it doesn't iterate into subframes from different
hosts, if the find was initiated by an unprivileged script. That could be
complicated.

Probably not a huge risk right now, because the time necessary to actually read
a whole page by this method is prohibitive; would-be victims will probably kill
their browser before the script finishes. This could be used more simply, for
example to see if a user is on a specific page by searching for key words, but
since the target page must be loaded in a frame this presents difficulties as
well. Essentially, while each find() call reveals a bit of information about a
page, it's a small amount of information only.
Comment 4 Akkana Peck 2002-06-18 16:12:53 PDT
The place to look is in nsWebBrowserFind::FindNext -- all the
iterate-through-frames code is there.

The check would probably go somewhere near the GetNext call at line 168 of
nsWebBrowserFind.cpp, and then again around the GetNext call at line 213.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-09-16 13:39:46 PDT
Since this is a pretty impractical attack, can't we open the bug?
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-10-12 19:31:07 PDT
Actually, this is fairly serious. Suppose there is a site blah.com where some
URL, say https://blah.com/login, gives a page containing the user's name, e.g.,
"Hello Robert O'Callahan!". Then you can reverse engineer the name using an
average of 13x(length of name) find's. (E.g. search for "Hello, A" ... "Hello,
Z" then "Hello Ra" ... "Hello, Rz", etc.) That's not at all unrealistic.
Comment 7 Mitchell Stoltz (not reading bugmail) 2002-10-31 15:23:55 PST
Over to akkana. I think we should make it so that find() does not recurse into
subframes containing pages from different hosts when called from a script. When
find() is called from the menu, of course, it should behave as normal.
Comment 8 Mitchell Stoltz (not reading bugmail) 2002-11-25 16:18:20 PST
Akkana, have you had a chance to look at this bug?
Comment 9 Akkana Peck 2002-11-25 17:06:58 PST
Sorry, no, but I can look at it now.  Any hints as to how to tell when we're
being called from a script?
Comment 10 chris hofmann 2003-02-11 15:52:43 PST
Kin of Simon, can you pick up the investigation on this one?
Comment 11 Simon Fraser 2003-02-11 15:56:39 PST
Taking.
Comment 12 Mitchell Stoltz (not reading bugmail) 2003-02-14 15:33:28 PST
Simon, have you had a chance to look at this bug? Please set a target milestone
(preferably in 1.4).
Comment 13 Simon Fraser 2003-02-14 16:29:04 PST
Will take a look.
Comment 14 Simon Fraser 2003-03-17 17:06:40 PST
I think the real bug here is that we can call a [noscript] method on
nsIDOMWindowInternal from JS. Is this just because it happens to have the same
name as the Find method on nsIDOMJSWindow?

Maybe we should rename the nsIDOMWindowInternal method.
Comment 15 Simon Fraser 2003-03-17 17:32:52 PST
Never mind. The nsIDOMJSWindow::Find impl grabs params from JS anyway, by design.
Comment 16 Simon Fraser 2003-03-17 19:01:38 PST
I can't get the testcase to work in current builds. At the end, the alert just
contains "<html>" so it looks like nothing was found.
Comment 17 Simon Fraser 2003-03-17 19:02:39 PST
Created attachment 117545 [details] [diff] [review]
Untested patch

Here's what I think a patch would look like, but since I'm unable to reproduce
the problem, I'm not sure it works.
Comment 18 georgi - hopefully not receiving bugspam 2003-03-18 02:26:23 PST
I also can't reproduce this now.
Shall check modifications.
Comment 19 georgi - hopefully not receiving bugspam 2003-04-04 03:27:34 PST
Per Mitch's request I tested this again.
Can't reproduce it anymore.
Checked for modifications few weeks ago, couldn't find an exploit.
Shall check for more modifications.
Comment 20 Mitchell Stoltz (not reading bugmail) 2003-04-10 16:57:15 PDT
Simon, you mentioned that find() no longer iterates into frames when called from
js. Is this because of a bug that will be fixed, or is this by design? If the
former, we need a permanent fix for 1.4b.
Comment 21 Simon Fraser 2003-04-16 12:14:33 PDT
There are a couple of issues going on here. First, the testcase doesn't work
correctly because of an issue with the current search frame. I'll attach a
patch, and a better testcase.
Comment 22 Simon Fraser 2003-04-16 12:17:28 PDT
Created attachment 120729 [details] [diff] [review]
Patch to nsGlobalWindow  to set the current search frame

The find init code uses the focussed frame as the default current search frame
(which is what you want for XUL). This patch makes the global window code set
the current search frame to the window on which .find() was called. This makes
the testcase work better (i.e. we are more vulnerable), but is the correct
thing to do.
Comment 23 Simon Fraser 2003-04-16 12:18:31 PDT
Created attachment 120730 [details]
Sample file for testcase
Comment 24 Simon Fraser 2003-04-16 12:19:54 PDT
Created attachment 120731 [details]
Testcase frame, with JS that calls window.parent.find()
Comment 25 Simon Fraser 2003-04-16 12:21:02 PDT
Created attachment 120733 [details]
Testcase frameset -- lood this to run the test
Comment 26 Simon Fraser 2003-04-16 14:47:46 PDT
Created attachment 120758 [details]
testcase frameset with remote left frame
Comment 27 Simon Fraser 2003-04-16 14:48:51 PDT
Created attachment 120759 [details]
second try for frameset testcase
Comment 28 Simon Fraser 2003-04-16 14:51:54 PDT
Comment on attachment 117545 [details] [diff] [review]
Untested patch

I've now tested this patch, using the testcases attached, and verified that it
correctly prevents access to frames from a different host.

The reason the testcase failed to work is that the Find code failed a same
origin check in nsRnage code, which is used internally by the Find
implmenetation.
Comment 29 Johnny Stenback (:jst, jst@mozilla.com) 2003-04-16 18:44:51 PDT
Comment on attachment 117545 [details] [diff] [review]
Untested patch

- In nsWebBrowserFind::FindNext():

+    // Get JSContext from stack
+    nsCOMPtr<nsIJSContextStack> stack =
do_GetService("@mozilla.org/js/xpc/ContextStack;1");
+    NS_ENSURE_TRUE(stack, NS_ERROR_FAILURE);
+
+    JSContext *cx = nsnull;
+    rv = stack->Peek(&cx);
+    NS_ENSURE_SUCCESS(rv, rv);
+    if (!cx)
+      return NS_ERROR_FAILURE;
+

A null context on the JS context stack just means that there's no JS running at
the moment, so you shouldn't return an error just because there's no JS
running. In stead, you should let the caller do the find, since there's nothing
to protect against if there's no JS on the stack.

And you can even remove the above code, you can pass null as the context to the
script security manager and it will do the above for you, no need to duplicate
that code here. Oh, and if you do that, no need to #include
"nsIJSContextStack.h"

sr=jst with that change.
Comment 30 Johnny Stenback (:jst, jst@mozilla.com) 2003-04-16 18:46:07 PDT
Comment on attachment 120729 [details] [diff] [review]
Patch to nsGlobalWindow  to set the current search frame

+  nsCOMPtr<nsIWebBrowserFindInFrames> framesFinder(do_QueryInterface(finder));
+  framesFinder->SetRootSearchFrame(this);   // paranoia
+  framesFinder->SetCurrentSearchFrame(this);

Should you null-check |framesFinder|, or do we not care?

sr=jst
Comment 31 Mitchell Stoltz (not reading bugmail) 2003-04-18 11:04:34 PDT
Comment on attachment 120729 [details] [diff] [review]
Patch to nsGlobalWindow  to set the current search frame

Patch 2 ooks good, r=mstoltz.
Comment 32 Mitchell Stoltz (not reading bugmail) 2003-04-18 11:23:09 PDT
Comment on attachment 117545 [details] [diff] [review]
Untested patch

+    rv = secMan->CheckSameOrigin(cx, docURI);
+    NS_ENSURE_SUCCESS(rv, rv);

NS_ENSURE_SUCCESS does an assertion when it fails, doesn't it? As it's
completely legitimate for this check to return failure (meaning different
origins) we should not assert in this  case, but just return.

Ditto on jst's suggestions. With those changes, r=mstoltz.
Comment 33 Johnny Stenback (:jst, jst@mozilla.com) 2003-04-18 13:26:22 PDT
NS_ENSURE_SUCCESS does a warning if it fails, and you're right Mitch, we don't
need that for this security check. if (NS_FAILED(rv)) return rv; is all we need.
Comment 34 Simon Fraser 2003-04-18 17:54:17 PDT
Checked in with comments addressed. Thanks all!
Comment 35 Daniel Veditz [:dveditz] 2004-07-20 04:49:16 PDT
Bugs published on the Known-vulnerabilities page long ago, removing confidential
flag.

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