Hash injection (security)

[homakov]

Using specially crafted requests we can trivially bypass secret_token protections on websites using sequalize.

Many people have code like this

db.Token.findOne({
      where: {
        token: req.query.token
      }
);

But Node.js and other platforms allow nested parameters, ie token[$gt]=1 will turn into token = {"$gt":1}. When we pass such hash to sequalize it will consider it a query (greater than 1) and find the first token in DB, bypassing security of this endpoint. This behavior was copied from Mongo https://docs.mongodb.com/manual/reference/operator/query/

Using finely tuned $gt we can iterate all tokens in db and impersonate every single user.

There are vulnerable sites in the wild.

That's how it can be exploited in Mongo http://blog.websecurify.com/2014/08/hacking-nodejs-and-mongodb.html and https://cirw.in/blog/hash-injection

My advice would be to either disable this functionality entirely (i.e. require passing token {"$eq":token} every time) or make sure the parameter isn't coming from req.query and is native hash object.

[janmeier]

This is definitely something we as a project are aware of, but I agree that a lot of our users might not be. In the best of all worlds, we should definitely do more to educate them :)!

make sure the parameter isn't coming from req.query and is native hash object.

How could we do that? Objects coming from req.query are plain objects, just like a user would write in their code (which is what makes this potentially so dangerous)

I think the best solution for now would be to better inform our users - PRs that improve the documentation are very welcome!

[homakov]

How could we do that? Objects coming from req.query are plain objects, just like a user would write in their code (which is what makes this potentially so dangerous)

In Rails params hash inherits completely different class. But in Node you have no power over req.query parsing so... Start with a warning. I never liked this behavior in mongo in the first place - such magic is dangerous.

[homakov]

@janmeier I don't see a way but enforcing people to use {$eq: token} using a shorter construction
where: {token: eq(req.query.token)} or eq_token: token or other way around requiring special clauses to use prefixed fields? lt_token: less_than.

Something has to be done. .toString-ing all the input is very tedious job. Even if it's not direct vulnerability in the project, it has a lot of potential. Maybe make an announcement to users and see what do they think?

[katanacrimson]

Considering this is a security issue, this should not be autoclosed.

[yonjah]

@homakov This is interesting idea.

One way to approach it will be instead of using strings for commands '$gt' will be to use Symbols.
Since they can't be faked clients wont be able to send them in.
But this would be a breaking change and users will need to get the symbols out of Sequelize to use them -

Some thing like -

const {gt, lt, eq} = Sequelize.symbols;
Token.findOne({
	token: req.query.token,
	expiry: {[lt]: new Date()}
});

@janmeier maybe this can be implemented behind a feature flag and later activated by default so it won't be a breaking change.

BTW @homakov not sure what frame work your using but I'm using hapi, which allows you pretty good control on user input. By default I don't allow any object properties to contain '$' if your not using hapi you can probably just use joi to quickly and easily sanitize your users input

[homakov]

I think i was using express, filtering out $ is one way to go, but it's not a good practice to my best knowledge.

[yonjah]

I'm not sure if there is specific best practices for this.
The only obvious thing is that you should always sanitize user input ORM reduces the risk of injections dramatically but it can only take you so far.
Thats one of the reason I like hapi, you have easy way to validate the input is exactly as you expect it even with very complex data structures

[snewell92]

Is this vulnerability present in a feathers applications using JWT tokens and feathers-sequelize? All of my API endpoints first go through feathers, which authenticates, then parses and runs hooks (akin to express middleware) before passing data to sequelize. However, I suppose if I don't properly validate and sanitize my user input, this could still happen?

[yonjah]

@snewell92 This is not a vulnerability in Sequelize.
If you don't properly sanitize and validate user input you are at risk of being compromised.
Sequelize (as any ORM) can help prevent many common vulnerabilities but it's not a magic bullet and you shouldn't rely on it to protect compromised apps.

If you are talking specifically about JWT one of the major feature of JWT is that you don't need to save them in the database (or anywhere else) since the token is signed and you can validate the data to be accurate. So unless feathers is doing something non standard it's JWT shouldn't reach the database layer.
If the client has a valid JWT but your not actually checking other parameters he controls and pass them blindly to Sequelize than you might still be in risk of data leaks and returning data user shouldn't have access to.

Always sanitize and validate user input

[snewell92]

Yes for sure sanitize + validate always.

Feathers only temporarily stores JWT in local storage (or http only secure cookies), and of course revokes/removes the JWT after a time limit or logout, requiring another login to get a fresh token. Also, I do role checks in my service layer before passing stuff to sequelize, so that covers the data leaks and access (role information is embedded into the claims of the JWT, not based on query params).

Thanks for the explanation / clarification!

[yonjah]

Feathers only temporarily stores JWT in local storage (or http only secure cookies)

But that's on the client side right ? it doesn't need to store it anywhere on the server

I do role checks in my service layer before passing stuff to sequelize, so that covers the data leaks and access (role information is embedded into the claims of the JWT, not based on query params).

Again if your not validating the actual input your at risk.
Lets think of a really bad example I doubt anybody would ever do something like this but it shows the risk.
Lets say I have a blog with an endpoint to see posts any user that's logged in is allowed to see posts and I don't consider them as a very sensitive information. I'm also a bit lazy and I don't want to start writing very specific API endpoints with individual parameters for limiting and sorting and all this nuances. So I decide to just encode Objects in a format Sequelize will understand from the UI and send them down to the server. You can't inject and as we said it's not sensitive data I only care the user is actually logged in.

So I do something like this

if (!verifyJWT(req)) {
    throw new Error('Not logged in');
}
return Post.findAll(req.query);

this is works great I can send from the UI queries like -

• { where: { userId: 4 } } to see all of user 4 posts
• { where : { created_at: {$gt: 2017-01-01}, limit: 10, order: [['created_at', 'asc'] } to first 10 posts of the year

Really nice and powerful.
Unfortunately I can also pass this input

• { include: [ 'User' ] }
  Now I get all the posts and all the data of the user that wrote it which probably include information I'm not suppose to have access to like his e-mail and his hopefully hashed password.

Again I doubt anyone will be doing it that carelessly and most implementation will only let the client to control the where property and not the entire options object, but the cause for the security issue here is failing to validate the user input

[snewell92]

But that's on the client side right ? it doesn't need to store it anywhere on the server

Correct. only client side. In fact, its encouraged to not use cookies at all, and move towards statelessness. Feathers is built simply to be an API. Ideal for SPA's and api end points.

However, in my case I use cookies because I have a multi page application that server-side renders the shell of all my pages. But since this is still a cookie - the JWT is never stored on the server in any way. The server merely inspects the cookie (httpOnly - can't stress this enough!) on the header. Because of that httpOnly option, client side javascript can't interact with it.

Nice example, thanks!

[felixfbecker]

I think one way forward would be to export operator names in v4 and updating docs. Then people can transition to using

const { eq } = Sequelize.operators;

where: {
  [eq]: value
}

and in v5 we can replace them all with Symbols.

That said though, you always need to validate your user input types. And it's really not tedious:

db.Token.findOne({
      where: {
        token: req.query.token + ''
      }
);

or String(req.query.token) if you prefer.

[homakov]

That said though, you always need to validate your user input types. And it's really not tedious:

You don't if that is what ORM is supposed to do. Readme also never says to cast all inputs manually. And it is tedious.

[felixfbecker]

The documentation tells you that the ORM provides an API to query specific properties with specific operators. If you want to use user input in that query syntax, but don't want to allow the user to provide all of the querying the ORM supports, you need to put a layer between the ORM and the user input. There is no way around that - the ORM can't just detect user input magically. It provides you an interface and will execute what you pass to it. You have to make sure you pass correct values. Converting these values from the serialised user input (HTTP request body, query parameters, ...) is not the job of an ORM. The ORM is one building block in your application and Express might be another. Wiring the interfaces up is the job of the developer, and that includes making sure invalid values are not mapped and only the subset of the ORM interface is (indirectly) accessible that you want to be (indirectly) accessible.

[homakov]

Part "Basic" from http://docs.sequelizejs.com/manual/tutorial/querying.html doesn't teach to escape user input, but ORMs are built to work with user input, it's what they are all about. So one thing is documentation about shell.exec (user's job to validate) and another is findAll (something expected to be safe)

[felixfbecker]

You claim that "ORMs are built to work with user input", can you explain why you think so? ORMs are first and foremost build to map relations and objects, not to handle user input. You won't find "user input" mentioned anywhere on the ORM wikipedia page, because it's not its job.

The ORM docs don't teach you how to validate user input just like the child_process.exec docs don't teach you how to validate user input. No method is magically "safe". If an interface allows to access sensitive data if told to do so, then it always depends on the context and your validation whether that function is "safe".

Of course, I would have absolutely no problem with some examples in the docs on how to validate user input and to avoid common caveats like this. If you wanna do a PR I would happily merge it 😉

[arifath]

@sushantdhiman Is there problem in future if I define operator alias?

[sushantdhiman]

@arifath Yes you are subjecting your application to hash injection as explained in this thread