Skip to content

Commit

Permalink
Use a zero value public key in PKCS8 where required
Browse files Browse the repository at this point in the history
  • Loading branch information
vcsjones committed Apr 4, 2020
1 parent 7e71707 commit e47356b
Show file tree
Hide file tree
Showing 3 changed files with 130 additions and 8 deletions.
91 changes: 88 additions & 3 deletions src/libraries/Common/src/System/Security/Cryptography/CngPkcs8.cs
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,23 @@ internal static unsafe Pkcs8Response ImportPkcs8PrivateKey(ReadOnlySpan<byte> so
}

bytesRead = len;
return ImportPkcs8(source.Slice(0, len));
ReadOnlySpan<byte> pkcs8Source = source.Slice(0, len);

try
{
return ImportPkcs8(pkcs8Source);
}
catch (CryptographicException)
{
AsnWriter? pkcs8ZeroPublicKey = RewritePkcs8ECPrivateKeyWithZeroPublicKey(pkcs8Source);

if (pkcs8ZeroPublicKey == null)
{
throw;
}

return ImportPkcs8(pkcs8ZeroPublicKey.EncodeAsSpan());
}
}

internal static unsafe Pkcs8Response ImportEncryptedPkcs8PrivateKey(
Expand All @@ -147,7 +163,21 @@ internal static unsafe Pkcs8Response ImportPkcs8PrivateKey(ReadOnlySpan<byte> so
}
catch (CryptographicException e)
{
throw new CryptographicException(SR.Cryptography_Pkcs8_EncryptedReadFailed, e);
AsnWriter? pkcs8ZeroPublicKey = RewritePkcs8ECPrivateKeyWithZeroPublicKey(decryptedSpan);

if (pkcs8ZeroPublicKey == null)
{
throw new CryptographicException(SR.Cryptography_Pkcs8_EncryptedReadFailed, e);
}

try
{
return ImportPkcs8(pkcs8ZeroPublicKey.EncodeAsSpan());
}
catch (CryptographicException)
{
throw new CryptographicException(SR.Cryptography_Pkcs8_EncryptedReadFailed, e);
}
}
finally
{
Expand Down Expand Up @@ -199,7 +229,22 @@ internal static unsafe Pkcs8Response ImportPkcs8PrivateKey(ReadOnlySpan<byte> so
}
catch (CryptographicException e)
{
throw new CryptographicException(SR.Cryptography_Pkcs8_EncryptedReadFailed, e);
AsnWriter? pkcs8ZeroPublicKey = RewritePkcs8ECPrivateKeyWithZeroPublicKey(decryptedSpan);

if (pkcs8ZeroPublicKey == null)
{
throw new CryptographicException(SR.Cryptography_Pkcs8_EncryptedReadFailed, e);
}

try
{
bytesRead = len;
return ImportPkcs8(pkcs8ZeroPublicKey.EncodeAsSpan());
}
catch (CryptographicException)
{
throw new CryptographicException(SR.Cryptography_Pkcs8_EncryptedReadFailed, e);
}
}
finally
{
Expand Down Expand Up @@ -305,6 +350,46 @@ internal static unsafe Pkcs8Response ImportPkcs8PrivateKey(ReadOnlySpan<byte> so
}
}

// CNG cannot import a PrivateKeyInfo with the following criteria:
// 1. Is a EC key with explicitly encoded parameters
// 2. Is missing the PublicKey from ECPrivateKey.
// CNG can import an explicit EC PrivateKeyInfo if the PublicKey
// is present. CNG will also re-compute the public key from the
// private key if they do not much. To help CNG be able to import
// these keys, we re-write the PKCS8 to contain a zeroed PublicKey.
//
// If the PKCS8 key does not meet the above criteria, null is returned,
// signaling the original exception should be thrown.
private static unsafe AsnWriter? RewritePkcs8ECPrivateKeyWithZeroPublicKey(ReadOnlySpan<byte> source)
{
fixed (byte* ptr = &MemoryMarshal.GetReference(source))
{
using (MemoryManager<byte> manager = new PointerMemoryManager<byte>(ptr, source.Length))
{
PrivateKeyInfoAsn privateKeyInfo = PrivateKeyInfoAsn.Decode(manager.Memory, AsnEncodingRules.BER);
AlgorithmIdentifierAsn privateAlgorithm = privateKeyInfo.PrivateKeyAlgorithm;

if (privateAlgorithm.Algorithm.Value != Oids.EcPublicKey)
{
return null;
}

ECPrivateKey privateKey = ECPrivateKey.Decode(privateKeyInfo.PrivateKey, AsnEncodingRules.BER);
EccKeyFormatHelper.FromECPrivateKey(privateKey, privateAlgorithm, out ECParameters ecParameters);

if (!ecParameters.Curve.IsExplicit || ecParameters.Q.X != null || ecParameters.Q.Y != null)
{
return null;
}

byte[] zero = new byte[ecParameters.D!.Length];
ecParameters.Q.Y = zero;
ecParameters.Q.X = zero;

This comment has been minimized.

Copy link
@bartonjs

bartonjs Apr 4, 2020

Member

If it recomputes it whenever it's wrong, then wouldn't we just need to set Q.X = Q.Y = d?

This comment has been minimized.

Copy link
@vcsjones

vcsjones Apr 4, 2020

Author Member

@bartonjs not sure I follow, are you suggesting setting X and Y to D?

I'm not sure how I feel about putting a private parameter in a place where public values are expected. Perhaps CNG internally does better "I will clear out D when I am done" when handling the PrivateKeyInfo.

This comment has been minimized.

Copy link
@bartonjs

bartonjs Apr 4, 2020

Member

Ah, that's a fair point. Pre-coffee thought. :)

return EccKeyFormatHelper.WritePkcs8PrivateKey(ecParameters, privateKeyInfo.Attributes);
}
}
}

private static void FillRandomAsciiString(Span<char> destination)
{
Debug.Assert(destination.Length < 128);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Buffers;
using System.Collections;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.InteropServices;
using System.Security.Cryptography.Asn1;

Expand Down Expand Up @@ -97,7 +98,14 @@ internal static unsafe ECParameters FromECPrivateKey(ReadOnlySpan<byte> key, out
out ECParameters ret)
{
ECPrivateKey key = ECPrivateKey.Decode(keyData, AsnEncodingRules.BER);
FromECPrivateKey(key, algId, out ret);
}

internal static void FromECPrivateKey(
ECPrivateKey key,
in AlgorithmIdentifierAsn algId,
out ECParameters ret)
{
ValidateParameters(key.Parameters, algId);

if (key.Version != 1)
Expand Down Expand Up @@ -468,7 +476,7 @@ private static void WriteAlgorithmIdentifier(in ECParameters ecParameters, AsnWr
writer.PopSequence();
}

internal static AsnWriter WritePkcs8PrivateKey(ECParameters ecParameters)
internal static AsnWriter WritePkcs8PrivateKey(ECParameters ecParameters, AttributeAsn[]? attributes = null)
{
ecParameters.Validate();

Expand All @@ -480,11 +488,31 @@ internal static AsnWriter WritePkcs8PrivateKey(ECParameters ecParameters)
// Don't need the domain parameters because they're contained in the algId.
using (AsnWriter ecPrivateKey = WriteEcPrivateKey(ecParameters, includeDomainParameters: false))
using (AsnWriter algorithmIdentifier = WriteAlgorithmIdentifier(ecParameters))
using (AsnWriter? attributeWriter = WritePrivateKeyInfoAttributes(attributes))
{
return KeyFormatHelper.WritePkcs8(algorithmIdentifier, ecPrivateKey);
return KeyFormatHelper.WritePkcs8(algorithmIdentifier, ecPrivateKey, attributeWriter);
}
}

[return: NotNullIfNotNull("attributes")]
private static AsnWriter? WritePrivateKeyInfoAttributes(AttributeAsn[]? attributes)
{
if (attributes == null)
return null;

AsnWriter writer = new AsnWriter(AsnEncodingRules.DER);
Asn1Tag tag = new Asn1Tag(TagClass.ContextSpecific, 0);
writer.PushSetOf(tag);

for (int i = 0; i < attributes.Length; i++)
{
attributes[i].Encode(writer);
}

writer.PopSetOf(tag);
return writer;
}

private static void WriteEcParameters(ECParameters ecParameters, AsnWriter writer)
{
if (ecParameters.Curve.IsNamed)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,9 +252,12 @@ internal static class KeyFormatHelper
}
}

internal static AsnWriter WritePkcs8(AsnWriter algorithmIdentifierWriter, AsnWriter privateKeyWriter)
internal static AsnWriter WritePkcs8(
AsnWriter algorithmIdentifierWriter,
AsnWriter privateKeyWriter,
AsnWriter? attributesWriter = null)
{
// Ensure both input writers are balanced.
// Ensure both algorithm identifier and key writers are balanced.
ReadOnlySpan<byte> algorithmIdentifier = algorithmIdentifierWriter.EncodeAsSpan();
ReadOnlySpan<byte> privateKey = privateKeyWriter.EncodeAsSpan();

Expand Down Expand Up @@ -287,7 +290,13 @@ internal static AsnWriter WritePkcs8(AsnWriter algorithmIdentifierWriter, AsnWri
// PKI.privateKey
writer.WriteOctetString(privateKey);

// We don't currently accept attributes, so... done.
// PKI.Attributes
if (attributesWriter != null)
{
ReadOnlySpan<byte> attributes = attributesWriter.EncodeAsSpan();
writer.WriteEncodedValue(attributes);
}

writer.PopSequence();
return writer;
}
Expand Down

0 comments on commit e47356b

Please sign in to comment.