867437 - Remove calls to nsIX509Cert.verifyForUsage in mailnews

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thunderbird has a few places that have this pattern:

   cert = certdb.FindCertByEmailAddress(...)
   cert.verifyForUsage(CERT_USAGE_EmailRecipient);

The call to verifyForUsage is unnecessary because FindCertByEmailAddress already verifies the cert is valid for CERT_USAGE_EmailRecipient:

    SECStatus srv = certVerifier->VerifyCert(node->cert,
                                             certificateUsageEmailRecipient,
                                             PR_Now(), nullptr /*XXX pinarg*/);

nsIX509Cert.verifyForUsage is being removed so these calls in Thunderbird need to be removed to avoid breaking Thunderbird's S/MIME support.

/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp (View Hg log or Hg annotations)

    line 833 -- cert->VerifyForUsage(nsIX509Cert::CERT_USAGE_EmailRecipient, &verification_result))

/mailnews/extensions/smime/src/nsSMimeJSHelper.cpp (View Hg log or Hg annotations)

    line 132 -- cert->VerifyForUsage(nsIX509Cert::CERT_USAGE_EmailRecipient, &verification_result)))
    line 276 -- cert->VerifyForUsage(nsIX509Cert::CERT_USAGE_EmailRecipient, &verification_result))

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The bug for the removal of nsIX509Cert.verifyForUsage is bug 867432.

Comment 2

[Honza Bambas (:mayhemer)]

Adding dep.

Comment 3

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

This removes it from the smime parts. 

I see testpilot got removed from firefox in bug 867445. We probably should too. Even if gets re-added it's probably just as easy to re-add as to port any changes they made. (We do seem to have the option to keep it, but scrub the verifyForUsage usage, per bug 867430)

Comment 4

[Magnus Melin [:mkmelin]]

Filed bug 883629 about test pilot removal.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 763137 [details] [diff] [review]
proposed fix

Review of attachment 763137 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +827,5 @@
> +                                           getter_AddRefs(cert));
> +      if (NS_FAILED(res)) {
> +        // Failure to find a valid encryption cert is fatal.
> +        // Here I assume that mailbox is ascii rather than utf8.
> +        SetErrorWithParam(sendReport, NS_LITERAL_STRING("MissingRecipientEncryptionCert").get(), mailbox);

Wrap this line at 80 characters.

@@ -850,5 @@
> -     if (...) no_clearsigning_p = true;
> -     (This is the only reason we even bother looking up the certs
> -     of the recipients if we're sending a signed-but-not-encrypted
> -     message.)
> -     */

I suggest that you leave this TODO comment in.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 763137 [details] [diff] [review]
proposed fix

Review of attachment 763137 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments that Brian mentioned fix.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

I went ahead and pushed this to fix comm-central's being busted by bug 867432 landing on m-c. I did my best to address Brian's comments before pushing (re-adding the deleted comment and attempting to wrap at 80 characters), but I didn't see an obvious way to get one of the lines under 80.

https://hg.mozilla.org/comm-central/rev/faccb99fbc10