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

Export certificate fails under some circumstances #38864

Closed
javiercn opened this issue Jul 7, 2020 · 11 comments · Fixed by #38884
Closed

Export certificate fails under some circumstances #38864

javiercn opened this issue Jul 7, 2020 · 11 comments · Fixed by #38884
Assignees
Milestone

Comments

@javiercn
Copy link
Member

javiercn commented Jul 7, 2020

As part of working on an improvement to dotnet-dev-certs, the export functionality will fail in some cases and will leave an array full of 0s instead of the value of the exported certificate.

Screen Shot 2020-07-07 at 12 42 10 PM

This is the appropriate line:
https://github.com/dotnet/aspnetcore/pull/23567/files#diff-a2cc6679db539b8b3a2815040aa2146cR451

I've tried to create a minimal repro project but I have not been able to do so. If you want to reproduce it, you can clone the branch and debug with the following command.

dotnet run -- https --trust -ep cert3.crt -p 1234 --key-format pem --debug

This is not blocking us since it is not a command we expect users to run manually and tooling would not run this combination of flags, but it would be good if we get to the bottom of why this is not working properly.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 7, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@vcsjones
Copy link
Member

vcsjones commented Jul 7, 2020

I'll take a look. /cc @bartonjs for labels.

@ghost
Copy link

ghost commented Jul 7, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

@vcsjones
Copy link
Member

vcsjones commented Jul 7, 2020

The problem is that Export(X509ContentType.Cert) hands out the underlying byte[] of the PAL, instead of a copy like RawData. Minimal repro on macOS:

X509Certificate2 cert = new X509Certificate2(Convert.FromBase64String(@"
MIIDMDCCAhgCCQD03ySqFW/iiTANBgkqhkiG9w0BAQsFADBaMQswCQYDVQQGEwJV
UzERMA8GA1UECAwIVmlyZ2luaWExEzARBgNVBAcMCkFsZXhhbmRyaWExDTALBgNV
BAoMBEhvbWUxFDASBgNVBAMMC0RldmVsb3BtZW50MB4XDTIwMDcwNzE1NDAxMloX
DTIwMDgwNjE1NDAxMlowWjELMAkGA1UEBhMCVVMxETAPBgNVBAgMCFZpcmdpbmlh
MRMwEQYDVQQHDApBbGV4YW5kcmlhMQ0wCwYDVQQKDARIb21lMRQwEgYDVQQDDAtE
ZXZlbG9wbWVudDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAM3SEHCc
5NaXKtYmajY72Z/XnAcxHVR8N7rsCxyoSbsloNji9RVVebvoD7uBeWpj3eimNUiw
Uy+mFi1EmNo+cg4Tn+W4yTp4hhqQwOYLtbhKfgpvtRZEO04NCj/4q4g+cA8i12M3
TJObXPT9vx+02OKDiysiy0mupQuJD6JYsxA8zG7tbZ12RMeq7Kk7f7Wt0Uit1KeY
4TQa/6Vqyq7yl4r+BeQpxomXvcmTFTfQFrx21vC5t/xkn6RiL/sYm8H+S1RK64OS
TckMooJ5R5z30wTx3RuH1k893bSEwdHW3DoFlFWxYHnvx+ye7q3KW0Lum9mhT4XC
LVXNdH7qUvTCpxMCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEACUjSqBTJ2QaCcY2/
vLXzSZueO3qTPGEfUp3dtOP5eXSnY7id2TTm7qRwhzG3dWqAnbVZ/ehuL2jgOOPz
db014K//tR/g/5PCt1T33epdiPrAN09B99y9yOLIzZdequvCTwdoVqd983fKmmRh
mjOG+/OTDMUyQXxro6faiY68CDrDYjMLAtKlKlGLmHO5mpSrYR7/iJaVHkjOJ7B5
vF2UvDMXN6lPurcOUsllmdVWqFtXIvleq3DpYV/hikrbFsQEb9LcTBcO1cPbj73C
I1ReDBol7RC0CULi1B+95+/4TgW+aENy2TfcPLXJXGNp5dDByo4CZ0pw5voQ+mca
xcveRg=="));
byte[] export = cert.Export(X509ContentType.Cert);
Array.Clear(export, 0, export.Length);
Console.WriteLine(BitConverter.ToString(cert.RawData));

@javiercn

In your case, you're calling export twice, once with the password to get the private key, and one again to get just the public part. After the first call, it cleans up at the end by disposing and zeroing things, including the internal rawdata of the certificate PAL.

Fix for now would be to not Array.Clear the result of Export only when the content type is Cert since it only contains the public part of the certificate.

This seems undesirable, so I will submit a PR to fix this here as well.

@javiercn
Copy link
Member Author

javiercn commented Jul 7, 2020

@vcsjones thanks for the quick turnaround!

If this is fixed before 5.0 I'm fine just waiting for the actual fix in corefx.

@vcsjones
Copy link
Member

vcsjones commented Jul 7, 2020

Interestingly, this reproduces in Core 3.1, so it isn't a regression.

@bartonjs
Copy link
Member

bartonjs commented Jul 7, 2020

Following the breadcrumbs, I assume this is only happening on macOS? Looks like all of the other PALs return new arrays from ICertificatePalCore.RawData, but that one returns a field.

Presumably that means both Export(Cert) and RawData need tests that call it twice and Assert.NotSame on the two results.

@javiercn
Copy link
Member Author

javiercn commented Jul 7, 2020

Yep, at least windows is fine

@bartonjs bartonjs added bug os-mac-os-x macOS aka OSX and removed untriaged New issue has not been triaged by the area owner labels Jul 7, 2020
@bartonjs bartonjs modified the milestones: Future, 5.0.0 Jul 7, 2020
@vcsjones
Copy link
Member

vcsjones commented Jul 7, 2020

I assume this is only happening on macOS?

Yes, does not reproduce on Linux or Windows.

@vcsjones
Copy link
Member

vcsjones commented Jul 7, 2020

@bartonjs Poking around more at this, there are other properties that exhibit similar behavior. For example, properties of type X500DistinguishedName:

byte[] export = cert.IssuerName.RawData;
Array.Clear(export, 0, export.Length);
Console.WriteLine(cert.Issuer); //Writes nothing

// Repeat above for SubjectName / Subject.

This actually reproduces on Linux as well.

Should these be addressed similarly?

@bartonjs
Copy link
Member

bartonjs commented Jul 7, 2020

Should these be addressed similarly?

Yes, please 😦

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants