HiveBrain v1.2.0
Get Started
← Back to all entries
patternModerate

Security issues in a SQL query and possible function?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
issuessqlqueryfunctionpossiblesecurityand

Problem

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 [Mini

Solution

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.

Code Snippets

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

Context

StackExchange Code Review Q#60026, answer score: 10

Revisions (0)

No revisions yet.