Remove calls to nsIX509Cert.verifyForUsage in mailnews

RESOLVED FIXED in Thunderbird 24.0


MailNews Core
Security: S/MIME
4 years ago
4 years ago


(Reporter: briansmith, Assigned: Magnus Melin)


Thunderbird 24.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment)

Thunderbird has a few places that have this pattern:

   cert = certdb.FindCertByEmailAddress(...)

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

    SECStatus srv = certVerifier->VerifyCert(node->cert,
                                             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))
The bug for the removal of nsIX509Cert.verifyForUsage is bug 867432.
See Also: → bug 867432
Summary: Remove calls to nsIX509Cert.verifyForUsage → Remove calls to nsIX509Cert.verifyForUsage in mailnews
Adding dep.
Depends on: 867432
Blocks: 867432
No longer depends on: 867432

Comment 3

4 years ago
Created attachment 763137 [details] [diff] [review]
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)
Assignee: nobody → mkmelin+mozilla
Attachment #763137 - Flags: review?(mbanner)
Attachment #763137 - Flags: feedback?


4 years ago
Attachment #763137 - Flags: feedback?


4 years ago
Depends on: 883629

Comment 4

4 years ago
Filed bug 883629 about test pilot removal.
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.
Attachment #763137 - Flags: feedback+
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.
Attachment #763137 - Flags: review?(mbanner) → review+
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.
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 24.0
Depends on: 885292
You need to log in before you can comment on or make changes to this bug.