3
\$\begingroup\$

I've written an encryption-decryption class that is to be used in my companies commercial web application that will handle sensitive information. The encrypted data will be passed over the internet via HTTPS but even so it needs to be internally secure.

I've tried to follow best practices but I'm limited in that I only have a password available to use as a key. I found what appears to be a good solution here (the second answer) which I've used as a base for this class:

''' <summary>
''' Class that provides encryption using AES with HMAC for authentication.
''' </summary>
''' <remarks></remarks>
Public NotInheritable Class AESHMACEncryption

    'See https://stackoverflow.com/questions/202011/encrypt-and-decrypt-a-string/10366194#10366194
    'and http://dotnetslackers.com/articles/security/hashing_macs_and_digital_signatures_in_net.aspx

    ''' <summary>
    ''' Encrypt the given text using the given password and create an authenticated message that can be decrypted by AESHMAC.Decrypt.
    ''' </summary>
    ''' <param name="cleartext">String</param>
    ''' <param name="password">String</param>
    ''' <returns>String</returns>
    ''' <remarks>AES encryption with HMAC.</remarks>
    Shared Function Encrypt(cleartext As String, password As String) As String

        'Sanity checks
        If password.Length < EncryptionSettings.MinimumPasswordLength Then Throw New Exception("Password is too short.")
        If cleartext.Length = 0 Then Throw New Exception("Nothing to encrypt.")

        'Generate two keys and salts
        Dim kspCrypt As KeySaltPair = KeyGeneration.GenerateKeySaltPair(password)
        Dim kspAuth As KeySaltPair = KeyGeneration.GenerateKeySaltPair(password)
        'Debug.Print("kspCrypt.Key" & ASCIIEncoding.UTF8.GetString(kspCrypt.Key) & vbCrLf)
        'Debug.Print("kspAuth.Key" & ASCIIEncoding.UTF8.GetString(kspAuth.Key) & vbCrLf)

        Dim encrypted As Byte()
        Dim signed As Byte()

        ' Create an AesCryptoServiceProvider object with the specified key and IV. 
        Using cryptoProvider As New AesCryptoServiceProvider With {
                 .KeySize = EncryptionSettings.KeyBitSize,
                 .BlockSize = EncryptionSettings.BlockBitSize,
                 .Mode = CipherMode.CBC,
                 .Padding = PaddingMode.PKCS7
            }

            'Use random IV
            cryptoProvider.GenerateIV()
            Dim iv As Byte() = cryptoProvider.IV

            'Create encryptor
            Using encryptor As ICryptoTransform = cryptoProvider.CreateEncryptor(kspCrypt.Key, iv)
                ' Create the streams used for encryption. 
                Using 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(cleartext)
                        End Using
                        encrypted = msEncrypt.ToArray()

                    End Using
                End Using
            End Using

            'Assemble encrypted message and add authentication
            Using hmac As New HMACSHA256(kspAuth.Key)
                Using encryptedStream = New MemoryStream()
                    Using binaryWriter = New BinaryWriter(encryptedStream)
                        'Prepend Salts
                        binaryWriter.Write(kspCrypt.Salt)
                        binaryWriter.Write(kspAuth.Salt)
                        'Prepend IV
                        binaryWriter.Write(iv)
                        'Write Ciphertext
                        binaryWriter.Write(encrypted)
                        binaryWriter.Flush()
                        'Authenticate all data
                        Dim tag = hmac.ComputeHash(encryptedStream.ToArray())
                        'Postpend tag
                        binaryWriter.Write(tag)
                    End Using
                    signed = encryptedStream.ToArray()
                End Using
            End Using
        End Using

        'Debug.Print("Message: " & ASCIIEncoding.UTF8.GetString(signed) & vbCrLf)

        Dim encryptedtext As String = Convert.ToBase64String(signed)
        Return encryptedtext

    End Function
    ''' <summary>
    ''' Decrypt a message encrypted using AESHMAC.Encrypt.
    ''' </summary>
    ''' <param name="encrypted">String</param>
    ''' <param name="password">String</param>
    ''' <returns>String</returns>
    ''' <remarks>AES encryption with HMAC.</remarks>
    Public Shared Function Decrypt(encrypted As String, password As String) As String

        'Sanity checks
        If password.Length < EncryptionSettings.MinimumPasswordLength Then Throw New Exception("Password is too short.")

        Dim encryptedMessage = Convert.FromBase64String(encrypted)
        'Debug.Print("Message: " & ASCIIEncoding.UTF8.GetString(encryptedMessage) & vbCrLf)

        Dim plaintext As String

        Dim kspCrypt As New KeySaltPair
        kspCrypt.Salt = New Byte(EncryptionSettings.SaltBitSize \ 8 - 1) {}
        Dim kspAuth As New KeySaltPair
        kspAuth.Salt = New Byte(EncryptionSettings.SaltBitSize \ 8 - 1) {}

        Dim SaltsLength As Integer = kspCrypt.Salt.Length + kspAuth.Salt.Length

        'Grab Salts from message
        Array.Copy(encryptedMessage, 0, kspCrypt.Salt, 0, kspCrypt.Salt.Length)
        Array.Copy(encryptedMessage, kspCrypt.Salt.Length, kspAuth.Salt, 0, kspAuth.Salt.Length)

        kspCrypt.Key = KeyGeneration.GenerateKey(password, kspCrypt.Salt)
        kspAuth.Key = KeyGeneration.GenerateKey(password, kspAuth.Salt)
        'Debug.Print("kspCrypt.Key" & ASCIIEncoding.UTF8.GetString(kspCrypt.Key) & vbCrLf)
        'Debug.Print("kspAuth.Key" & ASCIIEncoding.UTF8.GetString(kspAuth.Key) & vbCrLf)

        Using hmac As New HMACSHA256(kspAuth.Key)
            Dim sentTag = New Byte(hmac.HashSize \ 8 - 1) {}
            'Calculate Tag
            Dim calcTag = hmac.ComputeHash(encryptedMessage, 0, encryptedMessage.Length - sentTag.Length)
            Dim ivLength As Integer = (EncryptionSettings.BlockBitSize \ 8)

            'if message length is too small just return null
            If encryptedMessage.Length < sentTag.Length + SaltsLength + ivLength Then Throw New Exception("Message is too short to be a valid message.")

            'Grab Sent Tag
            Array.Copy(encryptedMessage, encryptedMessage.Length - sentTag.Length, sentTag, 0, sentTag.Length)

            'Compare Tag with constant time comparison
            Dim compare As Integer = 0
            For index As Integer = 0 To sentTag.Length - 1
                compare = compare Or sentTag(index) Xor calcTag(index)
            Next

            'if message doesn't authenticate throw an error
            If compare <> 0 Then Throw New Exception("Message does not pass authentication.")

            Using cryptoProvider As New AesCryptoServiceProvider() With {
                 .KeySize = EncryptionSettings.KeyBitSize,
                 .BlockSize = EncryptionSettings.BlockBitSize,
                 .Mode = CipherMode.CBC,
                 .Padding = PaddingMode.PKCS7
            }

                'Grab IV from message
                Dim iv = New Byte(ivLength - 1) {}
                Array.Copy(encryptedMessage, SaltsLength, iv, 0, iv.Length)

                Using decrypter As ICryptoTransform = cryptoProvider.CreateDecryptor(kspCrypt.Key, iv)
                    Using plainTextStream As New MemoryStream()
                        Using decrypterStream As New CryptoStream(plainTextStream, decrypter, CryptoStreamMode.Write)
                            Using binaryWriter As New BinaryWriter(decrypterStream)
                                'Decrypt Cipher Text from Message
                                binaryWriter.Write(encryptedMessage, SaltsLength + iv.Length, encryptedMessage.Length - SaltsLength - iv.Length - sentTag.Length)
                            End Using
                        End Using
                        'Return Plain Text
                        plaintext = Encoding.UTF8.GetString(plainTextStream.ToArray())
                    End Using
                End Using
            End Using
        End Using

        Return plaintext

    End Function

End Class

My simple unit test is as follows:

<TestMethod()> Public Sub EncryptDecryptAESHMACTest()
    Dim data As String = "TEST_DATA_GOES_HERE"
    Dim password As String = "testpassword"
    Dim encrypted As String = AESHMACEncryption.Encrypt(data, password)
    Debug.Print(encrypted)
    Dim decrypted As String = AESHMACEncryption.Decrypt(encrypted, password)
    Assert.IsTrue(decrypted = data)
End Sub

The settings are stored in a separate class in order to make it easier to alter them as required:

''' <summary>
''' Settings used for symmetric encryption.
''' </summary>
''' <remarks></remarks>
Public NotInheritable Class EncryptionSettings

    'Preconfigured Encryption Parameters
    Public Shared ReadOnly BlockBitSize As Integer = 128
    Public Shared ReadOnly KeyBitSize As Integer = 256

    'Preconfigured Password Key Derivation Parameters
    Public Shared ReadOnly SaltBitSize As Integer = 64
    Public Shared ReadOnly Iterations As Integer = 10000
    Public Shared ReadOnly MinimumPasswordLength As Integer = 10

    'Calculated values
    Public Shared ReadOnly BlockBytes As Integer = BlockBitSize \ 8
    Public Shared ReadOnly KeyBytes As Integer = KeyBitSize \ 8
    Public Shared ReadOnly SaltBytes As Integer = SaltBitSize \ 8

End Class

Assuming that a third party has the source code but not the password which is entered by the user and is not stored on disk/database.

How vulnerable to attack is the implementation above? Are there any flaws, incorrect assumptions, or other problems?

\$\endgroup\$
  • \$\begingroup\$ It looks like brute force will be approximately twice as effective as it should be, since you're using independent calls to KeyGeneration.GenerateKeySaltPair for kspCrypt and kspAuth. (You should call KeyGeneration.GenerateKeySaltPair once and then stretch.) \$\endgroup\$ – user33366 May 19 '15 at 10:06
  • \$\begingroup\$ You might want to check here for the best option. \$\endgroup\$ – the_lotus May 20 '15 at 16:57
  • \$\begingroup\$ @RickyDemer Stretch? \$\endgroup\$ – user59340 May 21 '15 at 15:18
  • \$\begingroup\$ Yes, probably with hkdf. \$\endgroup\$ – user33366 May 21 '15 at 17:32
3
\$\begingroup\$

This isn't anything huge or anything,

you can actually get rid of

Dim plaintext As String

This variable is only used to return a value, and you don't need to assign the value to this variable before you return it. by creating a variable you are assigning memory that could be used for other things. Encryption applications can get heavy while a couple of empty variables worth of memory may not wipe out the available memory, it is a good habit to break.

Don't create variables that you don't have to.

The value can be returned directly from inside the using statements.

right here

Using decrypter As ICryptoTransform = cryptoProvider.CreateDecryptor(kspCrypt.Key, iv)
    Using plainTextStream As New MemoryStream()
        Using decrypterStream As New CryptoStream(plainTextStream, decrypter, CryptoStreamMode.Write)
            Using binaryWriter As New BinaryWriter(decrypterStream)
                'Decrypt Cipher Text from Message
                binaryWriter.Write(encryptedMessage, SaltsLength + iv.Length, encryptedMessage.Length - SaltsLength - iv.Length - sentTag.Length)
            End Using
        End Using
        'Return Plain Text
        plaintext = Encoding.UTF8.GetString(plainTextStream.ToArray())
    End Using
End Using

just change it to this instead

 Using decrypter As ICryptoTransform = cryptoProvider.CreateDecryptor(kspCrypt.Key, iv)
     Using plainTextStream As New MemoryStream()
         Using decrypterStream As New CryptoStream(plainTextStream, decrypter, CryptoStreamMode.Write)
             Using binaryWriter As New BinaryWriter(decrypterStream)
                 'Decrypt Cipher Text from Message
                 binaryWriter.Write(encryptedMessage, SaltsLength + iv.Length, encryptedMessage.Length - SaltsLength - iv.Length - sentTag.Length)
             End Using
         End Using
         'Return Plain Text
         return Encoding.UTF8.GetString(plainTextStream.ToArray())
     End Using
 End Using

just

return Encoding.UTF8.GetString(plainTextStream.ToArray())

Same with this

Dim encryptedtext As String = Convert.ToBase64String(signed)
Return encryptedtext

you can do this inside the using statement in a single return statement

just like this

return Convert.ToBase64String(encryptedStream.ToArray())

then you get rid of 2 more variables

Dim signed As Byte()

And

Dim encryptedtext As String = Convert.ToBase64String(signed)
\$\endgroup\$

Your Answer

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