Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Bypassing same-origin by brute-force find()

RESOLVED FIXED in mozilla1.5alpha

Status

()

Core
Security
RESOLVED FIXED
16 years ago
13 years ago

People

(Reporter: Mitchell Stoltz (not reading bugmail), Assigned: Simon Fraser)

Tracking

Trunk
mozilla1.5alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:mustfix])

Attachments

(6 attachments, 3 obsolete attachments)

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.
(Reporter)

Comment 1

16 years ago
Created attachment 63892 [details]
findfrsearch2.html - part of the demonstration
(Reporter)

Comment 2

16 years ago
Created attachment 63894 [details]
findfr2.html - load this to see the demonstration (caution - very slow)
(Reporter)

Comment 3

16 years ago
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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.0.1

Comment 4

15 years ago
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.
(Reporter)

Updated

15 years ago
Target Milestone: mozilla1.0.1 → mozilla1.2alpha
Since this is a pretty impractical attack, can't we open the bug?
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.
Whiteboard: [sg:mustfix]
(Reporter)

Comment 7

15 years ago
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.
Assignee: mstoltz → akkana
Status: ASSIGNED → NEW

Updated

15 years ago
Target Milestone: mozilla1.2alpha → mozilla1.3alpha
(Reporter)

Comment 8

15 years ago
Akkana, have you had a chance to look at this bug?

Comment 9

15 years ago
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

15 years ago
Kin of Simon, can you pick up the investigation on this one?
(Assignee)

Comment 11

15 years ago
Taking.
Assignee: akkana → sfraser
(Reporter)

Comment 12

15 years ago
Simon, have you had a chance to look at this bug? Please set a target milestone
(preferably in 1.4).
(Assignee)

Comment 13

15 years ago
Will take a look.
Status: NEW → ASSIGNED
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: mozilla1.3alpha → mozilla1.4alpha
(Assignee)

Comment 14

15 years ago
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.
(Assignee)

Comment 15

15 years ago
Never mind. The nsIDOMJSWindow::Find impl grabs params from JS anyway, by design.
(Assignee)

Comment 16

15 years ago
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.
(Assignee)

Comment 17

15 years ago
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.
I also can't reproduce this now.
Shall check modifications.
(Assignee)

Updated

15 years ago
Target Milestone: mozilla1.4alpha → mozilla1.5alpha
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.
(Reporter)

Comment 20

15 years ago
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.
(Assignee)

Comment 21

15 years ago
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.
(Assignee)

Comment 22

15 years ago
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.
(Assignee)

Comment 23

15 years ago
Created attachment 120730 [details]
Sample file for testcase
Attachment #63892 - Attachment is obsolete: true
Attachment #63894 - Attachment is obsolete: true
(Assignee)

Comment 24

15 years ago
Created attachment 120731 [details]
Testcase frame, with JS that calls window.parent.find()
(Assignee)

Comment 25

15 years ago
Created attachment 120733 [details]
Testcase frameset -- lood this to run the test
(Assignee)

Comment 26

15 years ago
Created attachment 120758 [details]
testcase frameset with remote left frame
(Assignee)

Comment 27

15 years ago
Created attachment 120759 [details]
second try for frameset testcase
Attachment #120758 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #120729 - Flags: superreview?(jst)
Attachment #120729 - Flags: review?(mstoltz)
(Assignee)

Comment 28

15 years ago
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.
Attachment #117545 - Flags: superreview?(jst)
Attachment #117545 - Flags: review?(mstoltz)
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.
Attachment #117545 - Flags: superreview?(jst) → superreview+
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
Attachment #120729 - Flags: superreview?(jst) → superreview+
(Reporter)

Comment 31

15 years ago
Comment on attachment 120729 [details] [diff] [review]
Patch to nsGlobalWindow  to set the current search frame

Patch 2 ooks good, r=mstoltz.
Attachment #120729 - Flags: review?(mstoltz) → review+
(Reporter)

Comment 32

15 years ago
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.
Attachment #117545 - Flags: review?(mstoltz) → review+
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.
(Assignee)

Comment 34

15 years ago
Checked in with comments addressed. Thanks all!
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Bugs published on the Known-vulnerabilities page long ago, removing confidential
flag.
Group: security
You need to log in before you can comment on or make changes to this bug.