dotnet / runtime

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

[API Proposal] Expand ExchangeAlgorithmType, CipherAlgorithmType, HashAlgorithmType #100361

Open rzikm opened 3 months ago

rzikm commented 3 months ago

Background and motivation

Enums ExchangeAlgorithmType, CipherAlgorithmType and HashAlgorithmType haven't been updated in a long time and code which uses them (SslStream) sometimes even returns values which do not map to existing members. See e.g. https://github.com/dotnet/runtime/issues/55570. Similarly, many algorithms/ciphers belonging to the same general family are being mapped to the same enum member, discarding information in the process.

Since the expected use of these properites is mainly logging for auditing purposes, it makes sense to report more specific information.

API Proposal

This proposal adds missing members so that we are on par with

https://github.com/dotnet/runtime/blob/f92b9ef636a4cc9599b33446e30e7a489591ca46/src/libraries/System.Net.Security/src/System/Net/Security/TlsCipherSuiteNameParser.ttinclude#L11-L71

namespace System.Security.Authentication
{
    public enum ExchangeAlgorithmType
    {
        // existing members
        None = 0,
        RsaSign = 9216, // note: Not used by TlsCipherSuiteNameParser
        RsaKeyX = 41984,
        DiffieHellman = 43522, // the code is for Diffie-Hellman ephemeral kex

        // values chosen to match values from wincrypt
+       DiffieHellmanStatic = 0xaa01,
+       DiffieHellmanEphermal = DiffieHellman,
+       ECDiffieHellman 0xaa05,
+       ECDiffieHellmanEphermal = 0xaa06,

        // following are not present in wincrypt.h on which numerical values are based
        // are assigned values ok?
+       Kerberos5 = 1,
+       PSK,
+       SRP,
+       ECCPWD,
    }

    public enum CipherAlgorithmType
    {
        // existing members
        None = 0,
        Null = 24576,
        Des = 26113,
        Rc2 = 26114,
        TripleDes = 26115,
        Aes128 = 26126,
        Aes192 = 26127,
        Aes256 = 26128,
        Aes = 26129,
        Rc4 = 26625,

        //  wincrypt does not tell us difference between GCM and CCM?
+       AesGcm = 1,
+       AesCcm,
+       Aes128Gcm,
+       Aes256Gcm,
+       Aes128Ccm,
+       Aes128Ccm8,
+       Aes256Ccm,
+       Aes256Ccm8,

        //  No algorithm identifier in wincrypt.h, assign arbitrary values
+       Camellia,
+       Camellia128,
+       Camellia256,
+       Camellia128Gcm,
+       Camellia256Gcm,
+       ChaCha20,
+       ChaCha20Poly1305,
+       Seed,
+       Idea,
+       Aria,
+       Aria128,
+       Aria256,
+       Aria128Gcm,
+       Aria256Gcm,
    }

    public enum HashAlgorithmType
    {
        // existing members
        None = 0,
        Md5 = 32771,
        Sha1 = 32772,
        Sha256 = 32780,
        Sha384 = 32781,
        Sha512 = 32782,

        // No algorithm identifier in wincrypt.h
+       Aead = 1,
    }
}

API Usage

The values are expected to be used mainly for logging and audit purposes.

static void DisplaySecurityLevel(SslStream stream)
{
   Console.WriteLine("Cipher: {0} strength {1}", stream.CipherAlgorithm, stream.CipherStrength);
   Console.WriteLine("Hash: {0} strength {1}", stream.HashAlgorithm, stream.HashStrength);
   Console.WriteLine("Key exchange: {0} strength {1}", stream.KeyExchangeAlgorithm, stream.KeyExchangeStrength);
   Console.WriteLine("Protocol: {0}", stream.SslProtocol);
}

Alternative Designs

The above mentioned enum types are only used on properties of SslStream where they expose information about the negotiated TLS cipher suite. All information can be deduced from the SslStream.TlsCipherSuite so another option is to obsolete all of

And leave TlsCipherSuite SslStream.NegotiatedCipherSuite as the only source of truth.

Risks

If -- in the future -- Windows adds ALG_ID for algorithms we assigned an arbitrary value, the values will no longer be in sync. However, we plan to mitigate this by using the lookup table from

https://github.com/dotnet/runtime/blob/f92b9ef636a4cc9599b33446e30e7a489591ca46/src/libraries/System.Net.Security/src/System/Net/Security/SslConnectionInfo.Unix.cs#L41

on all platforms for consistency between platforms (to fix https://github.com/dotnet/runtime/issues/37578).

dotnet-policy-service[bot] commented 3 months ago

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

rzikm commented 3 months ago

cc @wfurt, @bartonjs

Let me know if you have any comments/concerns, if not I will put it to the API review queue.

wfurt commented 3 months ago

I'm wondering if we should bump the algorithms wincrypt does not have up high so there is less likelihood it will conflict with wincrypt in the future. We did something similar to AddressFamily

Also do we need to split the ephemeral vs static? I'm not sure the difference matters and we can always map either one to the base. For anybody who really needs to know exactly perhaps the TlsCipherSuite is the ultimate answer.

I'll probably lean toward recommendation from @vcsjones and @bartonjs

rzikm commented 2 months ago

@vcsjones, @bartonjs do you have any comments/recommendations here?

bartonjs commented 2 months ago

@bartonjs do you have any comments/recommendations here?

Not really. I can understand the thinking behind why these types exist, but I don't really understand their "purpose". Now that .NET exposes the ciphersuite value there's not much reasoning for these, other than they already exist.

I'm wondering if we should bump the algorithms wincrypt does not have up high so there is less likelihood it will conflict with wincrypt in the future.

If the data on all platforms is already driven by ciphersuite ID, then I wouldn't bother caring who gets what number. If Windows is still pass-through and you want to maintain that for as long as possible, then picking unlikely numbers has merit. Just beware that every now and then Windows comes back and surprises you by adding an algorithm that seemed impossible previously; and if you've already added it to this enum that'll force a mapping (plus have the confusion in the meantime where Windows and non-Windows use different values for the same thing).

OK, so I guess I do have one recommendation: ditch the Windows passthrough and drive everything off of ciphersuite. (Unless the ciphersuite is backformed on Windows from these breakdown products... I guess...)

do we need to split the ephemeral vs static?

It looks like Windows already has been (at least, ECDHE vs ECDH seem different, but DH and DHE are maybe the same?), so probably worth maintaining.

wfurt commented 2 months ago

OK, so I guess I do have one recommendation: ditch the Windows passthrough and drive everything off of ciphersuite. (Unless the ciphersuite is backformed on Windows from these breakdown products... I guess...)

we talk about it. And also sharing it with asp.net for Quic. AFAIK most users use this just as convenience for audits.

wfurt commented 2 months ago

and TlsCipherSuite is not available in netstandard & Framework so it is hassle for certain user base

rzikm commented 2 months ago

I can understand the thinking behind why these types exist, but I don't really understand their "purpose". Now that .NET exposes the ciphersuite value there's not much reasoning for these, other than they already exist.

AFAICT they exist only for logging purposes, so we feel we should either make them up to date or obsolete them and lead users to the TlsCipherSuite (which as wfurt mentioned is not part of .NET Standard, and, if it changes anything further, is apparently not CLS compliant).

If the data on all platforms is already driven by ciphersuite ID, then I wouldn't bother caring who gets what number.

OK, so I guess I do have one recommendation: ditch the Windows passthrough and drive everything off of ciphersuite. (Unless the ciphersuite is backformed on Windows from these breakdown products... I guess...)

That is the intention, I wanted to do that long time ago in https://github.com/dotnet/runtime/pull/66702, and I can ressurect it once we get this in.

rzikm commented 2 months ago

Since there have not been any major concerns, let's queue this for review.