CODEWITHSUNDEEP
c#sqlado.netsql-injection
user2288600
user2288600

Reputation: 23

Prevent SQL Injections

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";
           }





        }

Upvotes: 2

Views: 3745

Answers (5)

user2284545
user2284545

Reputation:

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.

Upvotes: 0

John Woo
John Woo

Reputation: 263683

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

Upvotes: 0

Robert Harvey
Robert Harvey

Reputation: 180787

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.

Upvotes: 1

Sachin
Sachin

Reputation: 40970

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();

Upvotes: 2

Polynomial
Polynomial

Reputation: 28316

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!

Upvotes: 0

Related Questions