Security - Marking client-provided data as "untrusted"

Security
1

The article, “Meteor Security in the Wild” does a good job of showing why its important to not trust data from clients. This is a basic tenant of client-server systems and not Meteor-specific but as the primary maintainer of the roles package, it got me thinking.

Is there a way that we could tell when an input is coming directly from the client?

With javascript being dynamic I think this is possible. Wanted to discuss this in the open and see people think.

If we override the way Meteor methods and publish work, we could add a flag to each parameter’s prototype. Code that validates the input can remove the flag.

For example, let’s consider the first example from the article:

Meteor.publish('users', function(userId){
    if(Roles.userIsInRole(userId, 'admin')){
        return Meteor.users.find({}, {fields: {...});
    }
});

If publish automatically added __meteor_untrusted to the userId parameter’s prototype, then we could do something like:

  // ...in Roles.userIsInRole
  if (user.__meteor_untrusted) {
    throw new Meteor.Error(401, "Untrusted input detected");
  }

(Leaving aside the discussion of the right error code to use and the unhelpful error message.)

I haven’t thought through all the ramifications of this… but that’s what this post is for. :-]

Has someone done this before? Other frameworks (Rails)?

What problems would this cause?

2

Instead of trying to detect a client initiated call, which we can never know for sure if it comes from the app itself or some console script, we could try and detect if the call comes from the server.

And it is very easy to do. Just store some global variable (a key) somewhere (meteor settings perhaps) and append that parameter to your server initiated calls. Then your method or whatever receiving function it is, can compare the incoming key with the global variable (since it also has access to that key) to conclude if this is a legitimate, secure, server-initiated, trusted code.

3

I’m not really sure what the benefit is here - can you explain more?

4

I don’t really understand where the problem is.
Meteor method arguments are always untrusted inputs and must be validated in production apps.
If you decide to allow update and insert queries directly from the client you need to validate the queries with allow and deny rules also. I think that covers all untrusted input sources for usual Meteor apps.

Based on your example I think that you assume that a admin should be able to do anything. I would treat an admin in the same way as any other user. The only difference is that he has other rights then a normal user.

4 Likes
5

I don’t think the article’s example is a very good one, because even as the article states that’s not at all how you should be doing it. The userId should not be passed as a parameter, it should be checked on the server in the publication using this.userId.

EDIT: To clarify, not a good example to illustrate your point as there already is a way to handle it securely (as the article goes on to state).

3 Likes
6

This code is wrong:

Meteor.publish('users', function(userId){
    if(Roles.userIsInRole(userId, 'admin')){
        return Meteor.users.find({}, {fields: {...});
    }
});

You should never trust client code. You need to get userId always from this.userId. That’s it.

2 Likes
7

To be clear, my goal is to help people, “fall into the pit of success.” We
all know that the code is wrong. The point of the article is that the code
is wrong.

A side point of the article is this is happening in the wild. People
are doing this kind of thing.

Wouldn’t it be great if we helped them spot it and in fact stopped it?

I think we can make it actually impossible to write that kind of insecure
code.

Consider the warning message that meteor gives when you have auto-publish
and a manual Meteor.publish. Meteor could have silently let that slide
(“it’s wrong, you should know better”) but it’s a very helpful feature to
warn new users when that happens.

Here we can take it one step further by both warning the user and stopping
them from writing something blatantly insecure.

1 Like
8

I think specifically talking about pitfalls here would help us all. May be there are pits that I’ve come across or never noticed. If I come across it after reading about it here, I can avoid it from the beginning and can save troubles!

9

To better explain the benefit, currently functions in the roles
authorization package can not be proactive in stopping the use of insecure
arguments because in the roles function we don’t know where the data
originated. Did it come from the client directly (which we all know is not
safe) or did it come from some validated, server-side code?

Right now there is no way for the roles function to know this because we
don’t know the context. But I think we could.

Tagging arguments to publish and method calls with some marker would allow
the context to flow through. Roles could use this to help users from
shooting themselves in the foot. Other security-related packages could
access that too. Ideally it would not affect packages that don’t care about
where the data comes from.

10

Okay. I get it. (and I know you tried to help new comers)

It’s a good idea to pass the subscription context(this in publication) to the role package and invoke rules based on that.

But, having a low level solution also helps who don’t have to share the sub. context.

How about having a high level API for roles package?