When to worry about SQL injection protection

Question

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.

You should always be concerned with it but it sounds like the only way someone outside of a developer can submit database requests is through the a dropdown selection so there's very little chance that they could inject anything. Is this a web app or console app, having a web app would greatly increase the risk because of different ways to post. using a console app it probably wouldn't be much of a problem. — Robert, Jan 16 '13 at 21:05
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. — ta.speot.is, Jan 16 '13 at 21:05
Why not use parameterized queries? It's not that much harder. And it produces way more solid SQL. So just use parameters! — Jacco, Jan 16 '13 at 21:05
SQL-Parameters don't prevent you only from sql-injection attacks. They also make the sql more readable, maintanable and they help to avoid [conversion](http://weblogs.sqlteam.com/jeffs/archive/2006/07/21/10728.aspx) or localization issues. — Tim Schmelter, Jan 16 '13 at 21:05
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 '13 at 21:07
Your query is not sufficient to avoid SQL injection. If the application is web-based then someone can send any value they want for `Date` regardless of the options you offer. — HABO, Jan 16 '13 at 21:07
This is not sufficiently parametrized at all. The parameters should never be concatenated/formated strings. — SmartK8, Jan 16 '13 at 21:08
Parameterized queries are a lot easier to write than the mess with quotes needed for the other kind. It is true that a malicious user can set arbitrary data on any form that takes input, eg. they could modify the html to let them send any option on the select. If you validate your input this is less of a problem. — Peter Wooster, Jan 16 '13 at 21:10
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. — Marc Gravell, Jan 16 '13 at 21:11
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 '13 at 21:12
@Pete: They are blissfully happy to believe that no one else at this company would have the knowledge to be able to do something like that. — PiousVenom, Jan 16 '13 at 21:14
@MarcGravell: I fully agree. It's a dll that was written by the programmer before me for some previous software the company used, and they feel it infallible. — PiousVenom, Jan 16 '13 at 21:15
Note: The OP never shows the definition of `Date`. If it's a `DateTime` object the statement should be fairly safe from injection attacks. — , Jan 16 '13 at 21:20
http://stackoverflow.com/questions/5468425/how-do-parameterized-queries-help-against-sql-injection — MadHenchbot, Jan 16 '13 at 22:46

score 6 · Accepted Answer · answered Jan 16 '13 at 21:08

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 :)

answered Jan 16 '13 at 21:08

Marc Gravell

1,026,079
266
2,566
2,900

@ Marc Gravell - VERY well said. I was just going to ask about those very things. +1 – Brian Jan 16 '13 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](http://blog.divergencehosting.com/2009/04/09/using-parameters-parameterized-queries-database-interactions-cshar-vbnet/), but it appears the query is created in the same method that executes it. – PiousVenom Jan 16 '13 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 '13 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 '13 at 21:23
@Pete: They are actually `date` objects. – PiousVenom Jan 16 '13 at 21:25
@Hogan if your user has direct access to the DB, then indeed there are already complications. – Marc Gravell Jan 16 '13 at 23:17
@MarcGravell - Well the application (and thus the user) does since it can send a select statement as shown above. – Hogan Jan 17 '13 at 00:51

score 4 · Answer 2 · answered Jan 16 '13 at 21:05

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/

Hogan · Answer 3 · 2013-01-16T21:25:16.167

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.

score 1 · Answer 4 · answered Jan 16 '13 at 21:07

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.

answered Jan 16 '13 at 21:07

jimbo

11,004
6
29
46

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 '13 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 '13 at 21:09
Sure. With something like Spy++ I can get the window handle of the drop down and add to the backing listbox. http://msdn.microsoft.com/en-us/library/dd460756.aspx – Eric J. Jan 16 '13 at 21:11
@EricJ. If you're going to use debugging tools, why not just replace the query string literal with whatever you want? – Jan 16 '13 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 '13 at 21:15
@ebyrob: Depending on the security exposure, it may well be advisable to obfuscate the program to make it at least more hack resistant. That would make it much harder (though not impossible) to alter the string literal. Getting a handle to a combobox and sending it a CB_ADDSTRING message is much simpler http://msdn.microsoft.com/en-us/library/windows/desktop/ff485901(v=vs.85).aspx and cannot be stopped by ordinary obfuscation mechanisms. – Eric J. Jan 16 '13 at 21:18

score 0 · Answer 5 · answered Jan 16 '13 at 21:15

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.

When to worry about SQL injection protection

5 Answers5