Read all available data in CryptoStream.Read()

[xPaw]

This appears to be a breaking change in .NET 6 caused by dotnet/runtime#53644

dotnet/runtime#55527 (comment)

[JustArchi]

My repro from #1007 in case somebody wanted to reliably test the patch:

private const string EncryptionKey = "zmkXyWQzRHiameIe";

private static string? Decrypt(string encryptedString) {
	if (string.IsNullOrEmpty(encryptedString)) {
		throw new ArgumentNullException(nameof(encryptedString));
	}

	byte[] key;

	using (SHA256 sha256 = SHA256.Create()) {
		key = sha256.ComputeHash(EncryptionKey);
	}

	byte[] decryptedData = Convert.FromBase64String(encryptedString);
	decryptedData = CryptoHelper.SymmetricDecrypt(decryptedData, key);

	return Encoding.UTF8.GetString(decryptedData);
}

private static string? Encrypt(string decryptedString) {
	if (string.IsNullOrEmpty(decryptedString)) {
		throw new ArgumentNullException(nameof(decryptedString));
	}

	byte[] key;

	using (SHA256 sha256 = SHA256.Create()) {
		key = sha256.ComputeHash(EncryptionKey);
	}

	byte[] encryptedData = Encoding.UTF8.GetBytes(decryptedString);
	encryptedData = CryptoHelper.SymmetricEncrypt(encryptedData, key);

	return Convert.ToBase64String(encryptedData);
}

private static int Main() {
	const string decrypted = "qF7Sr1RfOZ9K9bfTXfl3";

	Console.WriteLine(decrypted);

	var encrypted = Encrypt(decrypted);

	Console.WriteLine(encrypted);

	var decrypted2 = Decrypt(encrypted);

	Console.WriteLine(decrypted2);

	return decrypted == decrypted2 ? 0 : 1;
}

net48 and net5.0 targets:

qF7Sr1RfOZ9K9bfTXfl3
XC5d5iL8ZoejDjr5IaQeV1cpcHvd3Wr3vojo8yBI9snRwcADq3xb2TQ5Cz3nOBGd
qF7Sr1RfOZ9K9bfTXfl3
(exit code 0)

net6.0 (without patch):

qF7Sr1RfOZ9K9bfTXfl3
npQePt1w6VAiTRS9NCbxIeBpvITRNHujrH0niQ/8XKDmiwhbUUVVbGxSWfjT4SpC
qF7Sr1RfOZ9K9bfT
(exit code 1)

[JustArchi]

🚢 it

[JustArchi]

Reposting my thoughts from Discord:

https://docs.microsoft.com/en-us/dotnet/api/system.io.stream.read?view=net-5.0

An implementation is free to return fewer bytes than requested even if the end of the stream has not been reached.

We should include this change even if .NET folks somehow decide to revert this breaking change in .NET 6.0. The MSDN docs explicitly state that the implementation can return fewer bytes than requested, so even if it decides not to, it should be classified as bullet-proofing for the future.

To me this is one of those places where the code was wrong to begin with but it somehow worked, therefore it's IMHO worth it to fix it before everybody gets hit in the head just because .NET guys decided to enforce what the documentation has always stated.

[xPaw]

I was making a test case and found this behaviour:

CryptoStream.Read returns full data if the data length is divisible by 16. If it's not, then the last remaining chunk won't be returned and requires a second Read call.

For example for for strings with length of 16, 32, and even 1440 it returns correctly. But If I add one character, it will require the second Read() call to get that remaining character.

[JustArchi]

@yaakov-h @azuisleet if it doesn't clash with anything I'd appreciate Alpha.3 with those changes so we could test .NET 6.0 compatibility on official release, thank you in advance for considering it.

[yaakov-h]

I'll have a look at that tonight.

[JustArchi]

RIP release https://github.com/SteamRE/SteamKit/actions/runs/1078301158

[yaakov-h]

I’ve updated the secret but want to recheck a couple PRs before I resubmit the job.