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

Harden UnixPkcs12Reader AllocHGlobal #98331

Merged
merged 4 commits into from
Feb 13, 2024
Merged

Harden UnixPkcs12Reader AllocHGlobal #98331

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24e52ad
Harden UnixPkcs12Reader AllocHGlobal
bartonjs Feb 13, 2024
b7940c8
Switch to NativeMemory.Alloc
bartonjs Feb 13, 2024
9f1326d
Ensure that Dispose only disposes the memory manager once ever.
bartonjs Feb 13, 2024
ee16bc2
Apply feedback
bartonjs Feb 13, 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
3 changes: 2 additions & 1 deletion ...y.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidCertificatePal.cs
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions ...ity.Cryptography/src/System/Security/Cryptography/X509Certificates/AndroidPkcs12Reader.cs
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion ...raphy/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.Pkcs12.iOS.cs
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion ...phy/src/System/Security/Cryptography/X509Certificates/AppleCertificatePal.Pkcs12.macOS.cs
View file
Open in desktop
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
3 changes: 1 addition & 2 deletions ...y.Cryptography/src/System/Security/Cryptography/X509Certificates/ApplePkcs12Reader.iOS.cs
View file
Open in desktop
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
3 changes: 1 addition & 2 deletions ...Cryptography/src/System/Security/Cryptography/X509Certificates/ApplePkcs12Reader.macOS.cs
View file
Open in desktop
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
6 changes: 3 additions & 3 deletions ...ity.Cryptography/src/System/Security/Cryptography/X509Certificates/OpenSslPkcs12Reader.cs
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion ...curity.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.Android.cs
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion ...m.Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.iOS.cs
View file
Open in desktop
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
3 changes: 2 additions & 1 deletion ...Security.Cryptography/src/System/Security/Cryptography/X509Certificates/StorePal.macOS.cs
View file
Open in desktop
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
21 changes: 15 additions & 6 deletions ...curity.Cryptography/src/System/Security/Cryptography/X509Certificates/UnixPkcs12Reader.cs
View file
Open in desktop
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,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 @@ -125,7 +134,7 @@ public void Dispose()

fixed (byte* ptr = tmp)
Copy link
Member

Choose a reason for hiding this comment

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

Should we just expose the pointer directly? It doesn't really matter; it's just a bit of a headscratcher seeing this code pin a span and then free the pointer out from under it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this is the only place that combines the Pointer Memory Manager and an alloc/free; so it seems "better" to me to just have a weird line of code here than to de-encapsulate the pointer.

Copy link
Member

Choose a reason for hiding this comment

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

And/or use Unsafe.AsPointer to get the pointer instead of pinning:

Unsafe.AsPointer(ref MemoryMarshal.GetReference(tmp))

{
Marshal.FreeHGlobal((IntPtr)ptr);
NativeMemory.Free(ptr);
bartonjs marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Loading