|
I have a static class that needs to pass a generic List of strings to a function using an integer as a index to the List in the class. The problem is the static class doesn't have a List collect and I don't have a proper index to access the class in the function it is passed to. The class, the calling code, and the receiving function are below.
My Class:
public class QueryContainer
{
public static QueryContainer Instance = new QueryContainer();
private int _id;
private string _query = "";
private int _searchID;
public QueryContainer() { }
public string Query
{
get
{
if (Instance != null)
return Instance._query;
else
return "";
}
set { _query = value; _id =+ 1; }
}
public int ID { get { return _id; } }
public int SearchID
{
set { _searchID = value; }
get { return _searchID; }
}
}
The calling code:
public int GetAccountSortByAccountCode(int account)
{
int Id = 0;
QueryContainer.Instance.Query = "SELECT ac_sort_order FROM lkup_account_codes where ac_code = " + account.ToString();
return Convert.ToInt32(ExecuteScaler(Id));
}
The function that the static class is passed to:
public int GetAccountSortByAccountCode(int account)
{
int Id = 0;
QueryContainer.Instance.Query = "SELECT ac_sort_order FROM lkup_account_codes where ac_code = " + account.ToString();
return Convert.ToInt32(ExecuteScaler(Id));
}
Corrected <pre> tags to recognise C#, terminators, and indentation.
modified 15-May-15 11:19am.
|
|
|
|
|
Please use <pre> tags around your code, so it is more readable. Also, is this an ASP.NET issue?
|
|
|
|
|
As Sascha told you two days ago[^], that class does not resolve your SQL Injection[^] vulnerabilities.
Rather than wasting time trying to fix this class, concentrate on fixing the calling code. You can either do that by writing raw ADO.NET code using properly parameterized queries; or you can use something like Dapper[^]; or switch to one of the many available .NET ORM solutions.
Correct ADO.NET code would look something like this:
public int GetAccountSortByAccountCode(int account)
{
using (var connection = new SqlConnection("YOUR CONNECTION STRING"))
using (var command = new SqlCommand("SELECT ac_sort_order FROM lkup_account_codes where ac_code = @account", connection))
{
command.Parameters.AddWithValue("@account", account);
connection.Open();
return Convert.ToInt32(command.ExecuteScalar());
}
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
The problem is that I don't know what the Hard coded SQL expression is. All I am trying to do it run that hard coded statement without breaking out the statement's parameters. If this wont work is that way to pass a generic list of parameter strings and use a foreach statement to add the parameters to the string's parameters?
|
|
|
|
|
You don't have a choice - you MUST look at every single SQL command issued by your program to "break out" the parameters. If you don't, then your code will still be vulnerable to SQL Injection.
You have a major security vulnerability in your code, and you need to fix it. There's no short-cut to doing that.
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Below is the best I could come up with. Now the SQL injection error was showing up in the actual ExecuteScaler function using the SQL string being passed in as so:
The old version my code replaced: protected object ExecuteScaler(string queryString)
Now my version of the function is as so: protected Object ExecuteScaler(QueryContainer Instance)
The calling function is now converted as so:
public int GetAccountSortByAccountCode(int account)
{
QueryContainer Instance = new QueryContainer("SELECT ac_sort_order FROM lkup_account_codes where ac_code = " + account.ToString());
return Convert.ToInt32(ExecuteScaler(Instance.Query));
} <pre>
And here is my new class:
<pre>
public class QueryContainer
{
string _query;
public QueryContainer(string query) { _query = query; }
public string Query
{
get
{
return _query;
}
set { _query = value; }
}
}
<pre>
So what do you think. Does someone that I will still get SQL injection errors?
|
|
|
|
|
holdorf wrote: QueryContainer Instance = new QueryContainer("SELECT ac_sort_order FROM lkup_account_codes where ac_code = " + account.ToString());
No, no, no!!!
Your code is STILL vulnerable to SQL Injection.
You are STILL using string concatenation, rather than parameterized queries.
All you've done is store the compromised query in a field on a class, and then executed it. Like shutting the stable door after the horse has bolted, that provides precisely zero protection.
If you want to fix the SQLi vulnerability in your code, you MUST use parameterized queries. That means finding every part of your code which issues a query, and updating it to use parameters instead of string concatenation.
When you've finished, you should be able to mark all of the string variables containing your queries as const. If you can't, then you've almost certainly missed a parameter, and left your code vulnerable.
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Maybe you have the completely wrong idea about what SQL-injection is.
It does not mean that your query string, after being concatenated (wrong approach) can somehow still be modified by someone and that you therefore have to "hide" it somehow (in your QueryContainer).
It means that the values you get from user input (or from somewhere else outside your controllable domain) can already contain malicious SQL-statements. And currently you blindly concatenate them into your SQL-statement.
While SQL-Parameters can not prevent that your values can contain malicious SQL-statements, they do prevent that these will be executed by your database when you're executing your SQL-statement. Because the values will be treated just as values and not potentially as SQL-statements.
If you have understood this, then you will realize that your current approach to fixing this makes no sense.
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
Sascha,
Ok. I do understand. Know my problem is how do I break the strings like "delete 'Name' from table where x = 1 and y = 2" or "select * from table where x = 1 and y = 2" with the least possible work? Remember I have only one function to fix and handle this problem. My boss is upset and is giving me only a few days to fix this. Please help.
Thanks,
Steve Holdorf
modified 18-May-15 8:00am.
|
|
|
|
|
removed: was premature advice. Even a SQL-parser won't help, see Richard's reply.
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
modified 19-May-15 15:24pm.
|
|
|
|
|
holdorf wrote: My boss is upset and is giving me only a few days to fix this.
If you were the one writing the whole thing, then you should be able to modify the code that builds these sql-statements instead of replacing the literal values afterwards. If you were not the one writing the whole thing you should tell your boss that it takes more time to fix sh!t than to produce it.. maybe in other words 
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
holdorf wrote: Remember I have only one function to fix and handle this problem.
Then your project is doomed.
Once you have used string concatenation to inject parameters into a query, there is absolutely no way to undo that. Even if you manage to write or find a routine to parse a SQL string, it's too late; the damage has already been done.
What you are asking for is essentially the same as saying you've baked a cake, but now you've realised the eggs were off, and you want to go back and replace the eggs without re-making the cake. It can't be done.
The ONLY solution for this problem is to re-visit every piece of code which generates a SQL query and update it to use a parameterized query.
If your boss doesn't understand that, then point him to Troy Hunt's explanation[^], or Bobby Tables[^].
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Can someone please give me a small piece of code of how this should be implemented?
|
|
|
|
|
holdorf wrote: Can someone please give me a small piece of code of how this should be implemented?
How many times do we have to give you the same information? 
I gave you a perfectly good example of correct ADO.NET code earlier in this thread:
http://www.codeproject.com/Messages/5058941/Re-Help-with-a-static-class.aspx[^]
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Interesting Richard.
After reading that, I feel like I just wrote code to lower the level of knowledge needed to use one of my programs.
You were pretty clear in the 3rd post.
Now the several days deadline he had, was pissed away not understanding really bad coding practices.
I was wondering why there was so many posts here.
|
|
|
|
|
I know there are a lot of posts but I finally understand and did what I was told to do. I broke the query up with parameters and I am still getting the security error. My code is below the with the parameters removed from the hard coded string, the calling code, and the implementing code:
The 3 classes with the SQL w/ with the parameters broken out, the calling code, and the implementing code:
Class with the parameters broken out:
public class MyParam
{
public string name { get; set; }
public string value { get; set; }
}
public class QueryContainer
{
string _query;
public List<myparam> parameterList = new List<myparam>();
public QueryContainer(string query) { _query = query; }
public string Query
{
get
{
return _query;
}
set { _query = value; }
}
}
The calling code:
<pre>
public int GetAccountSortByAccountCode(int account)
{
QueryContainer Instance = new QueryContainer("SELECT ac_sort_order FROM lkup_account_codes where ac_code = <a href="http:
MyParam myParam = new MyParam();
myParam.name = "@account";
myParam.value = account.ToString();
Instance.parameterList.Add(myParam);
return Convert.ToInt32(ExecuteScaler(Instance, 1));
}
<pre>
The implementing code:
<pre>
if (_connection == null || _connection.State == ConnectionState.Closed)
{
OpenConnection();
}
DbCommand command = _provider.CreateCommand();
command.Connection = _connection;
{
command.CommandText = Instance.Query;
command.CommandType = CommandType.Text;
foreach (var p in Instance.parameterList)
{
SqlParameter param = new SqlParameter(p.name, p.value);
command.Parameters.Add(param);
}
if (_useTransaction) { command.Transaction = _transaction; }
try
{
returnValue = command.ExecuteScalar();
}
catch (Exception ex)
{
if (ex is EntryPointNotFoundException)
throw ex;
RollBack();
LogBLL bll = new LogBLL();
bll.WriteErrorLog(ex);
_iserror = true;
}
<pre>
modified 20-May-15 9:38am.
|
|
|
|
|
For which method do you get the security warning? I don't think it will be the one you posted here (GetAccountSortByAccountCode(..)).
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
The scan shows the command.ExecuteScalar() is where the security flaw is:
Here is the function and the line highlighted where the error occurs:
protected Object ExecuteScaler(QueryContainer Instance, int i)
{
object returnValue = null;
if (!_iserror)
{
if (_trace)
{
DoTrace("TAMIS.Data.Loader.ExecuteScalar", Instance.Query);
}
if (_connection == null || _connection.State == ConnectionState.Closed)
{
OpenConnection();
}
DbCommand command = _provider.CreateCommand();
command.Connection = _connection;
{
command.CommandText = Instance.Query;
command.CommandType = CommandType.Text;
foreach (var p in Instance.parameterList)
{
SqlParameter param = new SqlParameter(p.name, p.value);
command.Parameters.Add(param);
}
if (_useTransaction) { command.Transaction = _transaction; }
try
{
returnValue = command.ExecuteScalar();
}
catch (Exception ex)
{
if (ex is EntryPointNotFoundException)
throw ex;
RollBack();
LogBLL bll = new LogBLL();
bll.WriteErrorLog(ex);
_iserror = true;
}
<pre>
|
|
|
|
|
Then it's a dumb security report in that it's not able to pinpoint a more helpful location (not in that it's wrong in the first place). The highlighted line must be executed and it can't be executed in any other way. Doesn't the security message mention a method name (other than ExecuteScaler(..)) ?
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
Here is what the report printed out and line 445 is the returnValue = command.ExecuteScaler(). There has to be a reason it flags line 445 which is the code above.
2054 App_Code.dll tamis-root-bak/.../data/loader.cs 445 V.Likely
|
|
|
|
|
Which tool are you using for these security checks?
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
|
It should be able to show you somewhere the complete code path that was executed before it reached that line. Is there maybe a "Details"-link/button or similar?
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
Here is the error log posting:
89 Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') App_Code.dll loader.cs: 445 1 Open none
Attack Vector: system_data_dll.System.Data.IDbCommand.ExecuteScalar
Description: This database query contains a SQL injection flaw. The call to system_data_dll.System.Data.IDbCommand.ExecuteScalar() constructs a dynamic SQL query using a variable derived from user-supplied input. An attacker could exploit this flaw to execute arbitrary SQL queries against the database. ExecuteScalar() was called on the command object, which contains tainted data. The tainted data originated from earlier calls to system_data_dll.system.data.common.dbcommand.executereader, system_web_dll.system.web.httprequest.get_params, system_web_dll.system.web.httprequest.get_form, app_code_dll.tamis.webservice.reportingservice.uicforecastsummaryfybyuic, app_code_dll.tamis.webservice.reportingservice.uicforecastsummaryfy, app_code_dll.tamis.webservice.reportingservice.uicdailyexpended, app_code_dll.tamis.webservice.reportingservice.macomsummarybymonthbyfy, app_code_dll.tamis.webservice.reportingservice.macomsummarybyfy, app_code_dll.tamis.webservice.reportingservice.expendituresbyaccount, app_code_dll.tamis.webservice.reportingservice.currentauthorizations, and app_code_dll.tamis.business.e581.validator.isdocnumvalid.
Remediation: Avoid dynamically constructing SQL queries. Instead, use parameterized prepared statements to prevent the database from interpreting the contents of bind variables as part of the query. Always validate user-supplied input to ensure that it conforms to the expected format, using centralized data validation routines when possible.
|
|
|
|
|
To me this looks like it's not related to the code that you have posted but refers to some other code-path which you haven't yet refactored to use SQL-parameters.
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
