Link to home
Start Free TrialLog in
Avatar of tatton777
tatton777Flag for United States of America

asked on

Will this code prevent SQL Injection as well as optimize my SQL Statements automatically?

String insertSQL = "Insert into Stores (stor_id, stor_name, stor_address, city, state, zip) values ('" + storeid.Value + "', '" + storename.Value + "', '" + address.Value + "', '" + city.Value + "', '" + state.Value + "', '" + zip.Value + "')";

SqlConnection objConnect = new SqlConnection("server=(local);database=pubs;uid=sa;pwd=;");

SqlHelper.ExecuteNonQuery (objConnect, CommandType.Text, insertSQL);
 
Avatar of smegghead
smegghead
Flag of United Kingdom of Great Britain and Northern Ireland image

No, it's not protected - the best way to avoid injection is to use sqlparameters rather than concatenating strings.

If stroreid.Value contains the following text

xxx','xxx','xxx','xxx','xxx','xxx') \r\n go \r\n delete from [Stores] \r\n go

this would wipe out the Stores table.

Smg.
Avatar of bob20062006
bob20062006

the best security is with make sored procedure and call it bt pass  parameters
AFAIK it's safer to use parameters. Here is a little example (this should give you an idea, but I don't have VS in front of me, so there may be some syntax errors)

string insertSQL = "INSERT INTO Stores (StoreName) VALUES (@StoreName)

SqlConnection conn = new SqlConnection(connectionString");
SqlCommand cmd = new SqlCommand(insertSQL, conn);
cmd.Parameters.Add("@StoreName", storename.Value);

conn.Open();
cmd.ExecuteNonQuery();
conn.Close();


I may have missed something, this was just off the top of my head (don't have VS in front of me).
yes i think PoeticAudio  is the best programing solution to privent sql injection
Oops, found a couple... here's a better version...

string connectionString = "server=(local);database=pubs;uid=sa;pwd=;";
string insertSQL = "INSERT INTO Stores (StoreName) VALUES (@StoreName)"

SqlConnection conn = new SqlConnection(connectionString);
SqlCommand cmd = new SqlCommand(insertSQL, conn);
cmd.Parameters.Add("@StoreName", storename.Value);

conn.Open();
cmd.ExecuteNonQuery();
conn.Close();
I'm going to re-enforce what Smg and PoeticAudio say (and qualify bob's sproc statement) with the following:
1) The only way of preventing sql injection is by using named parameters in a statement
2) Creating dynamic sql without named parameters is still possible inside a stored procedure, and will _not_ prevent sql injection

Therefore named parameters in a statement, as PoeticAudio states, or stored procedures that contain explicit SQL statements, and not dynamic SQL, will prevent SQL injection.

The choice for Stored procedures over dynamic SQL with named parameters therefore comes down to where you sit on seperation of concerns, and your interpretation of the definition of loose coupling in your database and data access layers (I won't state my opinion on this). That notwithstanding, the advantage for storing SQL statements in the database in the form of explicit SQL inside stored procedures, is pre-compilation and performance. Arguably, for longer statements, it is also readability of code.

Hth,

Andy
Avatar of tatton777

ASKER

If know how to write some crappy little stored procedures but not long complex ones. If I take the time, the learn how to write the stored procedures that I need. Will the sqlHelper become a viable, useful tool for me to write secure and optimized code. IF NOT, how would I used the stored procedures. Still learning here, sorry my questions are a little lame.
There are a lot of advantages to using stored procedures. For example:
1) Not having to re-release your website when you find an error in your SQL.
2) Optimising longer SQL statements for which you have several options.
3) An extra layer of security. You can enforce, through the database, that the account that your application runs under only has execute permissions on certain stored procedures that it needs, and not full control over all objects in the database. This is arguably possible without using stored procedures, but it is a lot easier with stored procedures.
4) Easily improving on your SQL without re-compiling and releasing your application

OOP evangelists may even go so far as to say that you are implementing a definite declarative interface through which to carry out a particular piece of functionality in the database. Be that as it may, or may not be, I solely use stored procedures. I also control access to them by defining a "role" in the database that has execute permissions on the stored procedures a particular application needs access to, and putting the user account that runs that application into that role - this makes server deployment secure and very easy.

I also use the DAAB to execute the stored procedures (or rather, a considerably adapted version of it). Which version of the data access application block are you using? Are you using the Enterprise Library, and are you using it in .NET 1.1 or 2.0? Actually, given that you are talking about the SqlHelper you might be using DAB 2.0 - is that the case?

Either way, the SqlHelper takes a lot of the effort out of writing code to have conversations with a database. It doesn't necessitate you using stored procedures, you can still use parameterised sql, but that takes a lot of the point out of using the daab. The SqlHelper will not necessarily ensure you write "secure and optimized code". As I said, you can still open yourself up to SQL injection by writing dynamic, nonparameterised SQL in a stored procedure. It does, however, take a lot of the effort out of accessing the database, and more importantly it puts the code that manages the database conversations in one place, which makes it a lot easier to maintain and improve on. Yes, you can improve on the sqlhelper and the DAB in the Enterprise Library by making it better fit your scenarios. Thankfully the out-of-the-box functionality is so good that you won't need to consider that until a lot later on.

In terms of optimisation - the original SqlHelper was written to perform "within 5% of ADO.NET" (if i remember correctly). I'm not sure of the definition of 5%, but that seems like an acceptable hit to take when you are talking about reuseability, maintainability and reduction of boilerplate code.

Andy
So the best route for me to take is to learn to write Stored Procedures and combine them with DAAB. Can someone point me to a resource for writing complex stored procedures and then using them in conjunction with DAAB.
Here are some best practices... very good article
http://msdn.microsoft.com/msdnmag/issues/04/09/SQLInjection/
ASKER CERTIFIED SOLUTION
Avatar of AGBrown
AGBrown
Flag of United Kingdom of Great Britain and Northern Ireland image

Link to home
membership
This solution is only available to members.
To access this solution, you must be a member of Experts Exchange.
Start Free Trial
another quick solution to this, if you don't want to write stored procedures, is to double up the single quotes in your parameters before concatenating them to the string. This prevents sql injection.

for example, my injection example would be converted to ..

xxx'',''xxx'',''xxx'',''xxx'',''xxx'',''xxx'') \r\n go \r\n delete from [Stores] \r\n go

use MyPar=MyPar.Replace("'","''"); // there is one single quote in the first string, and two in the second... if you're having trouble focusing.

this way, the whole string would be treated as a parameter, not letting the 'injector' escape the quotes.

However, SP's with parameters are the long term solution to this.

Regards
Smg.