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.
SqlConnection
implementsIDisposable
and thus it needs to be inside ausing
block, just likeSqlCommand
. Same forSqlDataReader
. Do NOT keepSqlConnection
as some kind of global variable. Thecmd
inSubmit_btn_click
is pointless, it doesn't do anything. Do not use AddWithValue(). \$\endgroup\$ – BCdotWEB Nov 18 '15 at 9:06