Modifying AC7? Avoid SQL Injection attacks!

[igavemybest]

Don't ever use string concatenation (or a StringBuilder) to create SQL commands. An example is this:

string sql = "SELECT * FROM Products WHERE Category=" + cat;


There are a lot of reasons why not to do this:
1. Strings inside the command text needs to be enclosed between ' and '. You can have a problem when the value of cat contains a ' itself. You can avoid this by doubling all single quotes inside the cat string, but it still is not recommended.
2. SQL Injection attacks!!! Don't be tricked by this one, it's easy to avoid. Think of a string cat that contains the following value:

1; DROP TABLE Products; --


-- is the comment operator in T-SQL. So, the resulting command is this:

SELECT * FROM Products WHERE Category=1; DROP TABLE Products; --


The result: the Products table is dropped. Thus, pretty simple to do if the cat value comes from the querystring or from a form input.

How to avoid this:
1. Don't ever ever connect to the database as "sa" or another db owner with full access to the underlying database. Always connect with the least privileges needed to do the job.
2. Don't use string concat, but use parameterized commands instead, like this:

string query = "SELECT * FROM Products WHERE Category=@Category";


SqlCommand cmd = new SqlCommand(query, conn);


cmd.Parameters.Add("@Category", SqlDbType.NVarChar, 50);


cmd.Parameters["@Category"].Value = cat;


//...

This will make sure the anomalities with quotes are solved for you, as well as avoid basic injections and perform checkings for the input length of the strings (+ type checking etc).

3. Even better, use a stored procedure with parameters on the server and call it using SqlCommand. The idea is the same, but the SQL command with params itself is stored on the server. This allows better performance and even better security.

[afm]

Great advice!

3. Even better, use a stored procedure with parameters on the server and call it using SqlCommand. The idea is the same, but the SQL command with params itself is stored on the server. This allows better performance and even better security.

Also good advice, but I'd like to add a few nuances.

Stored procedures perform well because the database can create an optimized query plan. In the dark ages (well...a few years ago) SQL Server did not create a query plan for dynamic queries (like the example you show in #2). But the current SQL Server can. So it is not always true that stored procedures will perform better than dynamic queries...although dynamic queries will never perform better than stored procedures.

Stored procedures can have "better" security than dynamic queries because the admin can grant or deny permission to execute each individual stored procedure, but can only grant or deny the ability to run any dynamic query. But this is only a benefit if: 1) the application runs with the identify of the customer (impersonation), 2) the admin exercises their power (i.e. denies access to certain stored procedures), and 3) if the underlying application does not use any dynamic queries (so the admin can deny permission to run dynamic queries). Since AbleCommerce does not use impersonation all queries are executed using one identity. And since AC uses dynamic queries, the identify must have permission to run dynamic queries. So moving some queries to stored procedures does not enhance the security much, if at all.