SQL Safety.

#1 I just got finished writing an entire PHP/MySQL website for a local company and that's the way I wrote it to protect against injection attacks and such. From all the hacking I tried with it - even knowing the source code - I wasn't able to get anything bad to happen. I think it's a pretty safe bet - but nothing is ever 100% on the 'net, if they want it bad enough - they'll find a way to do it :)

Removing all extended ASCII characters doesn't really leave too much room for the attackers to move around.

#2 Wouldn't using stored procs instead of inline queries help as well?

#3 IMHO I don't really think stored procedures are any better than inline queries. Just with inline in open code it's, obviously, easy to read and easy to create attacks for. But you can still get hit with attacks with stored procedures just as easily if they're not well thought out.

#4 The only thing you really need to worry about is a client trying to pass in a single quote (') and the semicolon (;). SQL code is just a string of data. If user input allows a ' or ; then arbitrary SQL can be written. These are really the only two that you need to worry about. Most other symbols should be fine.

Here's why they work:
Lets say you have some kind of account signup and you want people to provide a username, password, firstname and email address. Your insert will look something like:

string sql = "INSERT INTO Accounts (UserName, Password, FirstName, Email) VALUES('" + susername + "','" + spassword + "','" + sfirstname + "','" + semail + "');";

Once the string is completely concatenated it will look like:
INSERT INTO Accounts (UserName, Password, FirstName, Email) VALUES('MyUserName','Password123','Matt','example@email.com');

What happens if you don't strip out or disallow semicolons and someone enters their email address as:
example@email.com'); DROP TABLE Accounts --

The resulting SQL will look like:

INSERT INTO Accounts (UserName, Password, FirstName, Email) VALUES('MyUserName','Password123','Matt','example@email.com'); DROP TABLE Accounts --'

This forms a SQL string that would execute 2 things. It will do the insert, then it will do a drop table (and the remaining syntax is commented out using -- to prevent errors).

Stripping out every other special character is not necessary. One way to allow common special characters in names (mainly the single quote) is to replace any instance of ' in the input before its put into the string with a replacement value, say, the HTML # code for ' or a character that would not normally appear in a name such as ~.

That being said, Jon E and Nathan, your solutions should be relatively secure (if the string / input validation is being done server side, although doing it client side in addition to server side can't hurt). For future reference, prefacing anything in SQL with -- makes the entire line not do anything. -- is the way you make a comment in SQL (exactly like // in C++/TorqueScript/C#/etc). So --DROP DATABASE would not do anything as its a comment. Removing every special character and space is a bit overkill although for something like user accounts that should be fine. If you want to get email addresses, physical addresses, phone numbers and other information that is full of special characters (like large paragraphs of text or complete sentences) you will obviously need to allow special characters in. Just convert/replace or remove ' and ;.

Finally, properly configured SQL accounts with only the permissions needed for the account doing a specific job will further secure a SQL database -- there's no reason that the SQL account used to connect to the database when creating new accounts from a client interface that is open to the public should have full sa privileges to every table / every database on your server. Create and configure accounts that only do what you need them to do and use them appropriately.

#5

Quote:Finally, properly configured SQL accounts with only the permissions needed for the account doing a specific job will further secure a SQL database -- there's no reason that the SQL account used to connect to the database when creating new accounts from a client interface that is open to the public should have full sa privileges to every table / every database on your server. Create and configure accounts that only do what you need them to do and use them appropriately.

100%, absolutely positively REQUIRED. Your game code should never #1 be creating/deleting tables or databases and #2 have the permissions in the db to even perform these things.

Having said that, you still should filter out any unwanted characters like ' and ; to keep them from throwing an error. Not only that, but you should be validating the input ANYWAY to make sure it's valid input. Don't let someone name their character da'jink or l33t4ax0r. and make sure emails are all alphanumeric with a @ and so on.

Quote:IMHO I don't really think stored procedures are any better than inline queries. Just with inline in open code it's, obviously, easy to read and easy to create attacks for. But you can still get hit with attacks with stored procedures just as easily if they're not well thought out.

I 100% disagree with this statement. Using sprocs is a much better solution for many reasons. With a SPROC you are caching the sproc in the database itself, making queries much faster. You don't have to worry about things like them passing in 'DROP TABLE' or anything like that because the sproc wont allow that to execute. If you use MS SQL 2005+ you can access web services directly from the db, saving your game server from having to serve as a go-between, and so on. Sprocs make it much more secure as well. You can encrypt/decrypt in the sproc itself, which will help with security.

#6 Are you actually letting users type arbitrary SQL and executing it? That's just asking for trouble.

Or do you have pre-made queries and they can type values used in the WHERE clause? 'cuz that's a completely different thing.

#7 @Ken - Me? I have pre-built queries atm, but am moving everything to sprocs to support mysql 5.0+ and MS SQL Server.

#8 My SQL queries of course are pre-crafted only dropping user-supplied data into the where clause. Seems my alphanumeric check on the user-supplied data is sufficent then. And of course, the game shouldn't connect with an unbridled administrator account.

Furthermore on the stored procedures:

They are open to attack just as much as a pre-crafted statement in code. Drop a 'bad variable' in, and nasty results.

And, I've read endless articles about how staying away from stored procedures when using SQL in C++ applications is a good thing. Stored procs are better off for large updates or website access.

For something as simple as:

SELECT status FROM users WHERE username = " %username " ;

...a stored procedure is... a little "much". imho.

#9 For something like 'SELECT status FROM users WHERE username = " %username " ;' there is a far better method than trying to anticipate anything the user might maliciously enter. Use bind variables. With bind variables injection attacks are impossible. Every decent DB supports them.

You would prepare the following statement: (exact syntax varies somewhat from DB to DB, and kinda pseudocode)
statement = DB->Prepare("SELECT status FROM users WHERE username = :1");

Then when you want to run the query with the user's data you simply do:
statement->Bind(1, user_data);
statement->execute();

I know MySQL supports it.

#10 This sort of stuff is much nicer in a scripting language, but if you are using C/C++:
http://dev.mysql.com/doc/refman/5.0/en/c-api-prepared-statement-function-overview.html

#11 @Ken:

I fail to see the benefit of the "bind method". If the user's data is malicious, bind is still just going to plop it into the query and ... well, yeah; nasty.

Does "bind" have some sort of security I'm unaware of?

#12 Yes, it does have security. The Prepare function takes your generic query and produces a "compiled" version of the query. Since this query is now compiled it is immutable so you cannot tack on additional statements. Adding "DROP TABLE" on the end would have no meaning because the statement has already been parsed and thus it CANNOT be executed. A user giving that would simply get a syntax error.

Using prepared statements can also be far more efficient since the query only needs to be parsed once no matter how many times you execute it.

#13 I still really want to know why people feel protected when using stored procedures with SQL. It's a false sense of security no matter what - unless you never change the query. But if you have to include some client data (i.e. - username, password, etc.) then a stored procedure is just as susceptible to an injection attack as a inline query if you don't filter out client data.

#14 @Jonathan:
In a way. Any decent DB will have bind variables used within their stored procs. Of course you could still just do a string concatenation, in which case you have opened yourself up to the same problems. A small win for stored procs is that many databases give you fine-grained permissions on them. But what it comes down to is, there are safe methods and if you ignore them then you will get bitten. Bind variables are one of those methods.

#15 @Jonathan - Here is an example of why it's more secure to use a SPROC

NON-SPROC SAMPLE:

SELECT * FROM myTable WHERE strName = %myName

say i put this without the brackets: {dishmal' DROP TABLE myTable --}

now the sql will look like this when it's executed:

SELECT * FROM myTable WHERE strName = 'dishmal' DROP TABLE myTable --


EXACT SAME THING with a sproc:

SELECT * FROM myTable WHERE strName = @myVariable

say i put this without the brackets: {dishmal' DROP TABLE myTable --}

The resulting sql that will execute is this:

SELECT * FROM myTable WHERE strName = 'dishmal DROP TABLE myTable --'

Still think they are the same? ;)

#16 @Dishmal:
You're right and you're wrong. As I said above, writing a stored proc badly (string cats inside) exposes the same holes as a plain string cat. Well written code that uses bind variables gets the same benefit as your proc example.

It's not the proc that makes it safe, it's that people who are inclined to uses procs will also generally use them correctly.

#17 But by the same token you can also use inline and if your code is written well enough, and with enough thought processes, it's not even an issue to begin with.

#18 Jonathan is correct. As long as it's written well enough, any method is fine. That can be agreed upon in general, I'd think.

Personally, I'm going to stick with inline statements and just make sure my user-supplied data is 'clean'. All in all it's the same, and from the last 4 hours of benchmarking transactions per second with these different methods, inline statements are a wee faster (P4 3.0 HT WinServ2003SBS MS SQL 2003)

#19 Stored procs are not necessary, but I would never rely on "code is written well enough, and with enough thought processes". Bind variables are easy to use and unlike whatever clever filtering anyone can come up with, they are guaranteed to be safe. Why work hard on something that's trivial to solve?

#20 Since this thread has a decent amount of attention, let me plop in another question I had in another thread that's getting no attention :P

It's SQL related:

If my gameserver's clientConnection.cs manages the auth'ing of clients, thus is what runs the select statement then instantly uses the results to determine active, correct password, etc...

If the SQL server "lags" for some reason, thus making the query take... oh lets just say 5 seconds...

Will the entire game server lag for 5 seconds? Players, projectiles, ... everything?

If so, how do I get around this? If not, why? I'm not a C++ master and I'm wondering how that works out.

If one of you can shed some light on this for me, I'd appreciate it. :)