dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

TripleDES is not allowing 16 byte keys #17811

Closed awatertree closed 4 years ago

awatertree commented 8 years ago

This works on full framework, but not core. This is a simple alteration of the dotnet new to isolate the problem (only other change is to add net452 to project.json).

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

namespace ConsoleApplication
{
    public class Program
    {
        public static void Main(string[] args)
        {
            var d = TripleDES.Create();
            d.Key = ASCIIEncoding.ASCII.GetBytes("1234567890123456");
            Console.WriteLine("Hello World!");
        }
    }
}

Running the above under net452 works fine, but under netcoreapp10 results in:

Unhandled Exception: System.Security.Cryptography.CryptographicException: Specified key is not a valid size for this alg
orithm.
   at System.Security.Cryptography.SymmetricAlgorithm.set_Key(Byte[] value)
   at System.Security.Cryptography.TripleDES.set_Key(Byte[] value)
   at ConsoleApplication.Program.Main(String[] args)
bartonjs commented 8 years ago

@AtsushiKan I remember there was some reason why we didn't add two-key 3DES. Can you remind me?

ghost commented 8 years ago

This is an underlying CNG restriction (see comment at https://github.com/AtsushiKan/corefx/blob/master/src/System.Security.Cryptography.Cng/src/System/Security/Cryptography/TripleDESCng.cs#L101) . On the full framework. TripleDes.Create() returns the CSP provider by default – on netcore, it returns the Cng provider.

So the workaround would be to use new TripleDesCryptoServiceProvider()

morganbr commented 8 years ago

We should also evaluate the security of exposing two-key 3DES (via CSP or CNG).

karelz commented 7 years ago

It should be 3 lines of code - anyone wants to grab it?

morganbr commented 7 years ago

@karelz the security review is the expensive/concerning part of this. Two-key 3DES is significantly less secure than three-key. NIST suggests it has 80 bits of security, which is generally considered close to breakable; That paper also suggests the algorithm lifetime ended in 2010.

bartonjs commented 7 years ago

If it works for TripleDES.Create() out of the box on Desktop, it should work out of the box on Core.

geasi commented 7 years ago

As proposed by Damien_The_Unbeliever in this post (https://stackoverflow.com/questions/39013264/tripledes-16-byte-not-working/39013863#39013863), I've repeated the first 8 bytes to form a valid 24 bytes key and it worked. This is a work around but works for now.

danmoseley commented 7 years ago

@karelz seems this will not make 2.0 and therefore should go in our list of upgrade issues you have?

karelz commented 7 years ago

@steveharter how much work is it to implement this? If it is cheap, we should do it in 2.0.

karelz commented 7 years ago

Moving to 2.0 to make full assessment.

steveharter commented 7 years ago

@karelz yes this is relatively cheap (1 day). The work includes: 1) Change TripleDES LegalKeySizesValue to allow 16 bytes 2) Intercept the setting of the key and if 16 bytes, make it 24 by adding the first 8 bytes 3) Modify existing tests as appropriate 4) Add new tests including ones that compare against literal values taken from netfx to ensure the results are consistent with corefx

karelz commented 7 years ago

OK, can you please do it @steveharter by EOW in rel/2.0.0 branch? (before Escrow starts)

steveharter commented 7 years ago

I am currently assuming we want to support this in the the S.S.C.Cng assembly as well meaning direct use of the TripleDESCng class would work the same as using TripleDES.Create() or TripleDESCryptoServiceProvider.

Pros: consistency with S.S.C.Algorithms and .Csp; shared tests Cons: the Cng assembly typically is a light wrapper on top of the native Cng functionality

karelz commented 7 years ago

Reopening for port into rel/2.0.0 branch. @steveharter can you please start the process? (email to shiproom, get the rel/2.0.0 branch PR ready) Thanks!

steveharter commented 7 years ago

Merged into rel/2.0.0