Security issues in a SQL query and possible function?

Question

By adding more and more features to my websites, I am now up to 7 different hard coded queries.

I have read a lot about SQL injections and possible flaws and security issues, and wanted to make sure that it was secure enough.

This is one of my regular UPDATE queries :

Using thisConnection As New OleDbConnection("Provider=Microsoft.ACE.OLEDB.12.0;Data Source=""Striped Source""")
    Using thisCommand As OleDbCommand = thisConnection.CreateCommand
        ' Open connection object
        thisConnection.Open()

        ' Initialize SQL UPDATE command to update the desired data
        With thisCommand
            .CommandText = "UPDATE Login SET ClientName = @ClientName, Fonction = @Fonction, CompanyName = @CompanyName, Address = @Address, Country = @Country, " & _
                                "[Phone Number] = @Phone, [Fax Number] = @Fax, [I prefer to be contacted] = @Contacted " & _
                                "WHERE Username = @Username"
            With .Parameters
                .AddWithValue("@ClientName", VB_Name)
                .AddWithValue("@Fonction", VB_Fonction)
                .AddWithValue("@CompanyName", VB_Company)
                .AddWithValue("@Address", VB_Address)
                .AddWithValue("@Country", VB_Country)
                .AddWithValue("@Phone", VB_Phone)
                .AddWithValue("@Fax", VB_Fax)
                .AddWithValue("@Contacted", VB_Preference)
                .AddWithValue("@Username", LBL_Welcome.Text)
            End With
        End With

        thisCommand.ExecuteNonQuery()

        thisConnection.Close()
    End Using
End Using

And this is one of my non-regular double SELECT queries (Lengthy) :

Using thisConnection As New OleDbConnection("Provider=Microsoft.ACE.OLEDB.12.0;Data Source=""Striped source""")
    Using thisCommand As OleDbCommand = thisConnection.CreateCommand

        thisConnection.Open()

        'This query is to get the information
        thisCommand.CommandText = "SELECT [Minimum Length] " & _
                                    "FROM Stats " & _
                                    "WHERE  ([N° Cylinder] = @cylinder) AND ([N° Section] = 0)"
        thisCommand.Parameters.AddWithValue("@cylinder", cylinder

        Dim thisReader As OleDbDataReader = thisCommand.ExecuteReader()
        If (thisReader.Read()) Then
            cylinderLengthMin = thisReader.GetValue(0)
        End If
        thisReader.Close()

        'This query is to get the number of stages of the cylinder
        thisCommand.CommandText = "SELECT Stroke " & _
                                    "FROM (Stats) " & _
                                    "WHERE ([N° Cylinder] = @cylinder) " & _
                                    "ORDER BY [N° Section]"
        thisCommand.Parameters.AddWithValue("@cylinder", cylinder)

        thisReader = thisCommand.ExecuteReader()
        count = -1
        Dim condition As Boolean = True
        Dim temp As String
        'This loop gets the strokes of the stages
        While (thisReader.Read()) And condition
            temp = thisReader.GetValue(0).ToString()
            If thisReader.GetValue(0).ToString() = "" Then
                condition = False
            Else
                count += 1
            End If
        End While
        countMax = count 'This is used later for array formatting
        thisReader.Close()

        If countMax = -1 Then
            Throw New SQLException("There has been an error with the database query.")
        End If

        'This query is to gather the data. It is stored in Imperial in the DataBase.
        ReDim cylinderOutsideDiameter(countMax)
        ReDim cylinderInsideDiameter(countMax)
        ReDim cylinderSectionStroke(countMax)
        ReDim cylinderSectionRetracted(countMax)
        ReDim cylinderIDStopRing(countMax)

        thisCommand.CommandText = "SELECT Stroke, [Retracted Length], [Inside Diameter], [Outside Diameter],  [Non-contact Diameter]" & _
                                    "FROM (Stats) " & _
                                    "WHERE ([N° Cylinder] = @cylinder) " & _
                                    "ORDER BY [N° Section]"
        thisCommand.Parameters.AddWithValue("@cylinder", cylinder)

        thisReader = thisCommand.ExecuteReader()

        count = 0
        While (thisReader.Read()) AndAlso count <= countMax 'Scans each row and adds it to respective string/double array
            For i = 0 To 4
                If thisReader.GetValue(i).ToString() = "" Then
                    Throw New SQLException("There has been an error with the database query.")
                End If
            Next
            cylinderSectionStroke(count) = thisReader.GetValue(0)
            cylinderSectionRetracted(count) = thisReader.GetValue(1)
            cylinderInsideDiameter(count) = thisReader.GetValue(2)
            cylinderOutsideDiameter(count) = thisReader.GetValue(3)
            cylinderIDStopRing(count) = thisReader.GetValue(4)
            count += 1
        End While

        thisReader.Close()
        thisConnection.Close()
    End Using
End Using

Questions

1. Are my SQL queries secure?

2. Should I make a function for the queries (as I am up to 7 queries now and I should never repeat the same code multiple times). If yes, how? (I'm not asking for a code dump, just a descriptive explanation on how I could do it would be awesome)

3. If there is anything I could improve in the code, don't hesitate. I would love to have feedback on how to improve it.

Answer 1

Make your code DRY (don't repeat yourself). Extract out your whole code of data retrieving to helper class

CommandBehavior.CloseConnection will avoid closing the connection so you need to explicitly close the connection. so use using keyword for VB. it will close the connection automatically

you are using parameterized query so it will make your SQL safer in case of SQL Injection.

Public Shared Function ExecuteReader(ByVal commandtext As String, ByVal parameters As Dictionary(Of String, Object)) As IDataReader
    Using thisConnection As New OleDbConnection("Provider=Microsoft.ACE.OLEDB.12.0;Data Source=""Striped source""")
        Using thisCommand As OleDbCommand = thisConnection.CreateCommand
            thisConnection.Open()
            thisCommand.CommandText = commandtext
            For Each parameter In parameters
                thisCommand.Parameters.AddWithValue(parameter.Key, parameter.Value)
            Next
            'CommandBehavior.CloseConnection will avoid closing the connection so you need to explicity close the connection
            Return thisCommand.ExecuteReader(CommandBehavior.CloseConnection)
        End Using
    End Using
End Function

Public Function HowToUseExecuteReader()
    Dim parameters As New Dictionary(Of String, Integer)
    parameters.Add("@cylinder", cylinder)

    Dim query As String = "SELECT [Minimum Length] FROM Stats WHERE  ([N° Cylinder] = @cylinder) AND ([N° Section] = 0)"
    Dim dataReader As IDataReader = ExecuteReader(query, New Dictionary(Of String, Object))
    Using dataReader
         While dataReader.Read
             Dim cylinderLengthMin = dataReader.GetValue(0)
         End While
    End Using
End Function

Answer 2

Is this secure? Well, nothing is ever really secure, but yes. Generally speaking it is. You're using parameterized queries, which is good. Malachi is right though, stored procedures would be even better. What you can do to make this a little more secure is specify the datatype and size of each parameter.

I'm not sure where this code resides, but you should make sure that it is separated from application logic in its own Data Access Layer. You open and close a lot of connections to your back end. That's a performance hit. Implementing a proper DAL should help mitigate that.

What does "3" mean in this piece of code? Nix the magic number and create a constant with a meaningful name.

        For i = 0 To 3
            If thisReader.GetValue(i).ToString() = "" Then
                Throw New SQLException("There has been an error with the database query.")
            End If
        Next

---

The elephant in the room...

Access is not a great back end for a website. It has notorious performance issues. Considering you're concerned about security as well, I would recommend moving to a SQL Server Express backend. You'll find the move relatively painless and have a faster, more secure database in the long run.

Answer 3

You should not hard code queries into your VB code if you can help it.

if you were using SQL Server I would say that you use Stored Procedures on the database instead of hard coded queries.

by passing parameters to a Stored Procedure from your code and having the database send you information back you are allowing the database to take care of a lot of the security of making sure that no SQL injection occurs, I am not saying that you shouldn't make sure that your input is not sanitized before you send it to your stored procedure.

separate the database from the presentation. let the database handle all the updating and retrieval of information, let your VB.NET application handle all the presentation of that data, and handle the input.

---

Access does have Stored Procedures.

Check out this Answer to: How do I make a stored procedure in MS Access?