2
\$\begingroup\$

This is the third iteration of my venture on creating an encryption/decryption solution. I asked a question here, which led to this question here, which led to this answer here, which led me to introducing Bouncy Castle into my solution to gain better security under the circumstances and application requirements.

Requirements: To encrypt and decrypt a string using AES 256 with a password/key (stored in web.config) in an ASP.net application.

History: If you follow the above links you'll find that I originally tried to stick with the core .net provided solutions without the inclusion of any additional libraries. This requirement has changed and I've added Bouncy Castle to my solution.

Thanks to @SEJPM regularly pointing me in the right direction I decided to implement AES GCM and ditch my previous attempts.

I found this example here from @jbtule who seems to have a pretty good handle on things, and honestly I didn't change a thing other than convert it to VB. However, based on previous suggestions given to me to use Argon2, I read that Bouncy Castle supports this now but I'm currently uncertain as how to properly implement it.

Although my code is essentially a copy @jbtule's original post on CodeReview, that was 6 years ago.

So based on the fact that I pull the encryption key/pass from web.config and I need simple encrypt/decrypt, how does this solution stack up?

Usage:

Dim password = RetrieveFromWebConfig()
Dim plainText = "Hello World"
Dim encrypted = SimpleEncryptWithPassword(plainText, password)
Dim decrypted = SimpleDecryptWithPassword(encrypted, password)

Code:

Imports System
Imports System.IO
Imports System.Text
Imports Org.BouncyCastle.Crypto
Imports Org.BouncyCastle.Crypto.Engines
Imports Org.BouncyCastle.Crypto.Generators
Imports Org.BouncyCastle.Crypto.Modes
Imports Org.BouncyCastle.Crypto.Parameters
Imports Org.BouncyCastle.Security

Namespace Utilities.Encryption
    Public Class Aesgcm
        Public Shared ReadOnly Random As SecureRandom = New SecureRandom()
        Public Shared ReadOnly NonceBitSize As Integer = 128
        Public Shared ReadOnly MacBitSize As Integer = 128
        Public Shared ReadOnly KeyBitSize As Integer = 256
        Public Shared ReadOnly SaltBitSize As Integer = 128
        Public Shared ReadOnly Iterations As Integer = 10000
        Public Shared ReadOnly MinPasswordLength As Integer = 12


        Shared Function SimpleEncryptWithPassword(secretMessage As String, password As String, ByVal Optional nonSecretPayload As Byte() = Nothing) As String
            If String.IsNullOrEmpty(secretMessage) Then Throw New ArgumentException("Secret Message Required!", "secretMessage")
            Dim plainText = Encoding.UTF8.GetBytes(secretMessage)
            Dim cipherText = SimpleEncryptWithPassword(plainText, password, nonSecretPayload)
            Return Convert.ToBase64String(cipherText)
        End Function

        Shared Function SimpleDecryptWithPassword(encryptedMessage As String, password As String, ByVal Optional nonSecretPayloadLength As Integer = 0) As String
            If String.IsNullOrWhiteSpace(encryptedMessage) Then Throw New ArgumentException("Encrypted Message Required!", "encryptedMessage")
            Dim cipherText = Convert.FromBase64String(encryptedMessage)
            Dim plainText = SimpleDecryptWithPassword(cipherText, password, nonSecretPayloadLength)
            Return If(plainText Is Nothing, Nothing, Encoding.UTF8.GetString(plainText))
        End Function

        Shared Function SimpleEncrypt(secretMessage As Byte(), key As Byte(), ByVal Optional nonSecretPayload As Byte() = Nothing) As Byte()
            If key Is Nothing OrElse key.Length <> KeyBitSize / 8 Then Throw New ArgumentException($"Key needs to be {KeyBitSize} bit!", "key")
            If secretMessage Is Nothing OrElse secretMessage.Length = 0 Then Throw New ArgumentException("Secret Message Required!", "secretMessage")
            nonSecretPayload = If(nonSecretPayload, New Byte() {})
            Dim nonce = New Byte(CInt(NonceBitSize / 8 - 1)) {}
            Random.NextBytes(nonce, 0, nonce.Length)
            Dim cipher = New GcmBlockCipher(New AesEngine())
            Dim parameters = New AeadParameters(New KeyParameter(key), MacBitSize, nonce, nonSecretPayload)
            cipher.Init(True, parameters)
            Dim cipherText = New Byte(cipher.GetOutputSize(secretMessage.Length) - 1) {}
            Dim len = cipher.ProcessBytes(secretMessage, 0, secretMessage.Length, cipherText, 0)
            cipher.DoFinal(cipherText, len)

            Using combinedStream = New MemoryStream()

                Using binaryWriter = New BinaryWriter(combinedStream)
                    binaryWriter.Write(nonSecretPayload)
                    binaryWriter.Write(nonce)
                    binaryWriter.Write(cipherText)
                End Using

                Return combinedStream.ToArray()
            End Using
        End Function

        Shared Function SimpleDecrypt(encryptedMessage As Byte(), key As Byte(), ByVal Optional nonSecretPayloadLength As Integer = 0) As Byte()
            If key Is Nothing OrElse key.Length <> KeyBitSize / 8 Then Throw New ArgumentException($"Key needs to be {KeyBitSize} bit!", "key")
            If encryptedMessage Is Nothing OrElse encryptedMessage.Length = 0 Then Throw New ArgumentException("Encrypted Message Required!", "encryptedMessage")

            Using cipherStream = New MemoryStream(encryptedMessage)

                Using cipherReader = New BinaryReader(cipherStream)
                    Dim nonSecretPayload = cipherReader.ReadBytes(nonSecretPayloadLength)
                    Dim nonce = cipherReader.ReadBytes(CInt(NonceBitSize / 8))
                    Dim cipher = New GcmBlockCipher(New AesEngine())
                    Dim parameters = New AeadParameters(New KeyParameter(key), MacBitSize, nonce, nonSecretPayload)
                    cipher.Init(False, parameters)
                    Dim cipherText = cipherReader.ReadBytes(encryptedMessage.Length - nonSecretPayloadLength - nonce.Length)
                    Dim plainText = New Byte(cipher.GetOutputSize(cipherText.Length) - 1) {}

                    Try
                        Dim len = cipher.ProcessBytes(cipherText, 0, cipherText.Length, plainText, 0)
                        cipher.DoFinal(plainText, len)
                    Catch unusedInvalidCipherTextException1 As InvalidCipherTextException
                        Return Nothing
                    End Try

                    Return plainText
                End Using
            End Using
        End Function

        Shared Function SimpleEncryptWithPassword(secretMessage As Byte(), password As String, ByVal Optional nonSecretPayload As Byte() = Nothing) As Byte()
            nonSecretPayload = If(nonSecretPayload, New Byte() {})
            If String.IsNullOrWhiteSpace(password) OrElse password.Length < MinPasswordLength Then Throw New ArgumentException($"Must have a password of at least {MinPasswordLength} characters!", "password")
            If secretMessage Is Nothing OrElse secretMessage.Length = 0 Then Throw New ArgumentException("Secret Message Required!", "secretMessage")
            Dim generator = New Pkcs5S2ParametersGenerator()
            Dim salt = New Byte(CInt(SaltBitSize / 8 - 1)) {}
            Random.NextBytes(salt)
            generator.Init(PbeParametersGenerator.Pkcs5PasswordToBytes(password.ToCharArray()), salt, Iterations)
            Dim key = CType(generator.GenerateDerivedMacParameters(KeyBitSize), KeyParameter)
            Dim payload = New Byte(salt.Length + nonSecretPayload.Length - 1) {}
            Array.Copy(nonSecretPayload, payload, nonSecretPayload.Length)
            Array.Copy(salt, 0, payload, nonSecretPayload.Length, salt.Length)
            Return SimpleEncrypt(secretMessage, key.GetKey(), payload)
        End Function

        Shared Function SimpleDecryptWithPassword(encryptedMessage As Byte(), password As String, ByVal Optional nonSecretPayloadLength As Integer = 0) As Byte()
            If String.IsNullOrWhiteSpace(password) OrElse password.Length < MinPasswordLength Then Throw New ArgumentException($"Must have a password of at least {MinPasswordLength} characters!", "password")
            If encryptedMessage Is Nothing OrElse encryptedMessage.Length = 0 Then Throw New ArgumentException("Encrypted Message Required!", "encryptedMessage")
            Dim generator = New Pkcs5S2ParametersGenerator()
            Dim salt = New Byte(CInt(SaltBitSize / 8 - 1)) {}
            Array.Copy(encryptedMessage, nonSecretPayloadLength, salt, 0, salt.Length)
            generator.Init(PbeParametersGenerator.Pkcs5PasswordToBytes(password.ToCharArray()), salt, Iterations)

            Dim key = CType(generator.GenerateDerivedMacParameters(KeyBitSize), KeyParameter)
            Return SimpleDecrypt(encryptedMessage, key.GetKey(), salt.Length + nonSecretPayloadLength)
        End Function
    End Class
End Namespace
\$\endgroup\$
1
\$\begingroup\$

This is probably not a good idea:

Public Shared ReadOnly NonceBitSize As Integer = 128

as the default nonce for GCM is 12 bytes or 96 bits. A different size nonce requires additional calculations and may not always be supported by API's (although they generally do).


The iteration count for key derivation is on the low side, say 40K is more recommended, but the actual value should simply be as high as you can manage within your limitations.


If String.IsNullOrEmpty(secretMessage) Then Throw New ArgumentException("Secret Message Required!", "secretMessage")

Generally we think that empty messages are OK (when it comes to the encryption layer). Now you could not send a message that is only protected for integrity / authenticity. it's good that you test for null of course.


Dim payload = New Byte(salt.Length + nonSecretPayload.Length - 1) {}

After this I would presume that you first copy the salt and then the nonSecretPayload, instead of the other way around. The name payload is confusing, because you would expect this to include the payload that also needs to be encrypted.


generator.Init(PbeParametersGenerator.Pkcs5PasswordToBytes(password.ToCharArray()), salt, Iterations)

the reason why the password is delivered as a character array is that you can zero an array. Commonly password interface elements will also return a char array for the same reason. Converting it from string doesn't make all that much sense - at least not at this level.


There is no need to convert back and forth to a KeyParameter. Once you have a KeyParameter, use it, possibly as private method from several public methods.


 Catch unusedInvalidCipherTextException1 As InvalidCipherTextException
                    Return Nothing

This is a big no-no. Never return Nothing or similar and sweep the exception under the carpet. If you don't know what to do, then wrap the exception in a more generic exception.


During decryption you're passing the length of the nonSecretPayload and you're never returning the data itself. That's a weird decision: you need to do handle the contents of this payload some way or another. It's not clear how you would retrieve or handle it currently.


Other protocol related notes:

  • Your scheme doesn't include a version number. You could use one of those to upgrade to a different scheme (or to up the iteration count) later on.
  • In general, if you derive the wrong key then decryption would fail. As such, there is no pressing need to include the salt in the additional authenticated data. I would not call that wrong though.
  • Currently you need to know the size of the AAD in advance; you could think of a way to send the AAD length within your protocol.
  • You could derive both the key and nonce as you're using a random salt anyway. Again, I would not call that wrong in any sense.

Conclusion: there is nothing particularly wrong if you use this for password based encryption, but there are a lot of small things that can be adjusted, with the version indicator and the nonce size being the main issues. The exception handling is not up to par.

I'd strongly recommend to write a small protocol description, so you can show what you implement without others having to read through your code to find out.

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