77485 - function definition isn't subject to domain checks (exploit involves targeted javascript links)

Status: VERIFIED FIXED

(Core :: Security: CAPS, defect, P2)

Description

[Jesse Ruderman]

Using a link targeted to a frame at another domain, I can create a function in 
another domain and then call that function.  Once the function is at the other 
domain, it runs with the privileges of that domain regardless of who calls it.

There are two ways to exploit this hole:

1. Name the function focus(), blur(), or close().  These functions will be 
callable cross-domain once bug 65012 (focus/blur) and bug 62168 (close) are 
fixed.

2. Call the function as part of the same statement that defines the function.  
In this case, the function name doesn't matter (it can even be nameless).

Both exploits require getting the victim to click on a link targeted to another 
frame or window (which shouldn't be too hard).

The problem seems to be that there's no domain check for statements or 
expressions like 
 function [name] (...) { ... }.

Btw, IE doesn't allow javascript: links to be targeted to another domain.  
Netscape 4 and Mozilla do allow them, using the principal of the linking window 
and the scope of the target window.  I think that feature was added to Mozilla 
in bug 25821 (see Brendan Eich's 2000-10-31 19:52 comment).

Comment 1

[Jesse Ruderman]

Attachment: demonstration

Comment 2

[Mitchell Stoltz (not reading bugmail)]

Attachment: Patch - fixes the first testcase but not the second

Comment 3

[Mitchell Stoltz (not reading bugmail)]

The above patch fixes the first testcase - the function will run with the
principals of the attack page, not the target page. In the second testcase, I
think we have another how-to-get-principals issue. Tracing through
nsScriptSecurityManager::GetSubjectPrincipal, called from canAccess() on the
access to document.links, I see that the loop in GetPrincipalAndFrame skips the
innermost frame, as it should because this is an XPConnect call. On the second
iteration, the call to GetFunctionObjectPrincipal sees a cloned, non-native
function and calls GetObjectPrincipal accordingly. This seems to be wrong in
this case. GetObjectPrincipal looks at the function object's scope, which leads
to the target document (not what we want) since the function was defined in the
target scope. If we instead call GetScriptPrincipal, as we normally do for
functions that are not cloned, we get the attacking page's principal, under
which the function was compiled, which is what we want.

I'm not sure how to distinguish this case from the normal case of a cloned
function in which we don't want to call GetScriptPrincipal. Brendan, can you advise?

To reproduce the bug, load Jesse's testcase, click the first link on the page to
load the target page, then click the last link on the page. You may have to
apply my patch to see the behavior I just described.

Comment 4

[Johnny Stenback (:jst)]

r=jst for the attached patch.

Comment 5

[Brendan Eich [:brendan]]

mstoltz: we need a flag in our principal base class that says "if I'm the
principal of a cloned function object's script -- a compile-time subject
principal, IOW -- use the clone's scope chain to find a global object with the
right principal when backtracing through an activation of the clone".

There is no other way to distinguish the XUL/XBL case (where we want to set this
flag) from the HTML one (where the flag is clear by default initialization).

So we'll need to whack the XUL script-compiling code, too.  Should we always set
this flag on the system principal?

/be

Comment 6

[Brendan Eich [:brendan]]

I didn't know we allowed cross-domain calls to focus, blur, etc.  Can we insist
that those name native methods before allowing conditional access?  That would
beef up our story a bit.

/be

Comment 7

[Brendan Eich [:brendan]]

On the patch: don't comment out code with C-style comments, do remove it if it's
extremely likely to be gone (CVS will remember), or use #if 0 and an XXX comment
if you're 50-50 on its going away for good.

@@ -3800,7 +3835,7 @@
             return NS_OK;
         }
     }
-
+/*
     nsCOMPtr<nsISupports> owner(aOwner);
     //
     // Check to see if an owner should be inherited...
@@ -3835,7 +3870,7 @@
             GetCurrentDocumentOwner(getter_AddRefs(owner));
         }
     }
-
+*/

/be

Comment 8

[Mitchell Stoltz (not reading bugmail)]

I'm not sure if it's safe to set the flag you describe on the principal class,
since principals are sometimes shared. Any two requests for the principal for a
given URL will return a pointer to the same object under certain circimstances.
Did you mean to put that flag on the JSPrincipals struct? I think those aren't
shared (but maybe you know differently).

If I understand correcly, you're saying we should set a flag on the principal
that tells us whether or not to get principals from the function's scope if the
function is cloned? Do you know where in the XUL/XBL code this flag needs to be set?

Comment 9

[Mitchell Stoltz (not reading bugmail)]

Attachment: The rest of the fix - only use scope principals if function principal is system.

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Attachment: The whole fix, with some corrections.

Comment 11

[Mitchell Stoltz (not reading bugmail)]

Attachment: Cleaned up the securitymanager part

Comment 12

[Johnny Stenback (:jst)]

sr=jst & r=vidur

Comment 13

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to 0.9.1 on behalf of drivers.  

Comment 14

[chris hofmann]

lets not make up a new process... when the fix gets checked in
lets mark the bugs fixed, and reopen if the fix doesn't work.
marking bugs fixed gets them on the regression testing radar. 
  ckritzer, can you jump on this in build the builds from
wed. morning?

thanks

Comment 15

[chris hofmann]

are there other areas outside the demostration test case that should 
get a good shaking out?

Comment 16

[ckritzer (gone)]

Jesse took a look at this with me on today's builds, and there's still some odd 
things going on.  For example, the function can be run in the location field, 
and will show the number of links on Amazon's page (this would be bad).  
However, we do seem to be checking for domain and denying cross domain from the 
originating page.

Comment 17

[ckritzer (gone)]

I'm updating the Platform/OS to ALL/ALL, unless Jesse objects.

Comment 18

[Mitchell Stoltz (not reading bugmail)]

Despite the fact that there's still some odd behavior, I suggest we take this
off the 0.9.1 radar and fix this more comprehensively for 0.9.2. The most
serious exploit has been fixed. Chris, Jesse, do you think this is OK, or is
this serious enough to hold up 0.9.1?

Comment 19

[ckritzer (gone)]

I'll defer judgement to Jesse, as it is his bug, and he better understands the 
workings as such.

Jesse?

Comment 20

[Mitchell Stoltz (not reading bugmail)]

Jesse thinks this is not a beta stopper.

Comment 21

[ckritzer (gone)]

Right, so after speaking with Mitch, we're keeping this open for now, and 
targeting it for 0.9.2.

Comment 22

[Jesse Ruderman]

Attachment: demo: can still set function that reports its parameters to attacker

Comment 23

[Jesse Ruderman]

Reopening.  A malicious page can no longer run code with the principal of
another domain, but it can still replace a function in another domain with one
that reports its parameters to the attacker.  Cross-domain function definition
should be blocked, just like cross-domain variable definition is blocked.

Comment 24

[Mitchell Stoltz (not reading bugmail)]

Adding nsBranch keyword as these bugs need to get into RTM.

Comment 25

[Johnny Stenback (:jst)]

Attachment: Do security checks in the AddProperty and DelProperty hooks

Comment 26

[Mitchell Stoltz (not reading bugmail)]

There are two bad things happening: one is that the same-origin check isn't
happening when we replace a function property in another domain using |function
foo() { ... }|. The other is that targeted javascript: links can be used to
insert JS code into a window with content from another domain. Fixing either one
of these problems prevents this particular exploit, but fixing them both
probably fixes some other exploits we don't know about, and generally makes
things safer. jst's patch above fixes the first problem, and the patch I'm about
to post fixes the second, and I'd like to see them both get onto the branch.

r=mstoltz on jst's patch.

Comment 27

[Mitchell Stoltz (not reading bugmail)]

Attachment: block cross-host targeted javascript: links from running

Comment 28

[Mitchell Stoltz (not reading bugmail)]

A bit of explanation: the last patch compares the principal of the script about
to be run (the body of the JS URL) with the principal of the window it will be
run in. In order to get that second bit of data, I had to add
nsScriptSecurityManager::GetObjectPrincipal() to the public interface, hence the
IDL change.

Comment 29

[harishd]

1) Double check for \t
2) Please break if(xxx) yyy; into two lines i.e. 
   if(xxx){
     yyy;
   }
3) Make sure to null check ptrs ( global & scriptContext )
4) If the return value is not important then there is no need for an assignment.
That is, 
nsCOMPtr<nsIPrincipal> systemPrincipal;
+ rv = securityManager->GetSystemPrincipal(getter_AddRefs(systemPrincipal));
  
Do you care for rv here?

5) Replace NS_FAILED(objectPrincipal->Equals(principal, &equals)) with
   nsresult res = objectPrincipal->Equals(principal, &equals);
   NS_FAILED(res)


r=harishd

Comment 30

[Johnny Stenback (:jst)]

What Harish said sr=jst.

Michell, once this goes into the branch you'll haveto hand apply this patch
since the JS protocol handler is quite a different place on the branch, but the
same change should work just as well there.

Comment 31

[Mitchell Stoltz (not reading bugmail)]

Checked in on the trunk; the changes should be in today's dailies. I'd like to
get this in on the branch today if possible.

Comment 32

[Prashant Desale]

Attaching Testcase to test if Javascript URL's are working fine or not.
Tom asked me to test JS URL's for this one. So just thought I'll attach one 
testcase to test JS URL's here & everyone can use it to test if JS URL's are 
working fine or not.
Simple testcase is following. 

Comment 33

[Prashant Desale]

Attachment: JS URL's testcase

Comment 34

[Mitchell Stoltz (not reading bugmail)]

Attachment: Localize the security warning just added to nsJSProtocolHandler

Comment 35

[Mitchell Stoltz (not reading bugmail)]

The patch I just added makes the JS-consle warning I just added to
nsJSProtocolHandler.cpp properly localizable. The patch contains the new
manifest and resource files I added to do this, and a change in build/mac which
should build the locale jar on the Mac. I have not tested the Mac part yet; it
works fine on WinNT. Neither this nor the previous two patches have been checked
into the branch; after everything checks out OK on the trunk, we can combine
them into one patch for the branch.

Johnny, is it OK to add a jar.mn file and a resources directory to the dom
directory?

Comment 36

[Johnny Stenback (:jst)]

Fine with me, sr=jst

Comment 37

[rubydoo123]

Looks like we have patch, r=, and sr=, can this be landed today?

Comment 38

[Nisheeth Ranjan]

yup, I am working on integrating this into the branch as I type.  Mitch is away
on vacation today and tomorrow.

Comment 39

[Prashant Desale]

I tested javascript URL's with fridays trunk builds [2001-07-13-07-trunk] on 
win-2000.
I tested it with testcase I attached earlier, & javascript URL's seem to be 
working fine on that builds.

Comment 40

[Nisheeth Ranjan]

Attachment: Patch that got checked into the 6.1 branch

Comment 41

[Nisheeth Ranjan]

I just checked in http://bugzilla.mozilla.org/showattachment.cgi?attach_id=42486
into the 6.1 branch.

Mitch, I decided against checking in the localization fix because we haven't
tested it on the trunk and it involves build changes.  I don't want to risk
breaking any branch build tonight, the night before we make the rtm candidate
builds.  Since we have other JS security error messages that aren't localized,
this change isn't critical for 6.1.

I've left this bug open so that you can check in the localization patch
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=42249) on the trunk. 
Lets also try to localize the other security warnings in time for 0.9.3.

Comment 42

[Nisheeth Ranjan]

Removing PDT+ marker now that the fix is on the branch so that this gets off the
PDT radar.

Comment 43

[Mitchell Stoltz (not reading bugmail)]

Clearing nsBranch.

Comment 44

[Mitchell Stoltz (not reading bugmail)]

Target is now 0.9.4, Priority P2.

Comment 45

[Mitchell Stoltz (not reading bugmail)]

Moving to 0.9.5.

Comment 46

[Mitchell Stoltz (not reading bugmail)]

time marches on. Retargeting to 0.9.6.

Comment 47

[Mitchell Stoltz (not reading bugmail)]

Moving the most time-critical bugs and minor security fixes to 0.9.7

Comment 48

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 49

[Jesse Ruderman]

Posted bug 129814 for the left-over localization patch, marking this bug as fixed.

Comment 50

[basic]

ok this is resolved, but what's with the mozilla0.9.3 target????

Comment 51

[Jesse Ruderman]

Basic: 0.9.3 was a guess based on the date this bug was fixed.

Comment 52

[bsharma]

Verified on 2002-04-02-Trunk build on WinNT.

1. Loaded 2 test cases of Jesse. Seem to perform fine.
2. Loaded test case provided by Prashant. Seem to work fine.