| Author |
Topic |
|
parrot
Posting Yak Master
132 Posts |
 Posted - 2010-04-07 : 15:39:08
|
| I am trying to code my program to ward off SQL injections by using parameterized user input. I cannot get the program to select any records using a LIKE command as shown below. Can anyone tell if there is something wrong with my code? It was working before I substituted the SearchName.Text field with the parameter @SearchNameparm.string strSQL = "DECLARE @SearchNameparm varchar(20) ";strSQL += "SELECT * FROM Obituary WHERE CemeteryCode = '" + Code.Text + "'";if (searchname.CompareTo("All") != 0 && searchname.CompareTo("") != 0)// old code strSQL += " AND LastName LIKE '" + SearchName.Text + "%'"; strSQL += " AND LastName LIKE @SearchNameparm"; OleDbDataAdapter adapter = new OleDbDataAdapter(); OleDbCommand selectCMD = new OleDbCommand(strSQL, OleDbConn1); adapter.SelectCommand = selectCMD; selectCMD.Parameters.Add("@SearchNameparm", OleDbType.VarChar, 20); selectCMD.Parameters["@SearchNameparm"].Value = "'" + SearchName.Text + "%'"; dataSet1 = new DataSet(); int ret = adapter.Fill(dataSet1, "Obits"); |
|
|
BryanBurroughs
Starting Member
8 Posts |
Posted - 2010-04-07 : 15:58:37
|
| You don't need to include the single quotes when setting the value of a parameter. Fixing that will probably make it run |
 |
|
|
parrot
Posting Yak Master
132 Posts |
Posted - 2010-04-07 : 16:15:49
|
| Thanks for your reply. I tried with and without quotes around the value and I still get no records returned. I have used parameterized input with update and insert commands successfully. This is the first time I have used them with a SELECT command and with a Fill. |
|
|
|
AndrewMurphy
Master Smack Fu Yak Hacker
2916 Posts |
Posted - 2010-04-08 : 03:52:45
|
| Why don't you use Stored Procedures? See previous advice here re the benefits. |
|
|
|
Kristen
Test
22859 Posts |
Posted - 2010-04-08 : 04:05:58
|
If you are using dynamic SQL in your application to build a WHERE clause I recommend that you use SP_ExecuteSQL to execute it. This will cache the query plan, for performance, and you can call it with parameters just like an ordinary Stored Procedure (but have the benefit of building the WHERE clause dynamically).Note that you should not use "SELECT *" but instead list the actual columns that you want to retrieve (i.e. only those used by your application) - otherwise someone may add monster TEXT column(s) in the future and every SELECT * query will be retrieving them, even if not used, which will be disastrous for performance - and probably take ages to fix at that time!I thinkselectCMD.Parameters["@SearchNameparm"].Value = SearchName.Text + "%"; should work - just make sure that the text entered by the user cannot exceed 19 characters (i.e. 20 in total) or make the declaration bigger ("DECLARE @SearchNameparm varchar(20)") |
|
|
|
Kristen
Test
22859 Posts |
Posted - 2010-04-08 : 04:06:30
|
P.S. or change the SQL to be:strSQL += " AND LastName LIKE @SearchNameparm + '%' "; |
|
|
|
Kristen
Test
22859 Posts |
Posted - 2010-04-08 : 04:10:20
|
P.P.S. ACtually: Does your approach work? (I've never tried doing it that way).You are passing a parameter from your application:selectCMD.Parameters.Add("@SearchNameparm", OleDbType.VarChar, 20);selectCMD.Parameters["@SearchNameparm"].Value = "'" + SearchName.Text + "%'";but then also declaring it in your SQL:string strSQL = "DECLARE @SearchNameparm varchar(20) "; are they one-and-the-same? or do they conflict in some way?As per my earlier suggestion about SP_ExecuteSQL - that would resolve that issue, but it may just be my ignorance of how to parametrise a simple SQL string |
|
|
|
parrot
Posting Yak Master
132 Posts |
Posted - 2010-04-08 : 11:09:55
|
| Thanks for the replies. I agree that I should not be retrieving all fields from the table. In fact, there are only 10 fields in the table and I am using 8 of them. I have successfully used parameter fields in UPDATE and INSERT command. This is the first time I tried them with a SELECT command and a fill. I will remove the redundancy of the parameter and try the example Kristen gave me. I have not used SP_ExecuteSQL but I will read up on it and try to use it in this example. Also, I have not used Stored Procedures because most of my programs require dynamic SQL since so much of the information is provided by the user for query purposes. There are 20 programs in this web application and all of them have to be reviewed for vulnerability to SQL Injection. I need to resolve this usage of parameters in order to complete the task. Thanks again for your help. I will post the results ASAP. |
|
|
|
Kristen
Test
22859 Posts |
Posted - 2010-04-08 : 11:38:46
|
| "I have not used Stored Procedures because most of my programs require dynamic SQL since so much of the information is provided by the user for query purposes"Its a job made for SP_ExecuteSQL in that case, IMHO!If you want to read a detailed account and all the ins-and-outs then have a look at http://www.sommarskog.se/dynamic_sql.html - excellent stuff on that site, but its not for the faint hearted! |
|
|
|
parrot
Posting Yak Master
132 Posts |
Posted - 2010-04-08 : 13:26:55
|
| I made an attempt to use sp_executesql and I keep getting an error message that says "Incorrect syntax near '000001' which is a useless message. My new code is shown below.string strSQL = "DECLARE @userinput varchar(20);";strSQL += "DECLARE @SQLString nvarchar(500);";strSQL += "DECLARE @ParmDefinition nvarchar(500);"; string sqlstring = "N'SELECT * FROM Obituary WHERE CemeteryCode = '" + Code.Text + "'";if (searchname.CompareTo("All") != 0 && searchname.CompareTo("") != 0) { sqlstring += " AND LastName LIKE @SearchNameparm"; searchvalue = SearchName.Text += "%"; } else searchvalue = "''";sqlstring += "';";strSQL += "SET @SQLString = " + sqlstring;strSQL += "SET @ParmDefinition = N'@SearchNameparm nvarchar(20)';";strSQL += "SET @userinput = " + searchvalue + ";";strSQL += "EXECUTE sp_executesql @SQLString, @ParmDefinition, @SearchNameparm = @userinput;";OleDbDataAdapter adapter = new OleDbDataAdapter();OleDbCommand selectCMD = new OleDbCommand(strSQL, OleDbConn1);adapter.SelectCommand = selectCMD;dataSet1 = new DataSet();int ret = adapter.Fill(dataSet1, "Obits");I copied the sql string that was created from the above code and am displaying it below. If anyone can see where the syntax error is, please let me know.DECLARE @userinput varchar(20);DECLARE @SQLString nvarchar(500);DECLARE @ParmDefinition nvarchar(500);SET @SQLString = N'SELECT * FROM Obituary WHERE CemeteryCode = '000001'';SET @ParmDefinition = N'@SearchNameparm nvarchar(20)';SET @userinput = '';EXECUTE sp_executesql @SQLString, @ParmDefinition, @SearchNameparm = @userinput; |
|
|
|
BryanBurroughs
Starting Member
8 Posts |
Posted - 2010-04-08 : 13:39:41
|
quote:
SET @SQLString = N'SELECT * FROM Obituary WHERE CemeteryCode = '000001'';
looks like this line has 1 too many single quotes in it at the end, before the semicolon. |
|
|
|
parrot
Posting Yak Master
132 Posts |
Posted - 2010-04-08 : 14:06:40
|
| I need that quote there for the literal comparison to Cemetery Code. If I remove it I get another error stating there is an unclosed quotation mark. |
|
|
|
parrot
Posting Yak Master
132 Posts |
Posted - 2010-04-08 : 14:44:20
|
| I figured out a way to put in a dynamic parameterized variable into my sql code as shown below:string sqlstring = " SELECT * FROM Obituary WHERE CemeteryCode = '" + Code.Text + "'";string strSQL = "";if (searchname.CompareTo("All") != 0 && searchname.CompareTo("") != 0) { strSQL = "DECLARE @SearchNameparm varchar(20); "; strSQL += "SET @SearchNameparm = '" + SearchName.Text + "%';"; sqlstring += " AND LastName LIKE @SearchNameparm"; }strSQL += sqlstring + " ORDER BY LastName ";OleDbDataAdapter adapter = new OleDbDataAdapter();OleDbCommand selectCMD = new OleDbCommand(strSQL, OleDbConn1);adapter.SelectCommand = selectCMD;dataSet1 = new DataSet();int ret = adapter.Fill(dataSet1, "Obits");Does this code prevent sql injection? |
|
|
|
Michael Valentine Jones
Yak DBA Kernel (pronounced Colonel)
7020 Posts |
Posted - 2010-04-08 : 15:01:41
|
quote:
Originally posted by parrot
I am trying to code my program to ward off SQL injections by using parameterized user input. I cannot get the program to select any records using a LIKE command as shown below. Can anyone tell if there is something wrong with my code? It was working before I substituted the SearchName.Text field with the parameter @SearchNameparm.string strSQL = "DECLARE @SearchNameparm varchar(20) ";strSQL += "SELECT * FROM Obituary WHERE CemeteryCode = '" + Code.Text + "'";if (searchname.CompareTo("All") != 0 && searchname.CompareTo("") != 0)// old code strSQL += " AND LastName LIKE '" + SearchName.Text + "%'"; strSQL += " AND LastName LIKE @SearchNameparm"; OleDbDataAdapter adapter = new OleDbDataAdapter(); OleDbCommand selectCMD = new OleDbCommand(strSQL, OleDbConn1); adapter.SelectCommand = selectCMD; selectCMD.Parameters.Add("@SearchNameparm", OleDbType.VarChar, 20); selectCMD.Parameters["@SearchNameparm"].Value = "'" + SearchName.Text + "%'"; dataSet1 = new DataSet(); int ret = adapter.Fill(dataSet1, "Obits");
What makes you think this will avoid SQL injection? It appears to be inserting user input directly into a SQL statement, the very definition of SQL injection.strSQL += "SELECT * FROM Obituary WHERE CemeteryCode = '" + Code.Text + "'"; If you are expecting something like this:SELECT * FROM Obituary WHERE CemeteryCode = 'value user typed in' If the user types this in the text box: ';drop table Obituary; --then you end up with:SELECT * FROM Obituary WHERE CemeteryCode = '';drop table Obituary; -- CODO ERGO SUM |
|
|
|
parrot
Posting Yak Master
132 Posts |
Posted - 2010-04-08 : 15:16:06
|
| I thought if you paramterized the input, the system would process the parameter as a variable and not as executable code. If not, then how does one paramterize input into a SELECT statement? I've tried two other ways shown above, neither of which work. If you have a better way of doing it, let me know. |
|
|
|
parrot
Posting Yak Master
132 Posts |
Posted - 2010-04-08 : 15:18:23
|
| I should also mention that I am filtering out quotes, asteriks, double dashes, backslashes, etc. from the input before I even attempt to process the data. |
|
|
|
BryanBurroughs
Starting Member
8 Posts |
Posted - 2010-04-08 : 15:32:35
|
Yes, if you correctly parameterize the code, it will avoid SQL injection. What you are doing, though, is NOT parameterizing.SET @sql = N'some text ' + @param in SQL is just as vulnerable as doing String sql = 'some text ' + aTextBox.Text in C# or VB.if you are writing in VB or C#, you add the parameters using your OleDbCommand object, like you did in the first post. However, there is no need for the first line of SQL code where you declared @SearchNameparm. The OleDbCommand object does that for you automatically. |
|
|
|
parrot
Posting Yak Master
132 Posts |
Posted - 2010-04-08 : 15:59:44
|
| I inserted the DECLARE for @SearchNameparm because if I didn't I would get an error message saying 'Must declare the scalar variable "@SearchNameparm".' If I use the DECLARE statement and eliminate the selectCMD.Parameters.Add statement I also get an error saying that "@SearchNameparm' is not contained by this OleDbParameterCollection." So I can't win either way, I need both. It appears to me that the code in the first post is correct but I still get no records back. |
|
|
|
Michael Valentine Jones
Yak DBA Kernel (pronounced Colonel)
7020 Posts |
|
|
Kristen
Test
22859 Posts |
Posted - 2010-04-08 : 16:10:01
|
Parrot you are missing an important point with SP_ExecuteSQL:You need to build two strings:The Dynamic SQL Query statement - this will be something like:string sqlQuery = " SELECT * FROM Obituary WHERE CemeteryCode = @CemeteryCode";if (searchname.CompareTo("All") != 0 && searchname.CompareTo("") != 0){ sqlQuery += " AND LastName LIKE @SearchNameparm";}sqlQuery += " ORDER BY LastName ";note that all "user provided data" is show as @Variables.Next you need to prepare a string defining all possible parameters (it does not matter whether the Parameter is actually used in this particular query - such as @SearchNameparm in this example - you can just include all possible parameters (or you can do it programmatically to only include the actually-used parameters)string sqlParameter = "@CemeteryCode varchar(99), @SearchNameparm varchar(20)"; Then you pass the Dynamic SQL query (sqlString), the Parameters list (sqlParameters) and then ALL the actual parameters, individually, to execute SP_ExecuteSQL (sorry, not familiar with your programming language, so I don't know the correct syntax, but it would eb the equivalent of:EXEC sp_ExecuteSQL @sqlstring, @sqlParameter, @CemeteryCode, @SearchNameparm You should never include any user-supplied data in the Dynamic SQL query (sqlString), only ever as a Parameter (defined in @sqlParameter and supplied as an additional parameter to the call to sp_ExecuteSQL.Hope that is clear, but if not post your attempt and I expect we can help clarify. |
|
|
|
parrot
Posting Yak Master
132 Posts |
Posted - 2010-04-08 : 16:59:26
|
| Kristen;I am programming in C# and have referenced examples of how to code sp_executesql. I understand that you should not include user input directly in an SQL statement. The cemetery code is really hard coded in a previous program and is carried through the various Sessions. I do think I am coding it with proper syntax. The code below shows the search for LastNme = Jones. I will keep trying to see if I can get rid of the syntax error that really doesn't tell me anything.DECLARE @userinput varchar(20);DECLARE @SQLString nvarchar(500);DECLARE @ParmDefinition nvarchar(500);SET @SQLString = N'SELECT * FROM Obituary WHERE CemeteryCode = '000001' AND LastName LIKE @SearchNameparm';SET @ParmDefinition = N'@SearchNameparm nvarchar(20)';SET @userinput = 'Jones';EXECUTE sp_executesql @SQLString, @ParmDefinition, @SearchNameparm = @userinput; |
|
|
|
Next Page
|
Parameterized input not working