Sign in to follow this  
ArchG

[web] Protection against SQL Injetion Attacks

Recommended Posts

Hello, Recently our company got hit by a SQL Injection attack. I realize this is due to inproper, or unsafe coding in the SQL queries. I'm trying to go thru all our web apps and find the unsafe area. I'm reading that one way to fix the problem is to use Parameterized Statements, like this C# code
using (SqlCommand myCommand = new SqlCommand("select * from Users where UserName=@username and Password=@password", myConnection))
                {                    
                    myCommand.Parameters.AddWithValue("@username", user);
                    myCommand.Parameters.AddWithValue("@password", pass);
 
                    myConnection.Open();
                    SqlDataReader myReader = myCommand.ExecuteReader())
                    ...................
                }


Which I will definately start to do, right now what most of strings look like is something like this
string sSQL = "SELECT * FROM Users WHERE UserName='" + user.Replace("'","''") + "' AND Password = '" + pass.Replace("'","''") + "'";


*for some reason the replace part isn't showing up correctly in the source block, it should be replace one quotation, with a double quotation. Do strings like this leave us open for attacks? If not, what is the advantage of using Parameterized Statements? Thanks

Share this post


Link to post
Share on other sites
You want to do more than replace single quotes. #39 is a single quote, as well, in some contexts.

The approach I use is to strip everything that's not a letter or number from the string for user names. Passwords are hashed before insertion, so they are guaranteed to only contain "good" characters.

Share this post


Link to post
Share on other sites
Quote:
Original post by ArchG
Do strings like this leave us open for attacks?

Really, I can't say for sure. You can't either. So why take a risk? There's no reason to not use parametrized queries, so use them.

The only situation I found so far that is a bit more tricky are queries with a dynamic number of elements within an IN (foo, bar, etc.) condition. You can't simply say IN (@elements) and pass a list of values or so. What you can do is to build the string with the parameters dynamically like:


void Query(IList<string> foos){
var sb = new StringBuilder();
sb.Append("... WHERE q IN(");
var index = 0;
foreach(var foo in foos){
sb.AppendFormat("@foo{0},", index++);
}
sb.Remove(sb.Length-1, 1);
sb.Append(")");

The query now looks like for example

"... WHERE q IN(@foo0, @foo1, @foo2);

You then pass the proper number of parameters:

var sqlCmd = new SqlCommand(sb.ToString(), connection);
var index = 0
foreach(var foo in foos){
sqlCmd.Parameters.AddWithValue(string.Format("@foo{0}", index++), foo);
}



Quote:

If not, what is the advantage of using Parameterized Statements?

Any non trivial query is unreadable if you don't use parameters. Even your simple example looks horrible. Compare this:

"SELECT * FROM Users WHERE UserName='" + user.Replace("'","'") + "' AND Password = '" + pass.Replace("'","'") + "'"

to this

"select * from Users where UserName=@username and Password=@password"


Conclusion: always(!) use parameters for
- correctness
- safety
- readability
- maintainability

If I was in charge for a specific code base I would reject any code that does not use parameters.

Share this post


Link to post
Share on other sites
Quote:
Original post by ArchG
Recently our company got hit by a SQL Injection attack. I realize this is due to inproper, or unsafe coding in the SQL queries. I'm trying to go thru all our web apps and find the unsafe area.


Any place where a variable is used in a string that is passed to SQL is an "unsafe area".

If the protection of your code is worth at least $31.50, I would highly recommend The Web Application Hacker's Handbook: Discovering and Exploiting Security Flaws (Paperback) and possibly Hack Proofing Your Ecommerce Site (Paperback).

Free resources include the SQL Injection Cheat Sheet.

Ok, back to your code. That code is still very, very vulnerable. I couldn't name all of the possible exploits that could be used against it, but here's a way to prevent most, if not all of them:

Loop through every single character
If it is not a letter, A-Z, a-z, AND
If it is not a digit 0-9, AND
It is not an underscore, _

reject the input.

Don't try to filter it out yourself, you setting yourself up for even more potential flaws if that code goes wrong.

If you wanted to filter it out yourself, you still have to account for entity encoding of characters, such as "&gt;" or even other locals of characters that might get interpreted as others.

Don't try to black list, white list. As soon as you find something that does not belong, discard the whole thing. That is one of the most effective ways to make sure you do not allow any bad characters to SQL attack your site again.

Just by doing that though, you do not guarantee that you'll see less attacks. That's why you need to get a book as mentioned above, or look for tutorials on how to SQL attack yourself to understand what people do to attack so you can guard against it.

Best of luck!

Share this post


Link to post
Share on other sites
Why are you using inline SQL? I'd suggest you used Stored Procedures; it promotes a cleaner data access layer and will help cut this down a bit (you still have to be wary, especially if you're using dynamic sql).

So your stored proc:


create proc usp_GetUserLogin @Username varchar(100), @Password varchar(100)
as
set nocount on

select * -- nb i'd be more explicit than select * by actually specifying column names I need
from Users
where Username = @Username
and Password = @Password


Then in your C#...

using (SqlCommand myCommand = new SqlCommand()
{
myCommand.Connection = myConnection;
myCommand.CommandType = CommandType.StoredProcedure;
myCommand.CommandText = "usp_GetUserLogin";

myCommand.Parameters.AddWithValue("@Username", user);
myCommand.Parameters.AddWithValue("@Password", pass);

myConnection.Open();
SqlDataReader myReader = myCommand.ExecuteReader())
...................
}



I'd also note that you probably just want to search for the user and do your validation on the password in code, or return a flag from the SQL to say whether the password matched or not.

Share this post


Link to post
Share on other sites
Here's an exploit for your code

string sSQL = "SELECT * FROM Users WHERE UserName='" + user.Replace("'","''''") + "' AND Password = '" + pass.Replace("'","''''") + "'";

(the value for user):

\'; drop database ImportantDatabase; --

After your replace it will be (notice the extra tick, one of which is escaped):

\''''; drop database ImportantDatabase; --

which inserted into your statement becomes:

string sSQL = "SELECT * FROM Users WHERE UserName='\''''; drop database ImportantDatabase; --' AND Password = 'pw'";

Which will delete your ImportantDatabase (the stuff after -- is ignored since it's a comment). Multistatement-strings may be turned off to prevent this. That is a prime example of a clueless security measure; I could do this:

\' OR UserName="Administrator"; --

And get administrator rights (because the password check is commented out).

Always use parameterized queries or stored procedures. Never quote manually (not even with the built-in quoting functions). Think like a cracker.

If only the libraries for SQL weren't so terrible. Why would you encourage enormous security holes. It should be hard to make sercurity holes, not the default behavior!

/rant

Share this post


Link to post
Share on other sites
What you should really be doing as well is locking down access rights per user or schema. For example, the service that is used to log in should only have read access to certain tables for logging in. If your site is running under an account that lets you drop tables and databases, you're asking for trouble.

Share this post


Link to post
Share on other sites
Also, I haven't always followed these rules myself, and some of my code may contain glaring security holes. It makes me uneasy, but from now on...

The forum software on this site has some wierd bugs, such as replacing '''' with ' (and replacing Java-Script (without the dash) by it's lower case twin javascript). This should probably make the site owners nerveous. Would it be all that surprising to find an SQL injection bug here?

Oh, and if you use stored procedures, don't use eval ;-) ... no, I'm serious, it's the same problem all over again.

And one last thing... one last IMPORTANT thing: you're not storing plaintext passwords in your database right? If you are, then please look into hashing and remember to use a salt. You're going to regread it some day if you don't do both.

evolutional: as shown above, you're in trouble even if you try to set up access rights (the exploit to get administrator rights). It is simply not a solution. And stacking half solutions on top of each other never makes a complete solution, no matter how many you stack. Solve the whole problem at once. It's easier to set up, easier to use, more maintainable, and more secure.

I'm being cocky here to provoke a response that can prove me wrong so I can learn something. Sorry to come off as a jerk in the process ;-)


[Edited by - Ahnfelt on September 19, 2008 9:27:10 AM]

Share this post


Link to post
Share on other sites
One of the other security measures we use is to only allow administrator access from an IP address that resolves to our internal network. No, I can't log on as admin from the net, but I can remote in to my work PC and access it there, if I have to.

Stored procedures are the right technique to use, for all your procedures. (Though I will admit to using inline SQL way back when, because it was "easier"...)

Share this post


Link to post
Share on other sites
Quote:
Original post by ID Merlin
Stored procedures are the right technique to use, for all your procedures. (Though I will admit to using inline SQL way back when, because it was "easier"...)

Additionally it's often beneficial to use tools like object relational mappers. Most queries in a live system are normally CRUD operations and it can be tedious to make all of them stored procedures. For example, efficient paging on SQL Server is pretty messy if you need to handwrite the code. An ORM can be used to create those simple queries automatically which saves a lot of time:
- simple queries don't need to be hand written
- schema changes are much easier to handle
- easier to migrate to another DBMS.
And of course the ORMs handle parameters well.

I normally have all my queries generated by the ORM (NHibernate) and only for very special, complex and/or performance critical queries - mostly OLAP queris - I use SPs.

The point is: there's a lot of options that are much better than concatenating strings:
- parametrized queries
- stored procedures
- automatic generation with tools such as ORM (for .NET: NHibernate, LINQ to SQL, LINQ to Entities, Gentle.NET etc.)

Share this post


Link to post
Share on other sites
Quote:
Original post by ID Merlin
No, I can't log on as admin from the net, but I can remote in to my work PC and access it there, if I have to.


That's good practice, if a bit cumbersome for the administrators. I doubt sites like this one gamedev.net is going to use it (although this is pure guesswork), at least not for moderators. But if you work with more sensitive things than forum posts and articles, it's definably worth doing.

However, people can still steal each other's rights in the same way, and you can't have all your customers go through localhost, it would defeat the purpose.

But as VizOne says, the problem lies in eval()-ing dynamically built strings. It's so damn dangerous. It'd be great if it weren't the main method taught in almost every tutorial around the web.

Share this post


Link to post
Share on other sites
Quote:
Original post by Ahnfelt
But as VizOne says, the problem lies in eval()-ing dynamically built strings. It's so damn dangerous. It'd be great if it weren't the main method taught in almost every tutorial around the web.


That's a problem in its own right. All the php/asp tutorials out there always seem to favour inline sql :(

Share this post


Link to post
Share on other sites
It's not exactly surprising, considering it's usually database agnostic (unlike a typical prepared statement), and because using prepared statements would just add extra noise and no immediate gain to a tutorial trying to explain other concepts. I'd think security needs teaching separately, but you can't really plan for people teaching themselves over the internet.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this