No-SQL injection in Mongo PHP(php.net)

59 pointsby taylorbuley15 anni fa57 comments

http://www.php.net/manual/en/mongo.security.php

54 comments

This is not the only NoSQL-injection issue. Extra fun bonus warning: nobody knows what the quoting domains are for all the NoSQL's. Be extraordinarily careful with your keys.

rbanffy15 anni fa

Any query language can be injected if you don't sanitize your input. Any object persistence layer that doesn't sanitize parameters is vulnerable to this kind of attack.

btilly15 anni fa

You are advocating a semi-reasonable solution to a common mistake, possibly because you can't conceive of not making the mistake. The better solution is to follow the advice in http://cr.yp.to/qmail/guarantee.html of using APIs that avoid parsing user input. Then no possibility of problems exists at all.

This is explained in item #5 of http://cr.yp.to/qmail/guarantee.html - Don't parse.

Note that this is definitely true in this case. MongoDB's API is based on the passing of data structures where you have well-defined places to put user input directly. The problem emerges because PHP decides to attempt to parse that user input in a way that it had no real need to, with bad results. Without that parsing, there would have been no need to sanitize data because there would be no possibility of ever having had bad data in the first place.

bmm6o15 anni fa

A light bulb went off for me when I saw SQL injection attacks described as "subverting the structure of the query". The problem isn't sneaking bad data into your query, the problem is that the data injected into the string alters the parse tree in the dbms. This distinction isn't immediately obvious if you've moved to a parametrized query solution, because there's still a lot left to parse - it's just that the part that gets parsed is now a compile-time constant.

tptacek15 anni fa

For whatever it's worth, even in parameterized SQL queries, the query structure isn't always entirely fixed. There are injectable parameterized query constructs, and some of them are fairly common.

bmm6o15 anni fa

I haven't heard of this before. Can you link to an example?

tptacek15 anni fa

On-the-fly table selection is a simple example, but there are more subtle examples that happen even more often.

bmm6o15 anni fa

Unless I'm misunderstanding you, changing the table doesn't affect the shape of the parse tree. (and I'm not trying to be pedantic - I'm genuinely curious about this).

btilly15 anni fa

The issue isn't how you parse the query. The issue is how you choose to execute it.

I'm not sure what tptacek is thinking, but here is an important case where you really want the execution path to change.

Tables often have a very uneven distribution of data. This can mean that the right query plan will depend on the data provided. Consider the following query:

  SELECT ...
  FROM lease l
    JOIN property p
      ON p.id = l.property_id
    JOIN management_company c
      ON c.id = p.management_company_id
  WHERE l.created >= ?
    AND l.created < ?
    AND c.name = ?;

Pretty straightforward query, right? Now here is the question, should the query plan start by filtering leases on date then flow forward to finally filter on the management company, or by getting the right management company and then flow backwards to filter on dates.

It turns out that there is no right answer. You want to start with whichever restriction eliminates the most records, quickest. If the date range is wide, we want to start with the management company. If the date range is narrow, we want to start with that.

So let us simplify our life:

  SELECT ...
  FROM lease l
    JOIN property p
      ON p.id = l.property_id
    JOIN management_company c
      ON c.id = p.management_company_id
  WHERE l.created >= '2011-02-01'
    AND l.created < '2011-03-01'
    AND c.name = ?;

There is still no right answer! Let's suppose some company, call it 'AIMCO', has a huge number of properties with a lot of lease volume while other companies generally have 1-5. Then it is better to filter on dates first for 'AIMCO', and better to filter on the company first for anything else. Getting the order wrong gives an order of magnitude performance difference. (This is a real life example. I really did have a bug that basically read, "Make this report run for AIMCO." And this was exactly the problem.)

Most databases will pick one query plan and go with it. But Oracle allows you to turn on Adaptive Cursor Sharing. With this it will analyze the data, see that AIMCO should get special treatment, and actually execute a different query plan if you passed in 'AIMCO' than if you passed in something else.

So now you have a data dependent query plan.

bmm6o15 anni fa

Ok, so your example consists of switching between semantically equivalent queries in the application layer to compensate for weaknesses in the DBMS execution planner. I get that CPU time is a valuable resource and you don't want to open yourself up to DOSing and all, but this isn't really anything like SQL injections. The attacker still can't subvert the structure of the query - changing the structure of the query was the solution to the problem.

btilly15 anni fa

Ok, so your example consists of switching between semantically equivalent queries in the application layer to compensate for weaknesses in the DBMS execution planner.

Not quite. My example consists of the DBMS execution planner deciding that it will switch between different semantically equivalent query plans depending on the parametrized data passed. This switch should not be noticed in the application layer.

Back to the main thread, I suspect that you are interpreting tptacek as saying something he didn't. You seem to be looking for a potential security flaw. But he didn't ever say that there was one. He only said that the query structure can vary with the parameters passed to a parametrized query. I just described one way that this can happen.

tptacek15 anni fa

Yeah these are great comments and all but I'm just saying that, last time I checked, you can't bind a table name as a parameter in the MySQL protocol; if your table names change at runtime based on input, you're back to concatenated queries.

tptacek15 anni fa

If people weren't talking up NoSQL specifically as a way to avoid SQL security problems, I wouldn't have written my comment.

rbanffy15 anni fa

The problem is that under the hood there is a query language. When you have no query language, you can't inject commands into it, either by carelessly passing user input down the stack or assembling your own queries.

It would be devilishly hard to inject malicious operations into ZEO/ZODB (the one "NoSQL" I am very familiar with). I can only assume it would be equally hard to do it with systems that do not try to replicate the human-friendly nature of SQL.

tptacek15 anni fa

See my original comment.

rbanffy15 anni fa

My comment has no relation to escaping. It's in fact, centered on the idea that any attempt to do so will introduce potential vulnerabilities and the only sane way out of this mess is not having a language in the first place.

Anyway, the idea of blindingly assuming that just because something doesn't use SQL it's invulnerable to any kind of code injection is a very optimistic position.

tptacek15 anni fa

Thank you for restating my comment. Perhaps your wording will be clearer than mine.

rbanffy15 anni fa

Anyway, NoSQL databases are, by definition, invulnerable to SQL-injection attacks. Perhaps we should start calling them "command injection" or something like it.

RyanMcGreal15 anni fa

> which PHP will magically turn into an associative array

I think I found the issue.

joelhaasnoot15 anni fa

The issue is trusting input you're receiving from users or the wild world out there. User data always needs to be sanitized in some way shape or form. Assume very little on the web.

Think of every language as a strictly typed, pure functional programming language, where I/O is the big bad world out there and evil and blocking and complicated. Likewise, on the web you should treat any inputs as being evil... (ok, so the comparison is far from perfect, but the idea holds true)

KaeseEs15 anni fa

I think you've conflated "not trusting" user data with sanitizing it. To use an analogy from SQL, using prepared statements/parameterized queries doesn't sanitize user data, but it also doesn't trust it (as it doesn't treat data as code). More generally, blacklist sanitizing is a losing game from the outset, whitelist sanitizing is hard, your best bet is to not treat data as code to begin with.

tptacek15 anni fa

"Always validate your inputs" is unfortunately one of the least useful bits of security advice given to developers; it is only epsilon away from "never make mistakes".

sausagefeet15 anni fa

I agree with you, but also pointing out a strength of a language that supports phantom types helps this issue quite a bit. You can design your DB API to not allow unquoted queries.

jameskilton15 anni fa

Rails does the same thing and it's insanely useful, you just need to know what you're doing and ensuring you're protecting against this.

emil0r15 anni fa

Which means a) You're going to forget it at least once. b) For every developer mindful of this, there will be hundreds not knowing about it.

If you ever put "you just need" into a sentence about security you've already lost.

rickmb15 anni fa

> a) You're going to forget it at least once.

Which is why you should never process raw input in multiple places in your app. One method I've used for over a decade of webdev in any language: there's only one place to get the input from, and that place requires you specify what input is allowed.

RyanMcGreal15 anni fa

Any security practice that addresses an insecure default state by starting with "you should never..." is doomed.

boundlessdreamz15 anni fa

The default Rails way of using Activerecord will automatically sainitize inputs for you. Not turning it into an associative array doesn't add anything to security.

User input need to be sanitized always. Rails make it easy and is the default way.

rapind15 anni fa

There's nothing stopping you from passing unsanitized params to the database. I.e. where("name LIKE '%#{params[:name]}%'") instead of where("name like ?", "%#{params[:name]}%"). While the 2nd option is definitely the recommended way to do it, there's nothing stopping you from doing it the wrong way, and I bet many do.

RyanMcGreal15 anni fa

The thing is, it's easier to do it the right way. It requires a special, deliberate effort to do it wrong. That seems like a reasonable implementation of a "magic" feature. PHP, as usual, gets it exactly backwards. It's easier to do it wrong, and requires a special, deliberate effort to do it the secure way.

rapind15 anni fa

Yeah maybe. Depends on the person I think. I actually find it easier to assemble a SQL string myself. If you're new to rails / AR and you're not familiar with SQL then you'll look up and figure out how to assemble the SQL via the ORM methods. If you're new to rails but an old hand at SQL, just building the string is super easy and takes you 2 seconds without looking up any documentation. Of course assembling the string AND sanitizing it requires a heck of a lot more effort.

Regardless, it's not a knock on rails. Just saying it's not necessarily a magic bullet to secure apps (and I don't believe rails nor PHP needs to be).

emil0r15 anni fa

All true. I was thinking of the submitted link still. Manually sanitizing instead of having something doing that for you is a bad idea because you will forget.

taylorbuley15 anni fa

The "hundreds not knowing about it" was was sparked me to submit this to HN. This is important stuff and probably should not be relegated to a php.net footnote.

Edit: It's not actually a footnote -- it's actually just the only item on the lonesome "Security" page.

jasonlotito15 anni fa

To be fair, it's the Mongo Security page, not the general security page, which can be found here: http://www.php.net/manual/en/security.php

wvenable15 anni fa

The web is full of these kinds of issues -- you need to escape, filter, etc, etc, every piece of user data. You either do it yourself or use some kind of abstraction layer (template engine, ORM, etc) to do it for you.

tptacek15 anni fa

Put differently: "we're all doomed". Squares with our professional experience.

SpikeGronim15 anni fa

It is a tradeoff between ease of use and security. These sorts of features require the developer to check every single query string consumer or you end up with gaping security holes. That will result in security exploits in any moderately large codebase with multiple developers, it's just a matter of time. I would much prefer to sanitize everything always and force the app developer to spend the 1 line of code to parse into an array when they want that feature.

drdaeman15 anni fa

Tornado always uses lists, even when there's a single parameter. I.e. `?spam=ham&spam=ham&eggs=eggs` results in `{"spam": ["ham", "ham"], "eggs": ["eggs"]}`.

While this may be not as easy to use as simple strings, it is consistent.

ronnoch15 anni fa

Somehow I managed to use PHP for 8 years and never notice this... Time to double check all my input sanitizing code, I guess.

jasonlotito15 anni fa

8 years and you didn't know this? I assume it's the part where it turns [] passed vars into arrays. Even still, this is only a problem is you are blindly passing around user input without any verification. The same thing is just as easily accomplished in numerous other languages.

Honestly, I'm trying to figure out how someone could sanitize their input and still be affected by this.

ronnoch15 anni fa

> Honestly, I'm trying to figure out how someone could sanitize their input and still be affected by this.

I don't think you could, unless you tried to write your own sanitizing functions from scratch and somehow screwed it up. In PHP, htmlspecialchars(), mysql_real_escape_string() and addslashes() all do fine sanitizing array input -- either throwing an exception or returning the string "Array".

Getahobby15 anni fa

Hmm. I think these functions escape output, not sanitize input.

mscarborough15 anni fa

http://www.php.net/manual/en/book.filter.php

Lots of built-in options, the only reason this should be an issue is if you're not validating inputs at all, or hacking your own as ronnoch noted.

sausagefeet15 anni fa

Which is quite possible unless you have a compiler forcing you to validate. I make silly mistakes all the time in Python.

grncdr15 anni fa

Not to pick on your comment specifically, but I've seen a couple of comments that imply this is the kind of mistake that one would make in a moment of absent-mindedness, and I disagree. Nobody with any semblance of a clue is writing (new) apps that access user input from PHP super globals directly. If you are ever using $_GET in your app-specific code you are doing it horribly wrong, not making a silly mistake.

That being said, there are legions of clueless devs out there who will do this, and that is unfortunate.

sausagefeet15 anni fa

I agree with the clueless argument, but I also think there are mountains of bugs that exist simply because one made a mistake and a dynamically typed languages can't really do much for them there.

s00pcan15 anni fa

I might be missing something here, having never used MongoDB, but what does this have to do with it? How else can this attack vector be used?

nbpoole15 anni fa

There's a very good example here:

http://www.idontplaydarts.com/2010/07/mongodb-is-vulnerable-...

methodin15 anni fa

$ne = not equals which translated to "give me everything not equal to foo". Using this you can basically get a dump of all the data or update with foobarred critera - updating all passwords for all users not equal to foo, for instance

antirez15 anni fa

Non existing problem with Redis (not possible at protocol level), for what it is worth.

albertzeyer15 anni fa

The old Redis protocol was also not binary safe. If you are able to access the value in some `redisCommand("SET %s %s", key, value)`, you are able to inject other commands there.

But the new Redis protocol is safe against that.

mrkurt15 anni fa

It's not exactly a protocol issue for mongo, though. If it weren't for Rails/PHP (and more) converting magical strings into objects of another type, it wouldn't be possible in Mongo either.

This would be the equivalent of some helper wrapper on top of a redis driver that turned strings with a special format into, say, an MGET command that returned more than a developer meant for it to.

redis is safer because it doesn't have the complex query-over-document functionality that mongo does, but it's not inherently immune.

antirez15 anni fa

The new Redis protocol (the only one used at this point) is inherently immune, with SORT we have complex queries. But the point is, the way we send every different argument to the server as a single entity with prefixed length, makes the attack impossible.

to15 anni fa