Making VBA generated dynamic SQL statements safe against injection

Question

I need to use SQL extensively in VBA macros that I write. Since the DB is from our enterprise application, I use vendor's library for this.

I generate dynamic queries, sometimes with many JOINs and UNIONs, and the queries can be quite complex.

The procedure that I coded screams "I'm open to SQL injection". The macros using this function are all internal but I need to make it safe anyway.

How can I make the below code only run SELECT statements and safe against injections?

Please note that it is not possible to make views for all possible SELECT statements and use them instead, according to the parameters, one or more DB's need to be queried and UNION'd sometimes.

How can I safely query an arbitrary number of columns with an arbitrary number of constraints from an arbitrary number of tables in an arbitrary number of DBs?

'***************************************************************************'
'Evaluates and executes the passed query and returns the results '
'Parameters: '
' Query: The main query body with optional placeholders like #0 starting '
' with 0 '
' Args : Optional arguments to replace placeholders '
'Returns: '
' The resulting recordset '
'***************************************************************************'
Public Function DirectQuery( _
 Query As String, _
 ParamArray Args() As Variant _
) As NetRS
'Declarations **************************************************************'
Dim ReturnRs As NetRS
Dim Params As Variant
'***************************************************************************'
 On Error GoTo ErrorHandler
 'This code shouldn't be used
 MsgBox "This code is dangerous. Don't copy-paste and use it."
 Exit Function
 Params = Args
 Set ReturnRs = 3rdPartyObject.NewRecordset(DBConnection)
 ReturnRs.Open EvaluateQuery(Query, Params)
 Set DirectQuery = ReturnRs
 Set ReturnRs = Nothing
Exit Function
ErrorHandler:
End Function
'***************************************************************************'

'***************************************************************************'
'Evaluates the arguments in a query string and returns the formed query '
'Parameters: '
' Query: The main query body with optional placeholders like #0 starting '
' with 0 '
' Args : Optional arguments to replace placeholders '
'Returns: '
' The formed query string '
'***************************************************************************'
Public Function EvaluateQuery( _
 Query As String, _
 ParamArray Args() As Variant _
) As String
'Declarations **************************************************************'
Dim i As Integer
Dim Params As Variant
'***************************************************************************'
 On Error GoTo ErrorHandler
 Params = Args
 EvaluateQuery = Query
 For i = LBound(Params(0)) To UBound(Params(0))
 EvaluateQuery = Replace(EvaluateQuery, "#" & i, Params(0)(i))
 Next i
Exit Function
ErrorHandler:
 EvaluateQuery = ""
End Function
'***************************************************************************'

• Mathieu Guindon: You can take a look at this code that lets you easily create parameterized ADODB queries (wrote that today!).

Answer 1

Error Handling / Execution Flow: Beware of Raptors

I realize you haven't implemented it yet, but you've set yourself up for some twisted execution flow:

 Exit Function 'fixed indentation
ErrorHandler:
 EvaluateQuery = ""
End Function

When you do implement error handling, you'll likely be cleaning up resources (closing recordsets, connections, etc.) - you'll want to do that in case of error, and in normal execution paths too.

I'd remove Exit Function and rework the ErrorHandler: part:

ErrorHandler:
 'todo: cleanup
 If Err.Number <> 0 Then
 'todo: handle errors
 End If
End Function

This way you'll avoid introducing another label (Cleanup:?) and a GoTo Cleanup, and raptors won't get you.

---

SQL Injection

I know only 1 way of preventing SQL injection: let the server handle the parameters. I don't know what 3rd party you're using nor what a NetRs is (why not use ADODB?), but the code in EvaluateQuery seems to be concatenating the parameters into the query.

This means a String parameter would have to be surrounded by single quotes, and that a string that says "Robert'); DROP TABLE Students;--" would be concatenated - and executed as such. Thus you are correct, this code is screaming SQL injection vulnerability.

Don't even try "sanitizing" the strings before concatenating them into your query. It's the server's job to deal with the parameters. With ADODB you'd be sending the server a query like:

"SELECT SomeField FROM SomeTable WHERE SomeOtherField = ?"

Where ? is replaced by the server, by its parameter value - no need to worry about single quotes around string parameters, the parameter itself has all the information needed for the server to know how to handle.

On top of being more secure, such queries are more performant: if you execute them in a loop, the server receives the same query everytime, only with different parameters - it reuses the same query plan every time and only calculates it once. If you send the parameter values embedded in the query, the server receives a different query every time, and has to recalculate a query plan each time.

See MSDN for more info on parameterized ADODB queries.