Use BCL ECDiffieHellman for KeyExchange instead of BouncyCastle (.NET 8.0 onward only)

[scott-xu]

This PR leverages BCL's ECDiffieHellman for key exchange instead of using BouncyCastle.
Cryptographic operations in BCL are done by operating system (OS) libraries.

Advantages:

It inherits the advantages described in https://learn.microsoft.com/en-us/dotnet/standard/security/cross-platform-cryptography

• .NET apps benefit from OS reliability. Keeping cryptography libraries safe from vulnerabilities is a high priority for OS vendors. To do that, they provide updates that system administrators should be applying.
• .NET apps have access to FIPS-validated algorithms if the OS libraries are FIPS-validated.

Notes:

1. It is only for .NET 8.0+ because method ECDiffieHellman.DeriveRawSecretAgreement(ECDiffieHellmanPublicKey) is avaliable only at .NET 8.0+. See [API Proposal]: Add ECDiffieHellman.DeriveSecretAgreement method dotnet/runtime#71613.
2. It will throw PlatformNotSupportedException if the OS is Windows and below Windows 10. See this line of code. For this case, we use BouncyCastle as fallback. Based on below lifecycle table, I think it should be fine to throw PlatformNotSupportedException in Windows 8.1 and Windows Server 2012 R2 rather than use BouncyCastle as fallback. Then we can remove the BouncyCastle dependency when target .NET 8.0 onward.

[scott-xu]

Windows 8.1 and Windows Server 2012 R2 does not support ECDiffieHellman.DeriveRawSecretAgreement(ECDiffieHellmanPublicKey).
Here's their lifecyle for reference.

Windows 8.1 Lifecycle

Support Dates

Listing	Start Date	Mainstream End Date	Extended End Date
Windows 8.1	Nov 13, 2013	Jan 9, 2018	Jan 10, 2023

Windows 8.1 EOL announcement: https://learn.microsoft.com/en-us/lifecycle/announcements/windows-8-1-end-support-january-2023

Windows Server 2012 R2 Lifecycle

Support Dates

Listing	Start Date	Mainstream End Date	Extended End Date
Windows Server 2012 R2	Nov 25, 2013	Oct 9, 2018	Oct 10, 2023

Releases

Version	Start Date	End Date
Extended Security Update Year 3	Oct 15, 2025	Oct 13, 2026
Extended Security Update Year 2	Oct 9, 2024	Oct 14, 2025
Extended Security Update Year 1	Oct 11, 2023	Oct 8, 2024
Original Release	Nov 25, 2013	Oct 10, 2023

Windows Server 2012 R2 EOL announcement: https://learn.microsoft.com/en-us/lifecycle/announcements/windows-server-2012-r2-end-of-support

Here's the link for Extended Security Updates Overview

[Rob-Hague]

Thanks. It's a nice idea but I'm not sure it's worth the additional complexity at this time - with respect to the OS support and that we still have the old code which is no longer exercised in CI. Of course we could add a target for net6.0 on the integration tests but my gut feeling is that this change would be better left for the future. I could be convinced otherwise.

[scott-xu]

Thanks, I updated the description with Advantages to elaborate on the change. I also updated note2.

[scott-xu]

I think we don't need to worry about OS issue too much. Using an EOL Windows with the latest .NET 8.0+ seems bizarre to me. If they really want, they can use .NET Framework or .NET 6.0. What do you think? @Rob-Hague

[mus65]

Using an EOL Windows with the latest .NET 8.0+ seems bizarre to me.

I don't think it's that bizarre, I have to deal with this exact situation at work myself. Our application is already on .NET 8 but there are quite a few customers who self-host and are still running Win2012.

But I think dropping support for this would be fine as long as this is mentioned in the release notes. This would actually give me a good reason to drop support for Win2012 in our application. :)

[Rob-Hague]

I think we can take this, if:

1. We guard it in the same way the BCL does, with a fallback to BouncyCastle, i.e.:

#if NET8_0_OR_GREATER
if (!OperatingSystem.IsWindows() || OperatingSystem.IsWindowsVersionAtLeast(10))
{
    // BCL stuff
    // wrap this in try { } catch (PlatformNotSupportedException) just to be safe?

    return ...
}
#endif

// BouncyCastle fallback

2. It provides a worthwhile performance improvement over the BouncyCastle path

We don't want to break anyone for no reason, and we will inevitably depend on BouncyCastle, so the only tangible benefit here might be performance

[scott-xu]

Done except

// wrap this in try { } catch (PlatformNotSupportedException) just to be safe?

What do you mean by safe?

[Rob-Hague]

I was thinking, since support does not seem to be universal across platforms, that maybe we wrap the BCL code in try-catch so that it still falls back to BouncyCastle if it's not supported. But I think it's OK, only Windows seems like a problem case from what I can tell: dotnet/runtime@be823a1

[Rob-Hague]

As far as I can tell, going through SshNamedCurves causes a bunch of extra lookup and static initialization compared to SecNamedCurves. Is there any reason to use it?

It's a shame we can't just directly refer to the static BC object when we know the curve we want.

[scott-xu]

I guess SshNamedCurves is designed for libraries like SSH.NET to consume?

[Rob-Hague]

Maybe, but I don't see why we would. If I run this program in a loop:

public static void Main()
{
    var s = Stopwatch.StartNew();

    //if (!SecNamedCurves.GetByName("secp256r1").G.IsInfinity)
    if (!SshNamedCurves.GetByName("nistp256").G.IsInfinity)
    {
        Console.WriteLine(s.ElapsedTicks);
    }
}

dotnet build -c Release; do { dotnet run -c Release --no-build } while ($true)

The result is about 28.3±0.7ms vs 29.2±0.8ms. Both extremely high (also includes first JIT of relevant BouncyCastle methods), but 1ms is 1ms

[scott-xu]

Would you mind trying SecNamedCurves.GetByOid(SecObjectIdentifiers.SecP256r1)?

[Rob-Hague]

I didn't try it but I'm pretty sure it would be the same as SecNamedCurves.GetByName. The problem with SshNamedCurves is the amount of static constructors that it forces to run

[Rob-Hague]

2. It provides a worthwhile performance improvement over the BouncyCastle path

Any idea here?

[scott-xu]

2. It provides a worthwhile performance improvement over the BouncyCastle path

Any idea here?

Actually I'm more leaning towards to the policy "we don't implement cryptographic algorithms, but instead defer to an OS implementation" rather than performance improvement.

[scott-xu]

Now the code structure is very similar with AesCipher's implementation. Ready to review and merge @Rob-Hague @WojciechNagorski

[Rob-Hague]

Looks nice thanks

[Rob-Hague]

I was going to suggest this but I imagine our style settings might have something to say about it.

Suggested change

	else
	{
	_impl = new BouncyCastleImpl(CurveParameter);
	}
	#else
	_impl = new BouncyCastleImpl(CurveParameter);
	#endif
	else
	#endif
	{
	_impl = new BouncyCastleImpl(CurveParameter);
	}

[scott-xu]

Yes, the style setting doesn't like it.
Here's the same situation: https://github.com/sshnet/SSH.NET/pull/1450/files#diff-4d37cacac3721823c238e98586d95622950554a40aff236a571339d813a53794R67-R72

[mattzink]

So this won't remove the BouncyCastle reference for .NET 8+? That's a bummer for us since we are forbidden to ship BouncyCastle library, as it's been a historically high source of security vulnerabilities.

[Rob-Hague]

Correct 😕 the library in previous versions was already using an internal point-in-time copy of BouncyCastle for ECDH, plus an internal point-in-time copy of Chaos.NaCl for ed25519. Now the supported BouncyCastle library is used for both.

We decided the benefits of directly referencing the library (not having unmaintained copies of cryptography implementations) outweighed the drawbacks (size on disk, potential false positive vulnerabilities from unused parts of the library)

Unfortunately, even .NET 9 does not provide the APIs we would need to remove third-party dependencies