0

Obviously this SQL statement is vulnerable:

Dim sql As String = "select * from person where fname = '" & fname & "'"

But what of this?

Dim sql as string = "select * from person where fname = '" & fname.Replace("'", "''") & "'"

Ideally we'd use SQL parameters but I'm wondering if the latter style of SQL statement is still safe or if I need to fix these as well...

ADyson
  • 57,178
  • 14
  • 51
  • 63
ekolis
  • 6,270
  • 12
  • 50
  • 101
  • 1
    why not just use parameters as everyone everywhere recommends, and stop worrying about it? What's preventing that? Whilst attempting to escape your quote marks _might_ help in some cases, it's not the only vulnerability, and it's not inconceivable that someone could work round it, if they become aware of what you're doing. – ADyson Feb 12 '20 at 14:20
  • AFAIK it would indeed be safe, but it would of course also totally depend on *every* programmer doing that *every* time for *every* query, and how are you going to ensure that... SqlParameters for the win, as you also seem to agree with. – Peter B Feb 12 '20 at 14:21
  • 1
    Prepared statements have benefits beyond preventing SQL injection. For example reusing a statement, and reducing overhead by only sending the parameter values to the server for repeated execution, providing conversion between language data types to expected server datatypes, etc. – Mark Rotteveel Feb 12 '20 at 14:24
  • @PeterB TBF you'd ensure it just the same way that you ensure that every programmer uses parameters every time for every query...by someone experienced doing a code review. But like you say, if you're going to enforce something, you might as well enforce the proper approach. – ADyson Feb 12 '20 at 14:24
  • @ADyson why not just use parameters everywhere? because this is a HUGE app with over 18,000 SQL statements, and it will take a while to comb through all of them. If I can skip some that will make this process quicker. – ekolis Feb 12 '20 at 14:27
  • 1
    @ekolis I see so it's a legacy maintenance issue. My advice would be, don't skip any of them. Just make a plan to gradually replace them with secure code. it might take a while, but it's 100% necessary. I don't think you can ever be certain that any alternative approach is truly safe. Maybe you can prioritise areas of the app which are more heavily used, or are accessed by a wider range of users, or something. That could be seen as taking a sensible risk-based approach to the maintenance, at least. – ADyson Feb 12 '20 at 14:31
  • Probably not all of those 18,000 SQL statements are vulnerable to SQL injection. Many have no program variables being concatenated into them. Others have program variables, but the variables don't contain untrusted content. Others have variables derived from external input, but they are integers, so they won't risk SQL injection. All of them need to be _reviewed_, but I'm sure not all need to be _changed_. – Bill Karwin Feb 12 '20 at 22:47
  • Given that you need to change some of those queries, why would you bother to change them to _another_ form that may be vulnerable to SQL injection? I mean, you're editing them, why not edit them to be really safe? – Bill Karwin Feb 12 '20 at 22:49
  • Yeah, I can change the ones that are definitely broken to use parameters; I'm just wondering about the ones that already use `Replace` to escape quotes. And there are some that literally concatenate *table* names from user input - I think I will need to do some sort of whitelisting there! – ekolis Feb 13 '20 at 14:35
  • Yes, whitelisting the table names against a list of known table names is one method. Or filtering them against a regular expression like `[a-z0-9_]`, and establish a policy that table names containing spaces or punctuation are not allowed. – Bill Karwin Feb 14 '20 at 15:36
  • 1
    But to answer your first question, no, the method of `Replace` is not sufficiently safe. The vulnerable cases are obscure, but real. Read some of the good answers here: https://stackoverflow.com/questions/5741187/sql-injection-that-gets-around-mysql-real-escape-string – Bill Karwin Feb 14 '20 at 15:44

0 Answers0