Program Crashes after checking database for password C#

Question

Well I am working on a simple login screen for a game and it uses username and password authentication. It connects to the database checks to see if username and password are there and then sees if it matches the data. If you insert the right username and password it works fine, but if you do one that is not in the database it fails and crashes. I was wondering am I doing this right? code below.

private void loginButton_Click(object sender, EventArgs e)
{
   string connectionString = "datasource=STUFFZ;database=users";
   string select = "SELECT Username, Password FROM RegularUsers WHERE Username = '" + usernameBox.Text + "' AND Password = '" + passwordBox.Text + "'";

   MySqlConnection my = new MySqlConnection(connectionString);

   MySqlCommand command = new MySqlCommand(select, my);
   my.Open();

   //String strResult = String.Empty;
   //strResult = (String)command.ExecuteScalar();
   string[] bba = new string[2];
   bba[1] = (String)command.ExecuteScalar();
   my.Close();

   if (bba[1].Equals(usernameBox.Text))
   {
      AdminPanel bb = new AdminPanel();
      bb.Show();
   }
   else
   {
      MessageBox.Show("INCORRECT USER/PASS!");
   }
}

The incorrect USER/PASS box never shows if you insert it wrong.

you really *really* **really** need to look up sql injection. — John Gardner, Mar 02 '11 at 21:38

score 4 · Answer 1 · answered Mar 02 '11 at 21:35

you are returning two things from your query, not one.
ExecuteScalar is returning null. Check your spelling and your database.
keeping clear-text passwords in your database is... in bad taste at least.
pray you never meet mr. ' or 1=1-- (insert obligatory xkcd link here)

score 2 · Answer 2 · edited May 23 '17 at 12:26

Firstly: About Sql Injection Attacks

You should encase the logic in a try catch block. You are missing the thrown exception so it's aborting the program.

try
{
    string connectionString = "datasource=STUFFZ;database=users";
    string select = "SELECT Username, Password FROM RegularUsers WHERE 
        Username = '" + usernameBox.Text + "' 
        AND Password = '" + passwordBox.Text + "'";

      MySqlConnection my = new MySqlConnection(connectionString);

      MySqlCommand command = new MySqlCommand(select, my);
            my.Open();

            //String strResult = String.Empty;
            //strResult = (String)command.ExecuteScalar();
            string[] bba = new string[2];
            bba[1] = (String)command.ExecuteScalar();
            my.Close();


            if (bba[1].Equals(usernameBox.Text))
            {
                AdminPanel bb = new AdminPanel();
                bb.Show();
            }
}
catch(Exception ex)
{
 //MessageBox.Show(ex.Message); //Will show what the exception message is.
    MessageBox.Show("INCORRECT USER/PASS!");
}

I believe this is where your problem is:

 if (bba[1].Equals(usernameBox.Text))

switch it to this:

  if (usernameBox.Text.Equals(bba[1]))

The reason is that if bba[1] is null it will throw a null reference exception when you try and use the Equals method. By switching them around, usernameBox.Text won't be null and calling Equals from the Text property will just result in a false comparison if bba[1] is null.

score 0 · Answer 3 · answered Mar 02 '11 at 21:36

Since you are checking for the username and password in your query, you would not have any result in case of incorrect username/passowrd.

I would just go with

            var result = command.ExecuteScalar();
            my.Close();


            if (result !=null )
            {
                AdminPanel bb = new AdminPanel();
                bb.Show();
            }
            else
            {
                MessageBox.Show("INCORRECT USER/PASS!");

            }

score 0 · Answer 4 · answered Mar 02 '11 at 21:39

Well, if this were my project I would not be using ExsecuteScalar to obtain the result of a SQL statement that gave me more than a one-field return. You can use an Exists clause to return "Y", or a Count to return a number. Basically all you want is to know that a user record exists with this username and password.

Also, referencing bba[1].Equals() assumes an instance exists in that element of the array. If it's null, you get the NullRefException. You should check for null before doing an equals check:

if (bba[1] != null && bba[1].Equals(usernameBox.Text))

marc_s · Accepted Answer · 2011-03-02T21:59:22.303

Couple of comments:

don't string together your SQL queries - use parametrized queries to avoid SQL injection
you should put your SqlConnection and SqlCommand into using(....) { ... } blocks
if you return two values, you should not use .ExecuteScalar() - that call only works for single row, single column returns

So all in all, your code should be something like this:

private void loginButton_Click(object sender, EventArgs e)
{
   string connectionString = "datasource=STUFFZ;database=users";
   string select = "SELECT Username, Password FROM dbo.RegularUsers " + 
                   "WHERE Username = @user AND Password = @Pwd"

   using(MySqlConnection myConn = new MySqlConnection(connectionString))
   using(MySqlCommand command = new MySqlCommand(select, myConn))
   { 
       command.Parameters.Add("@user", SqlDbType.VarChar, 50);
       command.Parameters["@user"].Value = usernameBox.Text.Trim();

       command.Parameters.Add("@pwd", SqlDbType.VarChar, 50);
       command.Parameters["@pwd"].Value = passwordBox.Text.Trim();

       myConn.Open();

       using(SqlDataReader rdr = command.ExecuteReader())
       {
          if(rdr.Read())
          {
             string userName = rdr.GetString(0);
             string password = rdr.GetString(1);

             rdr.Close();

             // here compare those values and do whatever you need to do
          }
       }

       myConn.Close();
   }    
}

Furthermore, I think this code is a tad messy since you're doing data access (selecting from SQL Server) and UI access (reading out textboxes, popping up dialog boxes) in the same snippet of code-

You should strive for more separation of concerns, e.g.

define a method CheckUserName that takes in a user name and password as string, and returns e.g. a bool
from your event handler, get the information from the UI (read out the textboxes), call that separate function with these values, and then handle the returned value

But mixing UI, logic and data access code - it gets messy and a maintenance nightmare really quickly!

Program Crashes after checking database for password C#

5 Answers5