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

Support ECParameters that contain only D (Win, Linux). #33874

Merged
merged 25 commits into from
Apr 5, 2020

Conversation

vcsjones
Copy link
Member

@vcsjones vcsjones commented Mar 20, 2020

These changes allows importing EC keys for ECDsa and ECDiffieHellman where the private key (D) is specified but the public key (Q) is not.

This change is to remove a limitation on ECPrivateKey which, by definition, is not required to carry the public key, only the private key. It is expected that such keys are able to re-derive the public key from the private key. Prior to this change, the pre-computed public key was required.

MacOS does not currently support this. CNG and OpenSSL do.

Contributes to #33744

If D (private key) is supplied but not the public key (Q), permit
this and allow the platform to re-calculate the public key from
the private key.
If Q is missing for an ECParameters structure, import it
as PKCS8 and let CNG construct Q appropriately.
@vcsjones vcsjones changed the title [WIP] Support ECParameters that contain only D. Support ECParameters that contain only D. Mar 30, 2020
@vcsjones

This comment has been minimized.

@vcsjones vcsjones marked this pull request as ready for review March 30, 2020 05:00
@vcsjones
Copy link
Member Author

/cc @bartonjs


try
{
if (!writer.TryEncode(pkcs8Buffer, out int encodedWritten))
Copy link
Member

Choose a reason for hiding this comment

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

Meta, for AsnWriter API purposes: I wonder if some sort of SendData<TState>(TState state, SpanAction<byte, TState> processor) would be useful to avoid this ceremony without over-exposing the Span.

Copy link
Member Author

@vcsjones vcsjones Mar 31, 2020

Choose a reason for hiding this comment

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

Well, it turns out I didn't need any of this code anyway 😅.

I am curious why not make EncodeAsSpan public?

Okay yes I see how it might be a bad idea. I suppose a public method that returns a span and marks it no longer writable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, were you thinking something like this?

public void SendData<TState>(TState state, SpanAction<T, byte> processor) {
  // Validate
  processor(state, _buffer.AsSpan(0, _offset));
}

Yeah that would be useful, I would think.

@bartonjs
Copy link
Member

I could not figure out a way to get CNG to accept a "no public key but has explicit parameters" key

Is that still true with the change to use Q=(0,0) blobs instead of PKCS#8?

@vcsjones
Copy link
Member Author

@bartonjs

Is that still true with the change to use Q=(0,0) blobs instead of PKCS#8?

For ECParameters, "(0,0)" works for explicit curves. However for PKCS8, it does not work. CNG does it's own PKCS8 handling it appears. That is, when using NCRYPT_PKCS8_PRIVATE_KEY_BLOB, the import fails (with an access violation?)

Unhandled exception. Internal.Cryptography.CryptoThrowHelper+WindowsCryptographicException: Unknown error (0xc0000005)
   at System.Security.Cryptography.CngKey.Import(ReadOnlySpan`1 keyBlob, String curveName, CngKeyBlobFormat format, CngProvider provider)
   at System.Security.Cryptography.CngPkcs8.ImportPkcs8(ReadOnlySpan`1 keyBlob)
   at System.Security.Cryptography.CngPkcs8.ImportPkcs8PrivateKey(ReadOnlySpan`1 source, Int32& bytesRead)
   at System.Security.Cryptography.ECDsaCng.ImportPkcs8PrivateKey(ReadOnlySpan`1 source, Int32& bytesRead)
   at Test.Program.Main(String[] args) in D:\code\personal\scratch\Program.cs:line 20
using ECDsaCng e = new ECDsaCng();
string keyStr = @"
MIIBMwIBADCCAQMGByqGSM49AgEwgfcCAQEwLAYHKoZIzj0BAQIhAP////8AAAAB
AAAAAAAAAAAAAAAA////////////////MFsEIP////8AAAABAAAAAAAAAAAAAAAA
///////////////8BCBaxjXYqjqT57PrvVV2mIa8ZR0GsMxTsPY7zjw+J9JgSwMV
AMSdNgiG5wSTamZ44ROdJreBn36QBEEEaxfR8uEsQkf4vOblY6RA8ncDfYEt6zOg
9KE5RdiYwpZP40Li/hp/m47n60p8D54WK84zV2sxXs7LtkBoN79R9QIhAP////8A
AAAA//////////+85vqtpxeehPO5ysL8YyVRAgEBBCcwJQIBAQQgO/HNs4GTcN1s
MVZ2GRwFlvPnq2YEUrzTEcSyUEr6axY=";
byte[] key = Convert.FromBase64String(keyStr);
e.ImportPkcs8PrivateKey(key, out _);
// generated with:
// openssl ecparam -genkey -name prime256v1 -noout | openssl ec -param_enc explicit -no_public | openssl pkey

That leaves the following options.

  1. Leave explicit parameters with no public keys unsupported.
  2. Allow empty Q for explicit curves on ECParameters-style imports, but leave it not-working with PKCS8. This would fix ImportParameters but not the ECPrivateKey/PKCS8 paths, with is where we were hitting the issue in the first place.
  3. Special-path "no public key" PKCS8 imports in CNG, decode it ourselves, import via ECParameters.

@vcsjones
Copy link
Member Author

vcsjones commented Apr 1, 2020

Re: MacOS, I can look at that in about a week when a new Mac Mini gets delivered. Until then, my MacBook just can't muscle through this.

@bartonjs
Copy link
Member

bartonjs commented Apr 1, 2020

For ECParameters, "(0,0)" works for explicit curves. However for PKCS8, it does not work

I've reached out to my contacts to see if we can get that state changed. In the meantime... so that we don't have to try supporting key state parity with their PKCS#8 attribute parsing maybe it's

  • Try PKCS#8 import, as-is. On success: done.
  • Crack the payload to see if it's explicit parameters with no public key (if not, let the error/exception go).
  • If the payload has no attributes: import the equivalent blob, done.
  • Import the equivalent blob, export it to get Q
  • Insert Q in the original PKCS#8, import that.

It's not pretty... but it works.

@vcsjones
Copy link
Member Author

vcsjones commented Apr 1, 2020

It's not pretty... but it works.

Makes sense. I think that will keep me busy for a bit. 😄

@vcsjones
Copy link
Member Author

vcsjones commented Apr 1, 2020

@bartonjs how much review have you given this? Is it okay for me to rebase / squash this branch?

@bartonjs
Copy link
Member

bartonjs commented Apr 1, 2020

how much review have you given this?

Fully, I think.

Is it okay for me to rebase / squash this branch?

If you need new stuff from master, merging would be preferred. (Sitting stale is fine)

If you expect that it's going to be effectively a total rewrite from here, then I guess things that require a force-push are OK.

@vcsjones
Copy link
Member Author

vcsjones commented Apr 1, 2020

If you expect that it's going to be effectively a total rewrite from here, then I guess things that require a force-push are OK.

No, I don't think that is the case. I'll stick with merge commits. I try not to let branches get too stale, at least hoping to be able to pick up some changes that fix test stability.

@vcsjones
Copy link
Member Author

vcsjones commented Apr 1, 2020

@bartonjs This will be *NO REVIEW* for a while. I'll ping when ready.

@bartonjs bartonjs added the NO-REVIEW Experimental/testing PR, do NOT review it label Apr 1, 2020
@vcsjones

This comment has been minimized.

@vcsjones
Copy link
Member Author

vcsjones commented Apr 4, 2020

@bartonjs I think this is ready for review again.

To clarify something: the CNG PKCS8 importer doesn't like no PublicKey on the ECPrivateKey just for explicit curves. However, I incorrectly determined that it needed the correctly computed public key. Pushing 0,0 into the PKCS8 for the public key is enough to get CNG happy again, so we can avoid round-tripping through ECParameters to get the right public key.

@bartonjs bartonjs removed the NO-REVIEW Experimental/testing PR, do NOT review it label Apr 5, 2020
@bartonjs bartonjs merged commit 6d395de into dotnet:master Apr 5, 2020
@bartonjs
Copy link
Member

bartonjs commented Apr 5, 2020

Thanks for jumping on this, @vcsjones.

@vcsjones vcsjones deleted the 33744-fix branch April 5, 2020 19:12
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants