Skip to content

Commit

Permalink
Harden UnixPkcs12Reader AllocHGlobal
Browse files Browse the repository at this point in the history
* Do not do the allocation and parsing in the constructor, since that prevents Dispose from running
* If something goes wrong between AllocHGlobal and moving the pointer into a PointerMemoryManager, call FreeHGlobal.
  * Past that point it will be correctly freed by Dispose.
  • Loading branch information
bartonjs committed Feb 13, 2024
1 parent d4bab6b commit 24e52ad
Show file tree
Hide file tree
Showing 11 changed files with 39 additions and 21 deletions.
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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,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 +42,24 @@ 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);
IntPtr tmpPtr = IntPtr.Zero;

try
{
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);
}
catch
{
if (tmpPtr != IntPtr.Zero)
{
Marshal.FreeHGlobal(tmpPtr);
}

throw;
}
}

ReadOnlyMemory<byte> tmpMemory = _tmpManager.Memory;
Expand Down

0 comments on commit 24e52ad

Please sign in to comment.