OT: Scrubbing Input Strings

MadGerbil posted on Thursday, August 24, 2006

I've written a tiny string scrubber for my ASP.NET application. I thought I'd post it here just to get input on it - a community built scrubber would probably give us all a better answer to work with so I'll throw this one out there as a place to start.

I imagine there are more efficient ways to do this - I went with the 'long hand' approach so that it would be clear what was going on and why.

Public Shared Function ScrubString(ByVal str As String) As String

'Purpose : To scrub strings of characters that might be used

' : in SQL injection attacks.

'Trim spaces

str = str.Trim

'Scrub potentially dangerous commands

str = Regex.Replace(str, "OPENROWSET", "", RegexOptions.IgnoreCase)

'Kills the first part of some powerful SQL commands such as xp_cmdshell

str = Regex.Replace(str, "xp_", "", RegexOptions.IgnoreCase)

'Scrub potentially dangerous non-alphanumeric characters

str = str.Replace("-", "") ' - :used to comment out portions of a SQL query.

str = str.Replace("*", "") ' * :used in hacked SQL to fish for information.

str = str.Replace("&", "and") ' & :used to create special characters on web pages.

str = str.Replace("=", "equals") ' = :used in hacked SQL to fish for information.

str = str.Replace(">", ")") ' > :used to create unwanted HTML elements.

str = str.Replace("<", "(") ' < :used to create unwanted HTML elements.

str = str.Replace("%", "") ' % :used in like statements to fish for data.

str = str.Replace("'", "") ' ' :used in string manipulation of SQL statements

str = str.Replace("/", "") ' / :used in SQL injection attacks

str = str.Replace("\", "") ' / :used in SQL injection attacks

str = str.Replace(";", "") ' / :used in SQL injection attacks

Return str

End Function

SonOfPirate replied on Thursday, August 24, 2006

While I appreciate your effort and intentions, you will find that there are many, many times that some of the characters you have being filtered are legitimate characters in a data entry field. This is a pretty generic and pessimistic approach.

With web-apps, the best solution is to provide validation for all data entry fields. At that point, you can apply a regular expression filter (for instance) to identify unwanted characters. You can certainly create a helper class, such as StringScrubber, that exposes methods to handle the different scenerios you run across and assists with the validation so that the process is shared by all of your objects - which is what I think your intention is.

An example what I mean is to provide a validation check to determine if the entered value is actually an e-mail address, a date, numeric, social security number, url, etc. By restricting the field during validation, you can accomplish the same thing in an even MORE restrictive way than you have shown. Afterall, if it is a valid e-mail address, then it doesn't have most of what you are filtering already.

Not sure if that's the kind of feedback you were looking for. Just my 2 cents.

xal replied on Thursday, August 24, 2006

You may have trouble with that.

What happens if you....
... run that filter when the user inputs an address like "123 O'Malley St.; Somewhere" ?
... enter the name of a company like "XYZ & ZYX" ?
... Have a "note" field where the user might want to enter information like "(*) The value / price should be in the range 10-20"

And I could go on and on...

None of those special characters should be a problem if you stay away from dynamic sql.

And by dinamic sql I mean:
- A sproc that concatenates strings or values passes as params and runs them through exec(@somenastycommand).
- cmd.CommandText = "select * from table where id = " & strMyID

These are the most common cases for problems.

So, do not dinamically generate sql code in your sps.
Do not concatenate in vb/c# that way.

If the user enters something like " 0 or 1=1 --" in the id field, and you run it by passing it as parameter, there will be no problems for you.

cmd.CommandText = "select * from table where id = @myID"
cmd.Parameters.AddWithValue("@myID", strMyID)

Whatch out with those filters, they could end up being a headache, and the origin of long hours of debugging to find out "why isn't the data being stored correctly?"

My 2 cents.

Andrés

MadGerbil replied on Thursday, August 24, 2006

I used stored procedures for everything, However, I'd heard at one point that parameterized procedures weren't entirely safe. I'm not finding information on that right now besides the following:

While stored procedures seem to be a wonderful panacea against injection attacks, this is not necessarily the case. As mentioned above, it is important to validate data to check that it is correct and it is a definite benefit of stored procedures that they can do this; however, it is doubly important to validate data if the stored procedure is going to use EXEC(some_string) where some_string is built up from data and string literals to form a new command. Source: http://www.codeproject.com/cs/database/SqlInjectionAttacks.asp

So it may very well be that my string scrubbing is unnecessary if I stick with the stored procedures since I wouldn't EXEC on a string in a stored procedure anyways.

ajj3085 replied on Thursday, August 24, 2006

MadGerbil:
I used stored procedures for everything, However, I'd heard at one point that parameterized procedures weren't entirely safe.

No, its not stored procedures which are the solution for all Sql injection attacks. Its using the SqlParameter objects and building your statements to leave in the appropriate place holders for the parameters which is the complete solution.

So you'd always do something like this:

SqlCommand cmd;
SqlParameter param;

cmd = new SqlCommand();
cmd.CommandText = "select * from person where lastName = @LastName";
param = cmd.CreateParameter( "@LastName" );
param.Value = "my unsafe string ' drop database master go;";
cmd.Parameters.Add( param );

cmd.ExecuteReader();

The above code is 100% safe; your master database won't be dropped. The string will be stored exactly as is. Oh, well not 100% if you exec on the database... but I think injection attacks are a reason not to use the exec method on the database. If you need to build your sql, just do it in the code.

ajj3085 replied on Thursday, August 24, 2006

Right, I think (if I understand what you're saying).

Directly inserting the values (without using objects which implement IDataParameter) is the cause of the problem.

So this is a never-do:

string lastName = "my last ' drop database master go";
string sql = "select * from person where lastname = " + lastName;

Building your strings like that is what leads to the injection attacks. Ado.Net provides Parameters which allow you to define placeholders within the sql, and you use the objects which implement IDataParameter to 'fill in' the value. That will safe guard against the attacks.

HTH
Andy