SSH.NET
SSH.NET copied to clipboard
sshnet
Replace internal BouncyCastle with NuGet package
Contributes to #1271 .
This was as simple as adding the NuGet package, deleting the old code and replacing the namespaces. It's just a matter of whether the tests pass and you actually want to do this. I don't think there was a clear decision in #1271 .
Guess it wasn't this simple after all, a lot of tests fail with a NullReferenceException. I'm gonna take a look within the next few days. Converted to draft.
How about leveraging BCL's ECDiffieHellman.DeriveRawSecretAgreement for .NET 8 onward? See https://github.com/dotnet/runtime/issues/71613
Thanks, that's good to know. Unfortunately this API is not supported on Windows Server 2012, so even net8.0 builds would still need the BouncyCastle reference.
Not saying this shouldn't be done. But since this wouldn't reduce the dependency to BouncyCastle it imho shouldn't block this PR and could be done separately.
Out of curiosity what is the lowest supported version of the Windows. Windows 2012 is EOL
@mus65 I created a separate PR https://github.com/sshnet/SSH.NET/pull/1371. Might bring a little bit conflicts with your PR but it should be straightforward to resolve and should not block this PR. @darkoperator See this please: https://github.com/sshnet/SSH.NET/pull/1371#issuecomment-2041099786
Thanks. Indeed there was no decision on this, and I don't have a preference either way (which probably means I'd lean towards not doing anything). I think I'll leave this one for a while for others to chime in.
One thing: this library has a similar internal dependency on some Chaos.NaCl code for ed25519. Perhaps as part of this change (if it were to be merged), that code could be removed as well and replaced with some additional usage of BouncyCastle. Would you be interested in investigating that?
Guess it wasn't this simple after all
FYI, running the integration tests locally is very simple - just install Docker Desktop and it should "just work". It's a lot faster than relying on CI.
One thing: this library has a similar internal dependency on some Chaos.NaCl code for ed25519. Perhaps as part of this change (if it were to be merged), that code could be removed as well and replaced with some additional usage of BouncyCastle. Would you be interested in investigating that?
I actually had a look at that, but unfortunately I'm not familiar enough with cryptography to do this.
Side note: the internal BouncyCastle doesn't support Ed25519 yet, so in order to get rid of Chaos.NaCI, either this PR needs to be merged first or the internal BouncyCastle needs to be updated. I also found this BouncyCastle issue about some implementation difference with NaCl (I don't know if this is relevant for SSH.NET).
Chiming in... Adding BouncyCastle as a NuGet package would add all of the lib features, or am I wrong about that? Wouldn't that be a bit excessive since only a very limited subset is actually implemented and NET has been supporting more and more crypto natively.
Updated to BouncyCastle 2.3.1 which fixed multiple CVEs. These CVEs are obviously unfixed in the internal implementation. I can't tell whether these actually affect SSH.NET, but either way this imho would be another reason in favor of the NuGet package.
If one of the CVEs affects SSH.NET, fixing it would require updating the internal implementation and making a new release. With the NuGet package, any application using SSH.NET could simply update to the newer BouncyCastle version to get the security fixes.
Wouldn't that be a bit excessive since only a very limited subset is actually implemented
Right, if we are only using it for ECDH it is arguably not worth it. But if we can also use it for ed25519, bcrypt, standard DH(?) and Chachapoly(?) then there is more of a case for it.
I can't tell whether these actually affect SSH.NET, but either way this imho would be another reason in favor of the NuGet package.
They don't look like they affect our usage but your point stands either way.
The hesitation stems from that the BC assembly does not play well with trimming. I've opened https://github.com/bcgit/bc-csharp/pull/534 to partly address that and a full fix is possible. I'd like to see if that gets anywhere. If it does then we will be able to say if you care about the extra size then you should trim your app, and if you don't care then you don't care. Right now a trimmed BC assembly is still 4MB.
If the BC trimming changes don't get anywhere then I guess we just bite the bullet.
Updated to BouncyCastle 2.4.0 which already includes @Rob-Hague's trimming fixes. :smile:
I think we should take this for this release. I will come back to it after some other stuff. I want to a) experiment with replacing the X25519 Na.Cl code as well; and b) check the size of an AOT app, perhaps with some check in CI
IMO, we should use BouncyCastle Nuget and mark all old releases as unsafe. Let's look at how many vulnerabilities they resolved in old versions. Then we can implement #1416 PR using this library. @Rob-Hague Do you agree or would you prefer to update our source to the latest version?
Chaos.NaCl can be removed if merge https://github.com/sshnet/SSH.NET/pull/1416, https://github.com/sshnet/SSH.NET/pull/1447 and https://github.com/sshnet/SSH.NET/pull/1448. @WojciechNagorski @Rob-Hague