235

Penetration testers found out that we allow single quotes in submitted data fields, and want us to apply rules (input validation) to not allow them in any value.

While I'm aware that single quotes are popular for SQL injection attacks, I strongly disagree that they should not be allowed as valid input. I am advocating for actually preventing SQL injection by means of using prepared statements (which properly quote the values) instead of filtering out anything that remotely looks like being an SQL fragment.

My case:

  • Person names can contain single quotes (such as O'Reilly)
  • Text fields can contain single quotes (such as I'm pretty sure)
  • Number fields can contain single quotes (EUR 1'000'000)
  • and many more

I've seen other cases where applying SQL injection prevention rules dicarded valid data for the silliest reasons (name "Andreas" rejected because it contains an AND, and various common words in plain text fields being rejected because they contained the keywords "select", "insert", "update" or "delete").

What's the security professionals' stance on that matter?

Shall we reject implementing input validation for single quotes for the reasons I stated?

  • Comments are not for extended discussion; this conversation has been moved to chat. – Rory Alsop Feb 9 at 11:39
  • I would be concerned if your pent testers think that preventing the user from submitting an apostrophe will somehow prevent sql injection attacks... – Storm Muller Feb 14 at 8:56
  • I agree with your logic. I would have them try to execute a sql injection attack on a UAT/QA environment. Also they need to understand that user input is not dangerous, it's how it's processed that makes it potentially dangerous. – Storm Muller Feb 14 at 9:02

12 Answers 12

320

You should implement input validation as a defense-in-depth method. So input validation should not be your primary defense against SQL injection, that should be prepared statements. As an additional defense you should restrict the allowed inputs.

This should never ever restrict functionality. If there is a legitimate use case to have apostrophes in input, you should allow it. So you should allow single quotes in name fields, descriptions, passwords, but not in number fields, username fields, license plate fields.

To block single quotes in all input is madness. This breaks functionality of the application and isn't even the correct solution against SQL injection.

Consider the possibility that you misunderstood the pentesting company. If this is seriously their advice, this reflects badly on the pentesting company and I would advise you to search for a pentesting partner that helps to properly secure your software, instead of making it unusable.

27

It's clearly wrong in the context of injection attacks - either your database layer is processing strings correctly or it doesn't. Since apostrophes are valid in names and free text, blocking them entirely will break the application, and blocking them selectively wouldn't fix the injection problems.

But strict input validation is good practice on general principles, and being overly permissive doesn't make sense in cases where the apostrophe is not part of a legitimate value. You give the example of EUR 1'000'000, which is a locale-specific format (Switzerland only, AFAIK) - but allowing the format to be part of the value makes no sense there. If the user enters 1,500, should your application store that as is? Will you have to decide each time it is processed whether it should be interpreted as 1.5 or as 1500? It would make more sense to handle the locale-specific presentation on the client side, and process the numeric value in a canonical form internally.

So the answer here would depend on whether the audit is complaining about specific fields where it makes sense, or recommending a blanket ban on apostrophes. If the former, it's a legitimate point. If the latter, they're stupid and probably blindly following a checklist.

  • 1
    Note that in the locale specific format case, the best answer may not be to disallow it but to parse it somehow and generate the number without it. – jpmc26 Feb 9 at 6:58
  • @jpmc26 Numbers are easy: an user supplied numeric input string can contain either an integer or a decimal number. For the first one just drop all non-digits and proceed. For decimals I usually look for the first dot or comma character from right to left drop the rest and then replace it with whatever decimal separator is needed and continue. – beppe9000 Feb 9 at 17:25
  • @beppe9000 Even easier would be to use a parsing library. It might need to be front end, though, to pick up on the user's locale. – jpmc26 Feb 9 at 22:32
  • @jpmc26 Yeah, localization belongs to front end. Data intake only cares about normalization: when serving pages one can use js libraries to format strings according to user preference (wether is stored in the db or as a cookie). – beppe9000 Feb 10 at 14:56
  • 2
    @beppe9000 So how do you interpret "1,500" without locale information? Fifteen hundred, or one and a half? – Guran Feb 13 at 9:02
17

Step 1) Parameterize your SQL.

Step 2) Ensure you are using the SQL DB Connection library to set values for your parameters, not just setting them inline. This is the actual defense against SQL injection.

Step 3) Don't do query building in SQL. That way lies madness.

Step 4) add a config switch to propagate the error all the way back to the user. Turn it on during testing.

Step 5) Tell your penetration testers to find a way to generate a SQL error with an odd number of single quotes or shut up.

  • +1 for step 3). Absolute truth. – bytepusher Feb 12 at 20:22
12

Prepared statements (parameterized queries) are great just make sure you implement it correctly. I've seen "prepared statement" implementations that were every bit as vulnerable. For discussion of implementation details I recommend stack overflow.

Also nothing wrong with defense in depth (input validation in this case) but do it well...rejecting all single quotes is probably not best practice :)

11

I'm not a security person. I'm a programmer who has to maintain secure code. This is what I call a "brittle" practice. Entry points are scattered all over a typical project. Finding and sanitizing all of them is a lot of work to address only a single problem, a lot of careful maintenance and hassle to ensure it remains effective as the code changes, and full of assumptions which render it ineffective.

Instead use practices which are easier to maintain, layered, contextual, and solve a broad swath of problems. Then you don't need expensive, overly-broad filtering.

You can't secure input if you don't know how they will be used.

Let's say you've "secured" your system by stripping out all single quotes from all input across the board. Great, you're safe against one type of SQL injection attack. What if that input is used in a...

  • MySQL query which allows double quotes
  • Filesystem operation
  • Shell command
  • Network query
  • Method name
  • Class name
  • eval

Each of these have different special characters, escape sequences, quoting rules, and security practices. You can't possibly predict how your input will be used when it comes in. Trying to strip out all special characters is madness and only "solves" one class of attack.

Or what if the user is allowed to enter a page limit. That limit is dutifully used in a parameterized query; no SQL injection, yay! The user enters 9999999999 and now you're open to a DOS attack.

You must apply the appropriate security measures at the point where the potentially insecure operation is performed. This takes into account many factors unique to the operation; sanitizing input characters is just one.

And as long as you're doing that, you might as well also parameterize your queries. Then there's no longer a need to do all the work and damage of blanket stripping quotes.

Filtering all input is hard.

There's many, many, many ways to get and pass around input in a given project:

  • form inputs
  • urls
  • file names
  • file contents
  • database queries
  • network reads
  • environment variables

These are typically pretty free form and can use many different libraries. I'm not aware of any static analysis tools which verify all potentially vulnerable input has gone through filtering. Some languages have a taint system, but they're difficult to use effectively. Even if you filter all inputs, without a static analysis tool unfiltered inputs will leak back in as development goes on. It's a lot of effort for an incomplete, expensive to maintain result which hampers functionality.

In contrast, there's typically only one way to execute SQL in a project. Static and runtime tools exist to automatically detect potential SQL injection. You can even disallow strings altogether and require that all queries be SQL query objects. These good practices are easy to maintain and increasingly baked into tools and SQL libraries.

"Firewalls" lead to lax security.

Similar to how some office networks have very insecure practices because "we have a firewall", there is a risk of the team becoming lazy about securing their code because "the input is safe". The input is most definitely not safe.

Opportunity Cost

Some might say "why not both?" You only have so many hours to work on a project. A low efficiency, high maintenance practice is a time suck. Implementing and maintaining it will take your limited time away from more efficient, easier to maintain practices. In the worst case you'll spend so much time playing whack-a-mole with inputs, and the subsequent problems caused by the too aggressive filtering, that you'll never get time for proper security measures.

In short, input filtering is expensive, leaky, difficult to maintain, cannot solve the problem, and might make it worse.

8

As you said yourself, if you're using parameterised queries, then the single quotes isn't a problem. In this case, you can reject their advice. If doing so, highlight the fact that you are using parameterised queries and that this aids usability as well (using your previous examples).

8

If you're 100% sure you always prevent SQL injection everywhere, this indeed is nonsense.

However, SQL injection is one of the most common security risks, and even if you're sure you've properly written your application to use parameters, a sloppy DBA might execute a query that's at risk for second-order SQL injection. It might not even be stored anywhere, it could just be a query to copy a table.

Second-order attacks are harder to execute, but harder to protect against. Protecting against second-order attacks means that every dynamic SQL statement ran on the database with write permissions needs to be checked for a risk of SQL injection, not only SQL statements that process input from untrusted sources.

Disallowing quotes everywhere is a sloppy protection against second-order attacks, but does make them less likely. In an ideal world it wouldn't be necessary, but unfortunately we aren't living in one.

If many users have any form of write access on the database and are able to write their own SQL statements, it might be a sensible security measure. If your application is the only way to access the database, and only very knowledgeable users can execute their own queries with write access, it's typically not necessary.

  • 3
    Admittedly I'm not well-versed in pentesting or PL/SQL (which I assume this is about) in general, but second-order SQL injection seems really horrible. If we collectively have learned that using eval() on user input is not a good idea, why then do we ever use that kind of construct in PL/SQL? I hope there is another way to do that query behind your link that does respect the difference between code and data. – tomsmeding Feb 5 at 9:53
  • 2
    @tomsmeding Second-order injection is possible in most SQL dialects (T-SQL through sp_executesql, PL/SQL through EXECUTE IMMEDIATE, MySQL through EXECUTE). They all support parameters, so you can do it properly, but there are limitations. For example, I've migrated Access SQL pivot queries to T-SQL, and the only way to do this in T-SQL is to use string concatenation to achieve dynamic field names like Access does (with escape characters, but escape characters are bad, and I've encountered example scripts without them), since you can't use parameters for field names. – Erik A Feb 5 at 10:03
  • @tomsmeding: Unless things have changed, there isn't any nice way to execute something of the form "select * where id is in [list]" without using dynamically-generated SQL to specify the list. I wouldn't try such a thing with user-specified items in the list, but with numeric IDs processed using numeric data types I don't see any injection risk. – supercat Feb 5 at 19:10
  • 4
    PostgreSQL provides a vastly better defense against injection into EXECUTE IMMEDIATE queries: a set of functions that will properly quote the input. They are quote_ident, quote_literal, and quote_nullable. Obviously, avoiding EXECUTE IMMEDIATE is preferred, but these are worlds better at sanitizing if you must use it. – jpmc26 Feb 6 at 21:58
  • 1
    @supercat I don't know about psycopg2 specifically, but I do know that some client libraries expand the array out into individual parameters for some databases, like IN (:p1, :p2, :p3...). So that's at least one technique. – jpmc26 Feb 6 at 22:01
6

While I do not know the specifics of your application, I follow your argument.

There are fields which do not need to contain certain characters. With those fields, you could use input validation to filter single quotes (and double quotes, and whatever else).

If your escaping didn’t work correctly, input validation might be a mitigation strategy, but using prepared statements (correctly) should be the preferable approach in mitigating risks of SQL injection.

4

If this is the result of a genuine Penetration Test, then they should be able to provide you with a value to submit that proves that this is an exploitable issue. If they cant then I would suggest asking for a proper penetration test, where they prove this is exploitable.

If however this is the result of a generic Vulnerability Scan then I would expect fuzzy generic responses like this, that would just flag on being able to insert a single quote. In this case, if you are happy that there is no issue, then you can happily ignore that result.

  • This answer doesn't actually address the question. The question is not about the testing methodology, but how to filter. Please make sure that your answers address the question directly. We love different perspectives and information, but this appears to be a tangent. – schroeder Feb 5 at 12:14
  • 2
    @schroeder: He says in two ways "so you could type a single quote in, now where is the vulnerability?" and the second challenges the frame that filtering out single quotes is good for security. – Joshua Feb 5 at 16:36
  • 1
    @schroeder The question is "What should I do about this report from a Penetration Test?" This answer says "Check if it's a genuine vulnerability, and if not, ignore it as a false positive." I'm not sure why that would be a tangent. – IMSoP Feb 8 at 14:28
  • @IMSoP that's not the question at all, that's a meta abstraction of the question. If that is in fact the question, then the OP self-answers (the OP outlines this very answer). – schroeder Feb 8 at 14:33
  • @Joshua the OP specifically challenges the frame in a very detailed way, so again, I'm no sure what this answe ris providing. – schroeder Feb 8 at 14:34
1

From an ex web developer and now a pen tester myself, I would not want restrict user input but this can be a major issue. I know that i have used this technique myself to compromise web applications and databases.

My opinion would be to check your DB install and web language (php) config for handling escape characters then code a module(s) to iterate the input to make sure it is properly formatted.

An apostrophe can be a valid input but can also escape your database statement being passed and introduce an attack vector. DB's and web languages have modules that can handle these types of instances but it is still a good idea to write your own module to double check.

  • 5
    OP has properly parameterized everything, so escape characters should not be used anywhere. Also, they don't always provide security, even when implemented sort-of properly. – Erik A Feb 4 at 15:54
  • 1
    Agreed, depending on the stack and frameworks you use, additional sanity checks can be useful. In my case, I'm using Java/JDBC with Prepared Statements, over Spring Data JPA, with parameterized JPQL queries - that stuff is pretty much rock solid. – Peter Walser Feb 4 at 16:08
1

I think this is a matter of perspective. Your system has a functional requirement that should come first. That doesn't mean it's not worth considering the issue of input validation. BTW, the two are not directly at odds. I don't recommend this, but it would be possible to encode and decode in the front end and have the SQL storage and query use the encoded form.

The balance is subjective and depends on a host of things not mentioned. You might try asking them to clarify. They may have a very good reason, they may not.

-7

I'll contradict most of the other current answers.

Your pentesters are almost certainly 100% correct.

My assumption: no pentester worth their salt would have reported this unless they had found that your application accepted and echoed back apostrophes in a situation where no apostrophes were valid.

Perhaps your usernames, phone numbers, domain names and dates should all have specific formats and character ranges. All should lack apostrophes. But instead, you're just accepting any old string they give you, for all these fields.

If this assumption is false, and you are already validating your inputs as strictly as you can, then they are wrong, and you are fine.

If this assumption is true and apostrophes would be invalid in that input, then:

  • Yes, you should be rejecting invalid inputs outright.
  • You should not be permitting people to pollute your database with corrupted data.
  • You should not be attempting to clean up that data at display-time, any more than you'd try to clean <script>, because your attacker will just find a way to exploit your cleaning algorithm, like <script > or <scr<script>ipt> or +ADw-script+AD4- or whatever.
  • You are right that there are exceptions, but they should be clearly defined as special cases: they should not be considered the norm.
  • Unless your requirements are explicitly to handle regionalised Swiss currencies with apostrophes in, your example of "1'000'000" is no more a valid integer than "1~000~000" or "1banana000apple000". Reject it. Don't try to clean "1'000", you don't know if they mean 1,000 or 1.000 or 14000 or a foot or a degree or something entirely different.

Query parameterization avoids only most classes of SQL injection: not all possible abuses of invalid data.

But what about all the other systems which rely on the username conforming to the company standard? You just broke them.

  • Do users have home directories? That's gonna be a problem.
  • Do their names get logged anywhere?
  • Do they ever get displayed?
  • Do usernames ever get used in command parameters of system calls?
  • Are they ever send in AJAX or XML data?
  • Are there any batch mode operations which run on batches of usernames, say the names starting A thru M one day, N thru Z the next, 0-9 on day three, then repeat? Those batches won't ever run against your user '-_haxxor_-'.
  • Are there cases where posing as another user would be harmful or useful to someone, so you want them to all have unique names? But they could pose as the user John by registering as something like John Ϳο𝗁ո Јоհ𝗇 or Ꭻօ𝚑𝗻.
  • All systems which use the name cannot reject the value you gave. They will instead have to handle invalid inputs, trying to clean or escape them, even though we've already shown that's insecure and doomed to fail. But since the user has long since logged out, they won't see any error that asked for a new email address.
  • Your DBA now has crap in his database. This will cause him a lot of pain not just in day to day work, but also when he has to migrate that data, because any tighter constraints in the target system will break on the old data.
  • Your colleagues and other consumers of your data now have to validate everything they read from the DB, because they can't trust you to have enforced even the fully documented standards.
  • Your users are now using a less secure system.
  • You now have to go and rework all those inputs to ensure you aren't feeding crap to your database any more.

Query parameterisation is not an alternative to correctly sanitizing your inputs.

  • Let us continue this discussion in chat. – Dewi Morgan Feb 6 at 18:56
  • 2
    It looks like my comment got deleted in a mass cleanup, so I'll reiterate for those not clear why this answer is getting downvotes: sanitising input is no substitute for correctly handling data on output. Of the examples given in this answer, only the impersonation case is reasonable to implement at input-only. Every one of the others can only be correctly handled by escaping or filtering data where it is being processed. Input validation is useful for giving feedback to users, but it will only increase security if you're doing something wrong somewhere else. – IMSoP Feb 9 at 14:38
  • 4
    Query parameterisation, plus its equivalent wherever else you use the data, is an alternative to sanitizing inputs; indeed, it's a superior alternative if your aim is security. It might feel reassuring to say "your username must not contain an apostrophe", but all it says to an attacker is "they must be using this in a raw query somewhere, I should find a way to trick them into creating a user with an apostrophe in". Unless you can be 100% sure your validation catches every problem, and there is no possible way of by-passing it, you have to defend against bad data as you use it. – IMSoP Feb 11 at 17:47
  • 2
    Fundamentally, the problem with treating input sanitization as a security measure is that there is no such thing as "secure data"; the security depends on the context where you use it. Since you can't predict every possible context you're going to have in the lifetime of your data, you can either limit your features and usability by restricting every field to A-Z, a-z, 0-9; or you can treat sanitization as an incomplete measure backed by the "real" security written into each context as it's encountered. – IMSoP Feb 11 at 18:08
  • 1
    I don't see how the range batching problem would be "unaffected by the other two axes". If you know you can 100% trust all inputs to be alphanumeric, the algorithm described is acceptable; but in the highly likely case it's not, you need to add an assertion that alerts an operator, or an "and anything else" clause. Thus, the most secure solution is the one closest to the processing, not the one that trusts some input form 10 years previously to have predicted the scenario. – IMSoP Feb 11 at 22:43

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.