How to prevent SQL Injection on following code in Visual Basic 2012

Question

I"m confused on how to prevent SQL injection, I've looked online. Do I use a store procedure, or do I Create variables, Im just completely lost.

 Try
 connection.Open()
 ’we got here so our connection to the db is sound
 chosen = cboBooks.SelectedIndex
 id = customerList(cboCustomers.SelectedIndex)
 isbn = isbnList(cboBooks.SelectedIndex)
 If number <= qty Then
     Dim sql As String
     sql = "INSERT INTO purchase(customer_id, ISBN, store_id, quantity)
                        VALUES(" & id & ", " & isbn & ", 1, " & number & ");"
     Dim cmd As New SqlCommand(sql, connection)
     Dim rows As Integer
     rows = cmd.ExecuteNonQuery()
     If rows >= 1 Then
     ’now update the inventory to reflect a sale
     sql = "UPDATE inventory SET quantity = (quantity -" & number & ")
            WHERE inventory.ISBN = " & isbn & " AND  store_id = 1"
     ’define and execute the query command
      Dim cmd2 As New SqlCommand(sql, connection)
      rows = cmd2.ExecuteNonQuery

May I suggest you reformat that questions so it is readable. Try formatting the code as "Code" — Pat Long - Munkii Yebee, Apr 11 '14 at 14:06
@roryap, I included the stackoverflow answer to the xkcd comic in my answer just for you. :) — crthompson, Apr 11 '14 at 14:23
Short version: If you build SQL statements using variables from the outside world, you are vulnerable. Use prepared statements. — Andy Lester, Apr 11 '14 at 14:27
@AndyLester, isnt that exactly what I say in my answer?!?! :) — crthompson, Apr 11 '14 at 14:53
My comment adds on to your answer. The key point that posters seem to miss, and that you didn't say explicitly, is the fact that they're building executable code with data from the outside world. — Andy Lester, Apr 11 '14 at 15:15
@AndyLester Ah, I see. I did mean exactly that when I said `variables that the user has direct access to`, but perhaps it wasnt as clear as the way you put it. I'll make an edit. — crthompson, Apr 11 '14 at 15:23

score 3 · Accepted Answer · edited May 23 '17 at 12:21

Whenever you are concatinating sql and use variables that the user has direct access to, you are in danger of SQL Injection.

In your case the fix might look something like this:

 sql = "INSERT INTO purchase(customer_id, ISBN, store_id, quantity)
                    VALUES(@id, @isbn , 1, @number);"
Dim cmd As New SqlCommand(sql, connection)
cmd.Parameters.AddWithValue("@id", id )
cmd.Parameters.AddWithValue("@isbn", isbn )
cmd.Parameters.AddWithValue("@number", number)
Dim rows As Integer
rows = cmd.ExecuteNonQuery()

The second query would look like this:

sql = "UPDATE inventory SET quantity = (quantity - @number)
        WHERE inventory.ISBN = @isbn AND store_id = 1"
Dim cmd2 As New SqlCommand(sql, connection)
cmd2.Parameters.AddWithValue("@number", id )
cmd2.Parameters.AddWithValue("@isbn", isbn )
rows = cmd2.ExecuteNonQuery

By using parameters, the string is escaped for any would be malicious code.

Other good references:

What is SQL injection?

How does the SQL injection from the "Bobby Tables" XKCD comic work?

EDIT:

As @AndyLester points out in the comments, what I am trying to suggest is that using user data to concatenate into your executable code is DANGEROUS!

So I don't have to worry about the Update statement than? Is that because nothing is being inserted into the table, and if someone did intervene they wouldn't be able to mess with things? — user3307636, Apr 11 '14 at 15:30
@user3307636, no, you still do. I just simply gave an example of the first, so you could modify the second. — crthompson, Apr 11 '14 at 15:37

score 2 · Answer 2 · answered Apr 11 '14 at 14:27

2

To prevent SQL injections, you can achieve it in two ways.

Validating user input: Using the appropriate types prevents SQL injections naturally. For instance, if customer_id, store_id, and quantity are all integers, then they are safe and you don't need to worry about them. As for the ISBN (I think you use string?), if you validate it to make sure it only contains digits and hyphens, then it is also safe.
Using parameterized queries: This is a great way to prevent SQL injections and also reduce possible bugs in queries.

A good code uses both technics.

You didn't specify the language/platform you're using, but it looks like VB.Net. Here is how you add parameters to your query:

sql = "INSERT INTO purchase(customer_id, ISBN, store_id, quantity)
       VALUES(@id, @isbn , 1, @quantity);"
Dim cmd As New SqlCommand(sql, connection)
cmd.Parameters.AddWithValue("@id", id)
cmd.Parameters.AddWithValue("@isbn", isbn)
cmd.Parameters.AddWithValue("@quantity", number)

answered Apr 11 '14 at 14:27

Racil Hilan

24,690
13
50
55

1

I like the comment about validating user input. [Do not trust user input directly!](http://msdn.microsoft.com/en-us/library/ee798441(v=cs.20).aspx). Good answer, well deserving of a +1. – crthompson Apr 11 '14 at 14:50
1

Plus you get a badge if you upvote your competitors :) – crthompson Apr 11 '14 at 15:04
@paqogomez Do you really? I've done it several times in the past and haven't noticed any related badge. What badge is that? Anyway, we are not competitors here. We're all learning collaboratively and helping each other, are we not? – Racil Hilan Apr 11 '14 at 15:06
1

The [SportsmanShip](http://stackoverflow.com/help/badges/805/sportsmanship) badge. :) And yes, we are. :) – crthompson Apr 11 '14 at 15:09
@paqogomez Glad to see your confirmation :). As for the badge, thanks for the info, I was not aware of it. Apparently I haven't done it a 100 times yet, but this encourages the good spirits of collaboration. – Racil Hilan Apr 11 '14 at 15:14
I'll be happy to receive an upvote if you'd like to get a little further on your badge progression... just out of, you know, sportsmanship. ;) – crthompson Apr 11 '14 at 15:16
1

@paqogomez No, I won't do it just to get a badge. Like I said, I did it many times in the past without knowing about the badge. I also upvoted your comments here although there is no badge for that (or is there?) because I thought they were useful. As for your answer, it provided good example (before my answer) and references, so I upvoted it. – Racil Hilan Apr 11 '14 at 15:25

How to prevent SQL Injection on following code in Visual Basic 2012

2 Answers2

Linked