Monday, February 28, 2011

The WRONG way to use stored procedure calls

Stored procedures are an excellent way to prevent sql injection attacks. However they must be called using parameters, and in addition, using parameters _correctly_.
The following is an example of how NOT to call a stored procedure.

We start with the following procedure
Create Procedure Proc_GetUser
@userId int
Select UserName, Password From User Where UserId = @userId 

And the following code call
using (SqlCommand command = new SqlCommand("proc_GetUser(@UserId='" + userId.ToString() + "')", ctx.Connection))
    SqlDataReader dataReader = command.ExecuteReader();
    //use data reader here..

What is wrong with this? It uses a named parameter, isn't that OK?


I've seen this done in production applications, and it is susceptible to sql injection. If an attacker sets UserId to something like:
nobody');EVILSQL;print ('
This makes our sql statment now
Proc_GetUser(@UserId='nobody');EVILSQL; print ('')
They have just executed the first call and then the second batch of "EVILSQL" and have successfully closed off your query with "print('" to form "print('')" which then does nothing.

The correct way to do this is as follows:

using (SqlCommand command = new SqlCommand("Proc_GetUser", ctx.Connection))
    command.CommandType = System.Data.CommandType.StoredProcedure;
    command.Parameters.AddWithValue("@UserId", userId);
    SqlDataReader dataReader = command.ExecuteReader();
    //use data reader here..

No comments:

Post a Comment