# Security issues in a SQL query and possible function?

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 " & _
With .Parameters
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)"

End If

'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]"

count = -1
Dim condition As Boolean = True
Dim temp As String
'This loop gets the strokes of the stages
condition = False
Else
count += 1
End If
End While
countMax = count 'This is used later for array formatting

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]"

count = 0
For i = 0 To 4
Throw New SQLException("There has been an error with the database query.")
End If
Next
count += 1
End While

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.

-

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
Next
'CommandBehavior.CloseConnection will avoid closing the connection so you need to explicity close the connection
End Using
End Using
End Function

Dim parameters As New Dictionary(Of String, Integer)

Dim query As String = "SELECT [Minimum Length] FROM Stats WHERE  ([N° Cylinder] = @cylinder) AND ([N° Section] = 0)"
End While
End Using
End Function

-
Impressive! This will enable me to use a shared function. This will clarify my code and simplify it in the same time! Thanks a lot for that (I will use this type of functions and use Malachi's suggestion (use stored procedures)) –  Sifu Aug 14 '14 at 15:12
Just a note, I don't think you want to have a function named the same way as a command (ExecuteReader). It could create confusion, in my opinion. I used what you suggested and modified for my personnal use, thanks a lot. –  Sifu Aug 14 '14 at 19:22
Even though I accepted this answer, I wanted to mention that if I could, I would of marked all three answers as accepted. All the three answers provided different parts to fix or improve. Thanks to all of you. –  Sifu Aug 14 '14 at 19:23

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
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.

-
Thanks for the info, I will specify the datatype and size of each parameter as you suggested. || I'm not sure I understand what you mean by "where this code resides", these pieces of codes are triggered from the website (asp.net) where a client enters some information (does that help?). || The "3" was actually a typo, thanks for noticing it! It was supposed to be "4" (as in 4 columns). It really exists just to spot a mistake in the database. || (Follows in next comment, not enough characters) –  Sifu Aug 14 '14 at 15:30
I understand your thoughts on performance issue, though I have never mentioned speed issues for a reason, the database is extremely small (3 tables of 10*40 cells). And Access was picked for a reason of ease of modifications (we can simply open the Access file, go into the desired table and change whichever data we want. All this without any SQL knowledge, which most of my collegues don't have) –  Sifu Aug 14 '14 at 15:32
Sounds like (knowingly or not) you have implemented a DAL then. Good. I wasn't sure if this was client side code or not. –  RubberDuck Aug 14 '14 at 15:33
Oh! Yes, this is server side code. –  Sifu Aug 14 '14 at 15:34
Thanks a lot for your advices, they are well appreciated. I added these comments for two reasons : for any future readers to understand everything (if they want to read through it) and for you to see that your answer was useful. –  Sifu Aug 14 '14 at 15:49

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?

-
@Sifu, you don't want to risk it. you are exposing your database to the world, someone out there knows how to exploit the vulnerabilities of your code, so make sure that you don't have many for them to work with. Get into the habit of writing nearly impenetrable code –  Lyle's Mug Aug 14 '14 at 14:08
@Sifu: what if my user name was: Phrancis; DELETE FROM Login; ? –  Phrancis Aug 14 '14 at 14:31
it's not good structure, you want to keep your database code on your database, you will still parameterize the input when you use stored procedures as well. you want the database to handle the data, the VB to handle gathering input and displaying output. the database is the box, VB is the Hand that reaches into the box or puts things in the box. –  Lyle's Mug Aug 14 '14 at 14:43
@IanRingrose you should have a separation of concerns, Data and Presentation. that is accepted by the majority of programmers. one way or the other you shouldn't have SQL in your VB. what is the VB going to do with it? VB is going to send the SQL to the DB, why send more information than you need to. version control is something that SQL databases have figured out by back ups. not sure how that works on Access though. –  Lyle's Mug Aug 14 '14 at 16:02
Comments are not the place for tutorials, discussions and general chatter. Please take this to a chat room, you are welcome to join The 2nd Monitor at any time –  rolfl Aug 14 '14 at 16:19