Recently we had an issue on our site where someone attempted SQL injection via a cookie (we'll call it lastID). NOC was in a frenzy and angry about how the cookie as an attack vector could be ignored. They had a developer create a class for managing cookies that will sanitize lastID (and eventually other cookies) to check that it is numeric.

However, I think that this is wrong. The problem wasn't that lastID wasn't numeric, it was that the query that uses it was vulnerable to injection. I'm in charge of the review of the code changes, and I want to bring up the fact that forcing lastID to be numeric is not particularly useful and perhaps even undesirable.

Is there a term or concept that describes sanitizing at the right time (i.e. sanitizing at query time rather than at cookie retrieval time), or "over-sanitizing" (e.g. forcing lastID to be numeric) that I can use to describe what's happening/should happen in my review? The second part of Postel's Law seems to apply here in my mind:

Be ... liberal in what you accept

  • How about "do what makes sense," or "do what will actually produce the desired result." In other news, won't requiring the ID to be numeric (or at least scrubbing things like semicolons) actually solve the problem, at least for the ID? Convince management that writing safe queries is the "correct" and "most secure" approach that will cause the least number of undesirable side-effects. – Robert Harvey Jan 11 '13 at 17:11
  • @RobertHarvey it solves a problem (that query is safe when using that specific value). I think that's insufficient. We can still require the ID to be numeric, but it would only be supplemental and not particularly useful. Just because you can do something doesn't mean you should. – Explosion Pills Jan 11 '13 at 17:15
  • @RobertHarvey I think that I'll use the title of this article in my review. Using prepared queries to do sanitation is probably as close to the principle it describes as possible. – Explosion Pills Jan 11 '13 at 17:24
  • 2
    Be liberal in what you accept is guidance for interoperating with other systems. It's not advice for dealing with malicious users sending malformed data to a system that has no reason to ever receive input from any source other than your own code. – Carson63000 Jan 11 '13 at 20:29

Your are right. Correct all your queries to injection-proof prepared statements, and stop worrying about the supplied data. Trying to sanitize inputs to be directly interpolated into SQL statements leads to web applications that reject last names containing apostrophes, or passwords containing certain special characters.

By the way, just because you are right doesn't mean you will be able to convince NOC. Good luck with that.

| improve this answer | |
  • Thanks for the affirmation. Is there a specific term or phrase that refers to doing what you suggest (correcting queries as opposed to worrying about supplied data)? – Explosion Pills Jan 11 '13 at 17:08
  • @ExplosionPills: Your NOC's solution is putting a band-aid over something they think is a paper cut but is really a sucking chest wound. Kevin's solution assesses the larger problem and does something about it. There isn't really a term for this other than "fixing the actual problem, not just masking the symptoms." – Blrfl Jan 11 '13 at 17:39
  • @ExplosionPills: The term is "prepared statement". Consistent use of prepared statements eliminates the possibility of SQL injection bugs. – kevin cline Jan 12 '13 at 18:25

I would simply point out that the vulnerability lies with the database procedure that can be exploited by SQL Injection. The cookie is merely an attack vector.

While it is important to take care of known attack vectors, the more comprehensive solution is to fix the vulnerability at the source. This will both solve the immediate issue of the cookie attack vector and other unknown (possibly future) attack vectors.

| improve this answer | |

You're definitely right. I think you're actually hitting on two concepts.

  1. KISS. (Keep It Simple) The basic tenet is that you use the simplest solution that actually solves the problem. (Making lastID numeric doesn't really solve the problem, just that particular case)

  2. DRY. (Don't Repeat Yourself) If there was ever another place where this query was executed with some other input than lastID, you'd have to solve the problem of SQL injection there too. Just fix the query and it's solved for all execution paths.

Moreover, the solution of enforcing lastID to be numeric is using the wrong level of abstraction.

| improve this answer | |
  • 1
    Here's a third concept though: Defense in depth. Non-numeric ID cookies are invalid input and should be rejected. An injectable SQL query is a horrible bug and should be fixed. But fix both of them. Maybe a non-numeric cookie can cause some non-injection bug elsewhere (e.g. causing a function to crash with a parse error halfway through performing some transaction). – Carson63000 Jan 11 '13 at 20:32
  • @Carson63000 Great observation. Hadn't heard that concept put so succinctly. Here's a link to the wikipedia article. en.wikipedia.org/wiki/Defense_in_depth_(computing) – Zachary Yates Jan 11 '13 at 21:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.