C# Login Form Secure/Correct?

Question

I'm making my final project for C# (school) this year and I promised the last time I got help on this site that I would make sure my SQL was secure and I would make my application secure. Could someone look over my Login Screen and tell me if this is a proper and secure way?

I start by opening my main mdiContainer via Program.cs:

private void Form1_Load(object sender, EventArgs e)
    {
        fL.ShowDialog();
    }

Then this login form shows:

string User = txtUser.Text;
        string Pw = txtPw.Text;
        int Correct = clDatabase.login(User, Pw);

        if (Correct == 1)
        {
            this.Hide();
        }
        else
        {
            MessageBox.Show("De gegevens die u heeft ingevult kloppen niet", "Fout!"); //Above means your input is not correct
        }

And in clDatabase.login

public static int login(string GebruikersnaamI, string WachtwoordI)
    {
        int correct = 0;
        SqlConnection Conn = new SqlConnection(clStam.Connstr);
        Conn.Open();
        using (SqlCommand StrQuer = new SqlCommand("SELECT * FROM gebruiker WHERE usernm=@userid AND userpass=@password", Conn))
        {
            StrQuer.Parameters.AddWithValue("@userid", GebruikersnaamI);
            StrQuer.Parameters.AddWithValue("@password", WachtwoordI);
            SqlDataReader dr = StrQuer.ExecuteReader();
            if (dr.HasRows)
            {
                correct = 1;
                MessageBox.Show("loginSuccess");
            }
            else
            {
                correct = 2;
                //invalid login
            }
        }
        Conn.Close();
        return correct;
    }

Dialog for loginsucces is only there for debug purposes atm Is this secure? Is this the proper way to have a login form?

EDIT Updated code login form:

private void button1_Click(object sender, EventArgs e)
    {
        ErrorProvider EP = new ErrorProvider();

        if (txtUser.Text == string.Empty || txtPw.Text == string.Empty)
        {
            if (txtUser.Text == string.Empty)
                txtUser.BackColor = Color.Red;
            if (txtPw.Text == string.Empty)
                txtPw.BackColor = Color.Red;

            MessageBox.Show("Er moet wel iets ingevuld zijn!", "Fout");
        }
        else
        {
            string User = txtUser.Text;
            string Pw = txtPw.Text;
            Boolean Correct = clDatabase.login(User, Pw);

            if (Correct == true)
            {
                this.Hide();
            }
            else
            {
                MessageBox.Show("Deze combinatie van username en password is niet bekend", "Fout!");
            }
        }
    }

clDatabase:
public static Boolean login(string GebruikersnaamI, string WachtwoordI)
    {
        Boolean correct = false;
        using (SqlConnection Conn = new SqlConnection(clStam.Connstr))
        {
            Conn.Open();
            using (SqlCommand StrQuer = new SqlCommand("SELECT * FROM gebruiker WHERE usernm=@userid AND userpass=@password", Conn))
            {
                StrQuer.Parameters.AddWithValue("@userid", GebruikersnaamI);
                StrQuer.Parameters.AddWithValue("@password", WachtwoordI);
                using (SqlDataReader dr = StrQuer.ExecuteReader())
                {
                    if (dr.HasRows)
                    {
                        correct = true;
                    }
                    else
                    {
                        correct = false;
                        //invalid login
                    }
                }
            }
            Conn.Close();
        }
        return correct;
    }

It's secure if they can't see your source code but you should define what you mean with _secure_. I'd also suggest to use using also for connection and reader. BTW why an integer return code instead of a simple boolean? — Adriano Repetti, Jun 25 '15 at 13:39
If I have time left I will add correct = 3; to check if you left one of the boxes empty and give a different option back or a different error if you could not connect to the database. — Strah Behry, Jun 25 '15 at 13:40
In login() I wouldn't handle _invalid_ input, don't mix responsibilities. It has to validate credentials, nothing else. To validate user input is responsibility of caller function (btw you also have bool?). Literal constants are VERY hard to follow (in case you have a long list of error codes...use an enum). — Adriano Repetti, Jun 25 '15 at 13:42

score 2 · Accepted Answer · edited May 23 '17 at 11:58

2

It is secure as far as SQL Injection is concerned, as you are passing parameters. But, do not store password as plain text, instead store its hashed value.

See: How to securely save username/password (local)?

edited May 23 '17 at 11:58

Community

1
1

answered Jun 25 '15 at 13:36

Habib

219,104
29
407
436

How do I store it as a hash? EDIT: Thanks! – Strah Behry Jun 25 '15 at 13:39
@StrahBehry, read from the link in the question and also from here. http://stackoverflow.com/questions/1054022/best-way-to-store-password-in-database – Habib Jun 25 '15 at 13:42
If I hash it how can I compare it to the password in the Database? Do I have to hash that one in the same way? To be completly honest I don't understand how it works :S, after hashing the password with the method shown in your answer I don't know where to go. Also updated the code the way it is at the moment in the question. – Strah Behry Jun 25 '15 at 14:03
@StrahBehry, yes. First you will hash the password then store that hashed value in the database, later when ever user will come back, you will get the password, hash it with the same method and compare the hashed value in the database. That will be one way hash. This way, you will never know the actual password of the user. *(which is a good thing)*. May be, *just may be*, it is too much for the school project, but it is something that you can always look into for a better/standard practice for storing password. – Habib Jun 25 '15 at 14:06

C# Login Form Secure/Correct?

1 Answers1