370461 - __proto__ of a function returned by Components.lookupMethod() comes from content

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P3)

Description

[moz_bug_r_a4]

Attachment: testcase

This is similar to bug 370127.

Since __proto__ of a function returned by Components.lookupMethod() comes from
content, it is unsafe to use properties and methods of that function.  If
chrome code calls .call() or .apply() on that function then content code can
run arbitrary code with chrome privileges by using Array.prototype methods
trick (bug 344495).

Bug 344495's trick is not required in some cases.  For example:

 chrome:
  var d = content.document;
  var s = Components.lookupMethod(d, "title").call(d);
  s = s.substring(s.indexOf(" ") + 1);

 content:
  var s = {
    substring : privileged_eval,
    indexOf : function() { return code + "//"; }
  };
  Function.prototype.call = function() { return s; };

Using Components.lookupMethod() with .call() or .apply() might be prevailing
usage, since there are many such examples in 1.7 branch code, including
XPCNativeWrapper.js (this means fx1.0.x and moz1.7.x are exploitable without
any extensions).

But, I'm not sure if there are any vulnerable extensions on addons.mozilla.org.

Comment 1

[Johnny Stenback (:jst)]

Blake, this is more than likely the same problem as in bug 370127 I just gave you.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

This turned out to be more difficult than bug 370127 because the offending functions are compiled way before lookupMethod ever gets to them. I'm not sure what the best fix for this is. We could potentially return a brand new wrapper function that does the right thing (really, it'd just call the inner function with the same |this| object and parameters) or try to force the prototype to be something else. Does that sound reasonable, or am I missing an easier option (that doesn't involve the word "wrapper!"

Comment 3

[Damon Sicore (:damons)]

Targeting A6 per conversation with Blake.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

I was hoping that brendan's recent JS_CloneFunctionObject changes were going to fix this, but we clone with the "wrong" parent, so we still get the wrong prototype, which suggests a fix like the one in bug 370127 -- but I'm going to need to think about it a little harder.

Comment 5

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do we know of any places in 1.9 code where Components.lookupMethod is used in an exploitable way? If so this might be a higher priority.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Not in Gecko per se.  Camino does it. Some extensions might too (though I hope not).

Comment 8

[Daniel Veditz [:dveditz]]

I can no long reproduce in FF2.0.0.14 or recent trunk, I get "invalid setter usage" errors. Did we fix this, or merely break this particular testcase?

Comment 9

[Daniel Veditz [:dveditz]]

We killed indirect eval on both trunk and branch and the setter in the testcase is trying to use <Marquee>.init.eval

That doesn't fix the underlying problem and there is surely another way to abuse it.

Comment 10

[moz_bug_r_a4]

Attachment: testcase 2

This uses location setter instead of eval.  (This uses Bug 344495's trick.)

This works on trunk and fx2.0.0.14.

Comment 11

[Daniel Veditz [:dveditz]]

If any chrome code uses this feature on content then this is sg:critical

Comment 12

[Samuel Sidler (old account; do not CC)]

This should really block 1.9.1.

Comment 13

[Samuel Sidler (old account; do not CC)]

Blake, how's this patch (or patches) coming?

Comment 14

[Daniel Veditz [:dveditz]]

This will need trunk baking, so pushing out to next releases

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Isn't this sg:critical since it allows full chrome access?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking blocking- per the new security-bugs-fixing strategy

Comment 17

[Brandon Sterne (:bsterne)]

Renominating since we're placing this on the Top Security Bugs list.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

There's a sad lack of code-sharing here, but all of our wrappers do things *ever so slightly* different. I call XPCNativeWrapperCtor directly here to ensure that we don't hand out implicit native wrappers by accident. Given how ancient this API is (and that nobody should be using it anymore anyway) I'm OK with expandos on any returned XPCNativeWrappers disappearing.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 345418 [details] [diff] [review]
Proposed fix

r=bzbarsky

Comment 20

[Robert Sayre]

Marking fixed:
http://hg.mozilla.org/mozilla-central/rev/ef515839d49b

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for the 1.9 branch

This conflicted because of JS_STATIC_DLL_CALLBACK removal on trunk.

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch for the 1.8 branch

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 348118 [details] [diff] [review]
Patch for the 1.9 branch

Approved for 1.9.0.5, a=dveditz for release-drivers

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 348122 [details] [diff] [review]
Patch for the 1.8 branch

Approved for 1.8.1.19, a=dveditz for release-drivers

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

Fixed in CVS.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.8.1.19 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19pre) Gecko/2008112503 BonEcho/2.0.0.19pre. 

Verified for 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008112505 GranParadiso/3.0.5pre. 

Verified for Trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081123 Minefield/3.1b2pre.

Comment 27

[Alexander Sack]

Comment on attachment 348122 [details] [diff] [review]
Patch for the 1.8 branch

a=asac for 1.8.0 branch