Safe Login Standards [closed]

Question

I'm really trying to wrap my head around application safety in some web pages. I'd like to mention some of the things I've done in the past and hear some critiques on how I can get better. Applications I have made in the past have been mostly for hobby, but I'm currently creating one with data that needs to be secure.

In order for someone to gain access to my website for the first time, on my login page there would be a sign-up button, this sign up button would take them to a location where they could type in a username of their choosing and then select submit.

From this, I would generate a username and password combination for them from the following:

SqlConnection conPW = new SqlConnection(ConfigurationManager.ConnectionStrings["CA2ConnectionString"].ConnectionString);
protected void Submit_btn_click(object sender, EventArgs e)
{
    conPW.Open();
    string pw = PW.Text; //This was typed in by me into the form. Giving them somewhat of a generic password to begin.
    MD5CryptoServiceProvider md5convert = new MD5CryptoServiceProvider();
    byt[] hash = md5convert.ComputeHash(ASCIIEncoding.ASCII.GetBytes(password));
    using(SqlCommand cmd = new SqlCommand("INSERT INTO Login (Username, Password, HasFormAccess) VALUES (@un, @pw, @access)", conPW)
    {
        cmd.Parameters.Add("@un",UN.Text);//This was also something typed into.
        cmd.Parameters.Add("@pw", SqlDbType.VarBinary, 24).Value=hash;
        cmd.Parameters.Add("@access", 1);
        cmd.ExecuteNonQuery();
    }
    conPW.Close();
    Response.Redirect("SomeThankYouPage.aspx");
}

Afterward, I would email, or just go show my colleagues what their username and password was, and how to change it because now they had the authentication required to change it themselves with this new password.

Starting with the storing of passwords:

I believe a lot of applications have an administrator type create a password for the user, and then the user goes in to password later using the created login, but I don't believe this is a scalable approach. What would be a direct way that I could determine someone wanting to "sign up" to view my data belonged to a group that is allowed to view my data.

Line 7: My password is not salted.

Line 8: The name of my database table is written into my sql command. Is this unsafe? How can I move away from having things like this in my code-behind if it is unsafe?

Next I'll show how the users were able to access my page. Let's say I have 2 pages, login.aspx and datapage.aspx.

In login.aspx:

SqlConnection conLogin = new SqlConnection(ConnectionString);
protected void Page_Load(object sender, EventArgs e)
{
     conLogin.Open();
    Session["Allow"] = "No";
}
protected void signin_btn_click(object sender, EventArgs e)
{
    //I'll get the password from the database and compare it with what has been typed into the password box.
    byte[] pw = null; string access = Convert.ToString(0);
    using(SqlCommand CMD_getPW = new SqlCommand("SELECT Password, HasFormAccess FROM Login WHERE Username = @un", conLogin))
    {
        CMD_getPW.Parameters.AddWithValue("@un_txt", username_txt.Text);
        SqlDataReader read = null;
        read = CMD_getPW.ExecuteReader();
        if(read.Read()){pw = (byte[])read["Password"]; access = Convert.ToString(read["HasFormAccess"]);}
    }
    if (pw !=null)
    {
        string pwout = Convert.ToBase64String(pw);
        MD5CryptoServiceProvider md5convertPW = new MD5CryptoServiceProvider();
        byte[] hasPW  md5convertPW.ComputeHash(ASCIIEncoding.ASCII.GetBytes(password_txt.Text));//this password comes from the client login.
        string pwcompare = Convert.ToBase64String(hashPW);
        if (pwcompare !=pwout)
        {
            noMatch.Visible =true; //This is basically a label stating that the username and password do not match.
        }
        else if (pwcompare == pwout && Convert.ToInt16(access) == 1)
        {
             SqlCommand RecordLogin = new SqlCommand("Insert into Record Values('" + username_txt.Text + "','" + DateTime.Now + "')",conLogin);
            RecordLogin.ExecuteNonQuery(); //Here I was saving a record of who was logging into the system.
            conLogin.Close();
            Session["Allow"]="Yes";
            Response.Redirect("datapage.aspx");
        }
    }
    else {noMatch.Visible = true;}

And here is what I had in the datapage.aspx under Page_Load:

protected void Page_Load(object sender, EventArgs e)
{
    if(Session["Allow"]=="Yes")
    {
        //Load up all the code-behind created objects. Enable the couple of buttons that allow you to search for data, etc.
    }
    else
    {
        //Show a single label that states that the user has stumbled upon this page the wrong way, and to return to the login page to log into this location. The button that allows you to search for data is disabled :(.
    }
}

So, when I look at this I see some flaws besides the datatable naming, etc. that was in the first code snippet.

The ENTIRE reason I thought to stop what I was doing and ask the internet, was because of this session sharing among multiple asp.net pages. Can someone discover that a session state of "Yes" allows them access to my data, and find a way to pass that session variable to the second page? Is that possible? If it is possible, how would I pass a successful login to my second page? Could someone with access to my database compare random md5strings with my string and eventually get the right answer?

Those are just a few things that I find could be flaws (or probably are). Anything else you can do to help me keep the data I'll be containing in the next project I'm working on secure, I will gladly take into consideration.

My formal training is not in this area, so I don't have a lot of contacts to discuss these matters with and bounce ideas.

Answer 1

1. Your password hash sucks. Password hashes need to be salted and slow. Good choices are PBKDF2, bcrypt, scrypt and Argon.

   See How to securely hash passwords? for details.

2. Encoding.ASCII silently corrupts non ASCII inputs, replacing any codepoint > 127 by a question mark. This can weaken passwords significantly.

3. AFAIK session can store arbitrary objects, so why are you using a string not a bool for Session["Allow"]? Also note that if Session["Allow"] has object as a static type, the comparison with the string "Yes" will use reference equality instead of value equality.

4. You should encapsulate the password hashing logic into a separate class, which offers to methods, string CreateHash(string password) and bool VerifyHash(string password, string hash).

5. new SqlCommand("Insert into Record Values('" + username_txt.Text... doesn't use a parameterized query and might be vulnerable to SQL injection.