Closed Bug 118657 Opened 23 years ago Closed 22 years ago

Bypassing same-origin by brute-force find()

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.5alpha

People

(Reporter: security-bugs, Assigned: sfraser_bugs)

Details

(Whiteboard: [sg:mustfix])

Attachments

(6 files, 3 obsolete files)

Untested patch
3.50 KB, patch
security-bugs
: review+
jst
: superreview+
Details | Diff | Splinter Review
Patch to nsGlobalWindow to set the current search frame
2.23 KB, patch
security-bugs
: review+
jst
: superreview+
Details | Diff | Splinter Review
Sample file for testcase
277 bytes, text/html
Details
Testcase frame, with JS that calls window.parent.find()
769 bytes, text/html
Details
Testcase frameset -- lood this to run the test
202 bytes, text/html
Details
second try for frameset testcase
210 bytes, text/html
Details
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.
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
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 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]
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
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?
Taking.
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.
Status: NEW → ASSIGNED
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 patch — — Splinter 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 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+
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!
Status: ASSIGNED → RESOLVED
Closed: 22 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.

Attachment

General

Creator:
Created:
Updated:
Size: