Closed Bug 118657 Opened 18 years ago Closed 17 years ago

Bypassing same-origin by brute-force find()


(Core :: Security, defect)

Not set





(Reporter: security-bugs, Assigned: sfraser_bugs)


(Whiteboard: [sg:mustfix])


(6 files, 3 obsolete files)

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

Suggest disallowing window.find() across frames from scripts.
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

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.
Target Milestone: --- → mozilla1.0.1
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.
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 where some
URL, say, 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]
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
Target Milestone: mozilla1.2alpha → mozilla1.3alpha
Akkana, have you had a chance to look at this bug?
Sorry, no, but I can look at it now.  Any hints as to how to tell when we're
being called from a script?
Kin of Simon, can you pick up the investigation on this one?
Assignee: akkana → sfraser
Simon, have you had a chance to look at this bug? Please set a target milestone
(preferably in 1.4).
Will take a look.
OS: Windows 2000 → All
Hardware: PC → All
Target Milestone: mozilla1.3alpha → mozilla1.4alpha
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.
Never mind. The nsIDOMJSWindow::Find impl grabs params from JS anyway, by design.
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.
Attached patch Untested patchSplinter Review
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.
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.
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.
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.
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.
Attachment #63892 - Attachment is obsolete: true
Attachment #63894 - Attachment is obsolete: true
Attachment #120758 - Attachment is obsolete: true
Attachment #120729 - Flags: superreview?(jst)
Attachment #120729 - Flags: review?(mstoltz)
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
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 =
+    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

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?

Attachment #120729 - Flags: superreview?(jst) → superreview+
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+
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.
Checked in with comments addressed. Thanks all!
Closed: 17 years ago
Resolution: --- → FIXED
Bugs published on the Known-vulnerabilities page long ago, removing confidential
Group: security
You need to log in before you can comment on or make changes to this bug.