|
It's concatenated with a + but it's not concatenating values as literals. All required values are either hard-coded (like "3,4,7,8") or provided via Sql-parameters.
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
You're only concatenating constant strings, not user input or other variables, so there's no vulnerability in that example. You could easily remove the concatenation and declare the query in a single string:
const string sqlQuery = "select isTamMacom = count(macom_key) FROM hier_fy WHERE hier_key = @aspKey AND fy = @fy AND @accountCode NOT IN (3,4,7,8) AND macom_key IN (select hier_key from lkup_e581_MacomThatRequireTAM) AND is_visible = 1 AND is_active = 1";
If you want to split the string onto multiple lines for readability, use a verbatim string literal:
const string sqlQuery = @"select
isTamMacom = count(macom_key)
FROM
hier_fy
WHERE
hier_key = @aspKey
AND
fy = @fy
AND
@accountCode NOT IN (3,4,7,8)
AND
macom_key IN
(
select hier_key
from lkup_e581_MacomThatRequireTAM
)
AND
is_visible = 1
AND
is_active = 1";
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
One of the concerns that I have is that if the query string has no parameters could that show up as a finding?
|
|
|
|
|
Presumably, the tool has detected that you've used string concatenation on your query, without actually checking whether the strings are variables or constants.
If you mark all of your query strings as const , that should get rid of the warnings, as well as making sure you don't have any SQLi vulnerabilities left.
"These people looked deep within my soul and assigned me a number based on the order in which I joined."
- Homer
|
|
|
|
|
As I've never worked with the tool you're using for SQL-injection-checking I can't tell for sure; but the "error"-report that you posted recently read like it bases its checks not on statically analyzing your code (the sql-statements) but on attempted (harmless) injections (and then identifying the injected values when they reappear in the finally executed sql). Which, if I'm right here, would mean that the tool would not stumble upon your harmless constant string concatenation here. It would mean that there's other code somewhere which actually still is susceptible to SQL-injection.
Richard's suggestion to mark all your query strings as const will definitely help.
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
I am trying to create my own theme and I am using the "wp_nav_menu()" function but, it is not working. The documentation on wordpress.org says that the function should return true if it finds the location and false if it doesn't. My function returns no value which leads me to believe that the PHP is not recognizing the function call. Could someone help me out? I am new to WordPress and out of ideas.
Thanks,
Kevin Haynes
modified 29-May-15 2:49am.
|
|
|
|
|
Hi,
I saw this article:
[^]
and I would like to ask if it's possible to host the DLL in PHP nstead of WPF? and how?
Thanks
Jassim[^]
Technology News @ www.JassimRahma.com
|
|
|
|
|
Message Removed
modified 22-May-15 14:38pm.
|
|
|
|
|
Hi All,
I have been given an existing WordPress project for a website, I am trying to open it, and I am not able to find which software should I have to open a WordPress project for adding new code to the Project.
Any link, code snippet or even a suggestion helps me great.
Thanks in advance.
Thanks,
Abdul Aleem
"There is already enough hatred in the world lets spread love, compassion and affection."
|
|
|
|
|
|
This is very similar to a previous post but with different code.
I have to eliminate a SQL injection error from within a method. Now, with only minor modifications this error must be eliminated. Here is the description from the scan:
Attack vector: system_data.system.data.IDbCommand.ExecuteReader
Description: The database query contains a sql injection flaw. The call to system_data_dll.System.Data.IDbCommand.ExecuteReader 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. ExecuteReader was called on the command object, which contains tainted data. The tainted data originated from earlier calls to system_data_dll.data.common.dbcommand.executereader, System_web_dll.system.web.httprequest.get_params, system_web_dll.data.common.dbadapter_fill, system_data_dll.system.data.common.dbwommand.executescarar and system_web_dll.system.web.httprequest.get_form
Code:
protected DataTable ExecuteDataTable(DbCommand command, ParamData[] pDataArr)
{
DataTable returnValue = null;
try
{
if (_connection == null)
OpenConnection();
else
{
if (_connection.State == ConnectionState.Closed)
OpenConnection();
}
command.Connection = _connection;
command.CommandType = CommandType.Text;
command.CommandTimeout = 12000;
for (int i = 0; i < pDataArr.Length; i++)
{
DbParameter parameter = command.CreateParameter();
parameter.ParameterName = pDataArr[i].pName;
parameter.DbType = pDataArr[i].pDataType;
parameter.Value = pDataArr[i].pValue;
command.Parameters.Add(parameter);
}
returnValue = new DataTable();
DbDataReader reader;
reader = command.ExecuteReader();
using (reader)
{
returnValue.Load(reader, LoadOption.OverwriteChanges);
}
reader.Close();
if (!KeepAlive && _connection.State == ConnectionState.Open)
{
CloseConnection();
}
}
catch (Exception e)
{
if (e is EntryPointNotFoundException)
throw e;
_iserror = true;
LogBLL bll = new LogBLL();
bll.WriteErrorLog(e);
}
pDataArr = null;
return returnValue;
}
Thanks in advance!
modified 12-May-15 17:16pm.
|
|
|
|
|
I assume that it's the same thing as in your previous question. Though there are SQL-parameters used in this method, it gets its command-object passed as an argument with the command-text apparently already assigned. I guess the calling code concatenates some values (other than there are in pDataArr) as literals into the query string.
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
I saw a very good answer to my question about parameterizing a string with the actual parameters; how would I know whether the command was an UPDATE, INSERT, or a DELETE?
SqlCommand cmd = new SqlCommand(commandText, connection);
SqlParameterCollection sp = cmd.Parameters;
List<SqlParameter> sp = new List<SqlParameter>()
{
new SqlParameter() {ParameterName = "@CmpyCode", SqlDbType = SqlDbType.NVarChar, Value= CV.Global.CMPYCODE},
new SqlParameter() {ParameterName = "@Code", SqlDbType = SqlDbType.NVarChar, Value = codeName},
new SqlParameter() {ParameterName = "@DisplayCode", SqlDbType = SqlDbType.NVarChar, Value = codeName + "-"},
new SqlParameter() {ParameterName = "@TotalDigit", SqlDbType = SqlDbType.Int, Value = CV.Global.PARAMTOTALDIGIT}
};
insertData(CV.Sps.SP_INSERT_PARAM_TABLE, sp);
SqlCommand cmd = new SqlCommand();
cmd.Parameters.AddRange(parameterPasses.ToArray());
|
|
|
|
|
Steve Holdorf wrote: how would I know whether the command was an UPDATE, INSERT, or a DELETE? I'm a bit confused - what is the context for this question? I don't see how it is related to your previous questions. And I don't see why you posted that code, which appears to be three separate fragments?
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
OK. Let me explain. I have found some code that I think will work but how would I know what sql command it would be. See code below:
SqlCommand cmd = new SqlCommand(commandText, connection);
SqlParameterCollection sp = cmd.Parameters;
List<SqlParameter> sp = new List<SqlParameter>()
{
new SqlParameter() {ParameterName = "@CmpyCode", SqlDbType = SqlDbType.NVarChar, Value= CV.Global.CMPYCODE},
new SqlParameter() {ParameterName = "@Code", SqlDbType = SqlDbType.NVarChar, Value = codeName},
new SqlParameter() {ParameterName = "@DisplayCode", SqlDbType = SqlDbType.NVarChar, Value = codeName + "-"},
new SqlParameter() {ParameterName = "@TotalDigit", SqlDbType = SqlDbType.Int, Value = CV.Global.PARAMTOTALDIGIT}
};
insertData(CV.Sps.SP_INSERT_PARAM_TABLE, sp);
SqlCommand cmd = new SqlCommand();
cmd.Parameters.AddRange(parameterPasses.ToArray());
|
|
|
|
|
You didn't explain a whole lot
Am I right in assuming that you want to change your code to use stored procedures instead of 'text-statements' (CommandType.Text) and in order to execute the right one, need to know if it should be an UPDATE, INSERT, or DELETE ?
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
No. I still have to use the static command but eliminate the hard coded parameters. That being the case I want to use the technique above for passing in the parameters, use the loop, which is the problem to begin with, to fill in a parameterized string like so:
command.CommandText = ("INSERT INTO TABLE (result, title, des) values(@store_result, @store_title, @store_des)");
-- modified 12-May-15 20:39pm.
|
|
|
|
|
I still can't follow you completely. You're omitting some steps of your train of thought
The method in your original post takes a DbCommand and ParamData[], already has some kind of parameter-filling-loop and then runs an ExecuteReader(). The DbCommand apparently already has a SELECT-statement assigned and my assumption was that this statement already contains some values as literals, which is why the method failed your SQL-injection test, despite the rest of the values are added via parameters.
As you're now quoting an INSERT-statement, you must be talking about some completely different method that I've not seen yet. Please post that method and also the calling code and maybe elaborate on why using a loop to create the parameters is a problem.
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
Lefevre,
You make a good point about me being confused. I really can't show you anything that doesn't require a loop. I have three other findings that are simple hard coded commands with parameters as part of the string. This is what I showed you in my first post that you answered before this one. In every case a loop is required to add the sql parameters to the command. I have been looking for some kind of Lambda expression to add the values to the sql command parameter list which I can not find.
|
|
|
|
|
How about we deal first with the method you posted in your original question?
To help you further with that, I would like to see the calling code - can you post that?
/Sascha
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
I think I have a solution. Can someone review the solution and let me know what they think?
The Singleton created with it's _id property that is passed in from the calling function:
public class QueryContainer
{
private static List<querycontainer> Container;
private static QueryContainer instance;
private int _id;
public int _searchID;
private string _query;
private QueryContainer () { }
public static QueryContainer Instance
{
get {
if (Instance == null)
{
instance = new QueryContainer();
}
return instance;
}
}
public string Query { get { return Container.Find(instance => instance._id == _searchID).Query; }
set { Container.Query = value; _id =+ 1; } }
}
}
}
public int ID { get { return _id; } }
}
The calling code that passes the id to access the query string from the singleton:
protected object ExecuteScaler(int id)
{
object returnValue = null;
Container Instance = new Container ();
Instance.searchID = id;
DbCommand command = _provider.CreateCommand();
command.Connection = _connection;
command.CommandText = Instance.Query;
command.CommandType = CommandType.Text;
if (_useTransaction) { command.Transaction = _transaction; }
try
{
returnValue = command.ExecuteScalar();
}
...
|
|
|
|
|
It's awful. It really is, sorry.
- A singleton is completely unneccessary for what you're trying to do.
- It's the worst possible implementation of the singleton pattern.
- It won't compile because of the setter of the Query-property.
- The getter of the Query-property will cause a stack overflow.
- There's no need for a "Query-Container-List" for what you're trying to do.
- It doesn't address the actual problem of SQL-injection / SQL-parameters at all.
I suggested a way for further course of action in my last message - it's up to you
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|
|
There is no substitute for doing it the right way.
|
|
|
|
|
I have to eliminate a SQL injection error from within a method. What the code is doing is passing in a SQL querystring as the command to a DbCommand object, see the code below. Now, with only minor modifications this error must be eliminated. Here is the description from the scan:
This database query contains a sql injection flaw. the call to system_data_dll.Data.IDbCommand.ExecuteNonQuery constructs a dynamic sql queryusing a variable derived from the user-supplied input.An attacker could exploit this flaw to execute arbitrary sql queries against the database ExecuteNonQuery was called on the command object, which contains tainted data. The tainted data originated from from earlier calls to system_data.system.data.common..dbconnand.execurereader, system_web_dll.wweb.httprequest.get_params, system_data_dll.system.data.system.data.common.dbaadapter.fill.
Below is the actual function code:
protected object ExecuteScaler(string queryString)
{
object returnValue = null;
if (!_iserror)
{
if (_trace)
{ DoTrace("TAMIS.Data.Loader.ExecuteScalar", queryString); }
if (_connection == null || _connection.State == ConnectionState.Closed)
{
OpenConnection();
}
DbCommand command = _provider.CreateCommand();
command.Connection = _connection;
command.CommandText = queryString;
command.CommandType = CommandType.Text;
if (_useTransaction) { command.Transaction = _transaction; }
try
{
returnValue = command.ExecuteScalar();
}
catch (Exception ex)
{
if (ex is EntryPointNotFoundException)
throw ex;
//if (_useTransaction == true)
//_transaction.Rollback();
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;
}
Thanks in advance for all of your help!
|
|
|
|
|
The way to avoid conventional SQL-injection attacks is to use SQL-parameters. That is, not passing the values for your query as literals concatenated into the SQL-statement but to have the names of SQL-parameters in those places of the SQL-statement instead and add SQL-parameters 'carrying' the actual values to the parameter-collection of the command-object.
E.g. instead of this:
SELECT col1 FROM table1 WHERE col2 = 'string from user input';
..it should be like this:
SELECT col1 FROM table1 WHERE col2 = @userinput;
..plus creating an SQL-Parameter with a name of "@userinput", assigning the value 'string from user input' to it and adding it to the parameter-collection of the command-object.
But the posted method is being passed the final query string as an argument, so it's already too late to do this. You will have to change this method
- either to accept an SQL-parameter-collection with the parameters created where the method is being called
- or to accept a value-collection and create SQL-parameters for those values
and then add the SQL-parameters to the command-object. And the calling code obviously has also to be changed accordingly.
http://www.dotnetperls.com/sqlparameter[^]
https://msdn.microsoft.com/en-us/library/system.data.common.dbparameter%28v=vs.110%29.aspx[^]
If the brain were so simple we could understand it, we would be so simple we couldn't. — Lyall Watson
|
|
|
|