dotnet / dotnet-api-docs

.NET API reference documentation (.NET 5+, .NET Core, .NET Framework)
https://docs.microsoft.com/dotnet/api/
Other
701 stars 1.55k forks source link

Bug in System.Security.Cryptography.AES contructor example #10119

Open BillClarkEarthPoint opened 1 month ago

BillClarkEarthPoint commented 1 month ago

Hello,

I would like to report a bug in https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.aes.-ctor?view=net-8.0 and suggest a correction. As written, the encryption example frequently, but not always, produces results that cannot be decrypted.

the line encrypted = msEncrypt.ToArray();

needs to be BELOW the CryptoStream "using" block

the example code is

                // Create the streams used for encryption.
                using (MemoryStream msEncrypt = new MemoryStream())
                {
                    using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
                    {
                        using (StreamWriter swEncrypt = new StreamWriter(csEncrypt))
                        {
                            //Write all data to the stream.
                            swEncrypt.Write(plainText);
                        }
                        encrypted = msEncrypt.ToArray();   //<<<<<<< need to move this line
                    }
                }

the fixed code is

                // Create the streams used for encryption.
                using (MemoryStream msEncrypt = new MemoryStream())
                {
                    using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
                    {
                        using (StreamWriter swEncrypt = new StreamWriter(csEncrypt))
                        {
                            //Write all data to the stream.
                            swEncrypt.Write(plainText);
                        }
                    }
                    encrypted = msEncrypt.ToArray();    //<<<<<<<<< moved BELOW CryptoStream "using" block
                }

A discussion on this is found here https://stackoverflow.com/questions/604210/padding-is-invalid-and-cannot-be-removed-using-aesmanaged

to quote:

For whats its worth, I'll document what I faced. I was trying to read the encryptor memory stream before the CryptoStream was closed. I was naive and I wasted a day debugging it.

    public static byte[] Encrypt(byte[] buffer, byte[] sessionKey, out byte[] iv)
    {
        byte[] encrypted;
        iv = null;
        using (AesCryptoServiceProvider aesAlg = new AesCryptoServiceProvider { Mode = CipherMode.CBC, Padding = PaddingMode.PKCS7 })
        {
            aesAlg.Key = sessionKey;
            iv = aesAlg.IV;
            ICryptoTransform encryptor = aesAlg.CreateEncryptor(sessionKey, iv);

            // Create the streams used for encryption.
            using (MemoryStream msEncrypt = new MemoryStream())
            {
                using (CryptoStream csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
                {
                    csEncrypt.Write(buffer, 0, buffer.Length);

                    //This was not closing the cryptostream and only worked if I called FlushFinalBlock()
                    //encrypted = msEncrypt.ToArray(); 
                }

                encrypted = msEncrypt.ToArray();

                return encrypted;
            }
        }
    }

Moving the encryptor memory stream read after the cypto stream was closed solved the problem. As Cheeso mentioned. You don't need to call the FlushFinalBlock() if you're using the using block.

end quote.

Thank you, Bill Clark

dotnet-policy-service[bot] commented 1 month ago

Tagging subscribers to this area: @dotnet/area-system-security, @vcsjones