3

This is more of a concept question.

I have build a small wpf app in c# to read a csv of wage roll data and insert it into my sql 2005 db, no edits or deletes required.

I have take all the steps I can think of to protect the application against users entering sql injection and read around the subject. The steps include connection string user as low grade sql user, tightly matching my datagrid control columns to the sql table and non editable user inputs (comboboxes, file and date picker, only a certain csv filename). My update method off the data source is merge the datatable where the csv is read into and displayed in the datagrid to the sql datatable and then update.

I have now had a thought that the most obvious weakness is the csv file itself (that I can see!). I could see a slight chance that someone could create a csv with the correct filename and input in a column of say 'delete [some statement] -- etc and read it in.

So now I feel the need to check this input but a lot of articles say this client side stuff is a waste of time. What are other peoples thoughts? I was thinking a simple class with a list of things to check e.g. 'sp', ';', 'Select', 'delete','--' and then a function to check the csv column input against the list and handle as required.

Is class idea a bit crude?

John Saunders
  • 160,644
  • 26
  • 247
  • 397
Ian W
  • 385
  • 2
  • 10
  • 30
  • 1
    Unlike forum sites, we don't use "Thanks", or "Any help appreciated", or signatures on [so]. See "[Should 'Hi', 'thanks,' taglines, and salutations be removed from posts?](http://meta.stackexchange.com/questions/2950/should-hi-thanks-taglines-and-salutations-be-removed-from-posts). – John Saunders Oct 05 '13 at 13:31

2 Answers2

5

I was thinking a simple class with a list of things to check e.g. 'sp', ';', 'Select', 'delete','--' and then a function to check the csv column input against the list and handle as required.

Is class idea a bit crude?

Yes. It's also ineffective. Looking for offending terms and filtering them out is a losing battle. Instead, what you should do is simply not execute user input as code in your database. If someone inputs the string "DELETE" and you execute that string as code then you are open to attacks. If, on the other hand, someone inputs the string "DELETE" and you simply save that string to the database then all they've done is input data to the database.

Treat user input strings as strings, not as code. Save them, don't execute them.

It's difficult to be more specific without seeing your actual data access code. You might be wide open to SQL injection attacks. Nobody here can know without seeing your code.

But the SQL injection vulnerability isn't in the UI. That might be what you alluded to in this odd statement:

a lot of articles say this client side stuff is a waste of time

Any SQL injection vulnerability is going to be in the code where you interact with the database. As long as input to that code is properly handled as values and not as executable code (for example, parameterized queries instead of query concatenation) then you shouldn't have a SQL injection vulnerability.

Community
  • 1
  • 1
David
  • 208,112
  • 36
  • 198
  • 279
  • I am using the standard (basic) update method provided by the .net methods in vs2010. I think that you are saying that if I have a column called NAME in my csv file and the row of data included in the csv data for the column name is 'EXEC sp_MSforeachtable @command1 = "DROP TABLE ?" then as I merge the csv datatable with the sql table WAGEDATA the statement EXEC sp_MSforeachtable command1 = "DROP TABLE ?" would not be executed nor when I update the sql table with the update method, is that correct? I can post the code if that helps – Ian W Oct 05 '13 at 18:53
  • @IanW: I don't know what you mean by "standard update method." There are lots of different data access technologies you could be using. The main thing is to use that technology in such a way that input is provided as parameters rather than as manually written SQL code. – David Oct 05 '13 at 18:57
  • Sorry I meant the vs prog writes/writes the data update method itself. What I am struggle to get my slow brain around is: The user does not generate any data really just selects the csv file to read and app reads the csv file into the data table, then that is added to the sql database. Could any sql statements in the csv file cause harm? – Ian W Oct 05 '13 at 19:34
  • @IanW: Even if the user isn't typing it, anything that's outside the application and coming in should be considered "user input." This would include the contents of a file. Without knowing anything about the data access technology you're using I can't be more specific, but as long as the input is never treated as SQL code itself (such as manually concatenating a SQL command based on user input) then you *should* be safe. But, again, I can't make guarantees without seeing the code. – David Oct 05 '13 at 19:39
  • Ok I understand the reason for the lack of guarantees. However thank you for taking the time to understand and to pass on your knowledge. I also believe that the contents of the file are not going to be concatenated, but to be safe I mock up the case I am thinking of and test it to be sure on my local test db I built. – Ian W Oct 06 '13 at 08:55
4

You should be able to use SQL Parameters rather than constructing SQL statements from input data:

See: Why do we always prefer using parameters in SQL statements?

Community
  • 1
  • 1
Ashigore
  • 4,618
  • 1
  • 19
  • 39