856042 - It's possible to bypass security wrappers by using mozContact

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[moz_bug_r_a4]

Chrome code that accesses non-waived objects can be abused in some cases.

For example:

function x(a) {
  return String(a);
}

If content can call a chrome function like this, content can get a result of toString method of an object whose toString method is not accessible.

mozContact.init accesses non-waived objects and does things that can be abused.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - bypass XOW

It's possible to get a result of a cross-origin window's location.toString().

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - bypass SOW

It's possible to get some properties of the native anonymous content of a file control.  (It's possible to get the full path from a file control on fx17-21, but not possible on trunk since bug 838675 changed the NAC of the file control.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

This is why we really need to fix bug 850430 and the like.  :(

Comment 4

[Bobby Holley (:bholley)]

Sigh. Yeah, I'd kind of imagined that this would be a problem. I'm actually just about to land a solution to this problem in bug 843829, but only for XBL scopes (which I think are safer compat-wise).

Bug 856067 is the long-term solution here, but that's a major behavior change that's a ways out, possible infeasible, and definitely not backportable.

In the mean time, the best solution is to audit the contacts API and make it as secure as possible from attacks like this.

Comment 5

[Bobby Holley (:bholley)]

Also, this bug should definitely be embargoed until bug 856067 is fixed.

Comment 6

[Bobby Holley (:bholley)]

Per comment 4, I think reuben or gregor would be the best assignee here.

Comment 7

[Bobby Holley (:bholley)]

As noted several times before, moz_bug_r_a4 is a contractor (with an @mozilla.com address).

Comment 8

[Reuben Morais [:reuben]]

Attachment: Only accept strings for the ContactProperties interfaces

We shouldn't be calling toString on those objects anyway. This patch rejects things that aren't strings in init. When writing the test for this I also had to fix test_contacts_basics.html (none of the checks involving checkStr were working before).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 731402 [details] [diff] [review]
Only accept strings for the ContactProperties interfaces

Note that this will prevent passing in String("foo")....  I'm not sure there's a better solution here for the current setup, though.

Comment 10

[Boris Zbarsky [:bzbarsky]]

On a more interesting note, what guarantees us that aArray[i] will return the same value during both the typeof check and the second time we do it?

Comment 11

[Reuben Morais [:reuben]]

Attachment: Only accept strings for the ContactProperties interfaces

(In reply to Boris Zbarsky (:bz) from comment #10)
> On a more interesting note, what guarantees us that aArray[i] will return
> the same value during both the typeof check and the second time we do it?

Nice catch! I added a test for it:

diff -u b/dom/contacts/ContactManager.js b/dom/contacts/ContactManager.js
--- b/dom/contacts/ContactManager.js
+++ b/dom/contacts/ContactManager.js
@@ -36,13 +36,11 @@
 function sanitizeStringArray(aArray) {
   let arr = [];
   if (Array.isArray(aArray)) {
-    for (let i = 0; i < aArray.length; ++i) {
-      if (typeof aArray[i] != "string") {
+    for (let i of aArray) {
+      if (typeof i != "string") {
         if (DEBUG) debug("Field is not a string and was ignored.");
-      } else {
-        if (aArray[i]) {
-          arr.push(aArray[i]);
-        }
+      } else if (i) {
+        arr.push(i);
       }
     }
     if (arr.length == 0) {
diff -u b/dom/contacts/tests/test_contacts_basics.html b/dom/contacts/tests/test_contacts_basics.html
--- b/dom/contacts/tests/test_contacts_basics.html
+++ b/dom/contacts/tests/test_contacts_basics.html
@@ -1221,12 +1221,11 @@
   },
   function () {
     ok(true, "Adding contact with invalid data");
-    createResult1 = new mozContact();
-    createResult1.init({
+    var obj = {
         name: [1, 2],
         familyName: 3,
         givenName: 4,
-        honorificPrefix: [5, 6],
+        honorificPrefix: [],
         honorificSuffix: {foo: "bar"},
         additionalName: 7,
         nickname: [8, 9],
@@ -1236,10 +1235,24 @@
         category: [15, 16],
         sex: 17,
         genderIdentity: 18
-    });
+    };
+    obj.honorificPrefix.__defineGetter__('0',(function() {
+      var c = 0;
+      return function() {
+        if (c == 0) {
+          c++;
+          return "string";
+        } else {
+          return {foo:"bar"};
+        }
+      }
+    })());
+    createResult1 = new mozContact();
+    createResult1.init(obj);
     req = mozContacts.save(createResult1);
     req.onsuccess = function () {
       checkContacts(createResult1, {
+        honorificPrefix: "string",
         sex: "17",
         genderIdentity: "18"
       });

Comment 12

[moz_bug_r_a4]

Er, I didn't explain what testcase 2 did.

this.email.push(new ContactField(aProp.email[i].type, aProp.email[i].value));

testcase 2 abuses this code to get type and value properties of a NAC, so it seems that the proposed patch does not fix testcase 2.

Comment 13

[Reuben Morais [:reuben]]

Attachment: Only accept strings for the ContactProperties interfaces, check ownProperties of passed objects

We *really* need to fix bug 850430 :(

So what I'm doing here is checking if at least one of the own properties of the passed object matches the interface we expect. I can't do a full match because we have optional properties, and content may want to pass their own duck typed objects.
I tried to bypass this by doing things like 

var tel = Object.create(node, {carrier: {writable:true, configurable:true, value: "val"}});
mc.init({ name : node, tel : tel });

or simply

node.carrier = "val";

And the security wrapper stopped me from extending the node object in all the cases I tried. Boris, JavaScript never ceases to surprise me, so can you check if there's some different way to extend a NAC in a way that bypasses my check?

Comment 14

[Daniel Veditz [:dveditz]]

sec-high for now, but combined with some way to grab a privileged frame could be sec-critical.

Comment 15

[Andrew McCreight [:mccr8]]

Is mozContact really web-exposed in desktop builds?  Can we disable it harder somehow?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 731565 [details] [diff] [review]
Only accept strings for the ContactProperties interfaces, check ownProperties of passed objects

So as discussed on IRC:

1)  sanitizeStringArray should not be stripping empty strings from the array.
2)  _checkOwnProperties makes no sense.  Better to check for "vanilla" objects
    via Object.prototype.toString.call().

That said, I'd like to understand the things we want to defend against here, really...  Are we ok with "email" having accessors? With "email.type" being an object with interesting toString and valueOf?  Or should we be doing stringOrBust on those somewhere?

Comment 17

[moz_bug_r_a4]

Also, we might need to prevent COWs from being bypassed.  Though, I'm not sure if there are any chrome objects that can be abused.  I'll attach a testcase for COW.

Comment 18

[moz_bug_r_a4]

Attachment: testcase 3 - bypass COW

Comment 19

[Gregor Wagner [:gwagner]]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> Is mozContact really web-exposed in desktop builds?  Can we disable it
> harder somehow?

You can do navigator.mozContacts in the desktop build but since it's not enabled on desktop it will return null.

Comment 20

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky (:bz) from comment #16)
> That said, I'd like to understand the things we want to defend against here,
> really...  Are we ok with "email" having accessors? With "email.type" being
> an object with interesting toString and valueOf?  Or should we be doing
> stringOrBust on those somewhere?

I was under the impression that because those properties are a DOMString in the IDL, that wouldn't be possible.

(In reply to Andrew McCreight [:mccr8] from comment #15)
> Is mozContact really web-exposed in desktop builds?  Can we disable it
> harder somehow?

The global constructor is enabled in desktop builds, regardless of the dom.mozContacts.enabled pref. One idea we discussed in IRC was to add a "pref=foo.bar" string in the manifest and change the parser to only add the category if the pref is true, but that doesn't allow we to change it for the test runners, since the manifest parser runs before the profile is loaded. I'll probably end up removing the category manually in the contacts code.

Comment 21

[Boris Zbarsky [:bzbarsky]]

> I was under the impression that because those properties are a DOMString in the IDL

Where does IDL come in here?  If there was IDL involved, how did we ever get our hands on raw objects?

Comment 22

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky (:bz) from comment #21)
> > I was under the impression that because those properties are a DOMString in the IDL
> 
> Where does IDL come in here?  If there was IDL involved, how did we ever get
> our hands on raw objects?

Nevermind, changing the type of the sex and genderIdentity properties only makes a difference in this case because it's passed directly as a DOMString in the ContactProperties object, but the adr, url, email, etc. fields are still jsvals.

Comment 23

[Gregor Wagner [:gwagner]]

(In reply to Gregor Wagner [:gwagner] from comment #19)
> (In reply to Andrew McCreight [:mccr8] from comment #15)
> > Is mozContact really web-exposed in desktop builds?  Can we disable it
> > harder somehow?
> 
> You can do navigator.mozContacts in the desktop build but since it's not
> enabled on desktop it will return null.

Oh you were talking about the mozContact constructor and not the mozContacts object on the navigator. Sorry for the confusion.

Comment 24

[Reuben Morais [:reuben]]

Attachment: Only accept strings and vanilla objects for the ContactProperties interfaces

Addressing bz's comments:
1) sanitizeStringArray doesn't remove empty strings
2) stringOrBust is called on everything that is supposed to be a DOMString.
3) Only vanilla objects are accepted for Contact(Tel|Address)?Field

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 734001 [details] [diff] [review]
Only accept strings and vanilla objects for the ContactProperties interfaces

>+        if (_isVanillaObj(email, ContactField)) {

Second arg is unused.  Should it be passed?

>+    this.bday =            (aProp.bday == undefined || aProp.bday == null) ? null : new Date(aProp.bday);

"aProp.bday" could have a non-idempotent getter.

>+    this.anniversary =     (aProp.anniversary == undefined || aProp.anniversary == null) ? null : new Date(aProp.anniversary);

Likewise for "aProp.anniversary".

Modulo those two nits, r=me on the sanitizing bit

Comment 26

[Reuben Morais [:reuben]]

(In reply to Boris Zbarsky (:bz) from comment #25)
Thanks!

> >+        if (_isVanillaObj(email, ContactField)) {
> 
> Second arg is unused.  Should it be passed?

Those are artifacts from a bad find-replace. Removed.

> >+    this.bday =            (aProp.bday == undefined || aProp.bday == null) ? null : new Date(aProp.bday);
> 
> "aProp.bday" could have a non-idempotent getter.
> 
> >+    this.anniversary =     (aProp.anniversary == undefined || aProp.anniversary == null) ? null : new Date(aProp.anniversary);
> 
> Likewise for "aProp.anniversary".

Does it matter in these cases?

Comment 27

[Boris Zbarsky [:bzbarsky]]

I don't know.  Maybe it doesn't.

Comment 28

[Gregor Wagner [:gwagner]]

Comment on attachment 734001 [details] [diff] [review]
Only accept strings and vanilla objects for the ContactProperties interfaces

Review of attachment 734001 [details] [diff] [review]:
-----------------------------------------------------------------

Thx!

::: dom/contacts/ContactManager.js
@@ +35,5 @@
> +
> +function sanitizeStringArray(aArray) {
> +  let arr = [];
> +  if (Array.isArray(aArray)) {
> +    for (let el of aArray) {

can we do
if (stringOrBust(el)) {
  arr.push(el);
}

@@ +309,5 @@
>      if (aProp.tel) {
>        aProp.tel = Array.isArray(aProp.tel) ? aProp.tel : [aProp.tel];
>        this.tel = new Array();
> +      for (let tel of aProp.tel) {
> +        if (_isVanillaObj(tel, ContactTelField)) {

I guess bz already caught this one...

::: dom/contacts/tests/test_contacts_basics.html
@@ +300,5 @@
>  
>      mozContacts.oncontactchange = function(event) {
>        is(event.contactID, createResult1.id, "Same contactID");
>        is(event.reason, "create", "Same reason");
> +      next();

Hm did this change? I thought we always called next here. Why this change?
Just make sure that we don't call next twice for a function and we don't end up in a strange state with random orange.

@@ +1259,5 @@
> +    req.onsuccess = function () {
> +      checkContacts(createResult1, {
> +        honorificPrefix: "string",
> +        sex: "17",
> +        genderIdentity: "18"

shouldn't we check that for example the url field is not set?

Comment 29

[Reuben Morais [:reuben]]

(In reply to Gregor Wagner [:gwagner] from comment #28)
> ::: dom/contacts/tests/test_contacts_basics.html
> @@ +300,5 @@
> >  
> >      mozContacts.oncontactchange = function(event) {
> >        is(event.contactID, createResult1.id, "Same contactID");
> >        is(event.reason, "create", "Same reason");
> > +      next();
> 
> Hm did this change? I thought we always called next here. Why this change?
> Just make sure that we don't call next twice for a function and we don't end
> up in a strange state with random orange.

While fixing the tests, I noticed that those checks were being called after that test called next(), because oncontactchange fires after the set onsuccess. The only difference with this change is that it doesn't mix output with the following test. I can remove it from this patch if you'd like.

Also, in order to fix the tests for this bug, I had to fix bug 852057 as well. I can either wait for it to land, or land it as part of this bug and close it as a dupe. What do you think?
 
> @@ +1259,5 @@
> > +    req.onsuccess = function () {
> > +      checkContacts(createResult1, {
> > +        honorificPrefix: "string",
> > +        sex: "17",
> > +        genderIdentity: "18"
> 
> shouldn't we check that for example the url field is not set?

We already do, because the passed object doesn't have the property set. That's why I had to change the tests to return true when comparing [] and undefined.

Comment 30

[Reuben Morais [:reuben]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b98b24f28623

Comment 31

[Andrew McCreight [:mccr8]]

In the future, you should request sec-approval before landing patches for bugs that apply to more than just trunk.

Comment 32

[Reuben Morais [:reuben]]

(In reply to Andrew McCreight [:mccr8] from comment #31)
> In the future, you should request sec-approval before landing patches for
> bugs that apply to more than just trunk.

Ah, I didn't know about that! :( Should I still request it? I don't see that flag anywhere in the page…

Comment 33

[Andrew McCreight [:mccr8]]

(In reply to Reuben Morais [:reuben] from comment #32)
> Ah, I didn't know about that! :(
That's okay, it happens.  We're still early in the cycle so it doesn't really matter.

> Should I still request it?
Nah, once a patch is landed it doesn't matter any more.

> I don't see that flag anywhere in the page…
It is a flag on the patch itself, like r?.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b98b24f28623

Comment 35

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Daniel Veditz [:dveditz] from comment #14)
> sec-high for now, but combined with some way to grab a privileged frame
> could be sec-critical.

Note that this bug does NOT require the mozContact permission to be exploited. The permission only controls access to mozContact.save(), .read() etc. Any page can create a mozContact object.

Alex, per my email, I think this bug should be uplifted to 1.0.1 if possible. (tef?'ing is still the correct nomination?)

Comment 36

[Ryan VanderMeulen [:RyanVM]]

This will need a branch-specific patch for the b2g18 uplifts.

Comment 37

[Reuben Morais [:reuben]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ab553a6a2493
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/0da545a83edd

Comment 38

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

Was Firefox 22 affected by this? I see mention of 17-21 but not 22 so it isn't 100% clear to me.

Comment 39

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

Potentially mistagged by me.

To clarify, this reads like something that does NOT affect Desktop Firefox and only affect 1.0.0 b2g (which we didn't ship to customers). 

Can this be exploited on desktop firefox and in what version(s), if it can be.

Comment 40

[Reuben Morais [:reuben]]

(In reply to Al Billings [:abillings] from comment #39)
> Can this be exploited on desktop firefox and in what version(s), if it can
> be.

I don't have access to all the other bugs mentioned here, so I can't say if the same type of problem is being exploited elsewhere, but we disabled mozContacts on desktop, so yes, it doesn't affect it.

Comment 41

[moz_bug_r_a4]

Firefox 22 is affected. (The mozContact constructor is disabled in Firefox >= 23.)

Also, Firefox 17.0.8esr is affected. (I didn't notice that status-firefox-esr17 has been set to unaffected until today.)

I'll attach a modified version of testcase 1 because now blog.mozilla.org sends X-Frame-Options: SAMEORIGIN.

Comment 42

[moz_bug_r_a4]

Attachment: testcase 1.0.1 (changed the framed URL) - bypass XOW

Comment 43

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

So this was effectively fixed in 23 by disabling mozContact for Desktop. Great.

Comment 44

[Lukas Blakk [:lsblakk] use ?needinfo]

Sounds like we should disable mozContact on ESR branch then to close the loop here.  Can someone create/nominate a patch for that?

Comment 45

[Reuben Morais [:reuben]]

I don't have an ESR17 tree (and am currently on a terrible connection). I'll put a patch up tonight.

Comment 46

[Reuben Morais [:reuben]]

Attachment: Disable Contacts on esr17

Argh, security bugs don't show up on bugzilla-todos so I forgot about this :(
It looks like ESR17 doesn't have any mechanism for only enabling the navigator property/global constructor during tests, so I'll just nuke dom/contacts.

Comment 47

[Reuben Morais [:reuben]]

Comment on attachment 798689 [details] [diff] [review]
Disable Contacts on esr17

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec bug
Fix Landed on Version: none, this is not an uplift, it disables the entire feature.
Risk to taking this patch (and alternatives if risky): none.
String or UUID changes made by this patch: none.

Comment 48

[Reuben Morais [:reuben]]

Do I need to request sec-approval on this patch?

Comment 49

[Bobby Holley (:bholley)]

(In reply to Reuben Morais [:reuben] from comment #48)
> Do I need to request sec-approval on this patch?

No, that's wrapped into approval-esr17.

Comment 50

[Andrew McCreight [:mccr8]]

(In reply to Reuben Morais [:reuben] from comment #48)
> Do I need to request sec-approval on this patch?

sec-approval is only relevant for the initial landing.  Once you've landed it anywhere, the cat is out of the bag. :)

Comment 51

[Reuben Morais [:reuben]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/a4aee2481cae

Comment 52

[Reuben Morais [:reuben]]

…aand backed out: https://hg.mozilla.org/releases/mozilla-esr17/rev/033b299726dd

Comment 53

[Reuben Morais [:reuben]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/09d9550bd906

Comment 54

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed issue on 17.0.8esr.
Confirmed fixed on 17.0.9esr and FF23.