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

Chain not properly set when signing a S/MIME mail with a PKCS#12 file #515

Closed
mtausig opened this issue Nov 1, 2019 · 9 comments
Closed
Labels
bug Something isn't working

Comments

@mtausig
Copy link

mtausig commented Nov 1, 2019

Describe the bug

I tried to sign a S/MIME mail with a key stored in a PKCS#12 file:

var smimeCtx = new TemporarySecureMimeContext();
smimeCtx.Import("/tmp/my.p12", "1234");


var message = new MimeMessage();
message.From.Add(new MailboxAddress("", "[email protected]"));
message.To.Add(new MailboxAddress("", "[email protected]"));
message.Subject = "Foo";

message.Body = new TextPart(MimeKit.Text.TextFormat.Html) { Text = "Test"}
var signature = MultipartSigned.Create(smimeCtx, message.From.Mailboxes.First(), DigestAlgorithm.Sha256, message.Body);
message.Body = signature;

The problem is, that the certificate's chain is not included in the signature (even after issue #500 was fixed).

I tried to trace the problem down and found that the GetTrustedAnchors function (which is used in the BuildCertificateChain method) simply returns all certificates imported from the P12 file as trust anchors, resulting in a chain which only contains the signing certificate.

protected override Org.BouncyCastle.Utilities.Collections.HashSet GetTrustedAnchors ()
{
var anchors = new Org.BouncyCastle.Utilities.Collections.HashSet ();
foreach (var certificate in certificates)
anchors.Add (new TrustAnchor (certificate, null));
return anchors;

protected IList<X509Certificate> BuildCertificateChain (X509Certificate certificate)

Platform (please complete the following information):

  • OS: Linux
  • .NET Runtime: Mono
  • .NET Framework: 4.7
  • MimeKit Version: 2.3.2 (from NuGet)

To Reproduce

Run the code above and parse the message produced

Expected behavior

The full certificate chain should be included in the signature

jstedfast added a commit that referenced this issue Nov 2, 2019
@jstedfast jstedfast added the bug Something isn't working label Nov 2, 2019
@jstedfast
Copy link
Owner

MimeKit 2.4.0 has just been released with this feature.

@mtausig
Copy link
Author

mtausig commented Nov 3, 2019

I haven't tested it, but if I read the patch correctly, this does not properly work for chains with >= 3 certificates, since all certificates with the KeyCertSign keyUsage (and thus also intermediates) are used as trust anchors.

keyUsage[(int) X509KeyUsageBits.KeyCertSign] = true;
selector.KeyUsage = keyUsage;

@jstedfast
Copy link
Owner

Fair enough. Got any suggestions on what restrictions to use?

@mtausig
Copy link
Author

mtausig commented Nov 3, 2019

Fair enough. Got any suggestions on what restrictions to use?

I'd just take all self signed certificates.

@jstedfast
Copy link
Owner

Unfortunately doing that makes quite a few of my S/MIME unit tests fail because Bouncy Castle cannot handle Version 1 certificates as CA certificates.

From what I can tell, that's not actually a bug in my code so much as it's a "bug" in my generated certificates that I use for testing.

I would appreciate it if you took the time to read over this script: https://github.com/jstedfast/MimeKit/blob/master/UnitTests/TestData/smime/gencerts.sh

Assuming that my understanding about Version 1 X.509 certificates is correct and that my generated certificates should be using Version 3, it would be a HUGE help if you could fix my gencerts.sh script to generate them.

If not, it'll probably take me a week or more to get to this because I'm not going to make a release where my unit tests fail and I probably won't have time to work on this again until next weekend at the earliest.

@jstedfast jstedfast reopened this Nov 4, 2019
@jstedfast
Copy link
Owner

Reopening this for now.

jstedfast added a commit that referenced this issue Nov 5, 2019
… chains

GetTrustedAnchors() now *only* includes self-signed certificates that
have the CertKeySign KeyUsage attribute.

GetIntermediates() now *only* includes non-self-signed certificates
that have the CertKeySign KeyUsage attribute.

Fixes issue #515
@jstedfast
Copy link
Owner

Please test this fix by installing a nuget from https://www.myget.org/feed/mimekit/package/nuget/MimeKit

You'll need >= v2.4.0.4

@mtausig
Copy link
Author

mtausig commented Nov 8, 2019

I can confirm that it works with 2.4.0.8.
Thanks a lot for the fix.

@jstedfast
Copy link
Owner

Awesome, thanks for confirming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants