views:

471

answers:

7

I am taking in a string from user input, and splitting it on whitespace (using \w) into an array of strings. I then loop through the array, and append a part of the where clause like this:

            query += " AND ( "

                 + "field1 LIKE '%" + searchStrings[i] +"%' "
                 + " OR field2 LIKE '%" + searchStrings[i] +"%' "
                 + " OR field3 LIKE '%" + searchStrings[i] +"%' "
                 + ") ";

I feel like this is dangerous, since I am appending user input to my query. However, I know that there isn't any whitespace in any of the search strings, since I split the initial input on whitespace.

Is it possible to attack this via a SQL injection? Giving Robert');DROP TABLE students;-- wouldn't actually drop anything, since there needs to be whitespace in there. In that example, it would not behave properly, but no damage would be done.

Can anyone with more experience fighting SQL injections help me either fix this, or put my mind at ease?

Thanks!

EDIT:

Wow, that is a lot of great input. Thank you everyone who responded. I will investigate full-text search and, at a minimum, parameterize my query.

Just so I can better understand the problem, would it be possible to inject if all whitespace AND single quotes were escaped?

+4  A: 

There are just too many ways to get this wrong, that I wouldn't rely if anyone told me "no, this will be safe, because ..."

What about escaping the whitespace in some form (URL-Encode or somethign). What about using non-obvious Unicode whitespace characters that your simple tests don't check for. What if your DB supports some malicious operations that don't require a white space?

Do the correct way: Use a PreparedStatement (or whatever your platform uses for injection-safe parameterization), append and prepend the "%" to the user input and use that as a parameter.

Joachim Sauer
call a stored procedure...
Joel Coehoorn
+1  A: 

Yes, they could still inject items, without spaces it might not do much, but it is still a vulnerability.

In general blindly adding user input to a query is not a good idea.

Mitchel Sellers
+12  A: 

Any time you allow a user to enter data into a query string like this you are vulnerable to SQL injection and it should be avoided like the plague!

You should be very careful how you allow your searchStrings[] array to be populated. You should always append variable data to your query using parameter objects:

+ field1 like @PropertyVal Or field2 like @PropertyVal Or field3 like @PropertyVal etc...

And if you're using SQL Server for example

Query.Parameters.Add(new SqlParameter("PropertyVal", '%' + searchStrings[i] + '%'));

Be very wary how you build a query string that you're going to run against a production server, especially if it has any data of consequence in it!

In your example you mentioned Little Bobby Tables

Robert');DROP TABLE students;--

And cited that because it needs white space, you couldn't do it - but if the malicious user encoded it using something like this:

Robert');Exec(Replace('Drop_Table_students','_',Char(32)));--

I would say - better to be safe and do it the right way. There's no simple way to make sure you catch every scenario otherwise...

BenAlabaster
+11  A: 

Yes, this script did not contain any whitespaces, just encoded characters that SQL decoded back and executed: http://www.f-secure.com/weblog/archives/00001427.html

The injected script was something like this:

DECLARE%20@S%20NVARCHAR(4000);SET%20@S=CAST(0x440045004300 4C00410052004500200040005400200076006100720063006800610072 00280032003500350029002C0040004300200076006100720063006800 610072002800320035003500290020004400450043004C004100520045 0020005400610062006C0065005F0043007500720073006F0072002000 43005500520053004F005200200046004F0052002000730065006C0065 0063007400200061002E006E0061006D0065002C0062002E006E006100 6D0065002000660072006F006D0020007300790073006F0062006A0065 00630074007300200061002C0073007900730063006F006C0075006D00 6E00730020006200200077006800650072006500200061002E00690064 003D0062002E0069006400200061006E006400200061002E0078007400 7900700065003D00270075002700200061006E0064002000280062002E 00780074007900700065003D003900390020006F007200200062002E00 780074007900700065003D003300350020006…

Wich SQL decoded to :

DECLARE @T varchar(255)'@C varchar(255) DECLARE Table_Cursor
CURSOR FOR select a.name'b.name from sysobjects a'syscolumns b where a.id=b.id and a.xtype='u' and (b.xtype=99 or b.xtype=35 or b…

and so on, so its possible to do whatever you want with every table in the database without using any whitespaces.

Stefan
Okay, that's far more impressive than my injection example, but somewhat less readable :P
BenAlabaster
I thought a real life example of when hundreds of thounsands servers was infected, would be a good example to show. ;) I +1 your answer for pointing at the parameter-objects. (My solution is to only using strongly typed datasets with tableadapters.)
Stefan
+1  A: 

Here's a trivial injection, if I set field1 to this I've listed all rows in your database. This may be bad for security...

'+field1+'

You should use parameters (these are valid in inline SQL too), e.g.

AND Field1 = @Field1
Greg Beech
A: 

The rule of thumb is: If the string you're appending isn't SQL, it must be escaped using prepared statements or the correct 'escape' function from your DB client library.

too much php
+2  A: 

I totally agree that query parameters are the absolutely safest way to go. With them you have no risk of SQL injection whatsover (unless you do something stupid with the parameters) and there is no overhead of escaping.

If your DBMS does not support query parameters, then it MUST support string escaping. In the worst case you can try to escape single-quotes yourself, although there still is a Unicode exploit that can circumvent this. However, if your DBMS does not support query parameters, it probably doesn't support Unicode either. :)

Added: Also. queries like you wrote up there are killers for performance - no indexes can be used. I'd advise to look up your DBMS' full-text-indexing capabilities. They are meant exactly for cases such as this.

Vilx-
How could you inject if single quotes were also escaped? Thanks!
pkaeding