Creating SQL Where clause dynamically that is not open to SQL Injection

Question

I wrote this bit of VBA code that creates a SQL query dynamically based on the number of fields the user has selected and values read from an XL spreadsheet. It basically just adds "FIELD_VARIABLE=VALUE_VARIABLE OR" to the where clause and then removes the final OR after the loop ends.

It works for N number of fields added like I was hoping but my concern is security because I think I could just put like ';DROP TABLE Projects or some other malicious code into the spreadsheet from where the program is reading FIELD_VARIABLES. To a lesser extent since the query is different every time the execution path must be different and that probably slows down execution time.

I'm thinking of looking into parameterized queries or T-SQL to improve this. Was hoping one of you smart folks could point me in the right direction before I waste too much time on this. Here is the relevant VBA code:

 '---loop through array of search fields and search values using the same index
 '---since the arrays sizes will always be the same and create where filters dynamically
          i = 1
          For i = LBound(sLookupFields) To UBound(sLookupFields)
                Set rngLookup = wsLookupSrc.cells(counter, lLookupCols(i))
            '---clear where from last iteration through loop
                SQLWhereDynamic = ""

                SQLWhereDynamic = SQLWhereDynamic & " p." & sLookupFields(i) & " = '" + CStr(rngLookup.Value) & "' OR"
          Next i
        '---remove extra ' OR'
          SQLWhereDynamic = Left(SQLWhereDynamic, (Len(SQLWhereDynamic) - 3))

          SQLValue = wsLookupSrc.cells(counter, lLookupCols(1)).Value

          SQLWhereDefault = "WHERE p.ClientId = " + CStr(iClientId) + ""
          SQLQuery = SQLSelect + SQLWhereDefault + " AND (" + SQLWhereDynamic + ");"

I would be surprised if you actually have access to execute a drop table command. The database login information you use to run the query should be read-only. — Yaegz, Oct 09 '15 at 16:36
Ha, that's true, but I guess I was worried that it was still inadequate. All that someone would have to do is change the connection string to an admin level, enter some malicious code, and then blame the program. Maybe I'm being too paranoid about a simple in office list processing app. I'm also just curious, for future use,if there is a way to create secure queries at run time in situations that allow users to update fields. — Joe Williamson, Oct 09 '15 at 17:36
It helps to restrict the options a user can select. Give them a list to choose from or something. Then, instead of having their input go directly into the SQL query, have an intermediate step where you validate their input and then if it passes your validation, add it into the SQL query. — Yaegz, Oct 09 '15 at 17:50
You can use a ADODB.Command object http://stackoverflow.com/questions/10352211/vba-ado-connection-and-query-parameters — Dick Kusleika, Oct 09 '15 at 20:14
@Yaegz I think that you are right, it is best to limit the user input options. I was able to execute a SQL Injection that dropped a test table using the code I had. — Joe Williamson, Oct 12 '15 at 18:26
@Dick Kusleika with that solution Id have to already have the fields of the where clause set and only the field filter values would be created dynamically. I may just have to accept that the where field columns will be hard-coded and use Coalesce in SQL if the value is null — Joe Williamson, Oct 12 '15 at 18:36

Dick Kusleika · Accepted Answer · 2015-10-22T18:16:58.763

Making the field name in the WHERE clause a parameter (and therefore dynamic and safe from injection) like you can with the value in the WHERE clause is impossible, I believe. However...

Here's how I would do it. Suppose you have an Excel range with all of the possible fields, search values filled in for those fields you want to search, and a data type (to be used in the code later). This below example shows two fields being searched

Field           Value      DataType
Sequence                     131
CustomerID                   200
InvoiceNumber                200
OrderNumber                  200
InvoiceDate     8/14/2015      7
Item            DS2          200
Location                     200
ExportFile                   200
DateImported                   7
OnHold                        11

The user fills in column 2. And the code builds the sql string

Sub MakeSQL()

    Dim aSql(1 To 4) As String
    Dim aWhere() As String
    Dim vaFields As Variant
    Dim lWhereCnt As Long
    Dim lCnt As Long, i As Long
    Dim cn As ADODB.Connection
    Dim cmd As ADODB.Command
    Dim pm As ADODB.Parameter

    'Skip number three until later
    aSql(1) = "SELECT *"
    aSql(2) = "FROM dbo.InvoiceLine"
    aSql(4) = "ORDER BY InvoiceNumber DESC;"

    'Grab all the search criteria
    vaFields = Sheet1.Range("A2:C11").Value
    'Set up the connection
    Set cn = New ADODB.Connection
    cn.Open sConn
    Set cmd = New ADODB.Command
    Set cmd.ActiveConnection = cn

    'Count how many criteria where filled in
    'You could Redim Preserve your aWhere() also
    On Error Resume Next
        lWhereCnt = Sheet1.Range("B2:B11").SpecialCells(xlCellTypeConstants).Count
    On Error GoTo 0

    'If there's at least one
    If lWhereCnt >= 1 Then
        ReDim aWhere(1 To lWhereCnt)

        'Fill in an array and create parameters
        For i = LBound(vaFields, 1) To UBound(vaFields, 1)
            If Len(vaFields(i, 2)) > 0 Then
                lCnt = lCnt + 1
                'Put in the place holder
                aWhere(lCnt) = vaFields(i, 1) & "=?"
                'column 3 holds the data type
                Set pm = cmd.CreateParameter(vaFields(i, 1) & "_p", vaFields(i, 3), adParamInput)
                pm.Value = vaFields(i, 2)
                'Variable length data types (I only use varchar, you may use more)
                'must have a size specified
                If vaFields(i, 3) = adVarChar Then pm.Size = Len(vaFields(i, 2))
                cmd.Parameters.Append pm
            End If
        Next i
        'Fill in the "where" section of your sql statement
        aSql(3) = "WHERE " & Join(aWhere, " OR ")
    End If

    cmd.CommandText = Join(aSql, Space(1))

    'Change this line to actually execute something
    Debug.Print cmd.CommandText
    For i = 0 To cmd.Parameters.Count - 1
        Debug.Print , cmd.Parameters(i).Name, cmd.Parameters(i).Value
    Next i


    cn.Close
    Set cn = Nothing

End Sub

For this example, the string comes out as

SELECT * FROM dbo.InvoiceLine WHERE InvoiceDate=? OR Item=? ORDER BY InvoiceNumber DESC;
          InvoiceDate_p 8/14/2015 
          Item_p        DS2

Thanks Dick Kusleika, wouldn't you be able to cause a SQL injection by putting something like the following in the field column? : InvoiceDate = LIKE 'XXXXX'; Drop Table testtable; — Joe Williamson, Oct 14 '15 at 15:01
Using the code above I put InvoiceDate = LIKE 'XXXXX'; Drop Table testtable; into column A and it generates the following SQL statement: `SELECT * FROM dbo.InvoiceLine WHERE InvoiceDate = LIKE 'XXXXX'; Drop Table testtable;=? OR Item=? ORDER BY InvoiceNumber DESC; InvoiceDate = LIKE 'XXXXX'; Drop Table testtable;_p 8/14/2015 Item_p DS2` This code did not run but with some work you could probably inject SQL. — Joe Williamson, Oct 14 '15 at 18:28
My understanding is that the Parameter object sanitizes the sql (but I am by no means an expert on the subject). See RoadWarriors answer here http://stackoverflow.com/questions/306668/are-parameters-really-enough-to-prevent-sql-injections. When you say "it generates the following" where is it doing that? My SQL stays the same no matter what the params are - that is, it contains the ? placeholders. — Dick Kusleika, Oct 19 '15 at 21:39
The problem I'm having is that I would like to dynamically set not only the WHERE filter value dynamically but also the WHERE column to which we are applying the filter. I agree that parameters sanitize the code, but I think you can only use parameters in the certain parts of a SQL statement. You can do WHERE dbo.InvoiceDate= ? but I'm trying to do something like WHERE ? = ?. I don't think there is a way to do that with parameters right? — Joe Williamson, Oct 22 '15 at 13:41
Yeah, I see what you're saying. No, I don't think there's any way to do what you want. In my example, the user can't change column 1 or column 3 - only column 2 so that's the only column I need to parameterize. I expose all the fields in my UI and let the user choose the value. They don't get to choose anything else, like the field name or data type, so there's no need to protect that against injection. — Dick Kusleika, Oct 22 '15 at 14:37
Dick Kusleika if you want to put our conclusion that it cannot be done in a new answer then I'll check that one as correct or I can just do it myself. Just wanna give you credit since you helped me suss that out. Thx! — Joe Williamson, Oct 22 '15 at 17:55
Edited my answer to note that the specific questions isn't possible. Let me know if you think it's not sufficient and I'll edit it. — Dick Kusleika, Oct 22 '15 at 18:18

Creating SQL Where clause dynamically that is not open to SQL Injection

1 Answers1