Renaming Your Way to Admin Rights

Learning from our previous mistakes, but also feeling especially clever, we came up with a new solution to our profile modification problem that minimized work for ourselves, and maximized the customizability of our users’ profiles.

Our new implementation of updateProfile accepts a database modifier from the client, checks that it’s an appropriate update to apply, and then applies it to the current user document:

Meteor.methods({
  editProfile(edit) {
    checkEdit(edit, Meteor.user());
    return Meteor.users.update(
      {
        _id: this.userId
      },
      edit
    );
  }
});

Our checkEdit function loops over every field of every operation in our edit object, and checks that the field being edited is listed as editable by our current user’s role in our userSchema:

function checkEdit(edit, user) {
  _.each(edit, operation => {
    _.keys(operation, field => {
      if (!_.includes(userSchema[field].editableBy, user.role)) {
        throw new Meteor.Error("Not authorized.");
      }
    });
  });
}

Our userSchema has an editableBy field attached to every field on our user document. Many fields are freely editable by all roles, but certain fields, like the isAdmin field is only editable by users with the admin role:

const userSchema = {
  isAdmin: {
    editableBy: ["admin"]
  },
  username: {
    editableBy: ["user", "admin"]
  },
  shareOnlineStatus: {
    editableBy: ["user", "admin"]
  },
  ...
};

As an example, a user with the user role could execute the following call to editProfile:

Meteor.call("editProfile", { $set: { username: "jane" } });

But that same user could not modify the isAdmin field of their user document because they lack the admin role:

Meteor.call("editProfile", { $set: { isAdmin: true} });

Our system holds up to most kinds of trickery as well. A user might try to increment ($inc) their isAdmin flag, but this is still recognized as an operation against the isAdmin field by a non-admin user, and is prevented.

Unfortunately, our solution is making a fatal assumption. We’re assuming that all MongoDB database updates only affect their source field. That is, all of our operations follow the same pattern:

{ [operation]: { [a]: [b] } }

We’re assuming that a will always be the field being updated by the operation, never b. There’s a single MongoDB update operation that violates this assumption.

Imagine if a malicious user with the user role were to execute the following update to their profile:

Meteor.call("editProfile", {
  $set: { shareOnlineStatus: true },
  $rename: { shareOnlineStatus: "isAdmin" }
});

They run a multipart edit that first sets their shareOnlineStatus flag to true. This is fine; this field is editable by normal users. Next, they run a $rename operation to rename the shareOnlineStatus field to isAdmin. This update is allowed because it’s technically operating on the shareOnlineStatus field, which the user has permission to edit, but the end result is that their user document now has the isAdmin flag set to true. Our malicious user has successfully made themselves an administrator.

We were assuming that users would only ever perform unidirectional updates, like $set, $inc, and $push to their user document. This assumption cost us dearly. Who knows what kind of havoc our malicious-user-turned-administator can wreak on our application and our users.

The fix to this vulnerability, like the fix to every other vulnerability, is to turn our assumptions into assertions. We really only ever want our users passing us $set update operators in their edit update objects. Let’s make that assumption check with a call to check:

  editProfile(edit) {
    check(edit, { $set: Object });
    checkEdit(edit, Meteor.user());
    return Meteor.users.update(
      {
        _id: this.userId
      },
      edit
    );
  }

Here we’re checking that edit is an object with a single field, $set. We’re also asserting that the value of $set must be an Object. Usually, I caution against this type of incomplete checking, but our subsequent call to checkEdit thoroughly checks the contents of $set for us.

Now, a malicious user trying to pass anything other than a $set operation will be rejected and our application and its users remain safe and sound.