Last Comment Bug 867437 - Remove calls to nsIX509Cert.verifyForUsage in mailnews
: Remove calls to nsIX509Cert.verifyForUsage in mailnews
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Security: S/MIME (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 24.0
Assigned To: Magnus Melin
:
Mentors:
Depends on: 883629 885292
Blocks: 867432
  Show dependency treegraph
 
Reported: 2013-04-30 16:05 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2013-06-25 05:19 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
proposed fix (5.02 KB, patch)
2013-06-15 12:54 PDT, Magnus Melin
standard8: review+
brian: feedback+
Details | Diff | Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-04-30 16:05:15 PDT
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?) 2013-04-30 16:10:12 PDT
The bug for the removal of nsIX509Cert.verifyForUsage is bug 867432.
Comment 2 Honza Bambas (:mayhemer) 2013-05-13 15:09:19 PDT
Adding dep.
Comment 3 Magnus Melin 2013-06-15 12:54:39 PDT
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)
Comment 4 Magnus Melin 2013-06-16 04:01:15 PDT
Filed bug 883629 about test pilot removal.
Comment 5 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-06-16 12:50:27 PDT
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) 2013-06-18 03:30:40 PDT
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] 2013-06-18 09:58:50 PDT
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

Note You need to log in before you can comment on or make changes to this bug.