2

I'm using AesCryptoServiceProvider and CryptoStream to encrypt some data and it seems to be working OK when I use the same key for decryption. However, If I try to decrypt it with the wrong key, I don't get an exception, just junk data. I can't find anything in the .Net documentation which says what is supposed to happen but according to this:

http://books.google.co.uk/books?id=_Y0rWd-Q2xkC&pg=PA631

and this:

Why does a bad password cause "Padding is invalid and cannot be removed"?

I should be getting a CryptographicException. Am I doing it wrong? my function is this:

public static byte[] Encrypt(byte[] data, string password, string salt, bool decrypt)
{
    SymmetricAlgorithm aes = new AesCryptoServiceProvider();
    Rfc2898DeriveBytes rfc2898 = new Rfc2898DeriveBytes(password, Encoding.UTF8.GetBytes(salt));
    aes.IV = rfc2898.GetBytes(aes.BlockSize / 8);
    aes.Key = rfc2898.GetBytes(256 / 8);
    ICryptoTransform enc;
    if (decrypt) {
        enc = aes.CreateDecryptor();
    } else {
        enc = aes.CreateEncryptor();
    }
    using (enc) {
        using (MemoryStream ms = new MemoryStream()) {
            using (CryptoStream cs = new CryptoStream(ms, enc, CryptoStreamMode.Write)) {
                cs.Write(data, 0, data.Length);
            return ms.ToArray();
        }
    }
}
Community
  • 1
  • 1
Andy
  • 10,412
  • 13
  • 70
  • 95

3 Answers3

1

Relying on padding errors is not a good way to determine if a key is correct or not. You should really consider using Authenticated Encryption for this purpose.

I have a public domain snip-it that works in C# for this Modern Examples of Symmetric Authenticated Encryption of a string. that I try to keep up to date and reviewed.

P.S. Also it's not clear if your salt is per domain, per user, or per ciphertext from your sample, but if it's not per ciphertext in your code the IV will be predictable and the same for many ciphertexts which is not good for AES-CBC. Implementing crypto is hard.

I've also worked on a highlevel encryption library , a C# port of Google Keyczar. But that may not work very well for you, it only supports randomly generate keys and keysets, and those keysets can then be password encrypted, but only the keysets. High level encryption frameworks are the best practice for encyption.

Community
  • 1
  • 1
jbtule
  • 31,383
  • 12
  • 95
  • 128
  • I used your code AESThenHMAC.cs from your gist (https://gist.github.com/jbtule/4336842#file-aesthenhmac-cs) and tested with the non-password methods. Using the same authKey and the same nonSecretPayload but a different cryptKey for SimpleDecrypt than the one I used for SimpleEncrypt, I ended up returning a non-null, non-empty, gibberish byte array from SimpleDecrypt. As luck would have it, I actually didn't get a padding exception. I was hoping the Authenticated Encryption portion of your implementation would return null. Maybe I am misunderstanding. – Ken Oct 09 '18 at 21:04
  • @Ken that’s probably a good reason to switch this example to use one key and use a HKDF for auth and crypt keys. The auth is the only thing protecting you. So if auth key is correct and crypt key is incorrect, what you say will happen. – jbtule Oct 09 '18 at 22:10
0

If you have no padding set on decryption then the decryption method won't be able to recognise junk. Set padding to PKCS#7 for both encryption and encryption and the decryption method will probably be able to recognise junk.

For full assurance, you will need authentication, as jbtule says. To include authentication and encryption in the one data pass use GCM mode. For separate authentication use HMAC.

rossum
  • 15,344
  • 1
  • 24
  • 38
  • I'm not trying to protect against the document being modified in this case, I just want to give the user a sensible error message most of the time when he puts in the wrong password instead of having a crash. So it seems that PKCS7 padding should be sufficient but I've checked and it's already using PKCS7 as the default, so I've still no idea why my program doesn't give an exception – Andy Feb 01 '13 at 15:50
  • 1
    PKCS padding will fail to detect junk approximately 1/256th of the time. Don't rely on defaults, make them explicit. You also appear to be generating a new password (bytes) for decryption. Put in a check to print out the exact bytes in `aes.Key` -- you need to check that the decryption password in *byte-for-byte* the same as the encryption password, not character-for-character. – rossum Feb 01 '13 at 16:21
0

I'm going to have to put my hands up here and say False Alarm.

I have no idea what was happening on Friday but now I'm getting what I would expect - most of the time the CryptographicException happens as expected. I've no idea whether I was just hugely unlucky with my test data or whether there was a bug in my test harness which I inadvertently fixed, but it's all behaving as expected now.

Incidentally I did a quick empirical test which validates rossum's 1/256 number but that's acceptable for my purposes. In the general case I completely accept the other comments here about HMACs etc, but what I'm doing is for a test tool

Andy
  • 10,412
  • 13
  • 70
  • 95