Does TryParse protect against sql injection?

Question

I have an SQL request where I need to concatenate data into the request:

if (dataChoosen != "randomValue")
{
    sCondition = " WHERE RandomField = '" + dataChoosen + "' ";
}               
cd.CommandText = "SELECT xData FROM table " + sCondition + "GROUP BY  xxx";

As I need to concatenate the condition, I don't think I can use a prepared request?

Also, I already tryparse the 'dataChoosed' value because it comes from a textbox and I need an integer. So is the the tryparse enough to prevent SQL injection?

You should use parameters. There is nothing wrong with concatenating SQL with parameters. — SLaks, Mar 16 '17 at 19:10
Always use parameters, see also [Best Practices - Executing Sql Statements](http://stackoverflow.com/documentation/.net/3589/ado-net/14261/best-practices-executing-sql-statements) for additional whys and hows. — Igor, Mar 16 '17 at 19:12
The past participle of "choose" is "chosen", not "choosed". It makes it easier for other people to look at your code if variable names have correct spellings in them :) — Andrew Morton, Mar 16 '17 at 19:14
since you are already using the tryparse, why not use the int value that you got out of the tryparse instead of the text value? — Kevin, Mar 16 '17 at 19:16
Read this. Burn it into your soul. http://stackoverflow.com/questions/7505808/why-do-we-always-prefer-using-parameters-in-sql-statements — Wesley Long, Mar 16 '17 at 19:18
@Kevin I would upvote that comment, but it might be best to not even mention that as it will lead someone into a false sense of security somewhere down the line. [If that comment disappears, I'll delete this one.] — Andrew Morton, Mar 16 '17 at 20:16

Kevin · Answer 1 · 2017-03-16T19:50:45.463

2

I would just use parameters, there's no reason not to.

if (dataChoosed != "randomValue")
{
    sCondition = " WHERE RandomField = @dataChoosed ";
}               
cd.CommandText = "SELECT xData FROM table " + sCondition + "GROUP BY  xxx";
cd.Parameters.Add("@dataChoosed", SqlDbType.VarChar).Value = dateChoosed;

edited Mar 16 '17 at 19:50

answered Mar 16 '17 at 19:23

Kevin

7,162
11
46
70

Please never ever use AddWithValue: [Can we stop using AddWithValue() already?](http://blogs.msmvps.com/jcoehoorn/blog/2014/05/12/can-we-stop-using-addwithvalue-already/) – Andrew Morton Mar 16 '17 at 19:32
@AndrewMorton how's that? – Kevin Mar 16 '17 at 19:51
It's OK if the OP is using a VarChar column for the integer value they mention (which they should not be). If it really is a VarChar type, then it is better to also specify the length to be the same as the declaration in the database. There is more information at [Best Practices - Executing Sql Statements](http://stackoverflow.com/documentation/.net/3589/ado-net/14261/best-practices-executing-sql-statements#t=201703162000398457387) (as linked to in Igor's comment). – Andrew Morton Mar 16 '17 at 20:02
1

@AndrewMorton yeah. just hard to write sample code when you don't really know what that datatypes involved are. – Kevin Mar 16 '17 at 22:28

Dmitry Bychenko · Answer 2 · 2017-03-16T19:55:08.663

No, you are not on the safe side. Even if dataChoosed is an innocent integer value, bad boys can hurt you with, say, negative value format:

  // It's good old "-1", with a bit strange format 
  // (let use "delete from table commit;" as an injection) 
  string dataChoosed = "1'; delete from table commit; --1";

  // A little hack: let "-" sign be... 
  CultureInfo hacked = new CultureInfo("en-US");
  hacked.NumberFormat.NegativeSign = "1'; delete from table commit; --";
  Thread.CurrentThread.CurrentCulture = hacked;

  if (dataChoosed != "randomValue")
  {
      int v;

      // since "1'; delete from table commit; --1" is of correct fotmat it will be parsed
      if (int.TryParse(dataChoosed, out v))
          sCondition = " WHERE RandomField = '" + dataChoosed + "' ";
  }               

  cd.CommandText = "SELECT xData FROM table " + sCondition + "GROUP BY  xxx";

And, woe! Where's my table? The command text will be

  SELECT xData FROM table = '1'; delete from table commit; --1'GROUP BY  xxx

which is efficently two queries:

  SELECT xData FROM table = '1'; -- the innocent one
  delete from table commit;      -- an arbitrary query from the attacker

(I've removed commented out --1'GROUP BY xxx fragment)

Please, use parameters, do not tempt us. Please, notice, that you don't want to change code: all you have to do is to change the Regional Settings in your Windows.

score -2 · Answer 3 · answered Mar 16 '17 at 19:20

-2

Does [BLANK] protect against sql injection?

Unless [BLANK] is 'parameters' the answer is always no.

answered Mar 16 '17 at 19:20

aquinas

23,318
5
58
81

1

Dunno. Wasn't me. Perhaps it was too terse, and `[BLANK]` should have been "some-new-idea-about-sql-injection-prevention". Let's ignore that "new-idea" isn't new. – Andrew Morton Mar 16 '17 at 20:39
@AndrewMorton, yeah I mostly thought it was funny/scary. Comments above "You should use parameters." 10 up votes. "Always use parameters" 6 up votes. This answer, basically saying the exact same thing: always use parameters because of reasons like Dmitry Bychenko mentioned above, among many others (multi-byte character exploits (https://security.stackexchange.com/questions/9908/multibyte-character-exploits-php-mysql) etc): 3 down votes. – aquinas Mar 17 '17 at 15:04

Does TryParse protect against sql injection?

3 Answers3