6

Small background: I'm the only programmer for this company. I'm working with pre-existing frameworks.

That said, the company has a dll(Database.dll) which contains "all the database interactions I need". As in, it has a Query(), Update(), Insert(), etc. Now, the project I'm writing sets a reference to Database.dll. My project accepts zero user input. The closest thing to user input is a dropdown box that the user can select a date from. Not having much experience with it, I'm curious if I still need to worry about SQL injections? And if so, would a query written like

var query = string.Format("SELECT timestamp FROM table1 WHERE date = \"{0}\" 
                           AND measured_dist = bit_loc AND rop > 0" , Date))

be sufficient as a parameterized query? Keep in mind, all of the query execution is handled by the pre-existing Query() that I'm told I have to use, and can't edit.

EDIT

This program is a WinForm application.

16
  • 19
    The correct answer is "always".
    – Kirk Woll
    Jan 16, 2013 at 21:02
  • 3
    Unless you're doing something that can't be parameterized, just make it parameterized. Security aside, you might get a performance increase: parameterization will save your RDBMS the effort of parameterizing the query itself to match it to an execution plan. One the client side you don't need to allocate/format lots of strings, code is neater, etc etc. Jan 16, 2013 at 21:05
  • 1
    As long as users can't type in text that becomes part of a query, then you should be fine. But what you're doing above is not a "parameterized query", that's a string.Format() call and gives you no protection against injection. For example, if the "Date" variable contained the value: "12/12/12\";DELETE * FROM table1;\"SELECT timestamp FROM table1 WHERE 1=1"; you'd be in trouble.
    – Pete
    Jan 16, 2013 at 21:07
  • 1
    Re the existing Query method: who-ever says you "have to use, and can't edit" is not a good person to be making technical decisions: every part of that is wrong. I can suggest some ways to make the change to parameters easy, if it helps. Jan 16, 2013 at 21:11
  • 1
    The "Database.dll" sounds like a disaster waiting to happen. Just curious, what keeps someone from using it in their own apps to wipe out your databases. For example, an unhappy employee who normally might not be given DB acccess?
    – Pete
    Jan 16, 2013 at 21:12

5 Answers 5

6

As noted in comments, the answer is "always". Since it would be so easy to add a parameter to that and do it properly, rather than concatenation: just do it right first time. Also: have you considered that injection is not the only problem in the code you've shown? That code is also susceptible to localisation / internationalisation. What happens for a user who has their PC configured in a different culture? The dates and numbers will get rendered differently - and will often break. That doesn't happen with parameters. Also: names often have apostrophes in :)

7
  • @ Marc Gravell - VERY well said. I was just going to ask about those very things. +1
    – Brian
    Jan 16, 2013 at 21:09
  • @MarcGravell: The database I'm querying against has a format(YYYY-mm-dd) that is the same in every case, and I format my Date to fit that. From what I'm reading about parameterized queries, is it possible to do so when the command execution is handled by another method? I've been reading over this, but it appears the query is created in the same method that executes it.
    – PiousVenom
    Jan 16, 2013 at 21:20
  • @MarcGravell - This is a C# local application. Parameters are not enough. Needs to have an specific user and stored procedures (which are the only access that user has) otherwise it is simple to look inside the app and get the connection string and go to town.
    – Hogan
    Jan 16, 2013 at 21:23
  • @TyrionLannister If the dates are stored in a date column type, then the parameters will get converted into a SQL compatible format. If they have a "very specific format" it makes me think they're storing their dates as strings which is, well, silly, to be kind.
    – Pete
    Jan 16, 2013 at 21:23
  • @Pete: They are actually date objects.
    – PiousVenom
    Jan 16, 2013 at 21:25
4

Do extend on @KirkWoll's very valid comment, any time you incorporate any user input (or input from automated sources for that matter) in a SQL statement, you place your program at risk of SQL injection.

As a matter of policy, you should never, ever build your own SQL statement using any such input.

Always sanitize input and always use parameterized queries as a first line of defense against SQL injection.

In case you have not seen it before, there's a great illustration on xkcd

http://xkcd.com/327/

0
2

Given that this is a WinForms program the only safe way to access the database is to use Stored Procedures that take parameters. Then create a user that only has access to those SPs. Anything else is not secure.

While queries with parameters work as a security measure when used with web applications which can have "attack" input, they fail when used with a local application which can be dis-assembled and re-written to anything. If you don't provide SP security you are lost.

1

Even though the user interaction may be a drop-down, it's possible for a sophisticated attacker to insert a value that is not in the list of selections. So YES, you should still be wary of SQL injection.

6
  • Not even a "sophisticated" attacker. Anyone with modest programming skills can view source on the HTML, edit it to their own liking, and use that in place of the original HTML. Those with a little more skill can use a browser add-in to semi-automate the process.
    – Eric J.
    Jan 16, 2013 at 21:08
  • Yeah, it wasn't clear to me from the question whether it was specifically about HTML interfaces. If it's a desktop app of some kind it might take more finesse, to Bart's comment.
    – jimbo
    Jan 16, 2013 at 21:09
  • Sure. With something like Spy++ I can get the window handle of the drop down and add to the backing listbox. msdn.microsoft.com/en-us/library/dd460756.aspx
    – Eric J.
    Jan 16, 2013 at 21:11
  • @EricJ. If you're going to use debugging tools, why not just replace the query string literal with whatever you want?
    – user645280
    Jan 16, 2013 at 21:14
  • @jimbojw: Certainly harder than with an HTML UI, but in the realm of a junior programmer who is versed in such things (speaking from experience).
    – Eric J.
    Jan 16, 2013 at 21:15
0

I would use prepared statements even if there was no such thing as SQL injection. They are just easier to use and in some cases they allow a database to cache the statement and not have to compile it the next time you use it. Oracle does that, I think SQL Server does, I don't know if MySQL does.

You should always assume that there are hackers, even on internal intranet projects, I use prepared statements and I use nonces to prevent CSRF.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.