Prevent SQL Injections

Question

I am trying to prevent sql injection: like 1=1 , etc. First time doing this and I'm not sure if I'm doing it right?

Here is the code: The connection string is there I just removed it for the purpose of this question.

   public void btnSubmit_Click(object sender, EventArgs e)
    {

        String login = txtUser.Text;
        String pass = txtPass.Text;

            string connString = "";
            SqlConnection conn = new SqlConnection(connString);
            conn.Open();



            SqlCommand cmd = new SqlCommand("Select Users,Pass from logintable where Users='" + txtUser.Text + "' and Pass='" + txtPass.Text + "'", conn);

            cmd.Parameters.Add("@Users", SqlDbType.VarChar, 20).Value = login;


            SqlDataReader dr=cmd.ExecuteReader();
            if(dr.Read())
            {
                new Login().Show();
            }
            else
            {
                 lblFail.Text="Invalid username or password";
           }





        }

http://stackoverflow.com/questions/60174/how-to-prevent-sql-injection-in-php?rq=1 — nathan hayfield, Apr 17 '13 at 00:11
Side-note: storing passwords in plaintext is *bad*. You should hash them properly, using a strong KDF. Check out [this question](http://security.stackexchange.com/questions/211/how-to-securely-hash-passwords) for details on what's necessary, and [this answer](http://security.stackexchange.com/questions/17421/how-to-store-salt/17435#17435) for an in-depth look at *why* we need to use KDFs instead of plain hashes. — Polynomial, Apr 17 '13 at 00:15

score 2 · Answer 1 · answered Apr 17 '13 at 00:11

You are directly passing values to your query. it causes the Sql Injection. So you need to use Sql Parameters to avoid it. here is an idea for you

SqlCommand cmd = new SqlCommand("Select Users,Pass from logintable where Users=@user and Pass=@password", conn);
cmd.Parameters.AddWithValue("@user", txtUserName.Text);
cmd.Parameters.AddWithValue("@password", txtPassword.Text);
reader = cmd.ExecuteReader();

Robert Harvey · Answer 2 · 2013-04-17T00:16:00.000

1

Note your use of string concatenation in:

"Select Users,Pass from logintable where Users='" + txtUser.Text + "' and Pass='" + txtPass.Text + "'"

That's what makes your code vulnerable to injection. You need parameter placeholders:

"Select Users, Pass from logintable where Users=@Users and Pass=@Pass", conn);

You can find a complete example of how to properly use parameters here.

edited Apr 17 '13 at 00:16

answered Apr 17 '13 at 00:10

Robert Harvey

178,213
47
333
501

score 0 · Answer 3 · edited Mar 17 '17 at 13:14

No, this is completely wrong. You can still be injected via txtUser.Text and txtPass.Text, because you're using string concatenation.

You need to use parameters for those two values in the query, then bind the two .Text properties onto the query before execution.

        SqlCommand cmd = new SqlCommand("Select Users,Pass from logintable where Users=@username and Pass=@password", conn);
        cmd.Parameters.AddWithValue("@username", txtUser.Text);
        cmd.Parameters.AddWithValue("@password", txtPass.Text);

Of course, you should never store passwords directly in plaintext like this. You should look into proper password storage practice!

John Woo · Answer 4 · 2013-04-17T00:17:19.980

You are using Command object but you are not parameterizing the value which defeats its purpose, you can use AddWithValue() method on the command object to define its parameters,

string query = "Select Users,Pass from logintable where Users=@user and Pass=@pass";
SqlCommand cmd = new SqlCommand(query, conn);
cmd.Parameters.AddWithValue("@user", txtUser.Text);
cmd.Parameters.AddWithValue("@pass", txtPass.Text);

Additionally, you can use ExecuteScalar() from the Command object rather than using DataReader object to fetch single value on the result.

Try this code snippet:

string connStr = "connection string here";
string sqlStatement = @"SELECT COUNT(*) TotalCount
                        FROM logintable 
                        WHERE Users=@user and Pass=@pass";
using (SqlConnection conn = new SqlConnection(connStr))
{
    using(SqlCommand comm = new SqlCommand())
    {
        comm.Connection = conn;
        comm.CommandText = sqlStatement;
        comm.CommandType = CommandType.Text;

        comm.Parameters.AddWithValue("@user", txtUser.Text);
        comm.Parameters.AddWithValue("@pass", txtPass.Text);

        try
        {
            conn.Open();
            int _result = Convert.ToInt32(comm.ExecuteScalar());
            if (_result > 0)
            {
                new Login().Show();
            }
        }
        catch(SqlException e)
        {
            // do something with the exception
            // do not hide it
            // e.Message.ToString()
        }
    }
}

For proper coding

use using statement for propr object disposal
use try-catch block to properly handle objects

score 0 · Answer 5 · 2013-04-17T00:25:23.857

You should never construct SQL statements using string concatenation, use parametrized queries always. Please try this code :

SqlCommand cmd = new SqlCommand("Select Users, Pass from logintable where Users= @Users  AND Pass=@Pass", conn);
cmd.Parameters.Add("@Users", SqlDbType.VarChar, 20);
cmd.Parameters.Add("@Pass", SqlDbType.VarChar, 20);
cmd.Parameters["@Users"].Value = login;
cmd.Parameters["@Pass"].Value = pass;

conn.Open();

SqlDataReader reader = cmd.ExecuteReader();
if (reader.Read()) 
{
   new Login().show();
}
else
{
   lblFail.Text = "Invalid username and password";
}
reader.Close();
reader.Dispose();

conn.Close();
conn.Dispose();

Hope this helps. You should use the above code in a try-catch block with Close/Dispose calls in the finally block.

Prevent SQL Injections

5 Answers5