Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix finalized SafeHandles in System.Security.Cryptography on Linux #72279

Merged
merged 2 commits into from Jul 15, 2022
Merged

Fix finalized SafeHandles in System.Security.Cryptography on Linux #72279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b83db4a
Fix finalized SafeHandles in System.Security.Cryptography on Linux
stephentoub Jul 15, 2022
1e4158a
Make Jeremy happy and Stephen sad
stephentoub Jul 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions ...s/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.EcKey.cs
View file
Open in desktop
Expand Up @@ -103,6 +103,7 @@ internal static SafeEcKeyHandle DuplicateHandle(IntPtr handle)

if (!Interop.AndroidCrypto.EcKeyUpRef(handle))
{
safeHandle.Dispose();
throw new CryptographicException();
}

Expand Down
1 change: 1 addition & 0 deletions ...ies/Common/src/Interop/Android/System.Security.Cryptography.Native.Android/Interop.Rsa.cs
View file
Open in desktop
Expand Up @@ -236,6 +236,7 @@ internal static SafeRsaHandle DuplicateHandle(IntPtr handle)

if (!Interop.AndroidCrypto.RsaUpRef(handle))
{
safeHandle.Dispose();
throw new CryptographicException();
}

Expand Down
160 changes: 82 additions & 78 deletions ...Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.EcDsa.ImportExport.cs
View file
Open in desktop
Expand Up @@ -125,36 +125,38 @@ internal static SafeEcKeyHandle EcKeyCreateByExplicitCurve(ECCurve curve)
out qy_bn, out qy_cb,
out d_bn_not_owned, out d_cb);

if (rc == -1)
{
throw new CryptographicException(SR.Cryptography_CSP_NoPrivateKey);
}
else if (rc != 1)
{
throw Interop.Crypto.CreateOpenSslCryptographicException();
}

using (qx_bn)
using (qy_bn)
using (d_bn = new SafeBignumHandle(d_bn_not_owned, false))
{
// Match Windows semantics where qx, qy, and d have same length
int keySizeBits = EcKeyGetSize(key);
int expectedSize = (keySizeBits + 7) / 8;
int cbKey = GetMax(qx_cb, qy_cb, d_cb);
if (rc == -1)
{
throw new CryptographicException(SR.Cryptography_CSP_NoPrivateKey);
}
else if (rc != 1)
{
throw Interop.Crypto.CreateOpenSslCryptographicException();
}

Debug.Assert(
cbKey <= expectedSize,
$"Expected output size was {expectedSize}, which a parameter exceeded. qx={qx_cb}, qy={qy_cb}, d={d_cb}");
using (d_bn = new SafeBignumHandle(d_bn_not_owned, false))
{
// Match Windows semantics where qx, qy, and d have same length
int keySizeBits = EcKeyGetSize(key);
int expectedSize = (keySizeBits + 7) / 8;
int cbKey = GetMax(qx_cb, qy_cb, d_cb);

cbKey = GetMax(cbKey, expectedSize);
Debug.Assert(
cbKey <= expectedSize,
$"Expected output size was {expectedSize}, which a parameter exceeded. qx={qx_cb}, qy={qy_cb}, d={d_cb}");

parameters.Q = new ECPoint
{
X = Crypto.ExtractBignum(qx_bn, cbKey),
Y = Crypto.ExtractBignum(qy_bn, cbKey)
};
parameters.D = d_cb == 0 ? null : Crypto.ExtractBignum(d_bn, cbKey);
cbKey = GetMax(cbKey, expectedSize);

parameters.Q = new ECPoint
{
X = Crypto.ExtractBignum(qx_bn, cbKey),
Y = Crypto.ExtractBignum(qy_bn, cbKey)
};
parameters.D = d_cb == 0 ? null : Crypto.ExtractBignum(d_bn, cbKey);
}
}
}
finally
Expand Down Expand Up @@ -212,15 +214,6 @@ internal static SafeEcKeyHandle EcKeyCreateByExplicitCurve(ECCurve curve)
out cofactor_bn, out cofactor_cb,
out seed_bn, out seed_cb);

if (rc == -1)
{
throw new CryptographicException(SR.Cryptography_CSP_NoPrivateKey);
}
else if (rc != 1)
{
throw Interop.Crypto.CreateOpenSslCryptographicException();
}

using (qx_bn)
using (qy_bn)
using (p_bn)
Expand All @@ -231,62 +224,73 @@ internal static SafeEcKeyHandle EcKeyCreateByExplicitCurve(ECCurve curve)
using (order_bn)
using (cofactor_bn)
using (seed_bn)
using (var d_h = new SafeBignumHandle(d_bn_not_owned, false))
{
int cbFieldLength;
int pFieldLength;
if (curveType == ECCurve.ECCurveType.Characteristic2)
if (rc == -1)
{
// Match Windows semantics where a,b,gx,gy,qx,qy have same length
// Treat length of m separately as it is not tied to other fields for Char2 (Char2 not supported by Windows)
cbFieldLength = GetMax(new[] { a_cb, b_cb, gx_cb, gy_cb, qx_cb, qy_cb });
pFieldLength = p_cb;
throw new CryptographicException(SR.Cryptography_CSP_NoPrivateKey);
}
else
else if (rc != 1)
{
// Match Windows semantics where p,a,b,gx,gy,qx,qy have same length
cbFieldLength = GetMax(new[] { p_cb, a_cb, b_cb, gx_cb, gy_cb, qx_cb, qy_cb });
pFieldLength = cbFieldLength;
throw Interop.Crypto.CreateOpenSslCryptographicException();
}

// Match Windows semantics where order and d have same length
int cbSubgroupOrder = GetMax(order_cb, d_cb);

// Copy values to ECParameters
ECParameters parameters = default;
parameters.Q = new ECPoint
using (var d_h = new SafeBignumHandle(d_bn_not_owned, false))
{
X = Crypto.ExtractBignum(qx_bn, cbFieldLength),
Y = Crypto.ExtractBignum(qy_bn, cbFieldLength)
};
parameters.D = d_cb == 0 ? null : Crypto.ExtractBignum(d_h, cbSubgroupOrder);
int cbFieldLength;
int pFieldLength;
if (curveType == ECCurve.ECCurveType.Characteristic2)
{
// Match Windows semantics where a,b,gx,gy,qx,qy have same length
// Treat length of m separately as it is not tied to other fields for Char2 (Char2 not supported by Windows)
cbFieldLength = GetMax(new[] { a_cb, b_cb, gx_cb, gy_cb, qx_cb, qy_cb });
pFieldLength = p_cb;
}
else
{
// Match Windows semantics where p,a,b,gx,gy,qx,qy have same length
cbFieldLength = GetMax(new[] { p_cb, a_cb, b_cb, gx_cb, gy_cb, qx_cb, qy_cb });
pFieldLength = cbFieldLength;
}

var curve = parameters.Curve;
curve.CurveType = curveType;
curve.A = Crypto.ExtractBignum(a_bn, cbFieldLength)!;
curve.B = Crypto.ExtractBignum(b_bn, cbFieldLength)!;
curve.G = new ECPoint
{
X = Crypto.ExtractBignum(gx_bn, cbFieldLength),
Y = Crypto.ExtractBignum(gy_bn, cbFieldLength)
};
curve.Order = Crypto.ExtractBignum(order_bn, cbSubgroupOrder)!;
// Match Windows semantics where order and d have same length
int cbSubgroupOrder = GetMax(order_cb, d_cb);

if (curveType == ECCurve.ECCurveType.Characteristic2)
{
curve.Polynomial = Crypto.ExtractBignum(p_bn, pFieldLength)!;
}
else
{
curve.Prime = Crypto.ExtractBignum(p_bn, pFieldLength)!;
}
// Copy values to ECParameters
ECParameters parameters = default;
parameters.Q = new ECPoint
{
X = Crypto.ExtractBignum(qx_bn, cbFieldLength),
Y = Crypto.ExtractBignum(qy_bn, cbFieldLength)
};
parameters.D = d_cb == 0 ? null : Crypto.ExtractBignum(d_h, cbSubgroupOrder);

// Optional parameters
curve.Cofactor = cofactor_cb == 0 ? null : Crypto.ExtractBignum(cofactor_bn, cofactor_cb);
curve.Seed = seed_cb == 0 ? null : Crypto.ExtractBignum(seed_bn, seed_cb);
var curve = parameters.Curve;
curve.CurveType = curveType;
curve.A = Crypto.ExtractBignum(a_bn, cbFieldLength)!;
curve.B = Crypto.ExtractBignum(b_bn, cbFieldLength)!;
curve.G = new ECPoint
{
X = Crypto.ExtractBignum(gx_bn, cbFieldLength),
Y = Crypto.ExtractBignum(gy_bn, cbFieldLength)
};
curve.Order = Crypto.ExtractBignum(order_bn, cbSubgroupOrder)!;

parameters.Curve = curve;
return parameters;
if (curveType == ECCurve.ECCurveType.Characteristic2)
{
curve.Polynomial = Crypto.ExtractBignum(p_bn, pFieldLength)!;
}
else
{
curve.Prime = Crypto.ExtractBignum(p_bn, pFieldLength)!;
}

// Optional parameters
curve.Cofactor = cofactor_cb == 0 ? null : Crypto.ExtractBignum(cofactor_bn, cofactor_cb);
curve.Seed = seed_cb == 0 ? null : Crypto.ExtractBignum(seed_bn, seed_cb);

parameters.Curve = curve;
return parameters;
}
}
}
finally
Expand Down
4 changes: 3 additions & 1 deletion src/libraries/Common/src/Microsoft/Win32/SafeHandles/SafeDsaHandle.Unix.cs
View file
Open in desktop
Expand Up @@ -37,7 +37,9 @@ internal static SafeDsaHandle DuplicateHandle(IntPtr handle)

if (!Interop.Crypto.DsaUpRef(handle))
{
throw Interop.Crypto.CreateOpenSslCryptographicException();
Exception e = Interop.Crypto.CreateOpenSslCryptographicException();
safeHandle.Dispose();
throw e;
}

safeHandle.SetHandle(handle);
Expand Down
4 changes: 3 additions & 1 deletion src/libraries/Common/src/Microsoft/Win32/SafeHandles/SafeEcKeyHandle.Unix.cs
View file
Open in desktop
Expand Up @@ -37,7 +37,9 @@ internal static SafeEcKeyHandle DuplicateHandle(IntPtr handle)

if (!Interop.Crypto.EcKeyUpRef(handle))
{
throw Interop.Crypto.CreateOpenSslCryptographicException();
Exception e = Interop.Crypto.CreateOpenSslCryptographicException();
safeHandle.Dispose();
throw e;
}

safeHandle.SetHandle(handle);
Expand Down
1 change: 1 addition & 0 deletions src/libraries/Common/src/System/Security/Cryptography/DSAAndroid.cs
View file
Open in desktop
Expand Up @@ -381,6 +381,7 @@ private void SetKey(SafeDsaHandle newKey)
// with the already loaded key.
ForceSetKeySize(BitsPerByte * Interop.AndroidCrypto.DsaKeySize(newKey));

FreeKey();
_key = new Lazy<SafeDsaHandle>(newKey);
}

Expand Down
1 change: 1 addition & 0 deletions src/libraries/Common/src/System/Security/Cryptography/DSAOpenSsl.cs
View file
Open in desktop
Expand Up @@ -373,6 +373,7 @@ private void SetKey(SafeDsaHandle newKey)
// with the already loaded key.
ForceSetKeySize(BitsPerByte * Interop.Crypto.DsaKeySize(newKey));

FreeKey();
_key = new Lazy<SafeDsaHandle>(newKey);
}

Expand Down
6 changes: 5 additions & 1 deletion src/libraries/Common/src/System/Security/Cryptography/ECDiffieHellmanOpenSsl.cs
View file
Open in desktop
Expand Up @@ -95,7 +95,11 @@ public override ECDiffieHellmanPublicKey PublicKey
get
{
ThrowIfDisposed();
return new ECDiffieHellmanOpenSslPublicKey(_key.UpRefKeyHandle());

using (SafeEvpPKeyHandle handle = _key.UpRefKeyHandle())
{
return new ECDiffieHellmanOpenSslPublicKey(handle);
}
}
}

Expand Down
15 changes: 13 additions & 2 deletions ...System/Security/Cryptography/AlgorithmImplementations/ECDiffieHellman/ECDhKeyFileTests.cs
View file
Open in desktop
Expand Up @@ -14,10 +14,21 @@ public class ECDhKeyFileTests : ECKeyFileTests<ECDiffieHellman>
protected override void Exercise(ECDiffieHellman key) => key.Exercise();

protected override Func<ECDiffieHellman, byte[]> PublicKeyWriteArrayFunc { get; } =
key => key.PublicKey.ExportSubjectPublicKeyInfo();
key =>
{
using (ECDiffieHellmanPublicKey publicKey = key.PublicKey)
{
return publicKey.ExportSubjectPublicKeyInfo();
}
};

protected override WriteKeyToSpanFunc PublicKeyWriteSpanFunc { get; } =
(ECDiffieHellman key, Span<byte> destination, out int bytesWritten) =>
key.PublicKey.TryExportSubjectPublicKeyInfo(destination, out bytesWritten);
{
using (ECDiffieHellmanPublicKey publicKey = key.PublicKey)
{
return publicKey.TryExportSubjectPublicKeyInfo(destination, out bytesWritten);
}
};
}
}
2 changes: 2 additions & 0 deletions ...em/Security/Cryptography/AlgorithmImplementations/ECDiffieHellman/ECDiffieHellmanTests.cs
View file
Open in desktop
Expand Up @@ -168,6 +168,8 @@ public static void UseAfterDispose(bool importKey)
key.KeySize = 384;
key.ExportParameters(false);
});

pubKey.Dispose();
}

#if NETCOREAPP
Expand Down