Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 33 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
901fd5a
Use BCL ECDiffieHellman for KeyExchange (.NET 8.0 onward only)
scott-xu Apr 6, 2024
f78488c
Merge branch 'develop' into ecdh-bcl
scott-xu Apr 6, 2024
4b8be40
Add back an empty line
scott-xu Apr 6, 2024
8e6d079
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu Apr 6, 2024
61e44a4
Remove the BouncyCastle dependency when target .NET 8.0 onward.
scott-xu Apr 7, 2024
767e692
Merge branch 'develop' into ecdh-bcl
scott-xu Apr 7, 2024
4628a73
Run KeyExchangeAlgorithmTests for .NET 6.0
scott-xu Apr 9, 2024
23c4ac2
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu Apr 9, 2024
23a1dd3
Build Renci.SshNet.IntegrationTests.csproj for net6.0
scott-xu Apr 9, 2024
182f586
Update filter
scott-xu Apr 9, 2024
fed031b
Merge branch 'develop' into ecdh-bcl
scott-xu Apr 18, 2024
cfd950f
Merge branch 'develop' into ecdh-bcl
scott-xu Apr 25, 2024
8392edb
Merge branch 'develop' into ecdh-bcl
scott-xu May 4, 2024
8b42e59
Merge branch 'develop' of https://github.com/scott-xu/SSH.NET into ec…
scott-xu May 23, 2024
b35ffdf
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu May 23, 2024
4b73a96
Merge branch 'develop' into ecdh-bcl
scott-xu Jun 9, 2024
5502918
Add back BouncyCastle as fallback
scott-xu Jun 12, 2024
f76ffeb
Merge branch 'develop' into ecdh-bcl
scott-xu Jun 13, 2024
a1b00e3
Add back the missing `SendMessage`
scott-xu Jun 13, 2024
6ed28ae
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu Jun 13, 2024
d754e93
Merge branch 'develop' into ecdh-bcl
scott-xu Jun 16, 2024
08c9594
Merge branch 'develop' of https://github.com/scott-xu/SSH.NET into ec…
scott-xu Jul 17, 2024
4495c57
Merge branch 'ecdh-bcl' of https://github.com/scott-xu/SSH.NET into e…
scott-xu Jul 17, 2024
bd093c7
Merge branch 'develop' into ecdh-bcl
scott-xu Jul 23, 2024
57bf19d
Merge branch 'develop' into ecdh-bcl
WojciechNagorski Jul 25, 2024
13dd7e1
Run ECDH KEX integration tests under .NET48
scott-xu Jul 25, 2024
9fa0c14
Merge branch 'develop' into ecdh-bcl
scott-xu Jul 25, 2024
344b744
Merge branch 'develop' into ecdh-bcl
scott-xu Jul 27, 2024
385e087
Use SshNamedCurves instead of SecNamedCurves for BouncyCastle.
scott-xu Jul 28, 2024
387e6da
typo
scott-xu Jul 28, 2024
73c9446
Fix build
scott-xu Jul 29, 2024
db0a98e
Use System.Security.Cryptography namespace if NET8_0_OR_GREATER;
scott-xu Jul 29, 2024
2441f77
Separate BCL and BouncyCastle implementation
scott-xu Jul 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ for:
- sh: dotnet test -f net8.0 -c Debug --no-restore --no-build --results-directory artifacts --logger Appveyor --logger "console;verbosity=normal" --logger "liquid.md;LogFileName=linux_unit_test_net_8_report.md" -p:CollectCoverage=true -p:CoverletOutputFormat=cobertura -p:CoverletOutput=../../artifacts/linux_unit_test_net_8_coverage.xml test/Renci.SshNet.Tests/Renci.SshNet.Tests.csproj
- sh: echo "Run integration tests"
- sh: dotnet test -f net8.0 -c Debug --no-restore --no-build --results-directory artifacts --logger Appveyor --logger "console;verbosity=normal" --logger "liquid.md;LogFileName=linux_integration_test_net_8_report.md" -p:CollectCoverage=true -p:CoverletOutputFormat=cobertura -p:CoverletOutput=../../artifacts/linux_integration_test_net_8_coverage.xml test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj
- sh: dotnet test -f net48 -c Debug --no-restore --no-build --results-directory artifacts --logger Appveyor --logger "console;verbosity=normal" --logger "liquid.md;LogFileName=linux_integration_test_net_48_report.md" -p:CollectCoverage=true -p:CoverletOutputFormat=cobertura -p:CoverletOutput=../../artifacts/linux_integration_test_net_48_coverage.xml --filter "Name=ChaCha20Poly1305|Name~Zlib" test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj
- sh: dotnet test -f net48 -c Debug --no-restore --no-build --results-directory artifacts --logger Appveyor --logger "console;verbosity=normal" --logger "liquid.md;LogFileName=linux_integration_test_net_48_report.md" -p:CollectCoverage=true -p:CoverletOutputFormat=cobertura -p:CoverletOutput=../../artifacts/linux_integration_test_net_48_coverage.xml --filter "Name=ChaCha20Poly1305|Name~Ecdh|Name~Zlib" test/Renci.SshNet.IntegrationTests/Renci.SshNet.IntegrationTests.csproj

-
matrix:
Expand Down
77 changes: 77 additions & 0 deletions src/Renci.SshNet/Security/KeyExchangeECDH.BclImpl.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
#if NET8_0_OR_GREATER
using System;
using System.Security.Cryptography;

namespace Renci.SshNet.Security
{
internal abstract partial class KeyExchangeECDH
{
private sealed class BclImpl : Impl
{
private readonly ECCurve _curve;
private readonly ECDiffieHellman _clientECDH;

public BclImpl(ECCurve curve)
{
_curve = curve;
_clientECDH = ECDiffieHellman.Create();
}

public override byte[] GenerateClientECPoint()
{
_clientECDH.GenerateKey(_curve);

var q = _clientECDH.PublicKey.ExportParameters().Q;

return EncodeECPoint(q);
}

public override byte[] CalculateAgreement(byte[] serverECPoint)
{
var q = DecodeECPoint(serverECPoint);

var parameters = new ECParameters
{
Curve = _curve,
Q = q,
};

using var serverECDH = ECDiffieHellman.Create(parameters);

return _clientECDH.DeriveRawSecretAgreement(serverECDH.PublicKey);
}

private static byte[] EncodeECPoint(ECPoint point)
{
var q = new byte[1 + point.X.Length + point.Y.Length];
q[0] = 0x04;
Buffer.BlockCopy(point.X, 0, q, 1, point.X.Length);
Buffer.BlockCopy(point.Y, 0, q, point.X.Length + 1, point.Y.Length);

return q;
}

private static ECPoint DecodeECPoint(byte[] q)
{
var cordSize = (q.Length - 1) / 2;
var x = new byte[cordSize];
var y = new byte[cordSize];
Buffer.BlockCopy(q, 1, x, 0, x.Length);
Buffer.BlockCopy(q, cordSize + 1, y, 0, y.Length);

return new ECPoint { X = x, Y = y };
}

protected override void Dispose(bool disposing)
{
base.Dispose(disposing);

if (disposing)
{
_clientECDH.Dispose();
}
}
}
}
}
#endif
44 changes: 44 additions & 0 deletions src/Renci.SshNet/Security/KeyExchangeECDH.BouncyCastleImpl.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
using Org.BouncyCastle.Asn1.X9;
using Org.BouncyCastle.Crypto.Agreement;
using Org.BouncyCastle.Crypto.Generators;
using Org.BouncyCastle.Crypto.Parameters;

using Renci.SshNet.Abstractions;

namespace Renci.SshNet.Security
{
internal abstract partial class KeyExchangeECDH
{
private sealed class BouncyCastleImpl : Impl
{
private readonly ECDomainParameters _domainParameters;
private readonly ECDHCBasicAgreement _keyAgreement;

public BouncyCastleImpl(X9ECParameters curveParameters)
{
_domainParameters = new ECDomainParameters(curveParameters);
_keyAgreement = new ECDHCBasicAgreement();
}

public override byte[] GenerateClientECPoint()
{
var g = new ECKeyPairGenerator();
g.Init(new ECKeyGenerationParameters(_domainParameters, CryptoAbstraction.SecureRandom));

var aKeyPair = g.GenerateKeyPair();
_keyAgreement.Init(aKeyPair.Private);

return ((ECPublicKeyParameters)aKeyPair.Public).Q.GetEncoded();
}

public override byte[] CalculateAgreement(byte[] serverECPoint)
{
var c = _domainParameters.Curve;
var q = c.DecodePoint(serverECPoint);
var publicKey = new ECPublicKeyParameters("ECDH", q, _domainParameters);

return _keyAgreement.CalculateAgreement(publicKey).ToByteArray();
}
}
}
}
91 changes: 59 additions & 32 deletions src/Renci.SshNet/Security/KeyExchangeECDH.cs
Original file line number Diff line number Diff line change
@@ -1,19 +1,28 @@
using System;

using Org.BouncyCastle.Asn1.X9;
using Org.BouncyCastle.Crypto.Agreement;
using Org.BouncyCastle.Crypto.Generators;
using Org.BouncyCastle.Crypto.Parameters;
using Org.BouncyCastle.Math.EC;

using Renci.SshNet.Abstractions;
using Renci.SshNet.Common;
using Renci.SshNet.Messages.Transport;

namespace Renci.SshNet.Security
{
internal abstract class KeyExchangeECDH : KeyExchangeEC
internal abstract partial class KeyExchangeECDH : KeyExchangeEC
{
#if NET8_0_OR_GREATER
private Impl _impl;

/// <summary>
/// Gets the curve.
/// </summary>
/// <value>
/// The curve.
/// </value>
protected abstract System.Security.Cryptography.ECCurve Curve { get; }
#else
private BouncyCastleImpl _impl;
#endif

/// <summary>
/// Gets the parameter of the curve.
/// </summary>
Expand All @@ -22,9 +31,6 @@ internal abstract class KeyExchangeECDH : KeyExchangeEC
/// </value>
protected abstract X9ECParameters CurveParameter { get; }

private ECDHCBasicAgreement _keyAgreement;
private ECDomainParameters _domainParameters;

/// <inheritdoc/>
public override void Start(Session session, KeyExchangeInitMessage message, bool sendClientInitMessage)
{
Expand All @@ -34,19 +40,20 @@ public override void Start(Session session, KeyExchangeInitMessage message, bool

Session.KeyExchangeEcdhReplyMessageReceived += Session_KeyExchangeEcdhReplyMessageReceived;

_domainParameters = new ECDomainParameters(CurveParameter.Curve,
CurveParameter.G,
CurveParameter.N,
CurveParameter.H,
CurveParameter.GetSeed());

var g = new ECKeyPairGenerator();
g.Init(new ECKeyGenerationParameters(_domainParameters, CryptoAbstraction.SecureRandom));

var aKeyPair = g.GenerateKeyPair();
_keyAgreement = new ECDHCBasicAgreement();
_keyAgreement.Init(aKeyPair.Private);
_clientExchangeValue = ((ECPublicKeyParameters)aKeyPair.Public).Q.GetEncoded();
#if NET8_0_OR_GREATER
if (!OperatingSystem.IsWindows() || OperatingSystem.IsWindowsVersionAtLeast(10))
{
_impl = new BclImpl(Curve);
}
else
{
_impl = new BouncyCastleImpl(CurveParameter);
}
#else
_impl = new BouncyCastleImpl(CurveParameter);
#endif
Comment on lines +48 to +54
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


_clientExchangeValue = _impl.GenerateClientECPoint();

SendMessage(new KeyExchangeEcdhInitMessage(_clientExchangeValue));
}
Expand Down Expand Up @@ -86,18 +93,38 @@ private void HandleServerEcdhReply(byte[] hostKey, byte[] serverExchangeValue, b
_hostKey = hostKey;
_signature = signature;

var cordSize = (serverExchangeValue.Length - 1) / 2;
var x = new byte[cordSize];
Buffer.BlockCopy(serverExchangeValue, 1, x, 0, x.Length); // first byte is format. should be checked and passed to bouncy castle?
var y = new byte[cordSize];
Buffer.BlockCopy(serverExchangeValue, cordSize + 1, y, 0, y.Length);
var agreement = _impl.CalculateAgreement(serverExchangeValue);

SharedKey = agreement.ToBigInteger2().ToByteArray().Reverse();
}

/// <inheritdoc/>
protected override void Dispose(bool disposing)
{
base.Dispose(disposing);

if (disposing)
{
_impl?.Dispose();
}
}

private abstract class Impl : IDisposable
{
public abstract byte[] GenerateClientECPoint();

public abstract byte[] CalculateAgreement(byte[] serverECPoint);

var c = (FpCurve)_domainParameters.Curve;
var q = c.CreatePoint(new Org.BouncyCastle.Math.BigInteger(1, x), new Org.BouncyCastle.Math.BigInteger(1, y));
var publicKey = new ECPublicKeyParameters("ECDH", q, _domainParameters);
protected virtual void Dispose(bool disposing)
{
}

var k1 = _keyAgreement.CalculateAgreement(publicKey);
SharedKey = k1.ToByteArray().ToBigInteger2().ToByteArray().Reverse();
public void Dispose()
{
// Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
Dispose(disposing: true);
GC.SuppressFinalize(this);
}
}
}
}
17 changes: 15 additions & 2 deletions src/Renci.SshNet/Security/KeyExchangeECDH256.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Org.BouncyCastle.Asn1.Sec;
using Org.BouncyCastle.Asn1.Sec;
using Org.BouncyCastle.Asn1.X9;

using Renci.SshNet.Abstractions;
Expand All @@ -15,14 +15,27 @@ public override string Name
get { return "ecdh-sha2-nistp256"; }
}

#if NET8_0_OR_GREATER
/// <summary>
/// Gets the curve.
/// </summary>
protected override System.Security.Cryptography.ECCurve Curve
{
get
{
return System.Security.Cryptography.ECCurve.NamedCurves.nistP256;
}
}
#endif

/// <summary>
/// Gets Curve Parameter.
/// </summary>
protected override X9ECParameters CurveParameter
{
get
{
return SecNamedCurves.GetByName("secp256r1");
return SecNamedCurves.GetByOid(SecObjectIdentifiers.SecP256r1);
}
}

Expand Down
17 changes: 15 additions & 2 deletions src/Renci.SshNet/Security/KeyExchangeECDH384.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Org.BouncyCastle.Asn1.Sec;
using Org.BouncyCastle.Asn1.Sec;
using Org.BouncyCastle.Asn1.X9;

using Renci.SshNet.Abstractions;
Expand All @@ -15,14 +15,27 @@ public override string Name
get { return "ecdh-sha2-nistp384"; }
}

#if NET8_0_OR_GREATER
/// <summary>
/// Gets the curve.
/// </summary>
protected override System.Security.Cryptography.ECCurve Curve
{
get
{
return System.Security.Cryptography.ECCurve.NamedCurves.nistP384;
}
}
#endif

/// <summary>
/// Gets Curve Parameter.
/// </summary>
protected override X9ECParameters CurveParameter
{
get
{
return SecNamedCurves.GetByName("secp384r1");
return SecNamedCurves.GetByOid(SecObjectIdentifiers.SecP384r1);
}
}

Expand Down
17 changes: 15 additions & 2 deletions src/Renci.SshNet/Security/KeyExchangeECDH521.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using Org.BouncyCastle.Asn1.Sec;
using Org.BouncyCastle.Asn1.Sec;
using Org.BouncyCastle.Asn1.X9;

using Renci.SshNet.Abstractions;
Expand All @@ -15,14 +15,27 @@ public override string Name
get { return "ecdh-sha2-nistp521"; }
}

#if NET8_0_OR_GREATER
/// <summary>
/// Gets the curve.
/// </summary>
protected override System.Security.Cryptography.ECCurve Curve
{
get
{
return System.Security.Cryptography.ECCurve.NamedCurves.nistP521;
}
}
#endif

/// <summary>
/// Gets Curve Parameter.
/// </summary>
protected override X9ECParameters CurveParameter
{
get
{
return SecNamedCurves.GetByName("secp521r1");
return SecNamedCurves.GetByOid(SecObjectIdentifiers.SecP521r1);
}
}

Expand Down