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

Add support for importing/exporting asymmetric key formats #30271

Merged
merged 11 commits into from Jun 21, 2018

Conversation

@bartonjs
Copy link
Member

@bartonjs bartonjs commented Jun 11, 2018

This adds support for SubjectPublicKeyInfo, PrivateKeyInfo (PKCS8) and
EncryptedPrivateKeyInfo (PKCS8) to AsymmetricAlgorithm, and adds support
for RSAPublicKey/RSAPrivateKey to RSA, and ECPrivateKey to the ECC types.

The input data to these methods is BER encoded. PEM/Base64 values must be
converted by the caller to the BER format.

Encrypted keys can be read provided they were encrypted under RFC 2898 PBES1,
RFC 7292 PKCS12-PBE, or RFC 2898 PBES2 (with PBKDF2 as the KDF).

New keys can be encrypted using PKCS12-PBE 3DES+SHA1 (for Windows
native interop support), or PBES2 with AES.

During the development of this change it was noticed that S.S.C.Algorithms
did not build with asserts enabled, so a small portion of this change is fixing
bad asserts written during the time that they were not running.

The CNG types defer to the native layer for PKCS8 operations, partially
because CNG makes use of attributes in the PrivateKeyInfo during import
and export (at least for ECDSA), and partially because CNG has a distinction
of exportable (encrypted) vs plaintext-exportable (not). The other platforms
currently just use the base class implementation, which utilizes the existing
ImportParameters and ExportParameters methods.

ECC keys are limited to named curves, based on RFC guidance. Explicit
curve support can be added when needed.

Fixes #20414.

This adds support for SubjectPublicKeyInfo, PrivateKeyInfo (PKCS8) and
EncryptedPrivateKeyInfo (PKCS8) to AsymmetricAlgorithm, and adds support
for RSAPublicKey/RSAPrivateKey to RSA, and ECPrivateKey to the ECC types.

The input data to these methods is BER encoded.  PEM/Base64 values must be
converted by the caller to the BER format.

Encrypted keys can be read provided they were encrypted under RFC 2898 PBES1,
RFC 7292 PKCS12-PBE, or RFC 2898 PBES2 (with PBKDF2 as the KDF).

New keys can be encrypted using PKCS12-PBE 3DES+SHA1 (for Windows
native interop support), or PBES2 with AES.

During the development of this change it was noticed that S.S.C.Algorithms
did not build with asserts enabled, so a small portion of this change is fixing
bad asserts written during the time that they were not running.

The CNG types defer to the native layer for PKCS8 operations, partially
because CNG makes use of attributes in the PrivateKeyInfo during import
and export (at least for ECDSA), and partially because CNG has a distinction
of exportable (encrypted) vs plaintext-exportable (not).  The other platforms
currently just use the base class implementation, which utilizes the existing
ImportParameters and ExportParameters methods.

ECC keys are limited to named curves, based on RFC guidance.  Explicit
curve support can be added when needed.
internal struct CRYPT_PKCS12_PBE_PARAMS
{
internal int iIterations; /* iteration count */
internal int cbSalt; /* byte size of the salt */
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Nit: is this commenting style used elsewhere? i.e. rather than:

// iteration count
// byte size of the salt

?

Copy link
Member Author

@bartonjs bartonjs Jun 11, 2018

Lazyish copying from wincrypt.h:

typedef struct _CRYPT_PKCS12_PBE_PARAMS
{
    int                 iIterations;        /* iteration count              */
    ULONG               cbSalt;             /* byte size of the salt        */
}
CRYPT_PKCS12_PBE_PARAMS;

I'll just remove them.


public override MemoryHandle Pin(int elementIndex = 0)
{
throw new NotImplementedException();
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Why NotImplementedException rather than return default;? Or if we want to throw an exception because it'll never be used, how about NotSupportedException? NotImplementedException suggests we just haven't gotten around to it yet.


namespace System.Buffers
{
internal unsafe class PinnedSpanMemoryManager<T> : MemoryManager<T> where T : struct
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Nit: Pointer instead of PinnedSpan? This type really doesn't care where the pointer and length came from.

Copy link
Member Author

@bartonjs bartonjs Jun 11, 2018

Fair point, changed (and changed the exception in Pin to NotSupportedException).

passwordBytes,
pbeParameters);

using (writer)
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Nit: could just be:

using (AsnWriter writer = RewriteEncryptedPkcs8PrivateKey(key, passwordBytes, pbeParameters)

Copy link
Member Author

@bartonjs bartonjs Jun 11, 2018

Hmm. I must have written it when the data triplet that became PbeParameters was still three adjacent parameters.

int rentWritten = 0;

// If we use 6 bits from each byte, that's 22 * 6 = 132
Span<char> randomString = stackalloc char[22];
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

It looks like we might be renting below if this is too small? We could afford a larger stackalloc'd buffer here if it's likely that would avoid renting.

Copy link
Member Author

@bartonjs bartonjs Jun 11, 2018

This span is just for the random password, as input. The write-to buffer is always rented (to cover the default size for all keys we'd need ~2kb)

{
// 33 (!) up to 33 + 63 = 96 (`)
destination[i] = (char)(33 + (randomKey[i] & 0b0011_1111));
}
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Could you just fill the original destination and then overwrite it? e.g. pseudo-code:

RandomNumberGenerator.Fill(MemoryMarshal.AsBytes(destination));
for (int i = 0; i < destination.Length; i++)
{
    destination[i] = (char)(33 + (destination[i] & 0b0011_1111));
}

? The downside there would be generating twice as many bytes as are actually needed, so it would be trading that against a stackalloc; not sure which is more costly.

if (typeof(TParsed) == typeof(ReadOnlyMemory<byte>))
{
ReadOnlyMemory<byte> tmp = spki.SubjectPublicKey;
parsed = (TParsed)(object)tmp;
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

You should be able to use Unsafe.As rather than relying on the JIT to recognize / transform this to avoid the boxing.

Copy link
Member Author

@bartonjs bartonjs Jun 11, 2018

I had that, and had a build error that I wasn't allowed to use the Unsafe package from shared framework. Theoretically there's a different Unsafe, which maybe is callable to things in the shared framework and not in System.Private.CoreLib; but it didn't seem worth hunting down.

Copy link
Member

@stephentoub stephentoub Jun 12, 2018

had a build error that I wasn't allowed to use the Unsafe package from shared framework

We use Unsafe from inside netcoreapp all over the place.

{
System.Text.Encoding encoding = System.Text.Encoding.UTF8;
int requiredBytes = encoding.GetByteCount(password);
Span<byte> passwordBytes = stackalloc byte[0];
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

This is to work around a compiler error about span lifetimes?

Copy link
Member Author

@bartonjs bartonjs Jun 11, 2018

Yep

System.Text.Encoding encoding = System.Text.Encoding.UTF8;
int requiredBytes = encoding.GetByteCount(password);
Span<byte> passwordBytes = stackalloc byte[0];
byte[] rentedPasswordBytes = Array.Empty<byte>();
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

How about moving the assignment to the else branch below?

{
CryptographicOperations.ZeroMemory(passwordBytes);

if (rentedPasswordBytes.Length > 0)
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Do you need this if check?

Copy link
Member Author

@bartonjs bartonjs Jun 11, 2018

Does ArrayPool special case the empty array to avoid the throw of "this wasn't my array"? Assuming no, yes :)

Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Yes, empty arrays can be returned. That said, I don't have a strong preference either way.


try
{
Span<byte> iv = stackalloc byte[cipher.BlockSize / 8];
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

This is guaranteed to be relatively small, right? There's no way this could be some user-written cipher that has a huge block size?

Copy link
Member Author

@bartonjs bartonjs Jun 11, 2018

Yep. An enum comes in, only 3DES (64 bit) and AES (128 bit) can come out. Added an assert.


private static CryptographicException AlgorithmKdfRequiresChars(string algId)
{
throw new CryptographicException(SR.Cryptography_AlgKdfRequiresChars, algId);
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Did you mean to return rather than throw here?

// The biggest block size (for IV) we support is AES (128-bit / 16 byte)
Span<byte> iv = stackalloc byte[16];

SymmetricAlgorithm cipher = OpenCipher(
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Does this need to be disposed?

byte[] tmpSalt = new byte[saltMemory.Length];

fixed (byte* tmpPasswordPtr = tmpPassword)
fixed (byte* tmpSaltPtr = tmpSalt)
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Why do these need to be pinned?

Copy link
Member

@stephentoub stephentoub Jun 11, 2018

(If it's to be able to clear them and know that no copies have been left in memory, it'd be worth adding a comment to that effect.)

Copy link
Member

@stephentoub stephentoub Jun 11, 2018

If that's the case, though, how does that work once this method returns? Doesn't the Rfc2898DeriveBytes instance store a reference to the arrays?

Copy link
Member Author

@bartonjs bartonjs Jun 12, 2018

Some of the pins are proper pin-then-fill; some of them are "we don't have API to allow that" best-efforts. Since compaction's "shadows" are problematic there's a feature request to coreclr to make compaction clear (and then most of the pinning in this PR can be removed).

Copy link
Member

@stephentoub stephentoub Jun 12, 2018

This one seems particularly useless, though. We're pinning only briefly and then handing out the arrays unpinned inside of the Rfc2898DeriveBytes instance, right?

Copy link
Member

@krwq krwq Jun 19, 2018

@stephentoub - array is cleared below (this might have got updated since when you wrote the comment)


byte[] tmpEnd = decryptor.TransformFinalBlock(Array.Empty<byte>(), 0, 0);

fixed (byte* tmpEndPtr = tmpEnd)
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Is this just defense in depth? There's a window here where tmpEnd could be moved around the heap, leaving shadows that won't be cleared.

Copy link
Member

@krwq krwq Jun 19, 2018

yes, we do it for as long as we can but we currently don't have ideal solution

<DefineConstants>INTERNAL_ASYMMETRIC_IMPLEMENTATIONS</DefineConstants>
<NoWarn>CA5351;$(NoWarn)</NoWarn>
<DefineConstants>$(DefineConstants);INTERNAL_ASYMMETRIC_IMPLEMENTATIONS</DefineConstants>
<NoWarn>CS3016;CA5351;$(NoWarn)</NoWarn>
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

As it stands, this library is paying for stackalloc's to be cleared. This change adds a bunch of them, so consider adding:

<ILLinkClearInitLocals>true</ILLinkClearInitLocals>

Copy link
Member Author

@bartonjs bartonjs Jun 14, 2018

To avoid potential scope creep I'll add ILLinkClearInitLocals in a followup change.

@@ -6,11 +6,15 @@
<AssemblyName>System.Security.Cryptography.Cng</AssemblyName>
<ProjectGuid>{4C1BD451-6A99-45E7-9339-79C77C42EE9E}</ProjectGuid>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<NoWarn>CS3016;$(NoWarn)</NoWarn>
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Same local init comment.

@@ -9,6 +9,7 @@
<ResourcesSourceOutputDirectory Condition="'$(IsPartialFacadeAssembly)' == 'true'">None</ResourcesSourceOutputDirectory>
<UsePackageTargetRuntimeDefaults Condition="'$(IsPartialFacadeAssembly)' != 'true'">true</UsePackageTargetRuntimeDefaults>
<IncludeDllSafeSearchPathAttribute>true</IncludeDllSafeSearchPathAttribute>
<NoWarn>CS3016</NoWarn>
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Same local init comment.

@@ -94,7 +134,7 @@ internal static byte[] ExportKeyBlob(SafeNCryptKeyHandle keyHandle, string blobT
IntPtr.Zero,
blobType,
IntPtr.Zero,
buffer,
ref buffer[0],
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

numBytesNeeded it guaranteed to be > 0?

Copy link
Member Author

@bartonjs bartonjs Jun 12, 2018

It doesn't really make sense in context that a zero-byte thing could be exported; but I added an if-zero-return-empty check anyways.

WriteKeyComponent(writer, dsaParameters.X, bitString: false);
writer.PopSequence();

CryptographicOperations.ZeroMemory(dsaParameters.X);
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

How important is this clearing? Does it need to be a finally in case, for example, we OOM while allocating AsnWriter? Also, is the writer dsaParameters.X copied to pinning / clearing the data? I'm generally wondering about all this clearing we're doing, and whether we're actually solving a problem.


_key.SetHandle(
keyHandle,
CngKeyLite.GetPropertyAsString(
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Can this throw? Just wondering if keyHandle would need to be disposed aggressively in that case.

Copy link
Member Author

@bartonjs bartonjs Jun 12, 2018

It can't throw under any reasonable situations. If we OOM on allocating the string I'm fine with letting keyHandle get finalized.

if (keyParameters != null && algId.Parameters != null)
{
ReadOnlySpan<byte> algIdParameters = algId.Parameters.Value.Span;
byte[] verify = ArrayPool<byte>.Shared.Rent(algIdParameters.Length);
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Where is this returned?

public static Pkcs8PrivateKeyInfo Decode(
ReadOnlyMemory<byte> source,
out int bytesRead,
bool skipCopy = false)
Copy link
Member

@stephentoub stephentoub Jun 11, 2018

Do we purposefully use skipCopy here but skipCopies in the public surface area earlier?

Copy link
Member Author

@bartonjs bartonjs Jun 11, 2018

Different methods.

The ctor takes in two ReadOnlyMemory values, and the same boolean controls both of them. So it's skipCopies.

Decode takes in one value, so it was skipCopy (singular). I'm fine with normalizing on plural, rationalized as it being about bytes-plural.

@bartonjs bartonjs requested review from krwq and morganbr Jun 16, 2018
[OptionalValue]
public byte? KeyLength;

[DefaultValue(
Copy link
Member

@krwq krwq Jun 18, 2018

some comment what this default value means would be useful

Copy link
Member Author

@bartonjs bartonjs Jun 19, 2018

it's in the ASN description as the struct comment. Though apparently with a weird newline. It's algid-hmacWithSHA1

new PbeParameters(
PbeEncryptionAlgorithm.TripleDes3KeyPkcs12,
HashAlgorithmName.SHA1,
10000);
Copy link
Member

@krwq krwq Jun 18, 2018

10000?

Copy link
Member Author

@bartonjs bartonjs Jun 19, 2018

That's a number that's small enough to be less than 0.1 seconds on almost all computers, but big enough to disassociate the derived key from the password. But, since we still would have the password in memory (because we'd be about to decrypt/re-encrypt) we could probably reduce this down to 1, with something like this statement as justification.

One presumes that if Windows is not currently rejecting numbers that are "too small" that they will continue to not reject them.

Copy link
Member

@krwq krwq Jun 19, 2018

Makes sense, might be useful to put a comment saying something similar

pbes2Params.EncryptionScheme.Algorithm.Value));
}

Rfc2898DeriveBytes pbkdf2 =
Copy link
Member

@krwq krwq Jun 19, 2018

any reason to put this outside of using? (same question for cipher below)

Copy link
Member Author

@bartonjs bartonjs Jun 19, 2018

Mainly just line length

throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
}

int iterationCount = ClampIterationCount(pbkdf2Params.IterationCount);
Copy link
Member

@krwq krwq Jun 19, 2018

Why clamping? Will it work with different implementation? Is this defined by spec or limitation on our side (if latter should we rather throw?)

Copy link
Member Author

@bartonjs bartonjs Jun 19, 2018

It does throw. So I'll rename it to ValidateIterationCount.

throw new CryptographicException(SR.Cryptography_Der_Invalid_Encoding);
}

int iterationCount = ClampIterationCount(pbeParameters.IterationCount);
Copy link
Member

@krwq krwq Jun 19, 2018

ditto

string hmacOid,
Span<byte> iv)
{
writer.PushSequence();
Copy link
Member

@krwq krwq Jun 19, 2018

is it possible to use serializer in here?

Copy link
Member Author

@bartonjs bartonjs Jun 19, 2018

It'd be possible, but because of the inside-out way things would need to be describe that would require a lot of stacked usings, and a couple intermediate byte arrays and probably some allocated Oid objects when only the string was needed.

if (isPkcs12)
{
    using (AsnWriter pkcs12PbeParams = AsnSerializer.Serialize(
        new PBEParameter
        {
            Salt = salt,
            IterationCount = iterationCount,
        })
    {
        AsnSerializer.Serialize(
            new AlgorithmIdentifierAsn
            {
                Id = new Oid(encryptionAlgorithmOid),
                Parameters = pkcs12PbeParameters.Encode()
            },
            writer);
    }
}
else
{
    // Like above, but with a different using statement for everything after WriteObjectIdentifier
}


// 1. Construct a string, D (the "diversifier"), by concatenating v/8 copies of ID.
int vBytes = v >> 3;
Span<byte> D = stackalloc byte[vBytes];
Copy link
Member

@krwq krwq Jun 19, 2018

should we check for max size here?

Copy link
Member Author

@bartonjs bartonjs Jun 19, 2018

The biggest defined v is 1024 (making vBytes 128). But I can add an assert, in case the RFC backpedals and adds more options (despite calling this KDF 'legacy').

int tmp = carry + into[i] + addend[i];
into[i] = (byte)tmp;
carry = tmp >> 8;
}
Copy link
Member

@krwq krwq Jun 19, 2018

should we check for carry == 0?

Copy link
Member Author

@bartonjs bartonjs Jun 19, 2018

I'm not sure I follow. Carry can be zero or one after any of the single-byte additions, and there's no prohibition of an overflow (it's just modular addition).

Treating I as a concatenation I_0, I_1, ..., I_(k-1) of v-bit
blocks, where k=ceiling(s/v)+ceiling(p/v), modify I by
setting I_j=(I_j+B+1) mod 2^v for each j.

The "mod" there just means "fixed-with addition disregarding carry".

Copy link
Member

@krwq krwq Jun 19, 2018

I was thinking of case when carry != 0 after the loop - if that is not an issue then disregard


private static void CircularCopy(ReadOnlySpan<byte> bytes, Span<byte> destination)
{
while (destination.Length > 0)
Copy link
Member

@krwq krwq Jun 19, 2018

I'd add a check for bytes.Empty so that we are not infinitely looping (or Debug.Assert if that shouldn't be possible)

Copy link
Member Author

@bartonjs bartonjs Jun 19, 2018

bytes.Length should always be 20 (all of the Pkcs12 KDFs are based on SHA-1), but I'll add the assert that it isn't empty.

{
if (destination.Length >= fullCopyLen)
{
int count = bigEndianUnicode.GetBytes(password, destination);
Copy link
Member

@krwq krwq Jun 19, 2018

I'd double check if this doesn't consume BOM (I think it doesn't but worth checking)

Copy link
Member Author

@bartonjs bartonjs Jun 19, 2018

It doesn't. StreamWriter is the only place that does BOMs.

<value>Field '{0}' on type '{1}' is declared [OptionalValue], but it can not be assigned a null value.</value>
</data>
<data name="Cryptography_AsnSerializer_PopulateFriendlyNameOnString" xml:space="preserve">
<value>Field '{0}' on type '{1}' has [ObjectIdentifier].PopulateFriendlyName set to true, which is not applicable to a string. Change the field to '{2}' or set PopulateFriendlyName to false.</value>
Copy link
Member

@krwq krwq Jun 20, 2018

nit: double space after dot

//
// FieldElement ::= OCTET STRING
[StructLayout(LayoutKind.Sequential)]
internal struct CurveAsn
Copy link
Member

@krwq krwq Jun 20, 2018

nit: Asn suffix seems redundant considering the namespace

//
// Since we don't support otherPrimeInfos (Version=1) just don't map it in.
[StructLayout(LayoutKind.Sequential)]
internal struct RSAPrivateKeyAsn
Copy link
Member

@krwq krwq Jun 20, 2018

ditto (unless the type name collides with something else then I'm ok with that)

Copy link
Member Author

@bartonjs bartonjs Jun 20, 2018

Yes, in general, the redundant Asn suffix is to avoid an existing conflict or a conceivable future conflict.

buffers[0] = new Interop.NCrypt.NCryptBuffer
{
BufferType = Interop.NCrypt.BufferType.PkcsSecret,
cbBuffer = checked(2 * (password.Length + 1)),
Copy link
Member

@krwq krwq Jun 20, 2018

why is it 2 * (password.Length + 1)? Isn't password already a byte array?

Copy link
Member

@krwq krwq Jun 20, 2018

ahh, it's char - nvm

}

// https://www.secg.org/sec1-v2.pdf, 2.3.4, #3 (M has length 2 * CEIL(log2(q)/8) + 1)
if (publicKeyBytes.Length != 2 * key.PrivateKey.Length + 1)
Copy link
Member

@krwq krwq Jun 20, 2018

is PrivateKey.Length already ceil(log2(q)/8)?

Copy link
Member Author

@bartonjs bartonjs Jun 20, 2018

Yeah, that means "the size in bytes"

}
finally
{
CryptographicOperations.ZeroMemory(publicKeyBytes);
Copy link
Member

@krwq krwq Jun 20, 2018

why do we care about clearing public key?

Copy link
Member Author

@bartonjs bartonjs Jun 20, 2018

Just tidiness. No reason to let it get fished back out of the array pool.

private AsnWriter WriteEcPrivateKey(bool includeDomainParameters)
{
bool returning = false;
AsnWriter writer = new AsnWriter(AsnEncodingRules.DER);
Copy link
Member

@krwq krwq Jun 20, 2018

Is it possible to use serializer in here?

[InlineData(null)]
public static void HashAlgorithm_NotVerified(string algId)
{
new PbeParameters(PbeEncryptionAlgorithm.Aes128Cbc, default(HashAlgorithmName), 1);
Copy link
Member

@krwq krwq Jun 20, 2018

I think this should be a separate test

Copy link
Member Author

@bartonjs bartonjs Jun 20, 2018

Fair enough. Split it up and added some verification that the non-validated values were preserved.

krwq
krwq approved these changes Jun 20, 2018
Copy link
Member

@krwq krwq left a comment

I think we can fix any issues separately. This PR is large enough I'm not sure I'll have time for the second pass - I didn't see anything outstanding.

@bartonjs
Copy link
Member Author

@bartonjs bartonjs commented Jun 20, 2018

@dotnet-bot Test Packaging All Configurations x64 Debug Build please (Jenkins: java.io.IOException: Failed to load build state)

@bartonjs bartonjs merged commit 9173a57 into dotnet:master Jun 21, 2018
15 checks passed
@karelz karelz added this to the 3.0 milestone Jul 8, 2018
@bartonjs bartonjs removed their assignment Nov 23, 2018
@bartonjs bartonjs deleted the crypto_keyformats branch May 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants