System.Security.Cryptography AES Decryption returns wrong results in latest builds

[unofficialdev]

Repro: Run the following program on current .NET 6 preview7 (preview.7.21362.5-win-x64)

Actual result: FAILED
Expected result: PASSED

using System;
using System.IO;
using System.Security.Cryptography;
using System.Text;

namespace AES_Encrypt
{
    class Program
    {
        private const string KEY_ENC = "584ff9dmskcdrovk";
        static void Main(string[] args)
        {
            // encrypt
            string inputText = "{\"status\":true,\"key\":\"808ffc0a-0cda-4358-9f01-a28d2a3x90db\",\"name\":\"unoficialdev\",\"add_date\":\"2021-04-20\",\"token\":\"Y79Y3SOhBwSruNwWPvo2XQbBv9s9eypfK9gioTuLhxnZWtW4cS8uGYj98cS2mqWr\"}";
            Console.WriteLine(inputText);
            Console.WriteLine("");


            string encryptText = Encryptor(inputText, KEY_ENC);
            Console.WriteLine(encryptText);
            Console.WriteLine("");
            // decrypt
            string decryptText = Decrypt(encryptText, KEY_ENC);
            Console.WriteLine(decryptText);

            Console.WriteLine((decryptText == inputText) ? "PASSED" : "FAILED");
        }


        static string Encryptor(string text, string key)
        {
            byte[] btkey = Encoding.ASCII.GetBytes(key);
            RijndaelManaged aes128 = new()
            {
                Mode = CipherMode.ECB,
                Padding = PaddingMode.PKCS7
            };
            ICryptoTransform encryptor = aes128.CreateEncryptor(btkey, null);
            MemoryStream msEncrypt = new();
            using (var csEncrypt = new CryptoStream(msEncrypt, encryptor, CryptoStreamMode.Write))
            using (var swEncrypt = new StreamWriter(csEncrypt))
            {
                swEncrypt.Write(text);
            }
            return Convert.ToBase64String(msEncrypt.ToArray());
        }

        static string Decrypt(string text, string key)
        {
            byte[] cipher = Convert.FromBase64String(text);
            byte[] btkey = Encoding.ASCII.GetBytes(key);
            RijndaelManaged aes128 = new()
            {
                Mode = CipherMode.ECB,
                Padding = PaddingMode.PKCS7
            };
            ICryptoTransform decryptor = aes128.CreateDecryptor(btkey, null);
            MemoryStream ms = new(cipher);
            CryptoStream cs = new(ms, decryptor, CryptoStreamMode.Read);
            byte[] plain = new byte[cipher.Length];
            int decryptcount = cs.Read(plain, 0, plain.Length);
            ms.Close();
            cs.Close();
            return Encoding.UTF8.GetString(plain, 0, decryptcount);
        }
    }
}

---

Hi, I have few projects that use AES encryption to decrypt data sent from webserver, with builds from 6.0.0-preview.5.21269.1 and earlier it works fine, but when updating to latest builds i get wrong decrypt result.

OS: Windows 10 20H2
OS Build: 19042.1083
SDK: .Net 5.0.3
ILCompiler version: Latest builds

Original text:
{"status":true,"key":"808ffc0a-0cda-4358-9f01-a28d2e3490db","token":"1QcSB5sqXv1AiBjduD8WV1o57MI4YpHnAKf3KTmlzPuumwUMyyXFg63hqohWyJEZ"}

Decrypt text:
{"status":true,"key":"808ffc0a-0cda-4358-9f01-a28d2e3490db","token":"1QcSB5sqXv1AiBjduD8WV1o57MI4YpHnAKf3KTmlzPuumwUMyyXFg63hqohWyJEZ
decryption result always missing characters after "

Encryption code:

Decryption code:

Is this a problem caused by the updates? I'm rolling back to the 6.0.0-preview.5.21269.1 build now so the app can work fine.
Any update on this, thanks

[jkotas]

Could you please make a program with Main method that demonstrates the problem? I am not sure what exact arguments to call the methods you have shared with to hit the problem.

This is likely a regression that we have picked up from dotnet/runtime.

[unofficialdev]

@jkotas I have created a project with full code here. you can check.

https://github.com/unofficialdev/AES_Encrypt

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Hi, I have few projects that use AES encryption to decrypt data sent from webserver, with builds from 6.0.0-preview.5.21269.1 and earlier it works fine, but when updating to latest builds i get wrong decrypt result.

OS: Windows 10 20H2
OS Build: 19042.1083
SDK: .Net 5.0.3
ILCompiler version: Latest builds

Original text:
{"status":true,"key":"808ffc0a-0cda-4358-9f01-a28d2e3490db","token":"1QcSB5sqXv1AiBjduD8WV1o57MI4YpHnAKf3KTmlzPuumwUMyyXFg63hqohWyJEZ"}

Decrypt text:
{"status":true,"key":"808ffc0a-0cda-4358-9f01-a28d2e3490db","token":"1QcSB5sqXv1AiBjduD8WV1o57MI4YpHnAKf3KTmlzPuumwUMyyXFg63hqohWyJEZ
decryption result always missing characters after "

Encryption code:

Decryption code:

Is this a problem caused by the updates? I'm rolling back to the 6.0.0-preview.5.21269.1 build now so the app can work fine.
Any update on this, thanks

Author:	unofficialdev
Assignees:	-
Labels:	area-System.Security, untriaged
Milestone:	-

[GrabYourPitchforks]

I suspect the problem is in this code, where the caller assumes Read will complete all at once rather than invoking the method in a loop.

            MemoryStream ms = new(cipher);
            CryptoStream cs = new(ms, decryptor, CryptoStreamMode.Read);
            byte[] plain = new byte[cipher.Length];
            int decryptcount = cs.Read(plain, 0, plain.Length);
            ms.Close();
            cs.Close();

Will investigate further.

[GrabYourPitchforks]

Yes, this is exactly what happened. In earlier versions of .NET, the Read method gives back a single 181-byte chunk which represents the entirety of the data. In recent versions, due to optimizations likely introduced by #53644, the Read method gives back two chunks: a 176-byte chunk followed by a 5-byte chunk. Even though it's a behavioral change, it is a perfectly valid implementation of the Read method, as the Read contract states that callers must invoke the method in a loop until it returns 0 to indicate EOF. This seems like the type of behavioral change which would be valid between major releases.

I am tempted to resolve this issue by design unless somebody has thoughts otherwise.

[bartonjs]

At most it might mean we want to call it out in the release notes (maybe a breaking change notice?). But, yeah, the error here is in the calling code for assuming that Read reads complete.

[GrabYourPitchforks]

Release notes seems appropriate. We can also mention the CanTransformMultipleBlocks optimization that Steve Toub checked in.

[xPaw]

This silent breaking change is not great. Just from a brief look I see plenty of code where Read() is called without looping, including a lot of Microsoft's own code.

I have a strong feeling that a lot of code will mysteriously break in .NET 6.

Should there be a code analysis out of the box that looks for Stream.Read() being used correctly?

[xPaw]

In recent versions, due to optimizations likely introduced by #53644, the Read method gives back two chunks: a 176-byte chunk followed by a 5-byte chunk.

I've looked more into this, and I don't really understand why this behavior needed to change. It seems that CryptoStream.Read() works in chunks of 16 bytes (EDIT: the chunk/block size is based on whatever transform decryptor uses), so if you give it 1440 bytes, it returns as expected. Adding one extra byte will require a second Read() call to get that single byte. This just feels like CryptoStream is missing a handler for the remaining <16 byte chunk.

Is there any good reason to not do that, to match the behavior of previous versions?

There's this code: https://github.com/stephentoub/runtime/blob/194ffff6de345631ad471417a3fe414891dc23a8/src/libraries/System.Security.Cryptography.Primitives/src/System/Security/Cryptography/CryptoStream.cs#L425-L429 which reads one block (when there's only one block to read), is that perhaps just not being used when it read multiple blocks above?

To me it looks like the loop doesn't run because of _inputBufferIndex, which is reset at the end of the method, and which is why next Read() call works.

I understand that this behavior was changed to be non blocking, but why not read until the end, if the data is there? (e.g. backed by a MemoryStream)

[stephentoub]

why not read until the end, if the data is there?

Because it's operating over an arbitrary Stream and has no idea whether the next call to Read{Async} will succeed immediately or not. Streams in general are supposed to return from Read once data is available; CryptoStream was failing to do that.

The breaking change is documented here:
https://docs.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/partial-byte-reads-in-streams
along with the reason for it.

[GrabYourPitchforks]

Looks like Steve already got the breaking change doc up. Closing.