CipherMode.CFB does not Encrypt the same in .NET 5.0 RC1 as in .NET 4.7
Roneri75 opened this issue · 17 comments
When using CipherMode.CFB as Mode in the System.Security.Cryptography.TripleDES to Encrypt e.g. a password you don't get the same encryption string back in .NET 5.0 RC1 as you get in .NET 4.7.
We have used code like this since .net 4.5 and it's worked exactly the same since, i should work exactly the same in .NET 5.0 as in 4.x or all apps using this for e.g. saving password encrypted will stop working after upgrade to
.NET 5.
I did a test console app to reproduce this, it's the exact the same code for both .NET 5.0 RC1 as for .NET 4.7.
Both apps where created in VS 2019 Preview (16..8.0 Preview 3.2).
And they do not generate the same result.
using System;
using System.Security.Cryptography;
using System.Text;
namespace TestConsole.net5
{
class Program
{
static void Main(string[] args)
{
Console.WriteLine("Hello, Typ in you text to be Encrypted!");
string text = Console.ReadLine();
TripleDESHelper tripleDESHelper = new TripleDESHelper();
string encryptedtext = tripleDESHelper.Encrypt(text);
Console.WriteLine("Your encrypted text results in this: " + encryptedtext);
Console.ReadLine();
}
}
public class TripleDESHelper
{
private readonly byte[] KEY_BYTES = { 2, 24, 1, 114, 15, 74, 23, 43, 81, 23, 87, 34, 55, 72, 32, 34 };
private readonly byte[] IV_BYTES = { 90, 12, 54, 27, 62, 3, 45, 23 };
private TripleDES tripleDES;
public TripleDESHelper()
{
tripleDES = TripleDESCryptoServiceProvider.Create();
tripleDES.Key = KEY_BYTES;
tripleDES.IV = IV_BYTES;
// Used for small chunck of data. In this one, we use it for user's password.
tripleDES.Mode = CipherMode.CFB;
}
/// <summary>
/// Encrypts the given plain text string into an encrypted, base 64 encoded value; using the Triple Data Encryption Standard algorithms
/// </summary>
/// <param name="clearText">The plain text.</param>
/// <returns>A string representing the encrypted text.</returns>
public string Encrypt(string clearText)
{
if (tripleDES == null || String.IsNullOrEmpty(clearText))
return String.Empty;
String encryptedText = String.Empty;
ICryptoTransform encryptor = tripleDES.CreateEncryptor();
byte[] textBytes = Encoding.UTF8.GetBytes(clearText);
encryptedText = System.Convert.ToBase64String(encryptor.TransformFinalBlock(textBytes, 0, textBytes.Length));
return encryptedText;
}
/// <summary>
/// Decrypts the given cipher text string into an plain text using the Triple Data Encryption Standard algorithms
/// </summary>
/// <param name="cipherText">The cipher text.</param>
/// <returns>A string representing the plain text</returns>
public string Decrypt(string cipherText)
{
if (tripleDES == null || String.IsNullOrEmpty(cipherText))
return String.Empty;
String decryptedText = String.Empty;
byte[] encryptedBytes = System.Convert.FromBase64String(cipherText);
ICryptoTransform decryptor = tripleDES.CreateDecryptor();
decryptedText = Encoding.UTF8.GetString(decryptor.TransformFinalBlock(encryptedBytes, 0, encryptedBytes.Length));
return decryptedText;
}
}
}Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @jeffhandley
See info in area-owners.md if you want to be subscribed.
It looks like there are two things:
net5.0defaults to a FeedbackSize of 64, andnet48defaults to a feedback size of 8.net5.0applies padding a little differently thannet48.
If you explicitly set the FeedbackSize, then the results are the same, minus the padding differences.
@bartonjs assuming you want something fixed here, how likely is it that it could make it in to RTM?
Ah, I see. TripleDES sets FeedBackSize to the block size, but TripleDESCryptoServiceProvider sets it to 8. So the data would be inconsistent (CFB8 vs CFB64) in the .NET Framework application if someone registered to CryptoConfig that TripleDESCng should be the default 3DES provider. The app would also work if it used new TripleDESCryptoServiceProvider(). (It looks like it's creating a TripleDESCryptoServiceProvider directly, but there's no TripleDESCryptoServiceProvider.Create method, it's falling back to TripleDES.Create(), and thus getting whatever CryptoConfig says the right answer is for 3DES)
I think that this is just an unfortunate circumstance of the type hierarchy for the crypto types here. The only change to consider would be making the TripleDESImplementation class default to FeedbackSize = 8 so that TripleDES.Create().FeedbackSize produced the same value across a clean .NET Framework installation execution and a .NET 5 execution.
@danmosemsft How would you feel about taking a change to 5.0 that changed the default value of TripleDES.Create().FeedbackSize to match .NET Framework? It pretty much only affects .NET 5, since it only impacts CFB mode, which we didn't bring back to Core until 5. So it's a "no existing .NET Core applications should be depending on it, but it makes .NET 5 slightly more compatible with .NET Framework than it otherwise would have been" change. I think it's a reasonable concession to compatibility; but I'm way happier changing it for 5 than for 6, so I don't think we would change it in master unless it was already likely to get approved; because then we're deciding if we wanted to be more compatible with .NET 5 or .NET Framework.
@bartonjs another issue is padding. It looks like for CFB8, we are on padding to the feedback size (so 1 byte of padding is added). In .NET Framework we are padding to the whole block (nearest 8 for 3DES).
I don't think that will impact padding removal and the data will interop as long as normal padding removal is used.
Yeah, I guess my willingness to take the FeedbackSize default change is predicated on it meaning that a value encrypted in .NET Framework can be decrypted in .NET 5, and vice-versa (even though they compute different padding).
I'll work on this over the weekend and you can decide if you want to port to release/5.0.
Thanks, @vcsjones. I so owe you a cup of coffee for the help you've given while I've been juggling multiple projects.
@bartonjs so if I understand right
- one value is much as good as the other, ie., we don't lose anything
- it would make us meaningfully more (ie., code didn't prevoiusly work) compatible with .NET Framework
- it would not break any customers coming from 3.1 because the mode didn't exist then
- we have only one customer report but it's also the case that we haven't actually shipped yet
- you believe the risk of the change is low or very low?
If so this sounds like worth taking to tactics for approval, but note @vcsjones we won't know their decision before Monday, in case that influences whether you want to spend time on it 😃
@danmosemsft Yeah, that's an accurate summary.
@vcsjones My psychic diagnostic powers believe the whole change should be to add
public TripleDESImplementation()
{
// Default CFB to CFB8 to match .NET Framework's default for TripleDES.Create()
FeedbackSizeValue = 8;
}around hereish
and a "set the minimum number of properties and make sure we can read data from netfx, even though we produce different data in the 'ignore me' bytes" test.
Hopefully yours agrees. I'm eager to find out how close my predictive powers were to reality.
I will guess 80% chance they take it..
I so owe you a cup of coffee
Sold!
@bartonjs You were right about the feedback size. Padding remains an interesting case.
The .NET Framework expects the ciphertext to be padded to the block size, even with CFB8. Otherwise it will blow up. As the way things are today, .NET 5 pads CFB8 data in a way that .NET Framework cannot de-pad (unless you get lucky).
This will work in .NET 5 and fail in .NET Framework:
byte[] ciphertext = Convert.FromBase64String("Vx8GlhTi");
using var tripleDes = TripleDES.Create();
tripleDes.FeedbackSize = 8;
tripleDes.Mode = CipherMode.CFB;
tripleDes.Key = Convert.FromBase64String("aaHRNTLQh++TK3lhShiicCPLQ6IO0YVH");
tripleDes.IV = Convert.FromBase64String("OQHTwYDPlMo=");
using var transform = tripleDes.CreateDecryptor();
Console.WriteLine(Encoding.UTF8.GetString(transform.TransformFinalBlock(ciphertext, 0, ciphertext.Length)));The reverse is not a problem: we can de-pad .NET Framework encrypted data in .NET 5.
@bartonjs The padding size thing is a bit thorny if we want to start encrypting with full blocks of padding. We might break something for someone going from RC to RTM.
Here are the options, as I figure.
-
We do nothing. This means that CFB8 encrypted data from .NET 5 will not decrypt in .NET Framework if padding is being used.
-
We go back to .NET Framework behavior. This will break anything encrypted with CFB8 and padding from RC going to RTM.
-
We try some kind of hybrid approach.
- Allow CFB8 decryption in .NET 5 to support arbitrary padding lengths, or "1" or "block size" padding lengths.
- Change CFB8 encryption in .NET 5 to always pad to a full block.
This will permit .NET 5 ciphertexts to be read by .NET Framework, and will continue to allow decrypting 1-octet padding sizes as well. This raises the question, "what does this mean for
PaddingMode.None?". This is also perhaps a large change for this late in the .NET 5 cycle.
@danmosemsft there are two potential .NET Framework compatibility breaks here. The first one that was originally discussed is fairly simple and aligns with your initial assessment. That one I still think is worth taking and discussing with "tactics" on Monday. That is fixed in #43259 and that is the only thing in that PR.
The reverse is not a problem: we can de-pad .NET Framework encrypted data in .NET 5.
Then I think at this point we stick with "We do nothing." (for padding, vs feedback size). Upgrading succeeds (old data can be read), but side-by-side is hampered (since old programs can't read new program data). Callers who need the padding to be there can do their encrypts with no padding and manually append the padding data. It's not great, but the workaround is required in the newer application, so it's at least not something that requires a time machine.
cc @jeffhandley as it's 5.0.