Using SQL with encryption

Question

This is my first project I am doing in VB.NET, and also my first real programming project. There is sensitive data, so I am utilizing Microsoft's Encryption/Decryption class (clsCrypt).

For optimization, quality and best practice standards, which code the 'best' way to retrieve encrypted data from a MS SQL Server 2008 R2 db, and decrypt it, based on what the user enters in text boxes? (First Name, Last Name)

Note: I did not accommodate the text box values into the last code snippet.

Public Class Form1

Dim eFirst As String
Dim eLast As String
Dim dFirst As String
Dim dLast As String

Public Sub Searchbtn_Click(sender As System.Object, e As System.EventArgs) Handles Searchbtn.Click

    Me.DataGridView1.Show()
    Dim SQLConnection As New SqlConnection("Data Source=SQLTEST_HR,4000\SQLEXPRESS;Integrated Security=True") 'Declare Connection String'
    Dim SqlCommand As New SqlCommand 'Declare variable for SQL command'

    Dim dt As New DataTable

    Dim strKey As String = "Key1" 'encryption Key'
    Dim clsEncrypt As clsCrypt         'Assigns a variable to clsCrypt class'
    clsEncrypt = New clsCrypt(strKey)   ' creates a new instance of the clsCrypt class'


    eFirst = clsEncrypt.EncryptData(SearchFirsttxt.Text.Trim.ToUpper)
    eLast = clsEncrypt.EncryptData(SearchLastTxt.Text.Trim.ToUpper)

    SQLConnection.Open() 'Opens database Connection'
    SqlCommand.Connection = SQLConnection 'Assigns connection to the command'


    If SearchFirsttxt.Text = "" Then
        SqlCommand.CommandText = "Select * FROM PARTICIPANT WHERE LAST_NM_TXT = '" & eLast & "';"
    ElseIf SearchLastTxt.Text = "" Then
        SqlCommand.CommandText = "Select * FROM PARTICIPANT WHERE FIRST_NM_TXT = '" & eFirst & "';"
    ElseIf SearchFirsttxt.Text IsNot Nothing And SearchLastTxt.Text IsNot Nothing Then
        SqlCommand.CommandText = "Select * FROM PARTICIPANT WHERE FIRST_NM_TXT = '" & eFirst & "' and LAST_NM_TXT = '" & eLast & "';"
    Else
        SqlCommand.CommandText = "Select * FROM PARTICIPANT;"
    End If
    'SQL Command returns rows where values in database and textboxes are equal'

    Dim myAdapter As New SqlDataAdapter(SqlCommand) 'holds the data'
    myAdapter.Fill(dt) 'datatable that is populated into the holder (DataAdapter)'
    DataGridView1.DataSource = dt 'Assigns source of information to the gridview (DataTable)'

    Try
        For i As Integer = 0 To dt.Rows.Count - 1
            dt.Rows(i)("FIRST_NM_TXT") = clsEncrypt.DecryptData(dt.Rows(i)("FIRST_NM_TXT"))
            dt.Rows(i)("LAST_NM_TXT") = clsEncrypt.DecryptData(dt.Rows(i)("LAST_NM_TXT"))
        Next
    Catch ex As Exception
        MessageBox.Show("Either the first name or last name did not match. Please check your spelling.")
    End Try

OR this way

    Me.DataGridView1.Show()
    Dim SQLConnection As New SqlConnection("Data Source=SQLTEST_HR,4000\SQLEXPRESS;Integrated Security=True") 'Declare Connection String
    Dim cmd As New SqlCommand 'Declare variable for SQL command
    Dim dt As New DataTable

    Dim strKey As String = "Key1" 'encryption Key
    Dim clsEncrypt As clsCrypt         'Assigns a variable to clsCrypt class
    clsEncrypt = New clsCrypt(strKey)   ' creates a new isntance of the clsCrypt class

    Dim eFirst As String
    Dim eLast As String

    eFirst = clsEncrypt.EncryptData(SearchFirsttxt.Text.Trim.ToUpper)
    eLast = clsEncrypt.EncryptData(SearchLasttxt.Text.Trim.ToUpper)

    SQLConnection.Open() 'Opens database Connection
    cmd.Connection = SQLConnection 'Assigns connection to the command
If SearchFirsttxt.Text = "" AndAlso SearchLasttxt.Text = "" Then
        ' Both emtpy so search everything'
        cmd.CommandText = "SELECT * FROM PARTICIPANT;"

    ElseIf SearchFirsttxt.Text = "" AndAlso SearchLasttxt.Text <> "" Then
        ' Search the last only if you have a last and not a first'
        cmd.CommandText = "Select * FROM Participant Where LAST_NM_TXT = @searchLast"
        cmd.Parameters.AddWithValue("@searchLast", eLast)

    ElseIf SearchLasttxt.Text = "" AndAlso SearchFirsttxt.Text <> "" Then
        ' Search the first only if you have a first and not a last'
        cmd.CommandText = "Select * FROM Participant WHERE FIRST_NM_TXT = @searchFirst"
        cmd.Parameters.AddWithValue("@searchFirst", eFirst)

    Else
        ' Both filled so search exactly (not sure if this is needed)'
        cmd.CommandText = "Select * FROM Participant " & _
                          "WHERE FIRST_NM_TXT = @searchFirst " & _
                          "OR LAST_NM_TXT = @searchLast"
        cmd.Parameters.AddWithValue("@searchFirst", eFirst)
        cmd.Parameters.AddWithValue("@searchLast", eLast)
    End If

    Dim myAdapter As New SqlDataAdapter(cmd) 'holds the data

    Dim ds As DataSet = New DataSet()
    myAdapter.Fill(ds)

    dt.Load(ds.CreateDataReader())


    DataGridView1.DataSource = dt 'Assigns source of information to the gridview (DataTable)

    'DECRYPTS ENCRYPTED DATA IN SPECIFICIED DT ROWS IN THE DGV1
    Try
        For i As Integer = 0 To dt.Rows.Count - 1
            dt.Rows(i)("FIRST_NM_TXT") = clsEncrypt.DecryptData(dt.Rows(i)("FIRST_NM_TXT"))
            dt.Rows(i)("LAST_NM_TXT") = clsEncrypt.DecryptData(dt.Rows(i)("LAST_NM_TXT"))
        Next
    Catch ex As Exception
        MessageBox.Show("Either the first name or last name did not match. Please check your spelling.")
    End Try

OR --

Public Function GetDetails() As DataSet

    Dim conn As New SqlConnection("Data Source=SQLTEST_HR,4000\SQLEXPRESS;Integrated Security=True")

    Dim strKey As String = "MarkKey" 'encryption Key
    Dim clsEncrypt As clsCrypt         'Assigns a variable to clsCrypt class
    clsEncrypt = New clsCrypt(strKey)   ' creates a new isntance of the clsCrypt class

    Try

        conn.Open()

        Dim SqlCmd As New SqlCommand("Select * From Participant", conn)
        Dim myAdapt As New SqlDataAdapter(SqlCmd)
        Dim DSEmp As New DataSet()

        myAdapt.Fill(DSEmp)

        Dim DTEmp As New DataTable()
        DTEmp.Load(DSEmp.CreateDataReader())

        DataGridView1.DataSource = DTEmp

        For i As Integer = 0 To DSEmp.Tables(0).Rows.Count - 1
            DSEmp.Tables(0).Rows(i)("FIRST_NM_TXT") = clsEncrypt.DecryptData(DSEmp.Tables(0).Rows(i)("FIRST_NM_TXT"))
            DSEmp.Tables(0).Rows(i)("LAST_NM_TXT") = clsEncrypt.DecryptData(DSEmp.Tables(0).Rows(i)("LAST_NM_TXT"))
        Next


        Return DSEmp

    Catch ex As Exception
        Throw (ex)
    Finally
        conn.Close()
    End Try
End Function

Private Sub LoadReport()

    Try

        Dim ds As New DataSet
        ds = GetDetails()
        Dim rds As ReportDataSource = New ReportDataSource("DataSet1", ds.Tables(0))

        rv1.LocalReport.DataSources.Clear()
        rv1.LocalReport.DataSources.Add(rds)
        rv1.RefreshReport()
    Catch ex As Exception
    End Try
End Sub

    Private Sub Form2_Load(sender As System.Object, e As System.EventArgs) Handles MyBase.Load
    LoadReport()
End Sub

Answer 1

Okay, best practices...

Data Access

Never do data access directly in the UI. Have a separate class file to do this. Best practice would suggest creating an interface for each 'aggregate root' (collection of classes that act as an integral whole). that you want to retrieve, then a subclass that inherits from the interface.

To be honest, and for simplicity, you can use something like the Gateway pattern. This is like a central place to do all database work. Its fine to start with, but may get cluttered depending on how complex your Data Access code will get:

Here is a typical pattern to separate the DB from the UI:

(Note - I don't work with VB.NET, so apologies for incorrect statements)

Public Interface IDbGateway 
   Function SearchForParticipant(firstName As String, surname As String) As IQueryable Of(Participant)
   Function GetParticipant(id As Int32) As Participant 
   Function AddOrUpdateParticipant(part As Participant) As Participant 
   Sub Delete(part as Participant);
End Interface

Public Class SqlDbGateway
       Implements IDbGateway

   Dim connectionString as String = null

   ' Your most commonly used way
   Public Sub New()
      Me.New "MyConnectionStringKey"

      ' In your App.config add this:
      ' <configuration>
      '   <connectionStrings>
      '      <add name="MyConnectionStringKey" connectionString="Data Source......."/>
      '   </connectionStrings>
      ' </configuration>
   End New

   Public Sub New (connectionStringKey as String)
      Me.connectionString = ConfigurationManager.ConnectionStrings.Get(connectionStringKey).ConnectionString;
   End New

   ' Implement interface here
End Class

Your UI could use a Dependency Injection framework, but you can simply go with (again, tactically)..

Public Class MyUIForm Inherits Form
   Dim dataAccess as IDbGateway = new SqlDbGateway();
End Class

Okay, step back - What have we achieved?

1. You can now develop and test the data access layer in isolation.
2. If your DB environment is different in production, than on your development (which is almost always the case), then you just supply a new App.config file.
3. The UI only sees what the IDbGateway interface is exposing, not what back end is hitting.
4. You have also (partly) removed the knowledge of the data source (XML, CSV, Oracle etc)

So next step is now removing the dependency of DataTables. You can happily use DataTables in the SqlDataAccess class, but convert the rows to Participant classes before returning them. Therefore the UI only sees concrete classes, and DataGrid's will happily binding to IEnumerable(Of T) classes.

DB Performance

Never do SELECT * FROM <Table> in a production application. Its fine for SSMS in a development environment to get a look over everything. In a production app, you should only pull out the columns you want, rather than everything. If a DBA decided to change this schema behind your application, you will start pulling in more columns, which could be dangerous, especially if there's something like 'IS_BLACK_LISTED', 'HAS_CRIMINAL_RECORD' .. etc.

Using T-SQL like you are is fine, as its parameterised. But .. (as you'll see in encryption), if you are super paranoid, you should use Stored Procedures, rather than revealing the T-SQL you are querying with. The barrier created by the IDbGateway ensures the UI doesn't have to care where it comes from, it just wants the data back. This pattern is called 'Inversion Of Control'.

Encryption

I need to know more details about what you are doing, but lets review what is going on:

1. A user types text into a UI (unencrypted)
2. The UI encrypts this text (encrypted)
3. Send data to the database (unencrypted transport)
4. Data returns from the DB (unencrypted transport)
5. User sees results from DB (unencrypted)

The UI encrypts it, but there is no other encryption going on as far as I can gather?

So simplify encryption..

1. Password fields can stop the UI revealing sensitive data.

2. Use the SecureString class to encrypt at UI level. It isn't the most friendly of classes, as you have to retrieve items character by character. But if you want secure - you've got it and its relatively easy to use, rather than digging into the Crypto32 API.

3. For database connections, you can install SSL certificates and used the "Encrypt=true". An MSDN article is here. This makes code even cleaner. Also use stored procedures to not reveal what the app is doing with the DB.

4. See (3)

5. User sees results.. 4 out of 5 isn't bad ;-)

--

Thats all I can say about it now. In summary:

1. Separate data access from UI and business logic
2. Only retrieve what you need, rather than everything
3. Use a DTO (Data Transfer Object) to convert from the data sources (tables) to the UI layer. There are tools that can do this for you like Entity Framework and NHibernate.
4. Look at all encryption options. How secure do you want this to be? To what extent should you go to (i.e. obfuscate the application code? SSL the DB connections? Encrypt the DB?)

Raise another StackOverflow question for (4) and you should get a more helpful answer.

Answer 2

Just make sure you NEVER do something like the following line:

SqlCommand.CommandText = "Select * FROM PARTICIPANT WHERE FIRST_NM_TXT = '" & eFirst & "';"

This would be VERY easy to use SQL Injection to retrieve all the data you have in your DB. And once an attacker has that they will run decryption algorithms on your data until they crack it.

Use parameterized queries like you have done in the second example you showed to protect yourself from SQL Injection.

 cmd.CommandText = "Select * FROM Participant WHERE FIRST_NM_TXT = @searchFirst"
 cmd.Parameters.AddWithValue("@searchFirst", eFirst)

This way your input from your users will be sanitized and SQL Injection will be prevented. Hurray!

P.S. I would store your ConnectionString in you web or app config file so that you can change you DB connection without recompiling your application. This will come in handy if your application is deployed and you just want to change the which database the app is pointing at but don't want to have to change the code, recompile, and redeploy.

Now you can just edit the config file on the server and WA-LA new DB connection! This will also make it easier to add multiple connections (Test and Production) and pull the correct DB connection depending on some environment variable or for example if you compiled your assemblies to Debug or Release.