1
\$\begingroup\$

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.

\$\endgroup\$

closed as off-topic by TheCoffeeCup, Phrancis, BCdotWEB, RobH, Nick Udell Nov 18 '15 at 10:36

This question appears to be off-topic. The users who voted to close gave this specific reason:

If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ As posted on SO: 0. Do not reinvent the security wheel. Use existing authentication libraries, such as ASP.NET Membership or ASP.NET Identity. 1. Your actual question is "How secure are ASP.NET sessions", which has been answered before (unfortunately only one answer mentions "session hijacking"). 2. Yes, search the web for "rainbow tables". Anyway this question is a tad too broad, try researching the smaller subjects of your question and ask a specific question. \$\endgroup\$ – CodeCaster Nov 17 '15 at 23:58
  • \$\begingroup\$ Thanks for the input. I'll look into the libraries further. I really like to understand how exactly I am securing this data; well enough to defend myself in court. I've got some reading to do. \$\endgroup\$ – pyathalon Nov 18 '15 at 0:10
  • 1
    \$\begingroup\$ Add reading about hashing algorithms too there's an excellent blog by Troy Hunt on the subject. Notice that over 3 years ago we could generate ~5 billion MD5 hashes per second. The TL;DR; is don't use MD5 for password storage. \$\endgroup\$ – RobH Nov 18 '15 at 8:43
  • 1
    \$\begingroup\$ Besides the pointless reinvention, your code also contains some other bad habits. SqlConnection implements IDisposable and thus it needs to be inside a using block, just like SqlCommand. Same for SqlDataReader. Do NOT keep SqlConnection as some kind of global variable. The cmd in Submit_btn_click is pointless, it doesn't do anything. Do not use AddWithValue(). \$\endgroup\$ – BCdotWEB Nov 18 '15 at 9:06
  • \$\begingroup\$ The blog by Troy Hunt was an excellent read. I didn't really know what @CodesInChaos meant by password hashing being slow until I read through that. Thanks for the link. \$\endgroup\$ – pyathalon Nov 18 '15 at 16:17
4
\$\begingroup\$
  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.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.