prevent sql-injection in asp.net

Question

i have following authentication method:

protected void Button1_Click(object sender, EventArgs e)
        {            
            string s;
            s = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString;
            SqlConnection con = new SqlConnection(s);
            con.Open();
            string sqlCmd;
            sqlCmd = "SELECT Username, UserPassword FROM Benutzer WHERE Username = @Username AND UserPassword =@Password";
            SqlCommand cmd = new SqlCommand(sqlCmd, con);
            String username = tbUsername.Text.Replace("'", "''");
            String password = tbPassword.Text.Replace("'", "''");
            cmd.Parameters.AddWithValue("Username", username);
            cmd.Parameters.AddWithValue("Password", password);
            string CurrentName;
            CurrentName = (string)cmd.ExecuteScalar();
            if (CurrentName != null)
            {
                Session["UserAuthentication"] = cmd.Parameters[0].ToString();
                Session.Timeout = 1;
                Response.Redirect("Default.aspx");
            }
            else
            {
                lblStatus.ForeColor = System.Drawing.Color.Red;
                lblStatus.Text = "Benuztername/Password ungültig!";
            }
        }

is this enough to prevent sql injections? i used to just the username and password directly into the command like this:

sqlCmd = "SELECT Username, UserPassword FROM Benutzer WHERE Username ='" + username + "' AND UserPassword ='" + pwd + "'";

where username and pwd where just string variables in which the contents of username and password textboxes were stored...

EDIT:

ok i have edited my code which now looks like this:

protected void Button1_Click(object sender, EventArgs e)
        {
            SqlConnection objcon = new SqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings["ConnectionString"].ToString());
            SqlDataAdapter objda = new SqlDataAdapter("[MembershipPruefen]", objcon);
            objda.SelectCommand.CommandType = CommandType.StoredProcedure;
            objda.SelectCommand.Parameters.Add("@Username", SqlDbType.VarChar).Value = tbUsername.Text;
            objda.SelectCommand.Parameters.Add("@UserPassword", SqlDbType.VarChar).Value = tbPassword.Text;
            objcon.Open();
            string CurrentName;
            CurrentName = (string)objda.SelectCommand.ExecuteScalar();
            if (CurrentName != null)
            {
                Session["UserAuthentication"] = tbUsername.Text;
                Session.Timeout = 1;
                Response.Redirect("Default.aspx");
            }
            else
            {
                lblStatus.ForeColor = System.Drawing.Color.Red;
                lblStatus.Text = "Benuztername/Password ungültig!";
            }
            objcon.Close();            
        }

this is my stored procedure:

CREATE PROCEDURE MembershipPruefen (@Username VARCHAR(50), @UserPassword VARCHAR(50))  
AS 

SELECT Username, UserPassword FROM Benutzer WHERE Username LIKE @Username AND UserPassword LIKE @UserPassword; 

is this sufficient? will my web app be secure against sql inections or is there still something to be done?

Answer 1

Use a stored procedure and only return a value to say it exists in the database (i.e. Row Count) or if you need to use the username for session data etc, then just return the username.

This reveals even less of your DB data to users :)

For info on Stored Procedures: http://support.microsoft.com/kb/306574

Answer 2

Use parameterized queries (SqlCommand with SqlParameter) and put user input into parameters. Don't build SQL strings out of unchecked user input. Don't assume you can build a sanitizing routine that can check user input for every kind of malformedness. Edge cases are easily forgotten. Checking numeric input may be simple enough to get you on the safe side, but for string input just use parameters. Check for second-level vulnerabilites - don't build SQL query strings out of SQL table values if these values consist of user input. Use stored procedures to encapsulate database operations.

or else use Prepared statements, which will be formed by using an ORM, like Linq to SQL or NHibernate, they internally use prepared statements.

Answer 3

I would create a dedicated sql server user to connect to the database (i guess you are connecting with ths 'sa' now?).

This means you add a new user to the database with no rights at all, and when you first run the app you then get a sql exception which says, as expected, that you have no read rights. You the grant SELECT rights to your 'Benutzer' table for the newly created user and so on.

When you do this, even when your connection gets compromised, the attacker can not execute system stored procedures etc.

One extra thing: it's adviced that you hash passwords, so your passwords can never be read in real-text. It's a large article, and i've seen you don't have much time, but i strongly encourage you to implement hashed passwords. http://crackstation.net/hashing-security.htm

EDIT: i see a LIKE in your stored procedure, i would state =, the user must type the EXACT password!

And i see you changed from a sql statement to a stored procedure: in the previous text change the SELECT rights on the table to EXECUTE rights on the stored procedure.

Answer 4

Use prepared statements:

SqlCommand.Prepare

MSDN