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.

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

Harden UnixPkcs12Reader AllocHGlobal #98331

Merged
merged 4 commits into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,9 @@ ref MemoryMarshal.GetReference(rawData),

private static AndroidCertificatePal ReadPkcs12(ReadOnlySpan<byte> rawData, SafePasswordHandle password, bool ephemeralSpecified)
{
using (var reader = new AndroidPkcs12Reader(rawData))
using (var reader = new AndroidPkcs12Reader())
{
reader.ParsePkcs12(rawData);
reader.Decrypt(password, ephemeralSpecified);

UnixPkcs12Reader.CertAndKey certAndKey = reader.GetSingleCert();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,17 @@ namespace System.Security.Cryptography.X509Certificates
{
internal sealed class AndroidPkcs12Reader : UnixPkcs12Reader
{
internal AndroidPkcs12Reader(ReadOnlySpan<byte> data)
internal AndroidPkcs12Reader()
{
ParsePkcs12(data);
}

public static bool IsPkcs12(ReadOnlySpan<byte> data)
{
try
{
using (var reader = new AndroidPkcs12Reader(data))
using (var reader = new AndroidPkcs12Reader())
{
reader.ParsePkcs12(data);
return true;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ private static AppleCertificatePal ImportPkcs12(
SafePasswordHandle password,
bool ephemeralSpecified)
{
using (ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData))
using (ApplePkcs12Reader reader = new ApplePkcs12Reader())
{
reader.ParsePkcs12(rawData);
reader.Decrypt(password, ephemeralSpecified);
return ImportPkcs12(reader.GetSingleCert());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ private static AppleCertificatePal ImportPkcs12(
bool exportable,
SafeKeychainHandle keychain)
{
using (ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData))
using (ApplePkcs12Reader reader = new ApplePkcs12Reader())
{
reader.ParsePkcs12(rawData);
reader.Decrypt(password, ephemeralSpecified: false);

UnixPkcs12Reader.CertAndKey certAndKey = reader.GetSingleCert();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ namespace System.Security.Cryptography.X509Certificates
{
internal sealed class ApplePkcs12Reader : UnixPkcs12Reader
{
internal ApplePkcs12Reader(ReadOnlySpan<byte> data)
internal ApplePkcs12Reader()
{
ParsePkcs12(data);
}

protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,8 @@ namespace System.Security.Cryptography.X509Certificates
{
internal sealed class ApplePkcs12Reader : UnixPkcs12Reader
{
internal ApplePkcs12Reader(ReadOnlySpan<byte> data)
internal ApplePkcs12Reader()
{
ParsePkcs12(data);
}

protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,8 @@ namespace System.Security.Cryptography.X509Certificates
{
internal sealed class OpenSslPkcs12Reader : UnixPkcs12Reader
{
private OpenSslPkcs12Reader(ReadOnlySpan<byte> data)
private OpenSslPkcs12Reader()
{
ParsePkcs12(data);
}

protected override ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data)
Expand Down Expand Up @@ -89,7 +88,8 @@ private static bool TryRead(

try
{
pkcs12Reader = new OpenSslPkcs12Reader(data);
pkcs12Reader = new OpenSslPkcs12Reader();
pkcs12Reader.ParsePkcs12(data);
return true;
}
catch (CryptographicException e)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,8 +118,9 @@ private static ICertificatePal[] ReadPkcs12Collection(
SafePasswordHandle password,
bool ephemeralSpecified)
{
using (var reader = new AndroidPkcs12Reader(rawData))
using (var reader = new AndroidPkcs12Reader())
{
reader.ParsePkcs12(rawData);
reader.Decrypt(password, ephemeralSpecified);

ICertificatePal[] certs = new ICertificatePal[reader.GetCertCount()];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,11 @@ private static ILoaderPal FromBlob(ReadOnlySpan<byte> rawData, SafePasswordHandl
if (contentType == X509ContentType.Pkcs12)
{
X509Certificate.EnforceIterationCountLimit(ref rawData, readingFromFile, password.PasswordProvided);
ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData);
ApplePkcs12Reader reader = new ApplePkcs12Reader();

try
{
reader.ParsePkcs12(rawData);
reader.Decrypt(password, ephemeralSpecified);
return new ApplePkcs12CertLoader(reader, password);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,11 @@ private static ApplePkcs12CertLoader ImportPkcs12(
bool ephemeralSpecified,
SafeKeychainHandle keychain)
{
ApplePkcs12Reader reader = new ApplePkcs12Reader(rawData);
ApplePkcs12Reader reader = new ApplePkcs12Reader();

try
{
reader.ParsePkcs12(rawData);
reader.Decrypt(password, ephemeralSpecified);
return new ApplePkcs12CertLoader(reader, keychain, password, exportable);
bartonjs marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Formats.Asn1;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Security.Cryptography.Asn1;
using System.Security.Cryptography.Asn1.Pkcs12;
Expand All @@ -30,7 +31,7 @@ internal abstract class UnixPkcs12Reader : IDisposable
protected abstract ICertificatePalCore ReadX509Der(ReadOnlyMemory<byte> data);
protected abstract AsymmetricAlgorithm LoadKey(ReadOnlyMemory<byte> safeBagBagValue);

protected void ParsePkcs12(ReadOnlySpan<byte> data)
internal void ParsePkcs12(ReadOnlySpan<byte> data)
{
try
{
Expand All @@ -42,10 +43,19 @@ protected void ParsePkcs12(ReadOnlySpan<byte> data)

unsafe
{
IntPtr tmpPtr = Marshal.AllocHGlobal(encodedData.Length);
Span<byte> tmpSpan = new Span<byte>((byte*)tmpPtr, encodedData.Length);
encodedData.CopyTo(tmpSpan);
_tmpManager = new PointerMemoryManager<byte>((void*)tmpPtr, encodedData.Length);
void* tmpPtr = NativeMemory.Alloc((uint)encodedData.Length);

try
{
Span<byte> tmpSpan = new Span<byte>((byte*)tmpPtr, encodedData.Length);
encodedData.CopyTo(tmpSpan);
_tmpManager = new PointerMemoryManager<byte>(tmpPtr, encodedData.Length);
}
catch
{
NativeMemory.Free(tmpPtr);
throw;
}
}

ReadOnlyMemory<byte> tmpMemory = _tmpManager.Memory;
Expand Down Expand Up @@ -115,26 +125,27 @@ public void Dispose()
// Generally, having a MemoryManager cleaned up in a Dispose is a bad practice.
// In this case, the UnixPkcs12Reader is only ever created in a using statement,
// never accessed by a second thread, and there isn't a manual call to Dispose
// mixed in anywhere.
if (_tmpManager != null)
// mixed in anywhere outside of an aborted allocation path.

PointerMemoryManager<byte>? manager = _tmpManager;
_tmpManager = null;

if (manager != null)
{
unsafe
{
Span<byte> tmp = _tmpManager.GetSpan();
Span<byte> tmp = manager.GetSpan();
CryptographicOperations.ZeroMemory(tmp);

fixed (byte* ptr = tmp)
{
Marshal.FreeHGlobal((IntPtr)ptr);
}
NativeMemory.Free(Unsafe.AsPointer(ref MemoryMarshal.GetReference(tmp)));
}

((IDisposable)_tmpManager).Dispose();
_tmpManager = null;
((IDisposable)manager).Dispose();
}

ContentInfoAsn[]? rentedContents = Interlocked.Exchange(ref _safeContentsValues, null);
CertAndKey[]? rentedCerts = Interlocked.Exchange(ref _certs, null);
ContentInfoAsn[]? rentedContents = _safeContentsValues;
CertAndKey[]? rentedCerts = _certs;
_safeContentsValues = null;
_certs = null;

if (rentedContents != null)
{
Expand Down
Loading