4

I need to encrypt/decrypt some strings. I've build my wrapper class according to the msdn documentation but with some changes.

Since I want to encrypt/decrypt data with a given string/passphrase, I don't use AesManaged for creating a key. (The user should be able to encrypt/decrypt with a key he enters, and therefore I cannot use the key from AesManaged and I cannot save the key).

I instead create the key by using Rfc2898DeriveBytes (PBKDF2) with a given salt. The given salt is used since I do not store the key and I think because of this, the salt must be always the same.

I then create an IV, encrypt the given string and concatenate the IV and the encrypted string. This will then eventually got saved in a file. This means the IV gets save together with the encrypted data.

Questions:

  1. Is it ok to store the IV together with the encrypted data?
  2. Is there another way to create the key without using the same salt everytime(Based on a given passphrase)?
  3. Is this encryption done using AES128 or AES256?
  4. Will the IV be always 16 bytes, or can this change?

    static void Main(string[] args)
    {
        const string stringToEncrypt = "String to be encrypted/decrypted. Encryption is done via AesManaged";
        const string password = "m1Sup3rS3cre!Password";

        string encrypted = EncryptString(stringToEncrypt, password);
        string roundtrip = DecryptStringFromBytes_Aes(encrypted, password);

        Console.WriteLine("Original:   {0}", stringToEncrypt);
        Console.WriteLine("Round Trip: {0}", roundtrip);

        Console.ReadLine();
    }

    static string EncryptString(string plainText, string password)
    {
        string encryptedString;

        using (AesManaged aesAlg = new AesManaged())
        {
            aesAlg.Key = PasswordAsByte(password);
            ICryptoTransform encryptor = aesAlg.CreateEncryptor(aesAlg.Key, aesAlg.IV);

            using (MemoryStream msEncrypt = new MemoryStream())
            {
                using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
                {
                    using (StreamWriter swEncrypt = new StreamWriter(csEncrypt))
                    {
                        swEncrypt.Write(plainText);
                    }
                    var encrypted = msEncrypt.ToArray();

                    encryptedString = Encoding.Default.GetString(aesAlg.IV);
                    encryptedString += Encoding.Default.GetString(encrypted);
                }
            }
        }
        return encryptedString;
    }

    static string DecryptStringFromBytes_Aes(string cipherText, string password)
    {
        using (AesManaged aesAlg = new AesManaged())
        {
            aesAlg.Key = PasswordAsByte(password);

            aesAlg.IV = Encoding.Default.GetBytes(cipherText).Take(16).ToArray();

            ICryptoTransform decryptor = aesAlg.CreateDecryptor(aesAlg.Key, aesAlg.IV);

            var encryptedByteArray = Encoding.Default.GetBytes(cipherText).Skip(16).ToArray();

            using (MemoryStream msDecrypt = new MemoryStream(encryptedByteArray))
            {
                using (CryptoStream csDecrypt = new CryptoStream(msDecrypt, decryptor, CryptoStreamMode.Read))
                {
                    using (StreamReader srDecrypt = new StreamReader(csDecrypt))
                    {
                        return srDecrypt.ReadToEnd();
                    }
                }
            }
        }
    }

    private static byte[] PasswordAsByte(string password)
    {
        byte[] salt = Encoding.Default.GetBytes("foobar42");
        Rfc2898DeriveBytes passwordBytes = new Rfc2898DeriveBytes(password, salt);

        return passwordBytes.GetBytes(32);
    }
Maarten Bodewes
  • 90,524
  • 13
  • 150
  • 263
Manuel
  • 1,985
  • 3
  • 31
  • 51
  • It's good that you provided *most* of the code - but if you'd included the using directives and class declaration, we could have just copied, pasted, and compiled... – Jon Skeet Mar 25 '15 at 09:29
  • 1
    This is not an "implementation" of AES, just you *using* the API. – Dai Mar 25 '15 at 09:44
  • I'm voting to close this question as off-topic because this is a code-review question. – Dai Mar 25 '15 at 09:45
  • This is unauthenticated encryption. Attackers can modify the encrypted data without you being able to tell when decrypting. Are you OK with that? – usr Mar 25 '15 at 09:48
  • About "implementation of AES": I just said i implement something with AesManaged. Maybe my English is not good enough. – Manuel Mar 25 '15 at 09:49
  • @Dai, i do not agree. I do have specific questions related to .net and security. Where if not on stackoverflow should i ask for help? Encryption to me seems not to be so easy to get it right from the beginning. – Manuel Mar 25 '15 at 14:48
  • I now have a cleaned up and working implementation of this. Can/may i add this implementation as a comment for further reference? – Manuel Mar 26 '15 at 08:59

3 Answers3

7

No, this is not okay.

1) You're using Encoding.Default in various places. Don't do that - it means you're at the whim of the platform you're on. Always use an explicit encoding, ideally UTF-8 in most cases.

2) You're using Encoding.GetString / Encoding.GetBytes to convert arbitrary binary data to a string and back. That's almost bound to lose data. (It happened to succeed on my machine, but it really depends on the encoding - and it's fundamentally a bad idea.) Encoding is designed for data which is inherently text data, and you're just applying an encoding one way or the other. Your encrypted data is inherently binary data. Use Convert.ToBase64String and Convert.FromBase64String instead.

For your other questions:

  • Yes, it's okay to store the IV with the encrypted data, as far as I know.
  • You could use the same approach for the password: generate a different salt each time, and store that with the encrypted text. Not sure whether that's generally recommended or not, I'm afraid.
  • I believe you're controlling whether the key size is 128 or 256 bits, with your call to passwordBytes.GetBytes(32) - that's a 256-bit key, so it's AES256.
  • I believe the IV size for AES is always 16 bytes (128 bits)
Jon Skeet
  • 1,421,763
  • 867
  • 9,128
  • 9,194
  • 1
    I did change the encoding according to your comment. Wasn't fully aware about that. – Manuel Mar 25 '15 at 09:51
  • That's a lot of uncertainty in your answer, Jon :) Basically you can indeed store the salt with the ciphertext. A static IV is fine as long as the key changes each time (because of the salt), although a random IV would still be recommended. I'll try and answer this evening (CET) as well, but you are basically correct here (with the small remark that the IV is required for CBC mode rather than AES, in which case it is equal to the blocksize of the block cipher, which is indeed 128 bits for AES). – Maarten Bodewes Mar 25 '15 at 10:32
  • @MaartenBodewes: I try not to express certainty in a security-sensitive answer unless I'm *really* confident :) Will look forward to your answer. – Jon Skeet Mar 25 '15 at 10:34
1

Normally salt is used together with cryptographic hashing of say passwords to protect against dictionary attacks. To get the same kind of protection for symmetric encryption with AES you should use a random initialization vector. So when you encrypt create a random IV and prepend it to the message (in cleartext). When you decrypt get the IV from the encrypted message and use it to decrypt the message. Then the ciphertext of the same message encrypted with the same key will be different.

  • So, yes, it is OK to store the IV together with the encrypted data.

  • You do not need a different salt every time because the purpose of the random IV is similar in how salt makes dictionary attacks on hashes harder.

  • AES can use key sizes of 128, 192 or 256 bits so to use AES 256 you need a 256 bit key (32 bytes) which is what you use.

  • AES uses a 128 bit block which requires a 128 bit IV (or 16 bytes).

Martin Liversage
  • 104,481
  • 22
  • 209
  • 256
  • Wrong, an IV is not an alternative for a salt, I'm afraid. – Maarten Bodewes Mar 25 '15 at 10:34
  • @MaartenBodewes: Can you clarify you statement? If you want to add a random salt then the encrypted message would be salt + IV + ciphertext. What kind of attack does adding a salt protect against? – Martin Liversage Mar 25 '15 at 11:05
  • It protects against dictionary attacks and rainbow table attacks on the key value. If you have just encrypted text then it would be more difficult to verify the key, but it doesn't add the same protection as PBKDF2 (as defined in the RFC 2898). PBKDF2 also contains a number of iterations (work factor) for the user & possible attackers. The salt is used within those iterations. An IV is only applied afterwards as a requirement for CBC mode encryption. So it is not taken into account in the work factor. – Maarten Bodewes Mar 25 '15 at 11:16
  • @MaartenBodewes: I do not see how you can make a rainbow table attack against AES encryption with a random IV. A rainbow table contains precomputed hashes but AES is not a hash. Also, I am not saying that PBKDF2 should not be used to make brute force dictionary attacks harder. My point is that using a random salt does not add any security that a random IV does not already provide. – Martin Liversage Mar 25 '15 at 11:30
  • You can precompute the keys and validate them against any ciphertext created with the same salt. The IV does nothing to protect against validation of the keys. The validation would consist of one or two block decrypts instead of the full work factor. Sure, it is not as fast as a simple lookup, but it is not as hard as it should be either. – Maarten Bodewes Mar 25 '15 at 11:35
0

Is it ok to store the IV together with the encrypted data?

Yes, it is ok. Moreover, you're using AesManaged without explicit setting of Mode - it this case mode is CBC, and in CBC mode IV should preceed cyphertext.

Is there another way to create the key without using the same salt everytime(Based on a given passphrase)?

Rfc2898DeriveBytes is pretty standard way to derive key from text password. There is no need to reinvent way of deriving key from password, just use Rfc2898DeriveBytes as you're doing it now.

Is this encryption done using AES128 or AES256?

It is AES256 since you're using 32-byte password.

Will the IV be always 16byte, or can this change?

The size of the IV property must be the same as the BlockSize property divided by 8. So it is 16 for 128-bit blocks.

Andrey Korneyev
  • 26,353
  • 15
  • 70
  • 71
  • One question about the IV already stored as prefix: AesManaged.CreateDecryptor requires an IV and i did not find a sample which shows the use of the prefix IV. Do you have a link for this? – Manuel Mar 25 '15 at 09:52
  • @Manuel Oh.... it looks like my misunderstanding of .NET AES implementation. Since by standard in CBC mode IV should be prefix of cyphertext - I supposed `CryptoStream` already does it. But in fact it doesn not prepends IV to cypertext when writing to stream. So you are right, you have to append IV manually. Sorry for confusion, I'll change the answer to prevent confusing someone else. – Andrey Korneyev Mar 25 '15 at 10:05
  • The question already uses `Rfc2898DeriveBytes` instead of reinventing it, so that remark should probably be scrapped. – Maarten Bodewes Mar 25 '15 at 11:17
  • @MaartenBodewes question was "Is there another way except Rfc2898DeriveBytes..." My answer supposed to be "Don't reinvent way of deriving key, just use Rfc2898DeriveBytes as you're doing". Probably this was not obvious from original answer formulation. – Andrey Korneyev Mar 25 '15 at 11:20
  • Please use copy/paste to create a quote. I don't see "Is there another way except Rfc2898DeriveBytes" anywhere within the question although this *may* be implied by "Is there another way to create the key without using the same salt everytime" – Maarten Bodewes Mar 25 '15 at 11:40