-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
CryptographicException decoding TLS cert in 5.0.0 preview, linux only (Fine in 3.1.2) #33744
Comments
@bartonjs regression apparently? |
I'll have a look. |
Full stack for reference. Unhandled exception. System.Security.Cryptography.CryptographicException: The certificate data cannot be read with the provided password, the password may be incorrect.---> System.Security.Cryptography.CryptographicException: A certificate referenced a private key which was already referenced, or could not be loaded. at Internal.Cryptography.Pal.UnixPkcs12Reader.BuildCertsWithKeys(CertBagAsn[] certBags, AttributeAsn[][] certBagAttrs, CertAndKey[] certs, Int32 certBagIdx, SafeBagAsn[] keyBags, RentedSubjectPublicKeyInfo[] publicKeyInfos, AsymmetricAlgorithm[] keys, Int32 keyBagIdx) at Internal.Cryptography.Pal.UnixPkcs12Reader.Decrypt(ReadOnlySpan`1 password, ReadOnlyMemory`1 authSafeContents) at Internal.Cryptography.Pal.UnixPkcs12Reader.VerifyAndDecrypt(ReadOnlySpan`1 password, ReadOnlyMemory`1 authSafeContents) at Internal.Cryptography.Pal.UnixPkcs12Reader.Decrypt(SafePasswordHandle password) --- End of inner exception stack trace --- at Internal.Cryptography.Pal.UnixPkcs12Reader.Decrypt(SafePasswordHandle password) at Internal.Cryptography.Pal.PkcsFormatReader.TryReadPkcs12(OpenSslPkcs12Reader pfx, SafePasswordHandle password, Boolean single, ICertificatePal& readPal, List`1& readCerts) at Internal.Cryptography.Pal.PkcsFormatReader.TryReadPkcs12(Byte[] rawData, SafePasswordHandle password, Boolean single, ICertificatePal& readPal, List`1& readCerts, Exception& openSslException) at Internal.Cryptography.Pal.OpenSslX509CertificateReader.FromBlob(Byte[] rawData, SafePasswordHandle password, X509KeyStorageFlags keyStorageFlags) at System.Security.Cryptography.X509Certificates.X509Certificate..ctor(Byte[] rawData, String password, X509KeyStorageFlags keyStorageFlags) at System.Security.Cryptography.X509Certificates.X509Certificate2..ctor(Byte[] rawData, String password) at Program.Main() in /code/personal/scratch/Program.cs:line 10 |
Reduced test case, this fails in Linux but works on Windows. using System;
using System.Security.Cryptography;
public class Program {
static void Main() {
string keyStr = "ME0CAQAwEwYHKoZIzj0CAQYIKoZIzj0DAQcEMzAxAgEBBCDaB5vgkVIrCMAOrzS5QzWy4DZ1Jrp7xZDaND0HOOweHaAKBggqhkjOPQMBBw==";
var key = Convert.FromBase64String(keyStr);
using var ecdsa = ECDsa.Create();
ecdsa.ImportPkcs8PrivateKey(key, out _);
Console.WriteLine(ecdsa.KeySize);
}
} |
@vcsjones ... I feel the need to inform you that you are a genius. Just sayin'. |
@joshlang @bartonjs gist of the issue is that the Basically, we're running in to this condition: runtime/src/libraries/Common/src/System/Security/Cryptography/EccKeyFormatHelper.cs Lines 108 to 109 in f80a514
The private key does carry the private key, so it seems that Windows is re-deriving the public key from the private key. The PKCS8 helpers require the pre-calculated public key. The comment there makes me think this is a known limitation. The behavior difference is that on Windows, CNG handles all of the PKCS8 key import. On Linux, it is decoded and the parameters are imported manually. It regressed in .NET Core 5.0 because the PKCS12 reader is going from a "implemented mostly with OpenSSL" to a more managed implementation. The no-public-key limitation exists in 3.1, however PKCS12 reading didn't use it until 5.0. I would assume that OpenSSL's PKCS12 reader is also re-deriving the public key when needed. As a work around, if you round-trip your certifcate / p12 through openssl, the re-exported contents will work in .NET Core 5.0.
Where |
So I guess we need to look into whether we can get an import to work when we have D but not Q. My gut says that something (probably macOS) was too picky about something with PKCS#8, which is why I didn't send it to the native layers on Unix systems. (Windows CNG respects some attributes to limit key usage, and we ignore attributes, so we had to ask Windows to do the import for us on Windows) |
I don't know about macOS, but for openssl we can re-calculate it with Is it worth fixing for the openssl code path at least? |
I've run into this D but not Q limitation several times before, whether for this, or stuff like deriving bitcoin addresses from private keys, etc. I must admit... I've always wondered - why not just do the calculation? I'm only familiar with a couple curves - so maybe it's not so easy as an EC multiplication for all cases? |
We don't have an ECC calculator in the .NET layer; we work with the underlying system libraries opaquely through key objects... so then we're subject to limitations they have at the import/export/create boundary.
Absent the PFX regression I'd say it's a nice-to-have enhancement. Given the context of this report, I'd say we should do it, to prevent the regression.
Looks like it might be EC_GROUP* group = EC_KEY_get0_group(key);
EC_POINT* pubkey = EC_POINT_new(group);
EC_POINT_mul(group, pubkey, d, NULL, NULL, NULL); with proper error checking, of course. We could do that in https://github.com/dotnet/runtime/blob/master/src/libraries/Native/Unix/System.Security.Cryptography.Native/pal_ecc_import_export.c#L374 as an If we can make Q.X == null && Q.Y == null work for Windows (worst case? make a PKCS#8 😄) and macOS, we can just soften the constraints in ImportParameters to require |
Fixing it has my vote too (even though there's a workaround). We generate certificates using the popular Certes library to generate TLS certificates from Let's Encrypt. I imagine that it's a common enough scenario that it's worth fixing.
^-- easy fix. Just import BouncyCastle into .net core runtime. ............lol |
Unfortunately, this doesn't work (or I'm doing it wrong). The command used to transform: Test code - the raw byte array is the
In windows, the certificate will not load. I get an
In linux, the certificate loads successfully. However, later:
|
Oh, I thought I saw this work, I'm guessing I did something silly. (See, there is plenty of time left for me to prove I am not a genius) It's actually outputting a textual dump of the p12 file. Let me see again.. |
Looking closer at this, OpenSSL seems to be trying really really hard to preserve the key as-written. The easiest thing at this point I think would be to do |
Our pipeline, including certificate generation, all runs in linux. I'll see if I can find a different workaround. I'll post if I find one. |
I understand it's complex. I really think it should be considered one day, lest ye always be chasing inconsistencies between platforms. |
I did some noodling to fix this in #33874. The Linux/OpenSSL case works, macOS I am tinkering with. I think this is doable in macOS, but I have the world's slowest Mac which doesn't help that macOS's APIs around all of this makes little sense to me. @bartonjs instead of broadly permitting ECParameters Q to allow a null point, how would you feel about making a more targeting fix just for the Pkcs8/ECPrivateKey import code paths to start? I'm not suggesting it one way or the other right now, but it might be easier and wondering if that's sensible. |
ImportParameters is abstract, so all custom types have to implement it. Doing magic in the PKCS8/ECPrivateKey layer means all custom types have to do the same "OK, I'll let this work" magic. Sure, some types might end up needing to reject no-Q; but that seems easier for them to handle than intercepting the other import flows. So... I'd rather not; but if it's complicatedly required, then I'll accept it. |
@bartonjs is this widespread enough impact that we should try to hurry it into preview 3? I see only one report above. |
@bartonjs another thought if we want something soonish for Preview 3: I have it working in CNG and OpenSSL, still need to figure out MacOS (haven't ruled it out..). Would you take a PR that fixes CNG/OpenSSL and MacOS throws a PNSE, and a separate PR to get MacOS working, or prefer to try and get it in all in one go? Trying to figure if I should focus on MacOS or getting what I have polished and fully tested. |
@vcsjones Let's go with "polish and PR". Maybe one of us, or a mysterious third party, will come up with something easy to slide in for macOS before it gets merged. |
This has been fixed. I don't believe the fixes made the cut for preview3, but I did confirm that your original repro works with the latest nightly on Linux.
This limitation has been removed. An EC private key that does not contain Q but does contain D will now work, and Q will be re-derived by the platform as needed. |
@vcsjones Thanks! I saw the pull request close yesterday, with much excitement :D I kept refreshing nuget to see if preview3 was out. Alas, I'll be patient and wait for the next one! Thanks for taking care of this. |
Hi.
Here's a let's-encrypt certificate that decrypted just fine in 3.1.2, but after upgrading to 5.0.0 preview 1, started exploding in linux only.
(It's valid - but it's a dev cert in a dev environment, so no big deal).
In linux-only, I get
System.Security.Cryptography.CryptographicException: 'The certificate data cannot be read with the provided password, the password may be incorrect.'
My docker-file is default from Right-click -> Add docker support in visual studio. The docker image is runtime/sdk :5.0-buster
The text was updated successfully, but these errors were encountered: