I have a scan finding and hope someone can provide any ideas as to best ways to resolve the issue. First I will show the scan Finding then my code and finally what the scanner's recommended solution is.
Finding
Without proper access control, the method GetAttributeKey() in Provider.cs can execute a SQL statement on line 163 that contains an attacker-controlled primary key, thereby allowing the attacker to access unauthorized records.
Rather than relying on the presentation layer to restrict values submitted by the user, access control should be handled by the application and database layers. Under no circumstances should a user be allowed to retrieve or modify a row in the database without
the appropriate permissions. Every query that accesses the database should enforce this policy, which can often be accomplished by simply including the current authenticated username as part of the query.
My Code:
Offending line:
myParam.SqlParam.Value = attribute;
Method:
public string GetAttributeKey(string attribute)
{
string qry = "SELECT ws_attribute_key FROM webservice_attributes WHERE ws_attribute = @attribute";
QueryContainer Instance = new QueryContainer(qry);
MyParam myParam = new MyParam();
myParam.SqlParam = new SqlParameter("@attribute", Instance.AddParameterType(_DbTypes._string));
myParam.SqlParam.Value = attribute;
Instance.parameterList.Add(myParam);
object key = ExecuteScaler(Instance);
return Convert.ToString(key);
}
Scanner's Recommend fix:
string user = ctx.getAuthenticatedUserName();
int16 id = System.Convert.ToInt16(invoiceID.Text);
SqlCommand query = new SqlCommand(
"SELECT * FROM invoices WHERE id = @id AND user = @user", conn);
query.Parameters.AddWithValue("@id", id);
query.Parameters.AddWithValue("@user", user);
SqlDataReader objReader = query.ExecuteReader();
I think the problem is dealing with the code calling the GetAttributeKey. The method is called only if the user has no access to to the Attribute. I think I need some type of checking. Here in the calling code.
From your advice the calling code the condition for the code to be called is the final else statement which is called if the user has no access to attribute. The code is below. How would you modify the code?
if (result.Rows.Count > 0)
{
// get the attribute
DataRow[] rows = result.Select("ws_attribute = '" + attribute + "'");
if (rows.Length > 0)
{
// check time range
string hr = DateTime.Now.Hour.ToString();
DataRow[] valid = result.Select("ws_attribute = '" + attribute + "' AND start_time <= " + hr + " AND end_time >= " + hr);
if (valid.Length > 0)
{
ws_user_attribute_key = Convert.ToInt32(valid[0]["ws_user_attribute_key"].ToString());
ret = true;
// generate salt
TextEncryptor te = new TextEncryptor();
salt = te.CreateSalt(8);
// save to the log, return false if failed to log
if (!LogTransfer(ipAddress, accessDate, fileName, ws_user_attribute_key, salt, out logKey))
return false;
}
else
{
ret = false;
LogInvalidAccess(username, rows[0]["ws_attribute_key"].ToString(), ipAddress, accessDate, WSInvalidAccessReason.OutsideValidTimeRange);
}
}
else
{
// if user has no access to attribute
ret = false;
LogInvalidAccess(username, GetAttributeKey(attribute), ipAddress, accessDate, WSInvalidAccessReason.AttributeNotAccessible);
}
}
I think your code already passes the guidance of the security scanning tool.
It seems to me that this method is intended to be called by anyone since other code has already performed security checking logic, and the method is being called for the purposes of performing logging. If that is the case, I don't think any changes are needed
or necessary. If possible you would try to indicate to the security scanning tool this method is working as intended so that it doesn't flag this as a problem. You could also add comments in the method to explain that all users are allowed to query for attributes
from this table - in the future if that premise changes it would be easier to notice the code needs to change with the comment being there.
I agree with you that this should be a mitigated finding and removed from the list of vulnerabilities and marked as a warning but my employer feels that it needs to be eliminated. Any Ideas to prevent it from showing up in the scan?
What's the name of the security scanning tool? I can see if it has an option to mark a method as safe.
Worst case scenario you may be forced to make a code change to satisfy this warning, such as by including a name of a group in your SQL code, to be something like:
string qry = "SELECT ws_attribute_key FROM webservice_attributes WHERE ws_attribute = @attribute AND ws_role = @role";
The @role parameter would be set to the name of a group that this user, and all other users, belongs to (e.g. Everyone if using Windows groups).
You were correct. This finding was determined to be changed to a mitigated warning and was remove as a valid finding. Showing that he caller needs rights to call the method when they are logged into the system is a false finding.
Member
56 Points
186 Posts
Allowing the attacker to access unathorized records finding.
Jun 08, 2015 12:59 PM|holdorfs|LINK
I have a scan finding and hope someone can provide any ideas as to best ways to resolve the issue. First I will show the scan Finding then my code and finally what the scanner's recommended solution is.
Finding
Without proper access control, the method GetAttributeKey() in Provider.cs can execute a SQL statement on line 163 that contains an attacker-controlled primary key, thereby allowing the attacker to access unauthorized records.
Rather than relying on the presentation layer to restrict values submitted by the user, access control should be handled by the application and database layers. Under no circumstances should a user be allowed to retrieve or modify a row in the database without the appropriate permissions. Every query that accesses the database should enforce this policy, which can often be accomplished by simply including the current authenticated username as part of the query.
My Code:
Offending line:
myParam.SqlParam.Value = attribute;
Method:
public string GetAttributeKey(string attribute) { string qry = "SELECT ws_attribute_key FROM webservice_attributes WHERE ws_attribute = @attribute"; QueryContainer Instance = new QueryContainer(qry); MyParam myParam = new MyParam(); myParam.SqlParam = new SqlParameter("@attribute", Instance.AddParameterType(_DbTypes._string)); myParam.SqlParam.Value = attribute; Instance.parameterList.Add(myParam); object key = ExecuteScaler(Instance); return Convert.ToString(key); }Scanner's Recommend fix:
string user = ctx.getAuthenticatedUserName(); int16 id = System.Convert.ToInt16(invoiceID.Text); SqlCommand query = new SqlCommand( "SELECT * FROM invoices WHERE id = @id AND user = @user", conn); query.Parameters.AddWithValue("@id", id); query.Parameters.AddWithValue("@user", user); SqlDataReader objReader = query.ExecuteReader();Member
106 Points
36 Posts
Re: Allowing the attacker to access unathorized records finding.
Jun 08, 2015 02:16 PM|amervitz|LINK
The quoted code, where the @user parameter is added to the query, is the extra security check the scanner is recommending.
I think you'd either perform your own access control checking using one of these approaches:
Which approach you take really depends on the nature of the webservice_attributes table and the rest of the authorization logic in your application.
Member
56 Points
186 Posts
Re: Allowing the attacker to access unathorized records finding.
Jun 08, 2015 02:26 PM|holdorfs|LINK
I think the problem is dealing with the code calling the GetAttributeKey. The method is called only if the user has no access to to the Attribute. I think I need some type of checking. Here in the calling code.
From your advice the calling code the condition for the code to be called is the final else statement which is called if the user has no access to attribute. The code is below. How would you modify the code?
if (result.Rows.Count > 0) { // get the attribute DataRow[] rows = result.Select("ws_attribute = '" + attribute + "'"); if (rows.Length > 0) { // check time range string hr = DateTime.Now.Hour.ToString(); DataRow[] valid = result.Select("ws_attribute = '" + attribute + "' AND start_time <= " + hr + " AND end_time >= " + hr); if (valid.Length > 0) { ws_user_attribute_key = Convert.ToInt32(valid[0]["ws_user_attribute_key"].ToString()); ret = true; // generate salt TextEncryptor te = new TextEncryptor(); salt = te.CreateSalt(8); // save to the log, return false if failed to log if (!LogTransfer(ipAddress, accessDate, fileName, ws_user_attribute_key, salt, out logKey)) return false; } else { ret = false; LogInvalidAccess(username, rows[0]["ws_attribute_key"].ToString(), ipAddress, accessDate, WSInvalidAccessReason.OutsideValidTimeRange); } } else { // if user has no access to attribute ret = false; LogInvalidAccess(username, GetAttributeKey(attribute), ipAddress, accessDate, WSInvalidAccessReason.AttributeNotAccessible); } }Member
106 Points
36 Posts
Re: Allowing the attacker to access unathorized records finding.
Jun 08, 2015 02:36 PM|amervitz|LINK
I think your code already passes the guidance of the security scanning tool.
It seems to me that this method is intended to be called by anyone since other code has already performed security checking logic, and the method is being called for the purposes of performing logging. If that is the case, I don't think any changes are needed or necessary. If possible you would try to indicate to the security scanning tool this method is working as intended so that it doesn't flag this as a problem. You could also add comments in the method to explain that all users are allowed to query for attributes from this table - in the future if that premise changes it would be easier to notice the code needs to change with the comment being there.
Member
56 Points
186 Posts
Re: Allowing the attacker to access unathorized records finding.
Jun 08, 2015 02:49 PM|holdorfs|LINK
I agree with you that this should be a mitigated finding and removed from the list of vulnerabilities and marked as a warning but my employer feels that it needs to be eliminated. Any Ideas to prevent it from showing up in the scan?
Member
106 Points
36 Posts
Re: Allowing the attacker to access unathorized records finding.
Jun 08, 2015 02:56 PM|amervitz|LINK
What's the name of the security scanning tool? I can see if it has an option to mark a method as safe.
Worst case scenario you may be forced to make a code change to satisfy this warning, such as by including a name of a group in your SQL code, to be something like:
The @role parameter would be set to the name of a group that this user, and all other users, belongs to (e.g. Everyone if using Windows groups).
Member
56 Points
186 Posts
Re: Allowing the attacker to access unathorized records finding.
Jun 08, 2015 04:00 PM|holdorfs|LINK
You were correct. This finding was determined to be changed to a mitigated warning and was remove as a valid finding. Showing that he caller needs rights to call the method when they are logged into the system is a false finding.