Beginner: Is this good encryption?

Question

I've managed to knock together some code to encrypt/decrypt a string and I'm wondering whether I've done a good job at it. Is it truly secure, in other words.

I read in a StackOverflow post that reusing the IV is bad practice and should be avoided. I can't say for sure, but I believe that's what I'm doing here. Confirm/deny?

That said, wouldn't using a different IV for each operation render the encrypted stream undecryptable? I'm afraid I don't know as much about this as I'd like.

Here's the code:

Private Function Crypt(InBytes As Byte(), Password As String, Action As EncryptionActions) As Byte()
  Dim oCryptoTransform As ICryptoTransform
  Dim oDesProvider As AesCryptoServiceProvider

  Dim _
    iBlockSize,
    iKeySize,
    iSize As Integer

  Dim _
    aSalt,
    aKey,
    aIV As Byte()

  oDesProvider = New AesCryptoServiceProvider
  oDesProvider.Mode = CipherMode.CBC

  aSalt = {&H10, &H20, &H12, &H23, &H37, &HA4, &HC5, &HA6, &HF1, &HF0, &HEE, &H21, &H22, &H45}

  For iSize = 1024 To 1 Step -1
    If oDesProvider.ValidKeySize(iSize) Then
      iKeySize = iSize
      Exit For
    End If
  Next

  iBlockSize = oDesProvider.BlockSize

  With New Rfc2898DeriveBytes(Password, aSalt, 12345)
    aKey = .GetBytes(iKeySize \ 8)
    aIV = .GetBytes(iBlockSize \ 8)
  End With

  Select Case Action
    Case EncryptionActions.Encrypt : oCryptoTransform = oDesProvider.CreateEncryptor(aKey, aIV)
    Case EncryptionActions.Decrypt : oCryptoTransform = oDesProvider.CreateDecryptor(aKey, aIV)
    Case Else : Throw New ArgumentException("Invalid encryption action specified.")
  End Select

  Using oOutStream As New MemoryStream
    Using oCryptoStream As New CryptoStream(oOutStream, oCryptoTransform, CryptoStreamMode.Write)
      oCryptoStream.Write(InBytes, 0, InBytes.Length)
    End Using

    Return oOutStream.ToArray
  End Using
End Function

Answer 1

I don't know .net so I don't understand the code in details but as far as I can see there are at least the following problems:

• You are not using authenticated encryption. You should use a MAC like HMAC to detect manipulations of the ciphertext. Otherwise, an attacker could manipulate the ciphertext so that it is decrypted to a plausible plaintext.
• For DES there exists weak keys. If you're unlucky your derived key is a weak one but the probability to get a weak key usually is negligible. Furthermore, you should use AES instead of 3DES.
• I can't see which cipher mode you use. ECB is not recommended. Today, CBC or CTR are recommended by most cryptographers.

The way you should do encryption is as follows:

• select a key at random or derive a key from a password
• use AES-256 in CBC mode with PKCS#5 padding
• select an IV at random
• encrypt the data
• compute an HMAC over the IV + ciphertext
• send IV + ciphertext + HMAC

When receiving the ciphertext:

• check that the HMAC of IV + ciphertext is correct
• decrypt the ciphertext

Answer 2

Your question lacks context. What exactly are you trying to achieve? More precisely what attacks are you trying to defend against?

As I understand it, your code always use the same key and initialization vector.

Both the salt parameter passed to PBKDF2 and the IV passed to the block cipher should be generated using a properly seeded CSPRNG. In .Net use RNGCryptoServiceProvider.

Edit:

The salt and IV are not secrets. They should be part of the message or file or whatever this is your code produces.

One more comment: you should probably not use a hard-coded number of PBKDF2 iterations. The number of iterations required increases over time as hardware becomes faster. Thus the number of iterations should also be part of the message/file/record/whatever.