Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AES with CipherMode.CFB and PaddingMode.PKCS7 incompatible between .NET Framework and .NET5.0 #46672

Closed
korny86 opened this issue Jan 7, 2021 · 12 comments · Fixed by #46686
Closed
Labels
area-System.Security untriaged New issue has not been triaged by the area owner

Comments

@korny86
Copy link

korny86 commented Jan 7, 2021

Description

I have a .NET Framework 4.6.2 program that encrypt/decrypt files and I tried to use the same encrypt/decrypt methods for a .NET5.0 console application. The code (below) compiles and works on both systems, but if I try to decrypt a file that is encrypted by the other system it is not compatible.

Encrypted with .NET Framework / Decrypted .NET Framework: Works fine.
Encrypted with .NET5.0 / Decrypted .NET5.0: Works fine.
Encrypted with .NET Framework/ Decrypted .NET5.0: Exception “Padding is invalid and cannot be removed”
Encrypted with .NET5.0 / Decrypted .NET Framework: Exception “Length of the data to decrypt is invalid”

I expected that the code in .NET Framework and .NET5.0 results in the same behaviour, but their seems to be differences.
Are there any properties to make the .NET5.0 project compatible? (The .NET Framework program is already released 2 years ago and changing the algorithm on that side is a bigger issue.)

Configuration

.NET Framework 4.6.2
.NET5.0

Windows 10, x86

Visual Studio 2019, V16.8.3

Regression?

Don't know. CipherMode.CFB is supported from .NET5.0 and should not work in older versions.

Other information

Decryption/Encryption Code Example:

/// <summary>
/// Decrypts an encrypted file with the FileEncrypt method through its path and the plain password.
/// </summary>
/// <param name="inputFile"></param>
/// <param name="outputFile"></param>
/// <param name="password"></param>
/// <returns>true if no error, false in all other cases</returns>
public static bool FileDecrypt(string inputFile, string outputFile, string password)
{
	bool returnValue = true; // init value must be true!
	byte[] passwordBytes = System.Text.Encoding.UTF8.GetBytes(password);
	byte[] salt = new byte[32];

	FileStream fsCrypt = new FileStream(inputFile, FileMode.Open);
	fsCrypt.Read(salt, 0, salt.Length);

	RijndaelManaged AES = new RijndaelManaged();
	AES.KeySize = 256;
	AES.BlockSize = 128;
	var key = new Rfc2898DeriveBytes(passwordBytes, salt, 50000);
	AES.Key = key.GetBytes(AES.KeySize / 8);
	AES.IV = key.GetBytes(AES.BlockSize / 8);
	AES.Padding = PaddingMode.PKCS7;
	AES.Mode = CipherMode.CFB;

	CryptoStream cs = new CryptoStream(fsCrypt, AES.CreateDecryptor(), CryptoStreamMode.Read);

	FileStream fsOut = new FileStream(outputFile, FileMode.Create);

	int read;
	byte[] buffer = new byte[1048576];

	try
	{
		while ((read = cs.Read(buffer, 0, buffer.Length)) > 0)
		{
			//Application.DoEvents();
			fsOut.Write(buffer, 0, read);
		}
	}
	catch (CryptographicException ex_CryptographicException)
	{
		returnValue = false;
		Console.WriteLine("CryptographicException error: " + ex_CryptographicException.Message);
	}
	catch (Exception ex)
	{
		returnValue = false;
		Console.WriteLine("Error: " + ex.Message);
	}

	try
	{
		cs.Close();
	}
	catch (Exception ex)
	{
		returnValue = false;
		Console.WriteLine("Error by closing CryptoStream: " + ex.Message);
	}
	finally
	{
		fsOut.Close();
		fsCrypt.Close();
	}
	return returnValue;
}

/// <summary>
/// Creates a random salt that will be used to encrypt your file. This method is required on FileEncrypt.
/// </summary>
/// <returns></returns>
private static byte[] GenerateRandomSalt()
{
	byte[] data = new byte[32];

	using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
	{
		for (int i = 0; i < 10; i++)
		{
			// Fill the buffer with the generated data
			rng.GetBytes(data);
		}
	}
	return data;
}

public static bool FileEncrypt(string inputFile, string outputFile, string password)
{
	bool returnValue = true; // init value must be true!
	//generate random salt
	byte[] salt = GenerateRandomSalt();

	//create output file name
	FileStream fsCrypt = new FileStream(outputFile, FileMode.Create);

	//convert password string to byte arrray
	byte[] passwordBytes = System.Text.Encoding.UTF8.GetBytes(password);

	//Set Rijndael symmetric encryption algorithm
	RijndaelManaged AES = new RijndaelManaged();
	AES.KeySize = 256;
	AES.BlockSize = 128;
	AES.Padding = PaddingMode.PKCS7;

	//"What it does is repeatedly hash the user password along with the salt." High iteration counts.
	var key = new Rfc2898DeriveBytes(passwordBytes, salt, 50000);
	AES.Key = key.GetBytes(AES.KeySize / 8);
	AES.IV = key.GetBytes(AES.BlockSize / 8);

	//Cipher mode
	AES.Mode = CipherMode.CFB;

	// write salt to the begining of the output file, so in this case can be random every time
	fsCrypt.Write(salt, 0, salt.Length);

	CryptoStream cs = new CryptoStream(fsCrypt, AES.CreateEncryptor(), CryptoStreamMode.Write);

	FileStream fsIn = new FileStream(inputFile, FileMode.Open);

	//create a buffer (1mb) so only this amount will allocate in the memory and not the whole file
	byte[] buffer = new byte[1048576];
	int read;

	try
	{
		while ((read = fsIn.Read(buffer, 0, buffer.Length)) > 0)
		{
			//Application.DoEvents(); // -> for responsive GUI, using Task will be better!
			cs.Write(buffer, 0, read);
		}

		// Close up
		fsIn.Close();
	}
	catch (Exception ex)
	{
		returnValue = false;
		Console.WriteLine("Error: " + ex.Message);
	}
	finally
	{
		cs.Close();
		fsCrypt.Close();
	}
	return returnValue;
}
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Jan 7, 2021
@ghost
Copy link

ghost commented Jan 7, 2021

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

Issue Details

Description

I have a .NET Framework 4.6.2 program that encrypt/decrypt files and I tried to use the same encrypt/decrypt methods for a .NET5.0 console application. The code (below) compiles and works on both systems, but if I try to decrypt a file that is encrypted by the other system it is not compatible.

Encrypted with .NET Framework / Decrypted .NET Framework: Works fine.
Encrypted with .NET5.0 / Decrypted .NET5.0: Works fine.
Encrypted with .NET Framework/ Decrypted .NET5.0: Exception “Padding is invalid and cannot be removed”
Encrypted with .NET5.0 / Decrypted .NET Framework: Exception “Length of the data to decrypt is invalid”

I expected that the code in .NET Framework and .NET5.0 results in the same behaviour, but their seems to be differences.
Are there any properties to make the .NET5.0 project compatible? (The .NET Framework program is already released 2 years ago and changing the algorithm on that side is a bigger issue.)

Configuration

.NET Framework 4.6.2
.NET5.0

Windows 10, x86

Visual Studio 2019, V16.8.3

Regression?

Don't know. CipherMode.CFB is supported from .NET5.0 and should not work in older versions.

Other information

Decryption/Encryption Code Example:

/// <summary>
/// Decrypts an encrypted file with the FileEncrypt method through its path and the plain password.
/// </summary>
/// <param name="inputFile"></param>
/// <param name="outputFile"></param>
/// <param name="password"></param>
/// <returns>true if no error, false in all other cases</returns>
public static bool FileDecrypt(string inputFile, string outputFile, string password)
{
	bool returnValue = true; // init value must be true!
	byte[] passwordBytes = System.Text.Encoding.UTF8.GetBytes(password);
	byte[] salt = new byte[32];

	FileStream fsCrypt = new FileStream(inputFile, FileMode.Open);
	fsCrypt.Read(salt, 0, salt.Length);

	RijndaelManaged AES = new RijndaelManaged();
	AES.KeySize = 256;
	AES.BlockSize = 128;
	var key = new Rfc2898DeriveBytes(passwordBytes, salt, 50000);
	AES.Key = key.GetBytes(AES.KeySize / 8);
	AES.IV = key.GetBytes(AES.BlockSize / 8);
	AES.Padding = PaddingMode.PKCS7;
	AES.Mode = CipherMode.CFB;

	CryptoStream cs = new CryptoStream(fsCrypt, AES.CreateDecryptor(), CryptoStreamMode.Read);

	FileStream fsOut = new FileStream(outputFile, FileMode.Create);

	int read;
	byte[] buffer = new byte[1048576];

	try
	{
		while ((read = cs.Read(buffer, 0, buffer.Length)) > 0)
		{
			//Application.DoEvents();
			fsOut.Write(buffer, 0, read);
		}
	}
	catch (CryptographicException ex_CryptographicException)
	{
		returnValue = false;
		Console.WriteLine("CryptographicException error: " + ex_CryptographicException.Message);
	}
	catch (Exception ex)
	{
		returnValue = false;
		Console.WriteLine("Error: " + ex.Message);
	}

	try
	{
		cs.Close();
	}
	catch (Exception ex)
	{
		returnValue = false;
		Console.WriteLine("Error by closing CryptoStream: " + ex.Message);
	}
	finally
	{
		fsOut.Close();
		fsCrypt.Close();
	}
	return returnValue;
}

/// <summary>
/// Creates a random salt that will be used to encrypt your file. This method is required on FileEncrypt.
/// </summary>
/// <returns></returns>
private static byte[] GenerateRandomSalt()
{
	byte[] data = new byte[32];

	using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider())
	{
		for (int i = 0; i < 10; i++)
		{
			// Fill the buffer with the generated data
			rng.GetBytes(data);
		}
	}
	return data;
}

public static bool FileEncrypt(string inputFile, string outputFile, string password)
{
	bool returnValue = true; // init value must be true!
	//generate random salt
	byte[] salt = GenerateRandomSalt();

	//create output file name
	FileStream fsCrypt = new FileStream(outputFile, FileMode.Create);

	//convert password string to byte arrray
	byte[] passwordBytes = System.Text.Encoding.UTF8.GetBytes(password);

	//Set Rijndael symmetric encryption algorithm
	RijndaelManaged AES = new RijndaelManaged();
	AES.KeySize = 256;
	AES.BlockSize = 128;
	AES.Padding = PaddingMode.PKCS7;

	//"What it does is repeatedly hash the user password along with the salt." High iteration counts.
	var key = new Rfc2898DeriveBytes(passwordBytes, salt, 50000);
	AES.Key = key.GetBytes(AES.KeySize / 8);
	AES.IV = key.GetBytes(AES.BlockSize / 8);

	//Cipher mode
	AES.Mode = CipherMode.CFB;

	// write salt to the begining of the output file, so in this case can be random every time
	fsCrypt.Write(salt, 0, salt.Length);

	CryptoStream cs = new CryptoStream(fsCrypt, AES.CreateEncryptor(), CryptoStreamMode.Write);

	FileStream fsIn = new FileStream(inputFile, FileMode.Open);

	//create a buffer (1mb) so only this amount will allocate in the memory and not the whole file
	byte[] buffer = new byte[1048576];
	int read;

	try
	{
		while ((read = fsIn.Read(buffer, 0, buffer.Length)) > 0)
		{
			//Application.DoEvents(); // -> for responsive GUI, using Task will be better!
			cs.Write(buffer, 0, read);
		}

		// Close up
		fsIn.Close();
	}
	catch (Exception ex)
	{
		returnValue = false;
		Console.WriteLine("Error: " + ex.Message);
	}
	finally
	{
		cs.Close();
		fsCrypt.Close();
	}
	return returnValue;
}
Author: korny86
Assignees: -
Labels:

area-System.Security, untriaged

Milestone: -

@vcsjones
Copy link
Member

vcsjones commented Jan 7, 2021

Thanks for the repro. I'll take a look.

@vcsjones
Copy link
Member

vcsjones commented Jan 7, 2021

Okay, there are a number of things that I am seeing. Starting with the first, it appears that RijndaelManaged in .NET 5 does strange things with the CFB FeedbackSize. FeedbackSize will be 128, however what actually ends up getting used is CFB8, not CFB128. Simpler repro:

using System;
using System.Security.Cryptography;

using var aes = new RijndaelManaged();
aes.Key = new byte[32];
aes.IV = new byte[16];
aes.Mode = CipherMode.CFB;
Console.WriteLine(aes.FeedbackSize); //Prints 128


byte[] input = System.Text.Encoding.UTF8.GetBytes("hello");
using var transform = aes.CreateEncryptor();
byte[] ciphertext = transform.TransformFinalBlock(input, 0, input.Length);
Console.WriteLine(ciphertext.Length); // Prints 6???

Even though we are supposedly in CFB128 mode, the resulting ciphertext is 6. So 5 bytes of input and 1 byte of padding = 6.

This may or may not be part of the issue at hand, but, it's something I've noticed that so far looks like incorrect behavior.

This is because the Rijndael class sets the underlying feedback size value field to the block size:

But the RijndaelImplementation wrapper doesn't override FeedbackSize to defer to _impl here:

@vcsjones
Copy link
Member

vcsjones commented Jan 7, 2021

@korny86

In your .NET 5 code, if you replace:

RijndaelManaged AES = new RijndaelManaged();

with:

Aes AES = Aes.Create();
AES.FeedbackSize = 128;

I think things should be interoperable now. Can you please try that and let us know if that fixes it?

@vcsjones
Copy link
Member

vcsjones commented Jan 7, 2021

@bartonjs For .NET 5/6 the RijndaelManaged class's behavior is odd and wonder what you think an appropriate fix would be. I don't think the current behavior is desirable - the class says it's using CFB128 but that is not what is happening. Options:

  1. Change RijndaelManaged to use CFB8 by default since that is actually what it is using, and FeedbackSize will return 8. This preserves compatibility with .NET 5 CFB encrypted data with the RinjdaelManaged class but now the default feedback size from .NET 5 and .NET Framework differ for RinjdaelManaged.
  2. Change RijnaelManaged to use CFB128 by default since that was probably the intended behavior. This preserves compat with .NET Framework but is a break from .NET 5.

@jakubbogacz
Copy link

I can confirm, that in my case, using Aes directly fixes the issue. BTW. I'm not sure, but for me, a lot of things in RijndaelManaged looks odd ;)

image
(aes variable is my RijdnaelManaged object)

@vcsjones
Copy link
Member

vcsjones commented Jan 7, 2021

@jakubbogacz Yeah, the protected fields in these classes are a little weird. However, since RijndaelManaged is sealed, I don't think the protected fields need to be kept in sync. The "source of truth" for RijndaelManaged is the _impl that it is wrapping.

The fields there are just a byproduct of existing on SymmetricAlgorithm.

@bartonjs
Copy link
Member

bartonjs commented Jan 7, 2021

This seems like Rijndael's version of #43234, where we decided "match Framework".

@vcsjones
Copy link
Member

vcsjones commented Jan 7, 2021

This seems like Rijndael's version of #43234, where we decided "match Framework".

Yeah. In addition to that, the FeedbackSize property on RindaelManaged and RijndaelImplementation is effectively broken.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 7, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 7, 2021
@korny86
Copy link
Author

korny86 commented Jan 8, 2021

@korny86

In your .NET 5 code, if you replace:

RijndaelManaged AES = new RijndaelManaged();

with:

Aes AES = Aes.Create();
AES.FeedbackSize = 128;

I think things should be interoperable now. Can you please try that and let us know if that fixes it?

I checked it and it works fine.

Thank you @vcsjones @bartonjs and @jakubbogacz for the fast reply and your help.

One last question: In which Version of .NET will the bugfix of RijndaelManaged published? .NET5.0.2 or .NET6.0?

@vcsjones
Copy link
Member

vcsjones commented Jan 8, 2021

@korny86 the fix has been merged for 6.0. #46713 is to fix it for 5.0 - but it has not been approved by servicing yet. When / if it gets approved and merged for 5.0, we'll know what release it will be in.

The solution I posted is the recommended way forward - the rijndael types exist for compatibility, but using the Aes factory method is the "correct" way in .NET Core and 5.

@vcsjones
Copy link
Member

vcsjones commented Jan 17, 2021

@korny86 just to follow up on the 5.0 port, it looks like the fix landed in 5.0.3.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security untriaged New issue has not been triaged by the area owner
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants