10
\$\begingroup\$

I wrote this for use internally at work and have never worked with encrypted data before, so can you please critique? It's written in VB.NET which I have decent experience in, just not the encryption portion of it.

I have written a simple internal messaging client. Basically a home grown email program but it uses file saves over the local network and never goes online. It also writes all data after encryption only so nothing is ever stored somewhere decrypted except volatile RAM. We don't want our users to have access to outside communication and we want it all encrypted due to HIPAA/PCI. We're a small collections company and want to inexpensively make communication easier internally without opening large risk or letting people get to the outside internet.

I'm storing this as XML data. I have one config file (also encrypted) which contains all the settings and user info. So name and password and userID, etc. When they try to log in, I decrypt the config file and look at the XML to see if their password is right. If it is, I let them into the program and decrypt their individual inbox file which is also XML encrypted with this same routine.

I have one 32 byte key that is saved into my source code and not shared with anyone else in the world but me. And then each file gets a unique IV every time its encrypted or re-encrypted and that IV is also saved to the file along with the encrypted data. I'm saving the bytes to the file as hexadecimal.

Here's the module the handles reading/writing files:

Imports System.Security.Cryptography
Imports System.IO
Imports System.Text

Module encryption
    Public Sub TESTING_in_encryption()

    End Sub

    Public Function ObfuscateString(str As String) As String
        'NOT ENCRYPTION! Just stops casual observers from reading the plain text
        Return System.Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes(str))
    End Function

    Public Function DeObfuscateString(str As String) As String
        'NOT ENCRYPTION! Just stops casual observers from reading the plain text
        Return System.Text.Encoding.UTF8.GetString(System.Convert.FromBase64String(str))
    End Function

    Public Sub EncryptFile_Aes(fullpath As String, plaintext As String)
        Dim cryptString As String
        'encrypt string
        cryptString = EncryptString_Aes(plaintext)
        Using sw As StreamWriter = New StreamWriter(fullpath, False)
            sw.Write(cryptString)
            sw.Close()
        End Using
    End Sub

    ''' <summary>
    ''' Encrypts data with the hardcoded key and new IV
    ''' </summary>
    ''' <param name="plainText">Readable string to be encrypted</param>
    ''' <returns>Returns bytes as string with first 16 being the IV</returns>
    ''' <remarks></remarks>
    Public Function EncryptString_Aes(ByVal plainText As String) As String
        Dim sb As StringBuilder = New StringBuilder()
        ' Create an AesCryptoServiceProvider object
        ' with the specified key and IV.
        Using aesAlg As New AesCryptoServiceProvider()

            aesAlg.Key = aesKey
            aesAlg.GenerateIV()

            ' Create a decrytor to perform the stream transform.
            Dim encryptor As ICryptoTransform = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV)

            ' Create the streams used for encryption.
            Dim msEncrypt As New MemoryStream()

            Using csEncrypt As New CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write)
                Using swEncrypt As New StreamWriter(csEncrypt)
                    'Write all data to the stream.
                    swEncrypt.Write(plainText)
                End Using
                sb.Append(BitConverter.ToString(aesAlg.IV))
                sb.Append(BitConverter.ToString(msEncrypt.ToArray))
                'For Each b As Byte In aesAlg.IV
                '    sb.Append(BitConverter.ToString({b}))
                'Next
                'For Each b As Byte In msEncrypt.ToArray
                '    sb.Append(b.ToString("000"))
                'Next
            End Using
        End Using

        Return sb.ToString.Replace("-", "")
    End Function 'EncryptStringToBytes_Aes

    Public Function DecryptFile_Aes(ByVal fullpath As String) As String
        Using sr As StreamReader = New StreamReader(fullpath)
            Dim answerValue As String = DecryptString_Aes(sr.ReadToEnd)
            sr.Close()
            Return answerValue
        End Using
    End Function

    ''' <summary>
    ''' Pass a string of contiguous bytes with leading 16 IV
    ''' </summary>
    ''' <param name="hexString"></param>
    ''' <returns>Plain text string</returns>
    ''' <remarks></remarks>
    Public Function DecryptString_Aes(ByVal hexString As String) As String
        If hexString.Length = 0 Then
            Return ""
        End If

        Dim plaintext As String = Nothing
        Dim iv(15) As Byte
        Dim cryptBytes As Byte() = {0}

        Dim itemindex As Integer = 0
        For i As Integer = 1 To Len(hexString) Step 2
            If itemindex <= 15 Then
                iv(itemindex) = Byte.Parse(Mid(hexString, i, 2), Globalization.NumberStyles.HexNumber)
            Else
                ReDim Preserve cryptBytes(itemindex - 16)
                cryptBytes(itemindex - 16) = Byte.Parse(Mid(hexString, i, 2), Globalization.NumberStyles.HexNumber)
            End If
            itemindex += 1
        Next

        Using aesAlg As New AesCryptoServiceProvider()
            aesAlg.Key = aesKey
            aesAlg.IV = iv
            ' Create a decrytor to perform the stream transform.
            Dim decryptor As ICryptoTransform = aesAlg.CreateDecryptor(aesAlg.Key, aesAlg.IV)
            ' Create the streams used for decryption.
            Using msDecrypt As New MemoryStream(cryptBytes)
                Using csDecrypt As New CryptoStream(msDecrypt, decryptor, CryptoStreamMode.Read)
                    Using srDecrypt As New StreamReader(csDecrypt)
                        ' Read the decrypted bytes from the decrypting stream
                        ' and place them in a string.
                        plaintext = srDecrypt.ReadToEnd()
                    End Using
                End Using
            End Using
        End Using
        Return plaintext
    End Function 'DecryptString_Aes 

End Module

First: Any obvious things I'm doing wrong or not thinking of?

Second: It doesn't take a very big "inbox" for it to take a really long time to decrypt when a user logs in. If your inbox file is 2+ MB then it can literally take a couple minutes. I assume if I stored it differently so I didn't have to decrypt all or nothing it would be better, but short of that because I'm trying to keep this as simple as possible, how can I improve speed?

Keep in mind, these are XML files I'm using, so you can imagine them as just simple text files if that's easier.

\$\endgroup\$
  • \$\begingroup\$ Why are you using a Module and not a Class? If you clutter your code with Module definitions everywhere you also clutter intellisense. A Public Class would be much more useable, as well. \$\endgroup\$ – 410_Gone Dec 1 '15 at 22:56
  • \$\begingroup\$ Why are you storing passwords encrypted in a text file? That's the kind of thing much better handled by a database. Actually, keeping the "inbox" encrypted in a Db would be safer & faster too. \$\endgroup\$ – RubberDuck Dec 1 '15 at 23:36
  • \$\begingroup\$ Side note: Pressing Ctrl+K, Ctrl+D in your IDE will fix the indentation. \$\endgroup\$ – RubberDuck Dec 1 '15 at 23:38
7
\$\begingroup\$

First of all, you have an empty Sub definition:

Public Sub TESTING_in_encryption()

End Sub

That code is doing nothing except making it look like you forgot to implement something. Remove it.


Second, obfuscation is by definition not encryption. You name your method ObfuscateString(string), so you don't need this comment:

'NOT ENCRYPTION! Just stops casual observers from reading the plain text

If you feel you do need that comment, you should create a doc comment for the method and mention it there so callers of the function know to not use it for encryption as well as maintainers.


Third, you have dead code:

'For Each b As Byte In aesAlg.IV
'    sb.Append(BitConverter.ToString({b}))
'Next
'For Each b As Byte In msEncrypt.ToArray
'    sb.Append(b.ToString("000"))
'Next

If you don't need this code, remove it. How are we to know why it is there, and what you originally had it there for? Also, if you use source control (which you totally should), you can see the history of each file, and which commits changed it, so you can safely remove it knowing you can get it back later if you need to.

\$\endgroup\$
  • \$\begingroup\$ Thank you for those thoughts. I keep a testing sub there to easily test snippets i'm working on. The ObfuscateString stuff I never actually use, a friend sent me that as an example alternative to encryption and I just didn't want to lose the code. And the dead code at the bottom, yes I can kill that. And yes i'm using TFS for source control. \$\endgroup\$ – Gary Ritter Dec 2 '15 at 15:00
  • \$\begingroup\$ Regardless of the fact that you're storing dead code, I should remind you that obfuscating strings is not a viable alternative to encryption, @GaryRitter. In addition, many programmers are going to be familiar with base 64 and will instantly recognise it. \$\endgroup\$ – Dan Pantry Dec 2 '15 at 16:56
  • \$\begingroup\$ Yes, i'm not at all using the obfuscation in this program, i just wanted to save the snippet in case i wanted it for something else later. \$\endgroup\$ – Gary Ritter Dec 2 '15 at 20:18
6
\$\begingroup\$

This is draft 2 of my answer, didn't notice the so I'll try to be gentle:

Why do you use a Module instead of a Class? I feel like this would be much more appropriate as an instance class, which would allow you to treat it much more intuitively.


On a more important note: this is not encryption. Yes, you are using an AES Encryption library to process your data, but you have one, unfortunate issue that prevents this from being considered encryption.

I have one 32 byte key that is saved into my source code and not shared with anyone else in the world but me. And then each file gets a unique IV every time its encrypted or re-encrypted and that IV is also saved to the file along with the encrypted data. I'm saving the bytes to the file as hexadecimal.

(Emphasis mine)

This is all well-and-good in theory, but anyone (and I do mean anyone) can decompile your source and get that key. This is basically a glorified obfuscation system.

A better alternative would be to use Rfc2898DeriveBytes on the user's password to get your encryption key. This has several added benefits, most notably:

Your key is no longer shared.

I don't care what anyone says: sharing private keys in encryption is a horrible idea. The more people you share a key with, the less secure it becomes. If you establish a key based on the user's password, you have now created actual encryption.


If this seems harsh, I do apologize. I just want to make sure that you know exactly what makes this poor, and how to fix it. Making the aforementioned change will definitely get you a very large step in the right direction.

\$\endgroup\$
  • \$\begingroup\$ Thank you for pointing out the shared key, but there's another giant red flag: "So name and password and userID, etc. When they try to log in, I decrypt the config file and look at the XML to see if their password is right." -- the passwords should be run through PBKDF2 (or bcrypt/scrypt) and the output saved; reversibly encrypted (retrievable) passwords are dangerous at best when not a business requirement. Of course, this would mean you cannot use the same KDF to get your encryption key, because you're saving the KDF output... \$\endgroup\$ – Bob Dec 2 '15 at 1:10
  • \$\begingroup\$ @Bob Ah yes, I completely missed that bit. I got caught up on the shared key. \$\endgroup\$ – 410_Gone Dec 2 '15 at 1:11
  • \$\begingroup\$ ...from there an alternative is to use the KDF output to encrypt a known string and test decryption using the KDF output from user password, i.e. you never store the key itself, but actually use it as a key even for the password-verification step. Or use it as a HMAC key, or even run it through the KDF a second time and store that for password-verification. Lots of possibilities, as long as you're sure you aren't able to retrieve the encryption key without the original password. \$\endgroup\$ – Bob Dec 2 '15 at 1:13
  • \$\begingroup\$ Hi. Thank you a million for this critique. Exactly what I was hoping for! Let me try to respond to everything. Module vs Class, I'm used to using modules for code and classes for things I'm treating as objects. What's better about a class for regular code? I will look up RFC2898DeriveBytes but what if I want management to be able to get into a users inbox (like if they're terminated for example?). Especially if I one way salt the passwords. I want management to be able to get into stuff if needed. I think I have code for 1 way salt already somewhere. \$\endgroup\$ – Gary Ritter Dec 2 '15 at 15:03
  • \$\begingroup\$ @GaryRitter That makes it more complicated. I'll try to see if I can come up with a better solution within the next few days. \$\endgroup\$ – 410_Gone Dec 2 '15 at 23:39
2
\$\begingroup\$

I think your decryption speed problem can be solved by not storing the entire inbox for a user in one file. For example store a directory listing in the user master file and each item in a separate file. Decrypt the directory listing file when you start and only decrypt the items when selected for viewing. As someone else posted, this is much better suited to a database. It also begs the question, if you really need a mail server not connected to the internet, when not download a free one with appropriate security and not connect it to the internet. Lots out there... https://en.wikipedia.org/wiki/Comparison_of_mail_servers

As supported by the above posts and the sheer number of security patches for common software, learning security, much less implementing it, is not a weekend project. :)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.