Closed Bug 118657 Opened 23 years ago Closed 21 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: 21 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: