In my last post, I threw down a gauntlet: could you write code secure enough that I wouldn’t be able to hack it?

There was a catch though: you could only rely on using Allow & Deny, not Meteor Methods.

So the goal was not to showcase my – non-existent – hacking skills, but to highlight just how hard it is to write secure Allow & Deny code.

Challenge Accepted

A couple days later, I’ve now received 18 submissions and spent a fair bit of time reading through Meteor security code. And the results have been very enlightening to say the least.

I’m not going to go through every single entry, but I thought it’d be helpful to point out a few common mistakes, as well as look at a couple interesting patterns.

Mistake 1: Not Checking Values

Juanpasta’s solution was one of the first submitted, but it includes a basic mistake:

Messages.allow({
  update: function (userId, doc, fields, modifier) {

    return userId == doc.userId && arrayEq(fields, ['body']) ||
      arrayEq(fields, ['likes', 'likesCount']) &&
      doc.likes.indexOf(userId) == -1;
  }
});

While this code is properly ensuring that only the body or likes and likesCount fields are being modified, it’s not checking which value they’re being set to. Meaning you can set the likes and likesCount properties to any arbitrary values:

Messages.update(Messages.findOne()._id, {$set: {likes: "foo", likesCount: 9000}})

Mistake 2: Not Checking (All) Operators

jgdovin made another common mistake. Can you spot the problem with his code?

Messages.allow({
  update: function (userId, doc, fields, modifier) {
    if(fields.isEqualTo(['body']) && userId === doc.userId) {
      return true; 
    }

    if(fields.isEqualTo(['likesCount', 'likes']) && doc.likes.indexOf(userId) < 0 && modifier['$inc']['likesCount'] === 1)     {
      return true; 
    }
    return false;
  }
});

This code does ensure that only the $inc modifier is used on the likesCount field and that its value is equal to 1, but doesn’t impose any limits on the likes field, making it possible to delete or overwrite it:

Messages.update("XNJaBZwG4e64GQ28X", {$set: {likes: [1,2,3,4]}, $inc: {likesCount:1} })

Thankfully, the updated version of his solution doesn’t have these problems.

lizunlong also made a similar mistake:

Messages.allow({
  update: function (userId, doc, fields, modifier) {
    var isOwner = doc && doc.userId === userId;
    var wasLiked = _.contains(doc.likes, userId);
    var willModify = function (field) {
      return _.contains(fields, field);
    }

    if (!userId) {
      return false;
    }

    if (willModify("body") && isOwner) {
      if (fields.length !== 1) {
        return false;
      }

      return true;
    }

    if (willModify("likes") && !wasLiked) {
      if (_.has(modifier, "$set")) {
        return false;
      }

      var whoLiked = modifier["$addToSet"]["likes"];
      var numInc = modifier["$inc"]["likesCount"];
      if (whoLiked === userId || numInc === 1) {
        return true;
      }
    }

    return false;
  }
});

Checking _.has(modifier, "$set") to ensure the user is not passing a $set modifier when they shouldn’t is a good idea, but leaves the door open to passing other modifiers, such as $unset.

So this code lets you unset any message’s userId or createdAt as long as you’re also modifying likes in the same update.

Mistake 3: Permissive Allow Rules

Splendido took some time off from working on his awesome useraccounts package to give the challenge a try.

He didn’t quite get it right on the first try though:

// Allow own body editing
Messages.allow({
  update: function (userId, doc, fields, modifier) {
    return _.contains(fields, 'body') && userId === doc.userId;
  }
});

//...

I don’t need to paste in the rest of his code, because this bit is enough to open up a security hole. Remember that only one Allow callback needs to return true for an update to be accepted.

So all that’s needed here is to update the body field of your own message to get a blank check to any modification you’d like:

Messages.update({_id: "QQ4mmHmio86PSjkWy"}, {$set: {body: 'new text', likesCount: 9999}});

Thankfully after I pointed out the issue, he was able to come back with a better solution.

You can make the same mistake even within a single Allow rule, as shown by khamoud’s submission:

Messages.allow({
  update: function (userId, doc, fields, modifier) {
      if(_.indexOf(fields, 'body') !== -1) {
          if(userId !== doc.userId) {
              return false;
          } else if (userId === doc.userId) {
              return true;
          }
      }
      if(_.indexOf(fields, 'likes') !== -1) {
          if(!!userId && _.indexOf(doc.likes, userId) !== -1) {
              return false;
          } else if(!!userId){
              return true;
          }
      }

      return false;
  }
});

Just like in the preceding example, the operation only needs to satisfy the first if to go through, and the code can be exploited with the exact same command:

Messages.update({_id: "QQ4mmHmio86PSjkWy"}, {$set: {body: 'new text', likesCount: 9999}});

Now that we’ve seen all the ways things can go wrong, let’s look at a couple patterns that actually work.

Pattern 1: Deny-Based Security

If you think about it, there’s two basic ways to approach the problem: use Allow to define what is accepted, or use Deny to define what isn’t.

In other words, one possible strategy is to accept every update by default, and then specify rejected cases using Deny.

This is the approach taken by Splendido in his second submission:

// Allow anything permitted by default
Messages.allow({
  update: function() {
    return true;
  }
});

// Deny likes using unknown modifier
Messages.deny({
  update: function (userId, doc, fields, modifier) {
    if (_.intersection(fields, ['likes', 'likesCount']).length > 0) {
      // the only allowed modifier is the following:
      var allowedModifier = {
        $addToSet: {likes: userId},
        $inc: {likesCount: 1}
      };
      return !_.isEqual(modifier, allowedModifier);
    }
  }
});

// Deny non-first likes
Messages.deny({
  update: function (userId, doc, fields, modifier) {
    if (_.intersection(fields, ['likes', 'likesCount']).length > 0) {
      return _.contains(doc.likes, userId);
    }
  }
});

// Deny any attempt to edit the body of others' messages
Messages.deny({
  update: function(userId, doc, fields, modifier) {
    return _.contains(fields, 'body') && userId !== doc.userId;
  },
});

// Deny any attempt to update createdAt or userId fields
Messages.deny({
  update: function(userId, doc, fields, modifier) {
    return _.intersection(fields, ['createdAt', 'userId']).length > 0;
  },
});

Elliot also used a similar pattern, but chose to keep everything in a single Allow callback instead.

Of course, the problem here is that it’s easy to let a use case slip to the cracks of your Deny rules. Can you really be sure you’re catching every single possibility?

Pattern 2: Pattern Matching

One of the most secure patterns submitted was probably Pete Corey’s Match-based entry.

Pete regularly blogs about Meteor security, so it makes sense he’d come up with a good solution:

Messages.allow({
  update: function (userId, doc, fields, modifier) {
    var checkEdit = {
        $set: {
            body: String
        }
    };

    var checkLike = {
      $addToSet: {
        likes: Match.Where(function(likes) {
          check(likes, String);
          return likes == userId &&
          !_.contains(doc.likes, userId);
        })
      },
      $inc: {
        likesCount: Match.Where(function(likesCount) {
          check(likesCount, Number);
          return likesCount == 1;
        })
      },
    };

    try {
      if (userId == doc.userId) { // Like or edit
        check(modifier, Match.OneOf(checkEdit, checkLike));
      }
      else { // Like
        check(modifier, checkLike);
      }
    }
    catch (e) {
      return false;
    }
    return true;
  }
});

Pete is defining a “fake” modifier object that contains what the modifier should look like, and then uses that as a model to make sure the actual modifier object being passed follows the same pattern.

This leaves no room for loopholes, and is also much easier to understand than a bunch of if statements glued together.

It might be worth reading up on Match if (like me) you weren’t aware of how useful it can be.

Results

So here are the final results: out of 18 submissions, 5 people got it right on the first try (at least, as far as I could tell):

If you’re one of the winners listed here (or if you’re not, but think you should’ve been), email me at hello @ this domain so I can get in touch.

And if I haven’t reviewed your code, I apologize! But at this point, my vision is starting to get blurry and I feel like my brain is going to jump out of my skull if I force it to read through more Allow & Deny code…

Lessons Learned

So what did we learn through all this? For me, this was a very persuasive demonstration that there are some things Allow & Deny are just not made to handle.

As Chet put it so eloquently:

What a nightmare! Allow-deny rules are such a round-about way of validating data. It’s so much more straightforward using a method.

That doesn’t mean you should never use Allow & Deny. But if you’re using it for everything in your app, I might start getting a little worried and think about introducing methods in the mix.

The other thing that was apparent is that there isn’t really any best practice for writing Allow & Deny code. Pretty much everybody had a different approach, and that’s probably another big reason why Allow & Deny code is often insecure.

This is a common problem in the Meteor world: the technology is here, but we need more people sharing and writing about coding patterns if we want to stop wasting time reinventing the wheel every time (especially if the wheel in question turns out to have a flat tire!).

A Definitive Guide

Also, I realize focusing on edge cases and gotchas is probably not that helpful if you’re still trying to master the basics.

For that reason, I’ve also been working on the definitive primer to understanding Allow/Deny, based on everything we’ve learned here. So if any of this still feels confusing, stay tuned for a more comprehensive explanation coming very soon!