0

i have following authentication method:

protected void Button1_Click(object sender, EventArgs e) { string s; s = ConfigurationManager.ConnectionStrings["ConnectionString"].ConnectionString; SqlConnection con = new SqlConnection(s); con.Open(); string sqlCmd; sqlCmd = "SELECT Username, UserPassword FROM Benutzer WHERE Username = @Username AND UserPassword =@Password"; SqlCommand cmd = new SqlCommand(sqlCmd, con); String username = tbUsername.Text.Replace("'", "''"); String password = tbPassword.Text.Replace("'", "''"); cmd.Parameters.AddWithValue("Username", username); cmd.Parameters.AddWithValue("Password", password); string CurrentName; CurrentName = (string)cmd.ExecuteScalar(); if (CurrentName != null) { Session["UserAuthentication"] = cmd.Parameters[0].ToString(); Session.Timeout = 1; Response.Redirect("Default.aspx"); } else { lblStatus.ForeColor = System.Drawing.Color.Red; lblStatus.Text = "Benuztername/Password ungültig!"; } }

is this enough to prevent sql injections? i used to just the username and password directly into the command like this:

sqlCmd = "SELECT Username, UserPassword FROM Benutzer WHERE Username ='" + username + "' AND UserPassword ='" + pwd + "'";

where username and pwd where just string variables in which the contents of username and password textboxes were stored...

EDIT:

ok i have edited my code which now looks like this:

protected void Button1_Click(object sender, EventArgs e) { SqlConnection objcon = new SqlConnection(System.Configuration.ConfigurationManager.ConnectionStrings["ConnectionString"].ToString()); SqlDataAdapter objda = new SqlDataAdapter("[MembershipPruefen]", objcon); objda.SelectCommand.CommandType = CommandType.StoredProcedure; objda.SelectCommand.Parameters.Add("@Username", SqlDbType.VarChar).Value = tbUsername.Text; objda.SelectCommand.Parameters.Add("@UserPassword", SqlDbType.VarChar).Value = tbPassword.Text; objcon.Open(); string CurrentName; CurrentName = (string)objda.SelectCommand.ExecuteScalar(); if (CurrentName != null) { Session["UserAuthentication"] = tbUsername.Text; Session.Timeout = 1; Response.Redirect("Default.aspx"); } else { lblStatus.ForeColor = System.Drawing.Color.Red; lblStatus.Text = "Benuztername/Password ungültig!"; } objcon.Close(); }

this is my stored procedure:

CREATE PROCEDURE MembershipPruefen (@Username VARCHAR(50), @UserPassword VARCHAR(50)) AS SELECT Username, UserPassword FROM Benutzer WHERE Username LIKE @Username AND UserPassword LIKE @UserPassword;

is this sufficient? will my web app be secure against sql inections or is there still something to be done?

Improve this question
4
  • 2
    can i know why you just dont use stored procedures and Multi Layer Technique ?? its realy helpfull Commented Mar 12, 2013 at 14:16
  • because i am fairly new to this and i have no clue where to start on that...also my time is a little limited so i must get this done asap...i read on many articles that a combination of sanitizing, parameterization and stored procedures are generally secure against sql injections so i have done the first 2 but i dont know if they are sufficient... Commented Mar 12, 2013 at 14:23
  • good for you you skipped the string concatenation! Commented Mar 12, 2013 at 15:08
  • The following: "because I am fairly new to this and i have no clue where to start on that...also my time is a little limited so i must get this done asap..." - is the #1 reason why there are so many coding related problems out there - and the #1 reason why so many websites are easily compromised. Commented Sep 5, 2013 at 17:22

4 Answers 4

Reset to default
2

Use a stored procedure and only return a value to say it exists in the database (i.e. Row Count) or if you need to use the username for session data etc, then just return the username.

This reveals even less of your DB data to users :)

For info on Stored Procedures: http://support.microsoft.com/kb/306574

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

2

Use parameterized queries (SqlCommand with SqlParameter) and put user input into parameters. Don't build SQL strings out of unchecked user input. Don't assume you can build a sanitizing routine that can check user input for every kind of malformedness. Edge cases are easily forgotten. Checking numeric input may be simple enough to get you on the safe side, but for string input just use parameters. Check for second-level vulnerabilites - don't build SQL query strings out of SQL table values if these values consist of user input. Use stored procedures to encapsulate database operations.

or else use Prepared statements, which will be formed by using an ORM, like Linq to SQL or NHibernate, they internally use prepared statements.

Improve this answer

2 Comments

well i am checking for single quote to replace them with double quotes...and i think i got the parameterized queries right as it seems to be working...now i just need to implement stored procedures and im good?
Don't do this well i am checking for single quote to replace them with double quotes. The parameters should fix that for you.
2

I would create a dedicated sql server user to connect to the database (i guess you are connecting with ths 'sa' now?).

This means you add a new user to the database with no rights at all, and when you first run the app you then get a sql exception which says, as expected, that you have no read rights. You the grant SELECT rights to your 'Benutzer' table for the newly created user and so on.

When you do this, even when your connection gets compromised, the attacker can not execute system stored procedures etc.

One extra thing: it's adviced that you hash passwords, so your passwords can never be read in real-text. It's a large article, and i've seen you don't have much time, but i strongly encourage you to implement hashed passwords. http://crackstation.net/hashing-security.htm

EDIT: i see a LIKE in your stored procedure, i would state =, the user must type the EXACT password!

And i see you changed from a sql statement to a stored procedure: in the previous text change the SELECT rights on the table to EXECUTE rights on the stored procedure.

Improve this answer

2 Comments

ok thanks for the hint with the LIKE keyword, i changed it to =...as for encryption of passwords, i think it would be a good idea but i dont want to overinflate this project as this is for a state exam for which i only have 70 hours for planning, realisation and control...regarding rights management, i thought i would do this in-code...like i have a field "usertype" in my db, values for this field can be admin, user, guest...so i check this column of the logged in user and decide what he can do and what not...can i do it just this way?
For a simple project you can do it without the encryption, but i would at least mention the encryption as as strong advice from you to the examinator, for which in real life you would spent some extra hours to implement it, because in real life it is not accepted anymore, plain text passwords. The role can be in the user table, correct. Keep it simple for this kind of project
1

Use prepared statements:

SqlCommand.Prepare

MSDN

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.