0

Is it safe method for C# against sql injection?

string sqlDeclare = $"DECLARE  @number nvarchar(MAX)  SET @number = '%{sNumber}%' ";


string sqlFilter = "";

if (!string.IsNullOrEmpty(Number))
{
   sqlFilter += $" and [table].[number] like @number ";
}


 string sql = sqlDeclare + " SELECT [table].[*] ";
 sql += " WHERE [table].[state] = 0 and [table].[column1] <> 3 AND [table].[id] > 0  ";

 if (!string.IsNullOrEmpty(sqlFilter))
 {
     sql += sqlFilter;
 }
 sql += " order by datein";

P.S. I can't use Parametr.Add()

Sergey
  • 21
  • 5
  • 2
    NO! See https://stackoverflow.com/questions/910465/avoiding-sql-injection-without-parameters – jason.kaisersmith Nov 30 '21 at 06:54
  • 1
    Does this answer your question? [Avoiding SQL injection without parameters](https://stackoverflow.com/questions/910465/avoiding-sql-injection-without-parameters) – jazb Nov 30 '21 at 06:57
  • 3
    the _only_ safe protections against SQL injection are _parameterised statements_ and whitelisting for those _very few_ cases where you can't use them. – Franz Gleichmann Nov 30 '21 at 06:59
  • 6
    *I can't use Parametr.Add()* - then you'll struggle to be safe from injection. Tell us more about why you can't – Caius Jard Nov 30 '21 at 07:07
  • 3
    *`DECLARE @number nvarchar(MAX)`* - declaring a string called number is probably a really good indicator that a better name is possible – Caius Jard Nov 30 '21 at 07:10
  • @CaiusJard, Because I have been provided with a wrapper function for sqlCommand that takes a string parameter( – Sergey Nov 30 '21 at 07:14
  • Post your wrapper function – Caius Jard Nov 30 '21 at 07:20
  • @CaiusJard, i have no source code. Signature like this Public DataTable Execute4Table(string query) – Sergey Nov 30 '21 at 07:22
  • @Sergey No, would not work, you did not pass to `Execute4Table` the parameter so the database would complain about a undefined parameter, the function you are calling should have an overload similar to `Execute4Table(string commandText, params SqlCommand[])` but... You already said you have no access to the source code. – Cleptus Nov 30 '21 at 07:36
  • 3
    The `Execute4Table` function is fundamentally flawed by *forcing* you to comingle code and data. You'll "never" be safe from SQL Injection whilst you continue to use it. – Damien_The_Unbeliever Nov 30 '21 at 07:41
  • 1
    @Cleptus meant to say SqlParameter[] rather than SqlCommand[], I think – Caius Jard Nov 30 '21 at 07:52
  • Indeed, cannot edit now, thanks for pointing it out – Cleptus Nov 30 '21 at 07:55
  • If you're truly forced to use this function you'll have to do the best you can with checking the inputs. I added some advice to the end of my answer – Caius Jard Nov 30 '21 at 08:08

2 Answers2

4

Is it safe method for C# against sql injection?

if sNumber actually was a numeric type, like int or decimal then I'd say "maybe" because it's pretty hard to inject SQL into a number, but at the end of the day, all you've done is change from concatenating a string value directly into an sql query, which is the classic fail:

"SELECT * FROM t WHERE x = " + str

into putting a string value into a db variable that you then use in an sql. It means the select query won't be the place where the injecting is done, but it just moves the injection point earlier:

Think what happens when sNumber is a value like '; DROP TABLE students;--

DECLARE  @number nvarchar(MAX)  SET @number = '%';
DROP TABLE Students;--%' 
SELECT ...

If you truly are restricted to using this function (it needs throwing away, tbh) then you'll have to do your best at preventing injection by sanitizing the input. In this case you say you're expecting a document number like 2-12 so your C# should look like

if(!Regex.IsMatch(userInput, @"^\d+-\d+$"))
  throw new ArgumentException("Bad input value, expecting document number like 2-12");

var dt = Execute4Table($"SELECT * FROM t WHERE x = '{input}');

In a general sense, it would be clunky, but you could perhaps do some wrapper function that takes N parameters and serializes them to JSON, then you pass them to sqlserver as a json string and get SQLS to deser them into N parameters that you use in the query. The two serializers should encode any user provided values in such a way that they are interpreted as data rather than code. You could also do a similar thing with base64 if your sqlserver is too old for json. To some extents this isn't much different to replacing all ' with '' but with a set/deser approach you're handing off responsibility for that encoding to a well tested library, which is probably the only thing you can do if you are rather hamstrung by this method you're forced to use

Caius Jard
  • 72,509
  • 5
  • 49
  • 80
  • Yeap, thanks. I already checked this). Now i must fix it. – Sergey Nov 30 '21 at 07:26
  • Javascript deserialization also incorporates its own [weird things security wise](https://stackoverflow.com/a/67968265/2265446) but this answer is a good one given how the question evolved (from "_is this code safe?_" to "_how to mitigate SQLi?_". Upvoting your question yet maintining mine because ultimately making other API secure your code is not totally secure (yet it is likely a good mitigation and far better than securing it yourself) – Cleptus Nov 30 '21 at 09:20
2

It would be safe if you type check the sNumber variable and you made sure it has no string dangerous data (your sNumber could have 0';TRUNCATE TABLE [table];-- and that would be SQL Injection). If you check if your snumber is integer, you would be safe on your string interpolation.

Otherwise it would not be safe. It is best to avoid string interpolation/string concatenation.

int checkedNumber;
if (Int32.TryParse(out checkedNumber, sNumber))
{
    string sqlDeclare = $"DECLARE  @number nvarchar(MAX)  SET @number = '%{sNumber}%' ";
    string sqlFilter = "";
    if (!string.IsNullOrEmpty(Number))
    {
       sqlFilter += $" and [table].[number] like @number ";
    }
     string sql = sqlDeclare + " SELECT [table].[*] ";
     sql += " WHERE [table].[state] = 0 and [table].[column1] <> 3 AND [table].[id] > 0  ";

    if (!string.IsNullOrEmpty(sqlFilter))
    {
        sql += sqlFilter;
    }
    sql += " order by datein";
}
Cleptus
  • 3,446
  • 4
  • 28
  • 34