34

Our website is 100% API based (with an SPA client). Recently, a hacker managed to get our admin's password (hashed with SHA-256) through SQL injection (and cracking pwd) like this:

https://example.com/api/products?userId=3645&type=sqlinject here

It is just an example, but it is deadly to us. Fortunately, it was a nice hacker and he just emailed us about the hack. Lucky for a website under constants attacks throughout it's 5 years of existence.

Yes we DO know that we need to check user input everywhere and do properly format/escape/prepared statement before sending data to MySQL. We know, and we are fixing it. But we do not have enough developers/testers to make it 100% safe.

Now assuming we have 0.1% chance of being SQL injected somehow. How do we make it harder for hacker to find it and limit the damage as possible?

What we do so far as quick fixes/additional measurements:

  1. At the output "gate" shared by all APIs, we no longer send the raw PDOException and message like before. But we still have to tell the client that exception occurred:

    {type: exception, code: err643, message: 'contact support for err643'}

    I am aware that if hacker see exception, he will keep trying...

  2. The user PHP uses to connect to MySQL can only CRUD to tables. Is that safe enough?

  3. Rename all tables (we use open source so hacker can guess the tables name).

What else should we do?

Update : since there are many comments, I would like to give some side info

  1. First of all, this is LEGACY app, developed by some student-like folks, not enterprise grade. The dev team is nowhere to be found, now only 1-2 hybrid dev-test guy handling hundreds of Data Access class.
  2. The password is hash with SHA-256, yes it sucks. And we are changing it to recommended php way.
  3. SQL injection is 17 years old. Why is it still around?
  4. Are prepared statements 100% safe against SQL injection?
Phung D. An
  • 1,051
  • 2
  • 11
  • 13
  • Comments are not for extended discussion; this conversation has been [moved to chat](https://chat.stackexchange.com/rooms/79300/discussion-on-question-by-phung-d-an-how-to-limit-the-impact-of-and-reduce-the). – Rory Alsop Jun 24 '18 at 19:07

11 Answers11

80

Don't spend lots of time on workarounds or half fixes. Every minute you spend trying to implement anything suggested here is a minute you could have spent implementing prepared statements. That is the only true solution.

If you have an SQLi vulnerability, the attacker will probably win in the end no matter what you do to try to slow her down. So be aware that while you may make improvements, in the end you are playing a losing game unless you fix the root cause of the issue.

As for what you have tried so far: Hiding error messages is a good start. I'd recommend you to apply even stricter permissions (see below). It is debatable whether changing table names help, but probably it won't hurt.

Ok, so what about those permissions? Limit what the database user can do as much as possible, down to table or even column level. You may even create multiple database users to lock things down even more. For instance, only use a database user with permission to write when you are actually writing. Only connect with a user that has the right to read the password column when you actually need to read the password column. That way, if you have an injection vulnerability in some other query it can not be leveraged to leak passwords.

Another option is to use a web application firewall (WAF), such as mod_security for Apache. You can configure it to block requests that look suspicious. However, no WAF is perfect. And it takes time and energy to configure it. You are probably better off using that time to implement prepared statements.

Again, don't let any of this lure you into a false sense of security. There is no substitution to fixing the root cause of the problem.

Community
  • 1
Anders
  • 65,052
  • 24
  • 180
  • 218
  • 5
    I can't endorse the blanket advice of only connecting with a particular user when needing a particular operation. That becomes impractical too quickly. If you really want, some modest separation would be okay, like separate writing to the user tables from everything else. But you need to have all queries for a single request in a single transaction. Trying to ensure that's the case for every possible code path would be an enormous burden if you have a lot of different DB users. – jpmc26 Jun 21 '18 at 17:51
  • Generally I agree with this, and there's a lot of good stuff here. I don't really agree that devoting time to one solution subtracts time from another. A good WAF can be setup by a systems person that doesn't have specific knowledge of the application code base. The same is true for database changes, which can be done by a DBA. The point being, peoples skills vary considerably, and each person is suited to different tasks. If you can leverage peoples skills well, you can work better as a team. – Steve Sether Jun 21 '18 at 18:10
  • 11
    _"It is debatable whether changing table names help, but probably it won't hurt."_ - It can hurt your own developers down the line, surprising them every time they have to access a table that's not named the way they expect. – marcelm Jun 22 '18 at 09:09
  • I have used PDO with prepared statements for quite a while now, but it becomes stressful after a while. I recently started using [Medoo](http://medoo.in). It's a simple, but powerful wrapper for PHP, useably for MySQL, MSSQL, SQLite and more. You should take a look at it, it really makes your life easy since you don't need to think about prepared statements anymore. – Robin K Jun 22 '18 at 12:16
  • 3
    @RobinK I'm very sceptical that using prepared statements is significantly more "stressful" than manually concatenating strings together everywhere... and, frankly, who cares when it's simply the bare minimum of responsibility to have when programming using SQL? I'm not sure just giving up and adding some third-party library is going to do much more than complicate things. – underscore_d Jun 25 '18 at 06:49
  • I'd also consider to invest the money you supposed to use to _"...limit the impact of and reduce the risk..."_ to pay a professional pentest (obviously while your devs are reasonably sure they did their job) – Adriano Repetti Jun 25 '18 at 13:25
65

The only correct way is to use prepared statements.

  1. If you disguise error messages, it a bit harder, but won't stop attackers.
  2. You can restrict the rights, but all rights granted to the user could gained by the attacker. It is also possible to execute shell commands from an SQL-Injection in some cases.
  3. Renaming tables won't help you. The tool sqlmap will brute force the table names for you char by char. This doesn't need much time.

You could restrict the number of calls too, to make it harder for attackers or make alerts on suspicious calls and stop this manual, but the only correct way addressing this issue is using prepared statements. Just grep through your source code and take a look on SQL-Statements with dynamic contents and replace them.

trietend
  • 844
  • 1
  • 6
  • 15
  • Take care with restrictions, since it can quickly impact legitimate users knowing that the issue here concerns a `GET` request, so could be injected in a `` tag so all users seeing the page with this tag will trigger a request, and might end up being locked down by the website. – Xenos Jun 21 '18 at 07:46
  • 1
    It might also be a good idea to make code reviews part of your process, e.g. review everything that is merged to the main or production branch. Have coding guidelines that define how things should be coded and what is prohibited (e.g. it should not be allowed to string-concatenate parameters into a SQL query). This will keep new vulnerabilities from being introduced after you fixed the current ones. – Georg Patscheider Jun 21 '18 at 08:36
  • 1
    I find "the only correct way" a bit grandiose, or ambiguous at best. Maybe using an SQL-database was incorrect from the beginning? – phresnel Jun 21 '18 at 13:29
  • Ok, there may be other options, but if the OP is already running too short on programming time, those other ways are just going to be slower. Prepared statements is probably the most practical way to close the hole. – Guy Schalnat Jun 21 '18 at 19:20
  • 1
    @phresnel What kind of database should be used instead? – curiousguy Jun 22 '18 at 23:27
  • @curiousguy, ...you don't get SQL injection in a [Datalog](https://en.wikipedia.org/wiki/Datalog) database. Then again, that tends to be because the recently-popular implementations ([DataScript](https://github.com/tonsky/datascript), [Datomic](https://www.datomic.com/)) take LISPy datastructures as query specifications -- not strings -- so there's no temptation to try to build those queries via string concatenation. – Charles Duffy Jun 24 '18 at 22:30
  • @curiousguy: It depends on the whole platform and its requirements. For basic authentication requirements and just a few ten thousand users, a simple map/dictionary/associative array loaded into a daemon's/service' memory and stored in a CSV, XML or JSON file could be the right thing. An implementation of this would be simple, or use CouchDB, for instance. Endless possibillities: It's basically just a data structure and algorithm choice, tho' I realise that even "Bachelors"/"Masters" in Germany seem to have forgotten data structures and algorithms. For starters: https://bit.ly/2lEy9GL – phresnel Jun 28 '18 at 07:46
  • "_The only correct way_" I strongly disagree with "only correct way". There are many ways corrects ways. (They end up doing something equivalent.) – curiousguy Jun 28 '18 at 18:05
26

We know, but we do not have enough developers/testers to make it 100% safe.

This is the real problem. Until you hire more people or reassign priorities, you're not safe. Stopping 99.9% of attackers means that your data has been compromised. Band-aid solutions are a bad thing, and they do not work. Don't waste time refactoring your SQL access pattern while you could be fixing actual problems.

As for the code solution, using prepared statements, like many people here suggest, is the easiest way to safely use SQL.

It's a lot easier than trying to use php to clean the arguments yourself, and costs a lot less time. Just grep your entire codebase for everything SQL related and fix it.

Gloweye
  • 388
  • 2
  • 8
  • 4
    I don't think hiring more people would help as a [short term solution](https://softwareengineering.stackexchange.com/questions/14968/why-does-adding-more-resource-to-a-late-project-make-it-later). And security concerns need immediate solutions. Your other suggestions are spot on. – xDaizu Jun 21 '18 at 15:32
  • @xDaizu and Jacco : thanks for your interest. we have like thousand of legacy Data Access Class but just 1-2 developer to fix things, we will do "prepared statements" but we are just not 100% sure – Phung D. An Jun 21 '18 at 17:48
  • 8
    @xDaizu While I normally am a firm supporter of the idea that you can't bring more people into a late project to speed it up, finding people who can translate SQL into prepared statements is relatively easy as it's a pretty basic and universal skill for the most part. – wedstrom Jun 21 '18 at 22:26
  • 1
    hiring lot of people late might in general be bad, but as @wedstrom said, just translating into prepared statements should be something people who reply to a PHP job opportunity can do without much introduction. Proper introductions could be after that. For example, refactor code until you have those thousand legacy data access class with a common superclass which contains all the SQL groundwork. – Gloweye Jun 22 '18 at 06:47
9

Purge all SQL from your code forever

The best solution to this problem is to delete all SQL code as literal strings from your code and never introduce it again. Instead, use ORM software like Hibernate or Entity Framework. This software is much nicer to use than raw SQL anyway, and automatically parameterizes your SQL when it eventually goes to the database. If it is possible for you, have this policy.

If you do not have the time to learn and implement these packages, or cannot use them for some other reason, trietend's answer is the next best thing, using prepared statements. This will allow you to be safe from SQL injection. That is, until you put any pressure on your developers to deliver quick solutions, at which time they may once again forget to parameterize a single variable, leaving you back where you started.

Nacht
  • 925
  • 1
  • 6
  • 12
  • 31
    This may be a good security advice, but it is a **terrible** development advice. ORMs are NOT a silver bullet, and using ORM can easily lead to performance issues. I have seen a ORM pull up a full table in memory and executing the filters in memory because it didn't manage to lower them down to SQL. I have also seen users of an ORM inadvertently making a query within a loop (because it just looked like code!). So, no, ORMs are not a silver bullet... and for any half-serious applications, I'd recommend to use good SQL framework. – Matthieu M. Jun 21 '18 at 10:25
  • 18
    @MatthieuM. not knowing how to use a technology is not an excuse to never use it. ORMs, as any other tool, are subject to caveats and a learning curve, and I can assure you they're perfectly fine to use in "half-serious applications" if used correctly. – BgrWorker Jun 21 '18 at 12:04
  • 10
    @BgrWorker: This is more an issue of leaky abstraction, than an issue of knowledge. It's actually very easy to use an ORM, and for most situations it'll work just fine. Then, suddenly, an "innocent" refactoring will introduce a performance issue for various reasons... but functional tests will pass because it's still correct (just doing a lot of unnecessary work). This is why I favor a good SQL framework: it emphasizes the difference between a cheap getter and an expensive SQL query, as well as clearly delineates which parts of the code are relying on an SQL connection. – Matthieu M. Jun 21 '18 at 12:33
  • 2
    I think "The best" is too absolute. Maybe SQL is not justified at all? – phresnel Jun 21 '18 at 13:31
  • 5
    @MatthieuM. An innocent refactoring of SQL can cause performance problems too if it prevents use of indexes. – AndrolGenhald Jun 21 '18 at 14:49
  • 1
    @AndrolGenhald: Yes, as can discrepancies in table structure between development/test/production environments, and a myriad variables. However, the main advantage of a SQL framework instead of an ORM here, is that it's immediately visible in the code review that you are touching SQL: which tables, which queries, etc... unlike ORMs where since it looks so much like code it's easy to overlook. – Matthieu M. Jun 21 '18 at 15:41
  • 13
    If the OP is having trouble to use their current technology properly, switching to a new technology is going to take even longer. First, secure the site as fast as possible. Second, when there is more time, investigate what is the best technology for the future. All technology solutions have pros and cons. Anyone who says otherwise is trying to sell you something. – Guy Schalnat Jun 21 '18 at 19:29
  • @MatthieuM. How are ORMs not a silver bullet for SQL injection, which is what this question is about? – Nacht Jun 22 '18 at 01:44
  • Furthermore, would you rather have a webpage that takes 30 seconds to load, or your data leaked all over the Internet? One of these problems is fixable; the other is not. – Nacht Jun 22 '18 at 01:48
  • 2
    @Nacht: ORMs are not a silver bullet from a *development* point of view. They may solve the particular issue of SQL injection, but do not solve all problems. As for your strawman question: both problems are fixable, and neither is inevitable. A good SQL framework *also* avoids SQL injections (because SQL queries are not strings) while making it obvious where database interactions occur, and what queries are executed. – Matthieu M. Jun 22 '18 at 12:02
  • 2
    @Nacht Most ORMs have their own query language e.g. HQL. Here's an example CVE for an HQL injection attack. ["Hibernate Query Language (HQL) injection attacks and obtain sensitive information via the entityName parameter."](https://nvd.nist.gov/vuln/detail/CVE-2016-1595). If all you do is change SQLi to HQLi, I'm not sure that really solves anything. – JimmyJames Jun 22 '18 at 14:49
  • @AndrolGenhald Refactoring to prepared statements does not require refactoring the SQL. It can often improve performance because the statements are not recompiled on the database on every execution. What gets tricky is when there is complex logic in trying to dynamically generate different statements (e.g. appending different where clauses) based on different conditions. Ibatis has a decent solution for this using templates and generates a prepared statement for each distinct query that is executed. – JimmyJames Jun 22 '18 at 15:01
  • @Nacht Here's a better example of how an ORM is 'not a silver bullet for SQL injection' that discusses how it can happen: https://cwe.mitre.org/data/definitions/564.html – JimmyJames Jun 22 '18 at 15:03
  • @JimmyJames I wasn't trying to imply that refactoring SQL would be required, I was providing a counterpoint to MatthieuM's point that an innocent refactoring could cause performance issues when using an ORM. – AndrolGenhald Jun 22 '18 at 15:12
  • 2
    @AndrolGenhald My experience with heavyweight ORMs is that trying to use them with a database that wasn't designed to work with them is problematic. It's really difficult to implement one without some level or refactoring of the code, database structures, and/or queries. Refactoring to use prepared statements *can* definitely be done without altering the structure of any of these but the level of effort will vary. – JimmyJames Jun 22 '18 at 15:54
  • 1
    @JimmyJames Again, not what I was saying. I was simply saying that arguing against using an ORM because "it'll cause performance problems" isn't necessarily a good argument because performance problems can happen using SQL directly too. – AndrolGenhald Jun 22 '18 at 16:05
  • 1
    @AndrolGenhald Is it the strongest argument for not moving? Maybe not but the OP already has SQL. Moving to an ORM is unlikely to be a drop in replacement where changing to prepared statements more or less it. In my (largely uniformed) estimation, it's much more likely that the OP will introduce changes to the execution plans will happen when moving to an ORM than when refactoring to use prepare statements. – JimmyJames Jun 22 '18 at 16:51
  • @JimmyJames let us [continue this in chat](https://chat.stackexchange.com/rooms/79246/) – AndrolGenhald Jun 22 '18 at 17:03
9

We know, but we do not have enough developers/testers to make it 100% safe.

It is not entirely clear what you mean by this statement, as prepared statements are trivial in every popular web language (PHP, Python, Java, node.js, etc) and are more-or-less a 100% effective measure against SQL injection.

Do you mean you subcontract out development and can't afford to QA the code they write for you on contract? You can't afford not to: you are still the responsible party here.

Do you mean that your developers are too busy implementing features to take a few hours to update the codebase? Everything other than prepared statements will take longer and cost more (e.g. ORM, obsfucation, fine-grained permissions, etc).

Do you mean your developers are not competent enough to perform this simple task? That's a waaaay bigger problem (or to be more precise, SQL injection should not be your biggest concern at that point).

Do you mean your codebase is such a shotgun-formatted spaghetti-logic mess of nested includes that even competent developers will take forever to perform what would otherwise be a simple task of a few hours? Likewise, bigger problem.

Whichever one it is, the solution is to use prepared statements, circa yesterday.

Jared Smith
  • 1,978
  • 1
  • 11
  • 12
  • I mean we have like thousand of legacy Data Access Class but just 1-2 developer to fix things, we will do "prepared statements" but we just aren't 100% sure. Like come on, even Facebook has security hole – Phung D. An Jun 21 '18 at 17:45
  • 4
    @PhungD.An I'm sorry, that's rough. 1000 places to change for 1-2 devs is not as trivial as it would be with a saner architecture. But it's still doable, just in days (like 1-2) instead of hours. As for 100% sure, you should be pretty sure just from running `grep` over your codebase but if you get sued "we were busy and it sounded hard" is not going to be a very credible defense...any more than "some guy at a university asked for all our user data and we just gave it to him and trusted him not to sell it" is going to be for fb. – Jared Smith Jun 21 '18 at 18:17
  • 4
    @PhungD.An The big problem with moving from concatenating strings into SQL to prepared statements is generally that it takes a while to figure out what the existing mess does. The time put into fixing this will: improve security, improve performance, and make the code easier to understand. Trying to sanitize inputs is an endless task that will never solve the root issue and will not improve the code base. – JimmyJames Jun 22 '18 at 14:53
  • 3
    @PhungD.An Security holes can and _will_ get very expensive for you if they ever get exploited. Much more expensive than any time or money you think you're saving by not doing the right thing. – Cubic Jun 22 '18 at 14:55
5

Once the initial "prepared statement" advice has been implemented, I would recommend some reading about SQL injection. Asking "how do I prevent SQL injection" is a rather broad question! Security is a subject that you (rather, your entire dev team) need to be very familiar with in order to reduce the risk of compromise. One hole undermines all your other effort.

Visit the Open Web Application Security Project (OWASP) here: https://www.owasp.org/index.php/SQL_Injection

Particularly the additional material:

  • SQL Injection Prevention Cheat Sheet
  • Query Parameterization Cheat Sheet
  • Guide article on how to Avoid SQL Injection Vulnerabilities

And also

  • How to Review Code for SQL Injection Vulnerabilities.
Pang
  • 185
  • 6
Nicolas
  • 159
  • 1
  • I love OWASP for understanding of why you are doing the right thing, but there are plenty of other resources that are specific to the OP's technology stack that are easier to start with. As with many security issues, the best thing is to not reinvent the wheel. The basic security practices of your chosen technology is the best start. – Guy Schalnat Jun 21 '18 at 19:23
  • Actually, how to prevent SQL injection is a very narrow question with a very simple answer: Don't construct query strings from user controlled data. Avoiding this in the first place is easy enough, but of course finding places that do this in a large existing codebase can be tricky. – Cubic Jun 22 '18 at 14:53
  • @Cubic Fair enough, the OP question itself was narrow. I was perhaps reading past the question itself: If someone is asking that type of question, then is seems that what they actually need is to gain a general understanding of best practices. (and, as Guy Schalat says above, best practices for _your technology_) – Nicolas Jun 22 '18 at 16:08
4

A stopgap measure would be to look into a web application firewall (WAF)- there are a few companies out there that provide this as a service, and a few cloud providers that also have offerings. CloudFlare seems to offer one, I've had one client use Incapsula, and I know the AWS WAF offering has some managed rulesets for SQL injection.

The way this works is all traffic is sent to/through the WAF (either by including it on servers, or setting DNS to point to the WAF as you would for a CDN), and it looks for things that look like SQL injection attempts in the parameters. It won't catch all attacks, and may block legitimate traffic, but it is an available bandaid fix.

CoderTao
  • 151
  • 4
  • +1 for actually answering Q and not repeating what was already said about user input sanitation. Of course prepared statements shoul be implemented but depending on s/w it might be a lot faster to get WAF up and running, it might not be perfect solution but it will probably help with immediate situation. – Sampo Sarrala - codidact.org Jun 24 '18 at 10:54
2

By "do not have enough developers" I take it you mean that some influential people in your organisation think there are higher priorities, so whatever resources you do have are devoted to those things. So fixing your security issue is losing out to other things in a cost-benefit analysis.

Your task here is to change their minds, as much as it is to implement the fix. Quantify what's at stake - what would be the damage, both in money and reputation terms, of another breach? There may be laws in your jurisdiction that require you to report compromises of your user's data. Would that be embarrassing? Might there be media reports of your organisation's poor practices?

Think of the investment to fix the security as insurance. In retrospect, after you know your building did not burn down for a ten year period, you might argue that your premiums were a waste. But you and your organisation do pay for insurance, because you face an uncertain future. It's risk management.

Risk has two factors: probability and impact. The probability of another breach is high - you know it can and has been done at least once. If the potential consequences are also bad, you should increase the priority of fixing your security vulnerabilities.

Concrete Gannet
  • 121
  • 3
1

As everyone has mentioned fix the code on the site.

You can make any patches you want to a wall, but until the wall is built correctly any patches will be a bandaide to bleeding problem..

Any other suggestion to clean code is what Linus Torvalds would call masterbation.

  • Fix your code (from most obvious injection points to the last I.E Get params) and use prepared statements. how can i prevent sql injection in php

    • You can try a WAF (I.E ModSecurity) temporarily until you're able to get things fixed.

    • Try Perhaps some sort of IP ban on 'people testing the site' or blocking unsafe queries being made.

    • If you have the ability make a temporary (safe) login screen in front until this is resolved.

    • Try a third party service until this is resolved something like cloudflare or whatever else is out there.

PDOs have been available since PHP 5.1 released in 24 November 2005 ... Devs need to understand that they're consequences to their poor coding habits and I personally think that there should be accountability to the quality of work being provided.

At any rate PDOs are an easy thing to implement . If your devs aren't able to provide better quality, then I would suggest a purge...

LUser
  • 836
  • 6
  • 12
0

The most important rule is:

Use libraries, tools and APIs that make the safe option the easiest option.

Nacht says "just use an ORM" which is a subset of this rule.

Prepared statements violate this rule, as they are cumbersome to use, result in code bloat, also they can be often slower than non-prepared statements depending on your database. They are a valid solution, I won't criticize anyone who uses them, but they are not necessarily the best and most convenient solution.

And.. we want the safe option to be easy and convenient, so the programmer uses it out of self-interest and laziness. We want the programmer to have to actually go out of their way and invest some effort in order to use the unsafe option!...

A much better solution is an API like Python dbapi. I wrote a php equivalent years ago, which was like:

db_query( "SELECT * FROM mytable WHERE id=%s", $id )

This is more convenient than prepared statements. Just one line to type. Also it returns the result in a form that foreach() can use, so it is a lot easier to use than fetch() and stuff. More important, the code hidden behind this API will handle escaping properly. This makes SQL injections irrelevant. This handles about 99% of queries if you don't use an ORM. Dynamic queries (usually search) will need special handling though, but it's still easy.

If you keep this rule in mind, then at some point you will have to wonder why you have to use htmlspecialchars(), and why the language doesn't have a feature that would make the safe setting (ie, no XSS vulns) the easiest to use? Why oh why? And that's when you drop PHP.

bobflux
  • 236
  • 1
  • 3
0

In addition to all the great answers so far, if you want to tackle the specific problem of extracting user credentials, you can refactor your database to have the passwords in a seperate table with strict access rights, similar to the way Unix systems put them into /etc/shadow.

To verify credentials, you need a stored procedure that takes the entered password (or hash) as a parameter and returns a boolean. In other words, you offload the job of verifying credentials to the database and the actual hash never leaves the database.

This requires minimal refactoring and solves your immediate problem at the root, instead of one specific SQL injection while leaving you vulnerable to the next one.

Tom
  • 10,201
  • 19
  • 51
  • While this sounds like a good idea at first, most databases don't support good password hashing functions, and database queries are often logged. I suppose you retrieve _just_ the salt and parameters, hash, then do another query to compare the hash, but that seems too complicated to be worth it for most sites. – AndrolGenhald Jul 06 '18 at 13:48
  • there is pgcrypto for PostgreSQL, for example. Don't know about other databases. Can't imagine stuff like Oracle doesn't have equivalents. – Tom Jul 06 '18 at 18:11