Catch the Security Flaw(s) #4
Identify as many security issues as you can with this piece of code:-
1: [WebMethod]
2: public string GetEmpName(string empid)
3: {
4: SqlConnection con = new SqlConnection("server=.;database=test;uid=sa;pwd=PassW2rd12");
5: SqlCommand cmd = new SqlCommand("select username from users where id = " + empid, con);
6: con.Open();
7: string empname = (string)cmd.ExecuteScalar();
8: con.Close();
9: return empname;
10: }
How many did you get? Lets go over them one by one:-
1. The SQL connection string uses SQL Authentication to connect to the server. If possible, you should use Windows authentication instead. This eliminates the need to store credentials in the application.
2. If Windows Authentication is not possible, store the connection string in the web.config file and encrypt it using the aspnet_regiis tool.
3. The account used to connect to the SQL server is “sa”, which is a member of the all powerful sysadmin server role. Instead, the account used to connect to the SQL Server must have the least amount of permissions on the server as needed to do the job.
3. There is no validation for the “empid” parameter. If only integers are expected, try to parse it into an integer, otherwise use a regular expression for white list validation.
4. The “empid” parameters is being concatenated to form a SQL statement, which is then executed. This means that the application is vulnerable to SQL Injection. Instead of concatenating user input to create a SQL statement, use parameterized SQL.
5. There is no structured exception handling in this code. As a result, verbose error messages containing managed exceptions will be sent to the end user. Implement structured exception handling and close the connection in a finally block.
This is how the code looks after restructuring:-
1:
2: [WebMethod]
3: public string GetEmpName(string empid)
4: {
5: int id;
6: if (int.TryParse(empid, out id))
7: {
8: SqlConnection con = new SqlConnection(System.Web.Configuration.WebConfigurationManager.ConnectionStrings["Conn"].ConnectionString);
9: SqlCommand cmd = new SqlCommand("select username from users where id = @id", con);
10: cmd.Parameters.Add(new SqlParameter("@id", id));
11: try
12: {
13: con.Open();
14: return (string)cmd.ExecuteScalar();
15: }
16: catch (Exception exp)
17: {
18: LogException(exp);
19: throw new ApplicationException("An unexpected error occured");
20: }
21: finally
22: {
23: if (con != null && con.State == ConnectionState.Open)
24: con.Close();
25: }
26: }
27:
28: return null;
29: }
Comments
Anonymous
December 02, 2008
PingBack from http://blog.a-foton.ru/index.php/2008/12/02/catch-the-security-flaws-4/Anonymous
December 02, 2008
I love these posts, I was able to identify all the issues before reading your list. The encryption of the web.config is something I don't see used as much as it should. It's probably because VS doesn't just integrate it. I wonder if that's changed in VS2010. One place the fixed up code can be "cleaned" up is use a using clause on the connection creation instead writing your try-finally and closing on your own. :)Anonymous
December 03, 2008
Pedantic I know, but if you're expecting an integer for empid, then why bother parsing it? public string GetEmpName(int empid) ... Granted, a Regex would need parsing as suggested.Anonymous
December 03, 2008
I laughed outloud when I saw this! Run anything the client likes... under sa! Another point to add to Mssrs Klawiter & Smith's admirable polishing: does this sort of catch-all error wrapping really belong in the method itself, or would it be better implemented in a lower layer. A quick goo^h^h^h web-search and ... there's a nice example of a soap extension at the end of this article -http://www.codeproject.com/KB/aspnet/ASPNETExceptionHandling.aspx