|
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
|
|
|
|
|
I don't really understand what your doing on the database code.
I'm not an expert in c#, but learning, but I started back in 2006 using more of this style, which is similar to what Richard said to use earlier on the 15th.
I'm not implying that you should use this code sample, but to look at it to get a better idea of how it works.
So you make a connection string or read a stored one
Create a connection to the database
create a command object
Add your parameters to the Command Object
Open the database
Read the values returned
Close the Database
Return the values
public int GetAccountSortByAccountCode(int account)
{
int pValue = 0;
string connStr = ConfigurationManager.ConnectionString("DefaultConnection").ConnectionString;
SqlConnection conn = new SqlConnection(connStr);
string query =
"SELECT " +
" ac_sort_order " +
" FROM lkup_account_codes " +
" WHERE ac_code = @account";
SqlCommand cmd = new SqlCommand(query, conn);
SqlParameter param_account = new SqlParameter("@account", SqlDbType.Int);
param_account .Value = account;
cmd.Parameters.Add(param_account);
try
{
conn.Open();
SqlDataReader reader = cmd.ExecuteReader();
while (reader.Read())
{
pValue = reader.GetInt32(0);
}
reader.Close();
}
catch (Exception ex)
{
}
finally
{
conn.Close();
}
return pValue;
}
|
|
|
|
|
Because you really wanted a code review: 
The configuration property is called ConnectionStrings, and the indexer in C# uses square brackets.
Your SqlConnection, SqlCommand and SqlDataReader objects should be wrapped in using blocks.
command.Parameters.AddWithValue is less verbose than creating a new parameter, setting its value, and adding it to the collection. Shorter code means you're more likely to stick with the pattern!
Since you're only returning a single value, there's no need for ExecuteReader; use ExecuteScalar instead. You can also add a TOP 1 to the query.
Your catch block is swallowing every exception, making it impossible to diagnose problems, or to even know when there is a problem.
public int GetAccountSortByAccountCode(int account)
{
string connStr = ConfigurationManager.ConnectionStrings["DefaultConnection"].ConnectionString;
const string query = "SELECT TOP 1 ac_sort_order FROM lkup_account_codes WHERE ac_code = @account";
using (var conn = new SqlConnection(connStr))
using (var cmd = new SqlCommand(query, conn))
{
cmd.Parameters.AddWithValue("@account", account);
conn.Open();
object commandResult = command.ExecuteScalar();
return (commandResult == null || Convert.IsDBNull(commandResult))
? 0
: Convert.ToInt32(commandResult);
}
}
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Now you have me thinking, I can implement similar techniques in VB perhaps
Original in VB
Public Shared Function check_4_Duplicate_AccountName( _
ByVal p_Test_AccountName As String) As Boolean
Dim pCount As Integer = 0
Dim dV As Boolean = True
Dim connStr As String = ConfigurationManager.ConnectionStrings("DefaultConnection").ConnectionString
Dim query As String = _
" SELECT " & _
" COUNT(AccountName) " & _
" FROM CUSTOMER_ACCOUNTS " & _
" WHERE AccountName = @Test_AccountName"
Dim conn As New SqlConnection(connStr)
Dim cmd As New SqlCommand(query, conn)
Dim param_Test_AccountName As SqlParameter
param_Test_AccountName = New SqlParameter("@Test_AccountName", SqlDbType.VarChar, 80)
param_Test_AccountName.Value = p_Test_AccountName
cmd.Parameters.Add(param_Test_AccountName)
Try
conn.Open()
Dim myReader As SqlDataReader = cmd.ExecuteReader()
While myReader.Read()
If Not myReader.IsDBNull(0) Then
pCount = myReader.GetInt32(0)
End If
End While
myReader.Close()
myReader = Nothing
If (pCount = 0) Then
dV = False
End If
Catch ex As Exception
Finally
conn.Close()
conn = Nothing
cmd = Nothing
End Try
Return dV
End Function
Similar Technique in VB that I just wrote, saw the tenary operator in there. Very Fancy, but I left the parameter the same, forgot to modify that.
So I don't need the try catch?
Public Shared Function check_4_Duplicate_AccountName_II( _
ByVal p_Test_AccountName As String) As Boolean
Dim pCount As Integer = 0
Dim dV As Boolean = True
Dim connStr As String = ConfigurationManager.ConnectionStrings("DefaultConnection").ConnectionString
Const query As String = _
" SELECT " & _
" COUNT(AccountName) " & _
" FROM CUSTOMER_ACCOUNTS " & _
" WHERE AccountName = @Test_AccountName"
Using conn As New SqlConnection(connStr)
Using cmd As New SqlCommand(query, conn)
Dim param_Test_AccountName As SqlParameter
param_Test_AccountName = New SqlParameter("@Test_AccountName", SqlDbType.VarChar, 80)
param_Test_AccountName.Value = p_Test_AccountName
cmd.Parameters.Add(param_Test_AccountName)
Try
conn.Open()
Dim cmdResult As Object = cmd.ExecuteScalar()
dV = If(cmdResult Is Nothing Or Convert.IsDBNull(cmdResult), False, True)
Catch ex As Exception
Finally
conn.Close()
End Try
End Using
End Using
Return dV
End Function
|
|
|
|
|
You don't need the Finally - the Using block will close the connection for you.
The Catch block is swallowing all exceptions. If something goes wrong with your query, your method will return the default value, with no indication that something went wrong.
You should only Catch exceptions which you can handle in your code, or which you want to log and ignore. Everything else should be allowed to propagate up the call-stack until it either meets some code which can handle it, or reaches the entry-point of the thread/process.
The only exception would be at the entry point of the thread or process, where you might want to log every unhandled exception before terminating the program. You still need to be careful, since your logging code might also fail, depending on how messed-up the application state is.
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
Thanks for the clarification on the subject.
I still have more to learn here.
So I don't' need the conn.close() then, since using will close the connection for me.
And if the query is correct, then I don't need try catch.
Alright, I get it now.
|
|
|
|
|
The Veracode Scaner finally did not find this as an SQL injection error. The final fix:
namespace TAMIS.Business.Common
{
public class MyParam
{
public SqlParameter SqlParam { get; set; }
public string Name { get; set; }
public object Value { get; set; }
public int Type { get; set; }
}
public class QueryContainer
{
string _query;
public List<myparam> parameterList = new List<myparam>();
public QueryContainer(string query) { _query = query; }
public SqlDbType AddParameterType(int type)
{
switch (type)
{
case 1:
return (SqlDbType) Enum.Parse( typeof(SqlDbType) , "int" , true );
case 2:
return (SqlDbType)Enum.Parse(typeof(SqlDbType), "NVarChar", true);
}
return SqlDbType.VarChar;
}
public string Query
{
get
{
return _query;
}
set { _query = value; }
}
}
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.SqlParam = new SqlParameter("@account", Instance.AddParameterType(1));
myParam.SqlParam.Value = account;
Instance.parameterList.Add(myParam);
return Convert.ToInt32(ExecuteScaler(Instance, 1));
protected Object ExecuteScaler(QueryContainer Instance, int i)
{
object returnValue = null;
if (!_iserror)
{
if (_trace)
{
}
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)
{
command.Parameters.Add(p.SqlParam);
}
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;
}
finally
{
if ((!KeepAlive && _connection.State == ConnectionState.Open) || _iserror == true)
{
CloseConnection();
}
}
}
}
else
{
returnValue = -1;
}
return returnValue;
}
<pre>
modified 21-May-15 11:18am.
|
|
|
|
|
Hi I'm using JQuery AJAX to call a web service and this webservice gets data from a database. Everything seems fine; I was able to get the data that the web service got from a database and display it on the front end.
The problem is when I deploy my application to IIS, my ajax calls returns "Request Failed!" which indicates that it cannot communicate with my web service. The following is my AJAX
$.ajax({
type: "post",
url: 'h t t p: // localhost:48690/MyWebService.asmx/MyWebserviceMethod',
data: JSON.stringify({newText: newText, pageURL: pageURl }),
contentType: "Application/json; charset=utf-8",
dataType: "json",
success: function (response) {
alert(response);
},
error: function (response, status) {
alert('Request Failed!');
}
});<br />
My question is, is there anything wrong with the way I write my URL in the AJAX method, if not what do you think is causing my web service to fail when I deploy my app.
If you are wondering why there are gaps in "http" in my url, that is so I can dispay my message without the anchor tag getting added to my post automatically. Thanks in advance for your replies.
modified 15-May-15 5:19am.
|
|
|
|
General 
News 
Suggestion 
Question 
Bug 
Answer 
Joke 
Praise 
Rant 
Admin 
Submit an article or tip
