How to rewrite VBA SQL statement to prevent SQL Injection?-CodePudding

I have this code below. I asked a non-related question about it yesterday and it was pointed out to me that it's wide open for SQL Injection. I did some research but I am a bit lost about what exactly to do here.

How should I rewrite this procedure to prevent SQL Injection possibility? Thanks. I would appreciate if you could point me in the right direction, not write the code entirely.

It's a code in MS Access VBA, using ADODB connection to SQL Server 2019.

This is my SQL Server module in VBA to make the code more readable:

Option Compare Database
Option Explicit

Private Const CONNECTION_STRING = "some random connection string"

Public Conn As ADODB.Connection

' ÚČEL FUNKCE: Spojení s SQL Server databází (bez DSN)
Public Function ConnectToServer() As String
    On Error GoTo Catch
    
    ConnectToServer = CONNECTION_STRING
    
    Exit Function
    
Catch:
    ConnectToServer = ""
    MsgBox "GetDSNLessCnnString function" & vbCrLf & vbCrLf & "Error#: " _
           & Err.Number & vbCrLf & vbCrLf & Err.Description
End Function

Public Sub Connect()

    Set Conn = New ADODB.Connection
    Conn.ConnectionString = ConnectToServer
    Conn.Open
    
End Sub

Public Sub Exec(Command As String)

    Conn.Execute Command

End Sub

Public Sub Disconnect()

    Conn.Close
    Set Conn = Nothing
    
End Sub

This is the main procedure:

Private Sub cmdShipOrder_Click()

    Dim intShipmentID As Integer
    
    On Error GoTo ErrHandler

    If Me.ReservationStatus <> "ZBOŽÍ KOMPLETNĚ REZERVOVÁNO" Then
        MsgBox "Nejprve je nutné do objednávky rezervovat hmotné zboží.", vbCritical   vbOKOnly, "Chyba"
        Exit Sub
    End If
    
    If MsgBox("Bude vytvořena expedice a celá objednávka bude označena jako expedovaná. Pokračovat?", vbExclamation   vbYesNoCancel, "Upozornění") <> vbYes Then Exit Sub
    
    Connect
    
    Exec "INSERT INTO tbl1Shipments (CustomerID, ShippingMethodID, ShipToID, CarrierID, ShipmentCode, DateShipped) " & _
            "VALUES (" & Me.CustomerID & ", " & _
                    Me.ShippingMethodID & ", " & _
                    Me.ShipToID & ", " & _
                    Me.CarrierID & ", " & _
                    "'" & Year(Date) & "EXP" & Format(DCount("*", "dbo_tbl1Shipments", "ShipmentCode LIKE '%" & Year(Date) & "EXP%'")   1, "000") & "', " & _
                    "GETDATE())"
                    
    intShipmentID = DMax("ShipmentID", "dbo_tbl1Shipments", "CustomerID=" & Me.CustomerID)
    
    Conn.BeginTrans

    Exec "INSERT INTO tbl1ShipmentDetails (ShipmentID, SalesOrderDetailID, Quantity) " & _
            "SELECT " & intShipmentID & ", SalesOrderDetailID, Quantity " & _
                "FROM v_SalesOrderSub " & _
                "WHERE SalesOrderID=" & Me.SalesOrderID

    Exec "UPDATE A " & _
            "SET ShipmentDetailID = B.ShipmentDetailID " & _
            "FROM tbl1Units A JOIN v_ShipmentSub B ON A.SalesOrderDetailID = B.SalesOrderDetailID " & _
            "WHERE B.ShipmentID = " & intShipmentID & " AND B.ProductTypeID <> 3"

    Conn.CommitTrans

    Disconnect
    
    RequeryForm ("frmSalesOrderDetails")
    RequeryForm ("frmSalesOrderList")
    
    DoCmd.OpenForm "frmShipmentDetails", , , "ShipmentID=" & intShipmentID

Exit Sub

ErrHandler:
    MsgBox "CHYBA: " & Err.Description
    Conn.RollbackTrans
    Exec "DELETE FROM tbl1Shipments WHERE ShipmentID=" & intShipmentID
    Disconnect

End Sub

CodePudding user response：

You remove the inline dynamic SQL statements where you just inject variables into the SQL Statement.

You create Stored Procedures for each database actions and your VB simply passes variables into the Input Parameters of those procedures.

So using the Delete as an example, you create a stored procedure called, "DeleteShipment"

DROP PROC IF EXISTS dbo.sproc_DeleteShipment
GO
CREATE PROC dbo.sproc_DeleteShipment(@ShipmentID INT)
AS
BEGIN
DELETE dbo.tbl1Shipments WHERE ShipmentID = ShipmentID 
END

then your VB DB calls will change to

Dim conn as New SqlConnection(YourConnectionString)
Dim cmd as SqlCommand = conn.createcommand
conn.open()
cmd.CommandType = CommandType.StoreProcedure
cmd.Parameters.Add(New SqlParameter("@ShipmentID ", intShipmentID)
cmd.CommandText = sproc_DeleteShipment
cmd.ExecuteNonQuery() 
conn.close()

In theory, your code is open to SQL Injection but just from that example, all your variables are INTs which makes it harder. However if your shipment ID was a string, and a hacker managed to get the string, "1 OR 1=1" into the shipment id, the whole table would get deleted.

If that same string found it's way into a input parameter for a SPROC, that can't happen.