1027241 - Import nsICMS* from Gecko to MailNews Core to fix bustage caused by their removal from Gecko

Status: VERIFIED FIXED

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

Description

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

See bug 611752.

For the most part, you just need to "hg cp" the removed files from an older revision of the gecko part comm-central into the non-Gecko part, and then hook up the implementation to the build system and XPCOM constructor mechanism.

I am going to attach my patch for bug 611752 to this bug and then I will annotate that patch with some suggestions for how to fix the stuff that can't be fixed with just "hg cp."

Comment 1

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

Attachment: Patch that removed CMS support from Gecko

Comment 2

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

Comment on attachment 8442277 [details] [diff] [review]
Patch that removed CMS support from Gecko

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

::: security/manager/pki/src/nsNSSDialogs.cpp
@@ -47,5 @@
>  {
>  }
>  
>  NS_IMPL_ISUPPORTS(nsNSSDialogs, nsITokenPasswordDialogs,
> -                  nsICertificateDialogs,

I guess you will need to create a new class for implementing nsICertificateDialogs. I actually recommend trying to do the implementation in JS instead of C++.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ -1399,5 @@
> -  if (SECSuccess != CERT_SaveSMimeProfile(mCert.get(), nullptr, nullptr))
> -    return NS_ERROR_FAILURE;
> -  else
> -    return NS_OK;
> -}

Instead of trying to reimplement nsISMimeCert, change your Thunderbird code to spawn a CryptoTask that does this. You can access the CERTCertificate from an nsIX509Cert by doing this:

ScopedCERTCertificate nsscert;
nsCOMPtr<nsIX509Cert2> cert2 = do_QueryInterface(cert);
if (cert2) {
    nsscert = cert2->GetCert();
}

See CryptoTask.h about how to spawn the CryptoTask.

::: security/manager/ssl/src/nsNSSModule.cpp
@@ -196,5 @@
>  NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsPkcs11)
> -NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsCMSSecureMessage)
> -NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsCMSDecoder)
> -NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsCMSEncoder)
> -NS_NSS_GENERIC_FACTORY_CONSTRUCTOR(nssEnsure, nsCMSMessage)

Instead of using NS_NSS_GENERIC_FACTORY_CONSTRUCTOR, just use the normal XPCOM constructor macros + change the Init() method of each of these classes to do_GetService() the PSM service; that will ensure that PSM is loaded, which will ensure that NSS is initialized correctly.

::: security/manager/ssl/src/nsVerificationJob.h
@@ -61,5 @@
> -  unsigned char *digest_data;
> -  uint32_t digest_len;
> -
> -  void Run();
> -};

Instead of mucking with nsBaseVerificationJob, I recommend that you rewrite this as a CryptoTask.

Comment 3

[Magnus Melin [:mkmelin]]

How does this bustage show?

Comment 4

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

(In reply to Magnus Melin from comment #3)
> How does this bustage show?

It isn't broken yet. It will happen when bug 611752 lands in mozilla-central later today. I think TBPL will show the bustage as the build system won't be able to find the nsICMS* and nsISMIMECert interfaces when compiling the S/MIME stuff in Mail/News core.

Comment 5

[rsx11m]

Well, it's broken now. Assuming that the patch fixes it, can you flag it for review?

Comment 6

[rsx11m]

[.../nsMsgComposeSecure.h:12:27: fatal error: nsICMSEncoder.h: No such file or directory]

Comment 7

[rsx11m]

Oops, never mind my comment #5 - that patch is only intended as a reference what was removed. :-[

Comment 8

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

Comment on attachment 8442277 [details] [diff] [review]
Patch that removed CMS support from Gecko

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

::: security/manager/pki/src/nsNSSDialogs.cpp
@@ -47,5 @@
>  {
>  }
>  
>  NS_IMPL_ISUPPORTS(nsNSSDialogs, nsITokenPasswordDialogs,
> -                  nsICertificateDialogs,

Disregard. This change is being reverted in Gecko and it seems it isn't really applicable to nsICMS* anyway.

Comment 9

[Philip Chee]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #0)

> For the most part, you just need to "hg cp" the removed files from an older

Note "hg cp" doesn't work across repositories. Both files need to be in the same repo. I tested this last night and hg cp complained.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8442277 [details] [diff] [review]
Patch that removed CMS support from Gecko

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

::: security/manager/ssl/src/nsCMS.cpp
@@ -170,5 @@
> -
> -  if (si->cert) {
> -    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("nsCMSMessage::GetSignerCert got signer cert\n"));
> -
> -    *scert = nsNSSCertificate::Create(si->cert);

nsNSSCertificate.h is not exported as public header. How should we handle nsNSSCertificate class?
I actually tried to export the header, but it needs other unexported header files.

Comment 11

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

Comment on attachment 8442277 [details] [diff] [review]
Patch that removed CMS support from Gecko

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

::: security/manager/ssl/src/nsCMS.cpp
@@ -170,5 @@
> -
> -  if (si->cert) {
> -    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("nsCMSMessage::GetSignerCert got signer cert\n"));
> -
> -    *scert = nsNSSCertificate::Create(si->cert);

Instead of constructing an nsNSSCertificate, use nsIX509Cert.constructX509 to create the nsIX509Cert instance.

Basically, remove all references to nsNSSCertificate and its header.

::: security/manager/ssl/src/nsCMSSecureMessage.cpp
@@ -111,5 @@
> -    PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("nsCMSSecureMessage::DecodeCert - can't decode cert\n"));
> -    return rv;
> -  }
> -
> -  nsCOMPtr<nsIX509Cert> cert =  nsNSSCertificate::ConstructFromDER((char *)data, length);

Use nsIX09CertDB.constructX509 here too.

Comment 12

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

(In reply to Philip Chee from comment #9)
> (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response)
> from comment #0)
> 
> > For the most part, you just need to "hg cp" the removed files from an older
> 
> Note "hg cp" doesn't work across repositories. Both files need to be in the
> same repo. I tested this last night and hg cp complained.

Thanks for letting me know. I misunderstood how comm-central worked. It is really up to the MailNews Core: Security S/MIME module peers regarding how they want the stuff from mozilla-central to be imported into comm-central. The simplest way is to copy the latest versions of those files over from an older revision of mozilla-central to comm-central, fix the build issues, and commit. Alternatively, if you want to preserve the version history you might try using "hg convert" as explained, for example, in http://stackoverflow.com/questions/3643313/mercurial-copying-one-file-and-its-history-to-another-repository.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP patch v1

This patch just makes comm-central buildable with the following modification in mozilla-central.

diff --git a/security/nss/lib/smime/smime.def b/security/nss/lib/smime/smime.def
--- a/security/nss/lib/smime/smime.def
+++ b/security/nss/lib/smime/smime.def
@@ -271,11 +271,12 @@ NSSSMIME_GetVersion;
 ;+    global:
 SEC_PKCS7VerifyDetachedSignatureAtTime;
 ;+    local:
 ;+       *;
 ;+};
 ;+NSS_3.16 {    # NSS 3.16 release
 ;+    global:
 NSS_CMSSignerInfo_Verify;
+NSS_CMSSignerInfo_VerifyCertificate;
 ;+    local:
 ;+       *;
 ;+};

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Is there any alternative method of certVerifier->VerifyCert() in nsCMSMessage::CommonVerifySignature?
I cannot find any effective methods for that, so I used NSS_CMSSignerInfo_VerifyCertificate there.

Comment 15

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

Comment on attachment 8444030 [details] [diff] [review]
WIP patch v1

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

Good work.

I didn't review all of the files; I just looked at the certificate-verification-related changes. If you'd like me to review your changes, please split the patches into two sets:

Set 1: Import the files into comm-central without modification, even though they don't build.
Set 2: A patch off the changes made from Set 1 to get comm-central working again.

Then I will give you feedback on Set 2.

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +804,5 @@
> +    mozilla::ScopedCERTCertificate nsscert;
> +    nsCOMPtr<nsIX509Cert2> cert2 = do_QueryInterface(mSelfEncryptionCert);
> +    if (cert2) {
> +      nsscert = cert2->GetCert();
> +      if (SECSuccess != CERT_SaveSMimeProfile(nsscert, nullptr, nullptr)) {

NOTE: You probably don't want to call CERT_SaveSMimeProfile on the main thread. But, you can fix the threading later.

::: mailnews/mime/src/nsCMS.cpp
@@ +215,5 @@
> +
> +  return CommonVerifySignature(aDigestData, aDigestDataLen);
> +}
> +
> +nsresult nsCMSMessage::CommonVerifySignature(unsigned char* aDigestData, uint32_t aDigestDataLen)

Note: You may no longer need both the synchronous and asynchronous variants of the VerifySignature function. Hopefully you only need the asnyc version. If so, you can simplify this code, perhaps in a follow-up bug.

@@ +272,5 @@
> +  // See bug 324474. We want to make sure the signing cert is 
> +  // still valid at the current time.
> +
> +  {
> +    SECStatus srv = NSS_CMSSignerInfo_VerifyCertificate(si, CERT_GetDefaultCertDB(), certUsageEmailSigner);

If you call this function, then you will be using the NSS-based certificate verification. In Gecko/Firefox, we switched from NSS to mozilla::pkix (see security/pkix/include/pkix.h and security/certverifier/CertVerifier.cpp). Also, we just removed all the support for configuring the NSS-based certificate verification from Gecko in bug 975229. Consequently, if you want to use NSS_CMSSignerInfo_VerifyCertificate, then you'd need to also add back the configuration stuff that got removed in bug 975229 too.

I'd like to suggest three alternatives:

1. Add a new method to the nsIX509CertDB interface in Gecko for verifying S/MIME certificates, which calls CertVerifier::VerifyCert like the code I removed did. Then, PSM will try to maintain this function for mailnews.

2. Check out the function VerifySignature in security/apps/AppSignatureVerification.cpp. That function is also trying to verify a CMS signature. The main difference is that it is verifying the certificate for the "object signing" EKU but you need to verify the certificate for an S/MIME EKU. You may be able to generalize that VerifySignature function so that it can work for either S/MIME or object signing by having it take a parameter indicating which certificateUsage to verify for. Again, if you do this, then PSM will try to maintain this function for mailnews.

3. Mailnews can fully take responsibility for certificate verification for S/MIME and come up with a completely different alternative separate from Gecko. For example, you may switch back to NSS, though I don't recommend it.

@@ +282,5 @@
> +    }
> +  }
> +
> +  // We verify the first signer info,  only //
> +  if (NSS_CMSSignedData_VerifySignerInfo(sigd, 0, CERT_GetDefaultCertDB(), certUsageEmailSigner) != SECSuccess) {

NSS_CMSSignedData_VerifySignerInfo calls NSS_CMSSignerInfo_VerifyCertificate, which means it is also probably very broken by the changes in bug 975229. Again, look at the function VerifySignature in securiry/apps/AppSignatureVerification for a solution.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Just a copied-and-paste files from Gecko

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: WIP patch v2

I am not sure I can take time, so I am uploading the current patch for the future works.

This patch can be applied upon attachment #8444067 [details] [diff] [review].
It's almost the same as the previous patch other than creating a CryptoTask for saving SMIME profile.

Comment 18

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

Comment on attachment 8444069 [details] [diff] [review]
WIP patch v2

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

Hey, in my previous review comments, I think I was letting the perfect be the enemy of the good. Could you try the LOCAL_INCLUDES thing I suggest below and see if this gets things building again?

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +782,5 @@
> +private:
> +  virtual void ReleaseNSSResources() MOZ_OVERRIDE {}
> +  virtual nsresult CalculateResult() MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());

NIT: This should be MOZ_ASSERT(!NS_IsMainThread()); Or, just omit it altogether, since one of the main points of CryptoTask is to execute stuff off the main thread.

@@ +784,5 @@
> +  virtual nsresult CalculateResult() MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    mozilla::ScopedCERTCertificate nsscert;

NIT: nsscert should be in the scope of the "if (cert2)" block.

::: mailnews/mime/src/nsCMS.cpp
@@ +275,2 @@
>    {
> +    SECStatus srv = NSS_CMSSignerInfo_VerifyCertificate(si, CERT_GetDefaultCertDB(), certUsageEmailSigner);

I suggest that you revert this chunk back to the code that uses CertVerifier and add security/manager/ssl/src to LOCAL_INCLUDES. Then we can clean this up in a follow-up bug.

@@ +368,5 @@
> +private:
> +  virtual void ReleaseNSSResources() MOZ_OVERRIDE {}
> +  virtual nsresult CalculateResult() MOZ_OVERRIDE
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());

Again, this will NOT happen on the main thread.

Comment 19

[Edmund Wong (:ewong)]

Attachment: Hiroyuki's copy patch + WIP v2 patch + :briansmith's comments

I took the liberty of taking the patches and combine it into one (to make it easier for Hiroyuki-san).

I have no understanding of this code.

It does not build due to the following:

mozmake[6]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/news'
mozmake[8]: Entering directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/mime/emitters'
mozmake[8]: Nothing to be done for 'libs'.
mozmake[8]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/mime/emitters'
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsMimeObjectClassAccess.obj -c -I../../../mozilla/dist/stl_wrappers  -DENABLE_SMIME -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"WINNT6.0\" -DOSARCH=WINNT -DNO_NSPR_10_SUPPORT -Il:/sm_bigprojs/sm_sync/security/manager/ssl/src -Il:/sm_bigprojs/sm_sync/mailnews/mime/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nspr -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nss  -I/l/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include -I/l/sm_bigprojs/sm_sync/mozilla/modules/zlib/src      -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -MD            -FI ../../../mozilla/dist/include/mozilla-config.h -DMOZILLA_CLIENT l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsMimeObjectClassAccess.cpp
nsSimpleMimeConverterStub.cpp
nsCMS.cpp
l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsCMS.cpp(231) : error C2065: 'SharedCertVerifier' : undeclared identifier
l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsCMS.cpp(231) : error C2133: 'certVerifier' : unknown size
l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsCMS.cpp(231) : error C2512: 'mozilla::RefPtr' : no appropriate default constructor available
l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsCMS.cpp(277) : error C3861: 'GetDefaultCertVerifier': identifier not found
l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsCMS.cpp(278) : error C2675: unary '!' : 'mozilla::RefPtr' does not define this operator or a conversion to a type acceptable to the predefined operator
l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsCMS.cpp(281) : error C2678: binary '->' : no operator found which takes a left-hand operand of type 'mozilla::RefPtr' (or there is no acceptable conversion)
        l:\sm_bigprojs\sm_sync\objdir-sm\mozilla\dist\include\mozilla/RefPtr.h(262): could be 'T *mozilla::RefPtr<T>::operator ->(void) const'
        while trying to match the argument list '(mozilla::RefPtr)'
l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsCMS.cpp(281) : error C2039: 'VerifyCert' : is not a member of 'mozilla::RefPtr'
        l:\sm_bigprojs\sm_sync\objdir-sm\mozilla\dist\include\mozilla/RefPtr.h(217) : see declaration of 'mozilla::RefPtr'
l:/sm_bigprojs/sm_sync/config/rules.mk:960: recipe for target 'nsCMS.obj' failed
mozmake[8]: *** [nsCMS.obj] Error 2
mozmake[8]: *** Waiting for unfinished jobs....
nsMimeObjectClassAccess.cpp
mozmake[8]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/mime/src'
l:/sm_bigprojs/sm_sync/config/recurse.mk:44: recipe for target 'src_libs' failed
mozmake[7]: *** [src_libs] Error 2
mozmake[7]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/mime'
l:/sm_bigprojs/sm_sync/config/recurse.mk:44: recipe for target 'libs' failed
mozmake[6]: *** [libs] Error 2
mozmake[6]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/mime'
l:/sm_bigprojs/sm_sync/config/recurse.mk:44: recipe for target 'mime_libs' failed
mozmake[5]: *** [mime_libs] Error 2
mozmake[5]: *** Waiting for unfinished jobs....
mozmake[6]: Entering directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/import/eudora/src'
mozmake[6]: Nothing to be done for 'libs'.
mozmake[6]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/import/eudora/src'
mozmake[5]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews'
l:/sm_bigprojs/sm_sync/config/recurse.mk:44: recipe for target 'libs' failed
mozmake[4]: *** [libs] Error 2
mozmake[4]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews'
l:/sm_bigprojs/sm_sync/mozilla/config/recurse.mk:153: recipe for target 'libs' failed
mozmake[3]: *** [libs] Error 2
mozmake[3]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mozilla'
l:/sm_bigprojs/sm_sync/mozilla/config/rules.mk:596: recipe for target 'default' failed
mozmake[2]: *** [default] Error 2
mozmake[2]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mozilla'
Makefile:52: recipe for target 'default' failed
mozmake[1]: *** [default] Error 2
mozmake[1]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm'
client.mk:392: recipe for target 'build' failed
mozmake: *** [build] Error 2

Comment 20

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

(In reply to Edmund Wong (:ewong) from comment #19)
> l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsCMS.cpp(231) : error C2065:
> 'SharedCertVerifier' : undeclared identifier

#include "nsNSSComponent.h"

using namespace mozilla::psm;

in the moz.build file:

LOCAL_INCLUDES = [
  'gecko/security/manager/ssl/src',
]

(Replace "gecko" with whatever directory comm-central imports gecko into.)

Note sure about all the RefPtr stuff. Perhaps the above is enough to fix it.

Comment 21

[Edmund Wong (:ewong)]

Attachment: Hiroyuki's copy patch + WIP v2 patch + :briansmith's comments (v3)

Updated patch.  This time, it's failing here:

l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsCMS.obj -c -I../../../mozilla/dist/stl_wrappers  -DENABLE_SMIME -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"WINNT6.0\" -DOSARCH=WINNT -DNO_NSPR_10_SUPPORT -Il:/sm_bigprojs/sm_sync/mozilla/security/certverifier -Il:/sm_bigprojs/sm_sync/mozilla/security/manager/ssl/src -Il:/sm_bigprojs/sm_sync/mozilla/security/pkix -Il:/sm_bigprojs/sm_sync/mailnews/mime/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nspr -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nss  -I/l/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include -I/l/sm_bigprojs/sm_sync/mozilla/modules/zlib/src      -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -MD            -FI ../../../mozilla/dist/include/mozilla-config.h -DMOZILLA_CLIENT l:/sm_bigprojs/sm_sync/mailnews/mime/src/nsCMS.cpp
nsCMSSecureMessage.cpp
nsCMS.cpp
l:\sm_bigprojs\sm_sync\mozilla\security\certverifier\CertVerifier.h(10) : fatal error C1083: Cannot open include file: 'pkix/pkixtypes.h': No such file or directory
nsNNTPArticleList.cpp
l:/sm_bigprojs/sm_sync/config/rules.mk:960: recipe for target 'nsCMS.obj' failed
mozmake[8]: *** [nsCMS.obj] Error 2
mozmake[8]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/mime/src'
l:/sm_bigprojs/sm_sync/config/recurse.mk:44: recipe for target 'src_libs' failed
mozmake[7]: *** [src_libs] Error 2
mozmake[7]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/mime'
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsNntpIncomingServer.obj -c -I../../../mozilla/dist/stl_wrappers  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"WINNT6.0\" -DOSARCH=WINNT -DNO_NSPR_10_SUPPORT  -Il:/sm_bigprojs/sm_sync/mailnews/news/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nspr -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nss  -I/l/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include -I/l/sm_bigprojs/sm_sync/mozilla/modules/zlib/src      -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -MD            -FI ../../../mozilla/dist/include/mozilla-config.h -DMOZILLA_CLIENT l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNntpIncomingServer.cpp
l:/sm_bigprojs/sm_sync/config/recurse.mk:44: recipe for target 'libs' failed
mozmake[6]: *** [libs] Error 2
mozmake[6]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/mime'
l:/sm_bigprojs/sm_sync/config/recurse.mk:44: recipe for target 'mime_libs' failed
mozmake[5]: *** [mime_libs] Error 2
mozmake[5]: *** Waiting for unfinished jobs....
nsNntpMockChannel.cpp
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsNntpMockChannel.obj -c -I../../../mozilla/dist/stl_wrappers  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"WINNT6.0\" -DOSARCH=WINNT -DNO_NSPR_10_SUPPORT  -Il:/sm_bigprojs/sm_sync/mailnews/news/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nspr -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nss  -I/l/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include -I/l/sm_bigprojs/sm_sync/mozilla/modules/zlib/src      -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -MD            -FI ../../../mozilla/dist/include/mozilla-config.h -DMOZILLA_CLIENT l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNntpMockChannel.cpp
nsNNTPNewsgroupList.cpp
nsNntpMockChannel.cpp
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsNNTPNewsgroupList.obj -c -I../../../mozilla/dist/stl_wrappers  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"WINNT6.0\" -DOSARCH=WINNT -DNO_NSPR_10_SUPPORT  -Il:/sm_bigprojs/sm_sync/mailnews/news/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nspr -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nss  -I/l/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include -I/l/sm_bigprojs/sm_sync/mozilla/modules/zlib/src      -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -MD            -FI ../../../mozilla/dist/include/mozilla-config.h -DMOZILLA_CLIENT l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNNTPNewsgroupList.cpp
nsNNTPNewsgroupPost.cpp
nsNntpIncomingServer.cpp
l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNntpIncomingServer.cpp(1655) : warning C4146: unary minus operator applied to unsigned type, result still unsigned
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsNNTPNewsgroupPost.obj -c -I../../../mozilla/dist/stl_wrappers  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"WINNT6.0\" -DOSARCH=WINNT -DNO_NSPR_10_SUPPORT  -Il:/sm_bigprojs/sm_sync/mailnews/news/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nspr -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nss  -I/l/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include -I/l/sm_bigprojs/sm_sync/mozilla/modules/zlib/src      -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -MD            -FI ../../../mozilla/dist/include/mozilla-config.h -DMOZILLA_CLIENT l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNNTPNewsgroupPost.cpp
nsNNTPProtocol.cpp
nsNNTPNewsgroupPost.cpp
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsNNTPProtocol.obj -c -I../../../mozilla/dist/stl_wrappers  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"WINNT6.0\" -DOSARCH=WINNT -DNO_NSPR_10_SUPPORT  -Il:/sm_bigprojs/sm_sync/mailnews/news/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nspr -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nss  -I/l/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include -I/l/sm_bigprojs/sm_sync/mozilla/modules/zlib/src      -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -MD            -FI ../../../mozilla/dist/include/mozilla-config.h -DMOZILLA_CLIENT l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNNTPProtocol.cpp
nsNntpService.cpp
nsNNTPNewsgroupList.cpp
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsNntpService.obj -c -I../../../mozilla/dist/stl_wrappers  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"WINNT6.0\" -DOSARCH=WINNT -DNO_NSPR_10_SUPPORT  -Il:/sm_bigprojs/sm_sync/mailnews/news/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nspr -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nss  -I/l/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include -I/l/sm_bigprojs/sm_sync/mozilla/modules/zlib/src      -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -MD            -FI ../../../mozilla/dist/include/mozilla-config.h -DMOZILLA_CLIENT l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNntpService.cpp
nsNntpUrl.cpp
nsNNTPProtocol.cpp
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.cl  cl -FonsNntpUrl.obj -c -I../../../mozilla/dist/stl_wrappers  -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"WINNT6.0\" -DOSARCH=WINNT -DNO_NSPR_10_SUPPORT  -Il:/sm_bigprojs/sm_sync/mailnews/news/src -I. -I../../../mozilla/dist/include -I../../../mozilla/dist/include/nsprpub  -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nspr -Il:/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include/nss  -I/l/sm_bigprojs/sm_sync/objdir-sm/mozilla/dist/include -I/l/sm_bigprojs/sm_sync/mozilla/modules/zlib/src      -TP -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -wd4251 -wd4244 -wd4345 -wd4351 -wd4482 -wd4800 -wd4819 -we4553 -GR-  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -O1 -Oi -Oy -MD            -FI ../../../mozilla/dist/include/mozilla-config.h -DMOZILLA_CLIENT l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNntpUrl.cpp
nsNntpService.cpp
nsNntpUrl.cpp
rm -f mailnews_news_src.lib 
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe l:/sm_bigprojs/sm_sync/mozilla/config/expandlibs_gen.py --depend .deps/mailnews_news_src.lib.pp -o mailnews_news_src.lib.desc nsNewsDownloadDialogArgs.obj nsNewsDownloader.obj nsNewsFolder.obj nsNewsUtils.obj nsNNTPArticleList.obj nsNntpIncomingServer.obj nsNntpMockChannel.obj nsNNTPNewsgroupList.obj nsNNTPNewsgroupPost.obj nsNNTPProtocol.obj nsNntpService.obj nsNntpUrl.obj  
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe l:/sm_bigprojs/sm_sync/mozilla/config/nsinstall.py -t -m 644 l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNewsAutoCompleteSearch.js l:/sm_bigprojs/sm_sync/mailnews/news/src/nsNewsAutoCompleteSearch.manifest ../../../mozilla/dist/bin/components
l:/sm_bigprojs/sm_sync/objdir-sm/mozilla/_virtualenv/Scripts/python.exe -m mozbuild.action.buildlist ../../../mozilla/dist/bin/chrome.manifest "manifest components/nsNewsAutoCompleteSearch.manifest"
mozmake[8]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/news/src'
mozmake[7]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/news'
mozmake[7]: Entering directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/news/test'
mozmake[7]: Nothing to be done for 'libs'.
mozmake[7]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/news/test'
mozmake[6]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews/news'
mozmake[5]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews'
l:/sm_bigprojs/sm_sync/config/recurse.mk:44: recipe for target 'libs' failed
mozmake[4]: *** [libs] Error 2
mozmake[4]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mailnews'
l:/sm_bigprojs/sm_sync/mozilla/config/recurse.mk:153: recipe for target 'libs' failed
mozmake[3]: *** [libs] Error 2
mozmake[3]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mozilla'
l:/sm_bigprojs/sm_sync/mozilla/config/rules.mk:596: recipe for target 'default' failed
mozmake[2]: *** [default] Error 2
mozmake[2]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm/mozilla'
Makefile:52: recipe for target 'default' failed
mozmake[1]: *** [default] Error 2
mozmake[1]: Leaving directory 'l:/sm_bigprojs/sm_sync/objdir-sm'
client.mk:392: recipe for target 'build' failed
mozmake: *** [build] Error 2

Comment 22

[Edmund Wong (:ewong)]

Attachment: Hiroyuki's copy patch + WIP v2 patch + :briansmith's comments (v4)

Neil over irc suggested I add 'mozilla/security/pkix/include'  (I had mozilla/security/pkix' before.)

Comment 23

[Edmund Wong (:ewong)]

Attachment: Hiroyuki's copy patch + WIP v2 patch + :briansmith's comments + Neil's suggestion(v5)

Comment 24

[Edmund Wong (:ewong)]

Comment on attachment 8445049 [details] [diff] [review]
Hiroyuki's copy patch + WIP v2 patch + :briansmith's comments + Neil's suggestion(v5)

FWIW, this patch builds on my system.

Pushed to try:
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1454551ac8c7

Comment 25

[Edmund Wong (:ewong)]

Attachment: Hiroyuki's copy patch + WIP v2 patch + :briansmith's comments + Neil's suggestion(v6)

I don't know how that + got into the patch.  Updated the patch and pushed
to try again.

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=716904d975ea

Comment 26

[Kai Engert (:KaiE:)]

You removed code that created instances of signature verifier. Why? No replacement?
Maybe it builds, but does it work? What happens if you receive S/MIME signed email?
Let me know if you want me to send test messages to you (and to which address).

Comment 27

[Ian Neal]

Just as a note, I am in the process of using hg convert to create a "copy patch" that will keep the history (hopefully) and will look at getting an rs= for landing those files as "not part of build" as soon as it is ready.

Comment 28

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8445115 [details] [diff] [review]
Hiroyuki's copy patch + WIP v2 patch + :briansmith's comments + Neil's suggestion(v6)

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

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +838,5 @@
>    if (aEncrypt && mSelfEncryptionCert) {
>      // Make sure self's configured cert is prepared for being used
>      // as an email recipient cert.
> +    RefPtr<CryptoTask> task = new SMimeSaveProfileTask(mSelfEncryptionCert);
> +    task->Dispatch("SMimeSave");

This changes a synchronous dispatch to a asynchrouns one--I'm not sure this is safe.

Comment 29

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8445115 [details] [diff] [review]
Hiroyuki's copy patch + WIP v2 patch + :briansmith's comments + Neil's suggestion(v6)

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

::: mailnews/mime/src/nsCMS.cpp
@@ +811,5 @@
> +  if (isAlreadyShutDown())
> +    return NS_ERROR_NOT_AVAILABLE;
> +
> +  PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("nsCMSDecoder::Start\n"));
> +  GetUIContext(getter_AddRefs(m_ctx));

This is a change from the original code, I suppose, to make it compile in comm-central. Is this necessarily correct? I think UI contexts are mostly used for smartcard support, which I would expect to see more of in S/MIME than regular usage, so I'm hesitant about these changes.

Comment 30

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

Attachment: fix-build-after-removal-of-nsISignatureVerifier.patch

This patch should be applied on top of the patch in attachment #8445115 [details] [diff] [review]. It replaces the use of nsISignatureVerifier since nsISignatureVerifier got removed from mozilla-central after that patch was written. With these two patches, I can successfully build comm-central from a checkout I did today, using today's mozilla-central.

Comment 31

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

Attachment: Part 1: Move nsICMS* interfaces and implementations from Gecko

Comment 32

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

Attachment: Part 2: Integrate nsICMS* interfaces and implementations into comm-central's build system and tweak implementation to work from outside of PSM

Hiroyuki-san,

Thanks for writing these patches. For the most part, they are very good. I made some updates to them; see Part 3, Part 4, and Part 5. In particular, please study my changes to your implementation of SMimeVerificationTask in Part 4 to better understand how the CryptoTask framework is to be used.

どうもありがとうございます!

Comment 33

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

Attachment: Part 3: Deal with removal of nsISignatureVerifier from Gecko

After Hiroyuki wrote his patch, we removed nsISignatureVerifier from Gecko. This patch adjusts Hiroyuki's patch for that.

Comment 34

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

Attachment: Part 4: Improve CryptoTask-related parts of Part 2

This improves Hiroyuki's patch from part 2 by: (a) Saving the S/MIME profile synchronously instead of asynchronously, as jcranmer requested, and (b) fixing some bugs in the implementation of SMimeVerificationTask.

Comment 35

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

Attachment: Part 5: Document sub-optimal usage of NSS_CMSSignedData_VerifySignerInfo

The function NSS_CMSSignedData_VerifySignerInfo does not work as well after we removed the NSS certificate verification configuration stuff in bug 975229. However, it still mostly works. Somebody should update this code soon similarly to how we fixed bug 1028643. But, this isn't a show-stopper issue; we have a similar issue in Gecko in bug 1028781 (which also affects Thunderbird). This patch adds a comment to this effect.

Comment 36

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

Note: When landing these patches in comm-central, I recommend folding them all together so that bisecting doesn't get broken. I only split them apart to make reviewing easier. Also, when you land the folded patches, please ensure that  Hiroyuki Ikezoe is credited as the author. In Mercurial, you can do that by passing "hg -u "Hiroyuki Ikezoe <[e-mail]>" to "hg commit" or (probably) "hg qrefresh".

Comment 37

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

Edmund, thanks for updating the patches to get them building. Your patches were very useful and all your changes are also integrated into the patch set.

Comment 38

[Alfred Peters]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #1)
> Created attachment 8442277 [details] [diff] [review]
> Patch that removed CMS support from Gecko

This was part of the SSL dir tree. So could this have had impact on the SSL connections?

I ask because even with the last patches (2014-06-24) I cannot connect to my servers (POP3/IMAP/NNTP) via SSL.
For example POP3 server log:

# > STLS
# < +OK Begin TLS negotiation
# TSrvPOP3Cli.StartSSL
# Secure connection with TLSv1.2, cipher AES128-SHA, 128 secret bits (128 total)
# OpenSSL error: SSL_read_ERROR: error:14094416:SSL routines:ssl3_read_bytes:sslv3 alert certificate unknown

Comment 39

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

(In reply to Alfred Peters from comment #38)
> (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response)
> from comment #1)
> > Created attachment 8442277 [details] [diff] [review]
> > Patch that removed CMS support from Gecko
> 
> This was part of the SSL dir tree. So could this have had impact on the SSL
> connections?
> 
> I ask because even with the last patches (2014-06-24) I cannot connect to my
> servers (POP3/IMAP/NNTP) via SSL.
> For example POP3 server log:
> 
> # > STLS
> # < +OK Begin TLS negotiation
> # TSrvPOP3Cli.StartSSL
> # Secure connection with TLSv1.2, cipher AES128-SHA, 128 secret bits (128
> total)
> # OpenSSL error: SSL_read_ERROR: error:14094416:SSL
> routines:ssl3_read_bytes:sslv3 alert certificate unknown

It is very unlikely that the CMS-related stuff is causing this issue. It is more likely that fixing bug 975229 had the side-effect of of causing this regression. Please file a new bug report in mailnews about this and CC: me.

Comment 40

[Ian Neal]

Missing files landed as https://hg.mozilla.org/comm-central/pushloghtml?changeset=50f5b5fc3f53
I will now create a single patch from the 5 parts.

Comment 41

[Ian Neal]

Attachment: Folded patch from parts 1-5

This folds the patches in parts 1-5 rebased against the files converted from m-c prior to removal.

Comment 42

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

Comment on attachment 8446192 [details] [diff] [review]
Folded patch from parts 1-5

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

Thanks for doing all this work!

::: mailnews/mime/src/nsCMS.cpp
@@ +865,5 @@
> +  nsCOMPtr<nsIPrompt> prompt;
> +  rv = wwatch->GetNewPrompter(nullptr, getter_AddRefs(prompt));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return CallQueryInterface(prompt, aRequestor);

I am a little bit unsure about this function, as even in Gecko/Firefox I basically don't understand how the UI Context stuff works! But, probably this is OK.

@@ +978,5 @@
> +  rv = wwatch->GetNewPrompter(nullptr, getter_AddRefs(prompt));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return CallQueryInterface(prompt, aRequestor);
> +}

It is probably a good idea to factor this logic, and the PSM initialization logic, into reusable functions to avoid the code duplication. But, that can be done in another less-urgent bug.

::: mailnews/mime/src/nsCMSSecureMessage.cpp
@@ +289,5 @@
>      PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("nsCMSSecureMessage::ReceiveMessage - can't base64 decode\n"));
>      goto done;
>    }
>  
> +  GetUIContext(getter_AddRefs(ctx));

NIT: I recommend moving the declaration of ctx to be right above this line, so that we don't have to worry about it being used uninitialized.

@@ +377,5 @@
> +  rv = wwatch->GetNewPrompter(nullptr, getter_AddRefs(prompt));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return CallQueryInterface(prompt, aRequestor);
> +}

Again, this repeated logic should eventually be consolidated into a single place.

Comment 43

[Ian Neal]

Attachment: Folded patch from parts 1-5 including files outside of mailnews/mime

Sorry, forgot to include nsMailModule.cpp and nsMailModule.cpp

Comment 44

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

Comment on attachment 8446198 [details] [diff] [review]
Folded patch from parts 1-5 including files outside of mailnews/mime

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

All my comments on the previous version of this patch apply to this one.

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +812,5 @@
> +    nsscert = cert2->GetCert();
> +    if (!nsscert) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    // XXX: This does not respect the nsNSSShutDownObject protocol.

We should file a follow-up bug for fixing this.

::: mailnews/mime/src/nsCMS.cpp
@@ +296,5 @@
> +  // XXX: NSS_CMSSignedData_VerifySignerInfo calls CERT_VerifyCert, which
> +  // requires NSS's certificate verification configuration to be done in
> +  // order to work well (e.g. honoring OCSP preferences and proxy settings
> +  // for OCSP requests), but Gecko stopped doing that configuration. Something
> +  // similar to what was done for Gecko bug 1028643 needs to be done here too.

And a follow-up bug for fixing this.

Comment 45

[[e-mail]]

(In reply to Brian Smith from comment #42)
> (From update of attachment 8446192 [details] [diff] [review])
> >      goto done;
> >    }
> >  
> > +  GetUIContext(getter_AddRefs(ctx));
> 
> NIT: I recommend moving the declaration of ctx to be right above this line,
> so that we don't have to worry about it being used uninitialized.

My understanding is that you can't because of the goto. (Of course, the code should use an RAII object for cert, so you could early return instead.)

Comment 46

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

(In reply to [e-mail] from comment #45)
> > > +  GetUIContext(getter_AddRefs(ctx));
> > 
> > NIT: I recommend moving the declaration of ctx to be right above this line,
> > so that we don't have to worry about it being used uninitialized.
> 
> My understanding is that you can't because of the goto. (Of course, the code
> should use an RAII object for cert, so you could early return instead.)

Oh, right. It is OK to leave the patch as-is. Or, to be more like the existing code, it is OK to move the call to GetUIContext(getter_AddRefs(ctx)) to be right underneath the declaration of ctx.

Comment 47

[Alfred Peters]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #39)
> (In reply to Alfred Peters from comment #38)
> > # OpenSSL error: SSL_read_ERROR: error:14094416:SSL
> > routines:ssl3_read_bytes:sslv3 alert certificate unknown
> 
> It is very unlikely that the CMS-related stuff is causing this issue. It is
> more likely that fixing bug 975229 had the side-effect of of causing this
> regression.

OK - sorry to bother you. Atm. I cannot get a clean build.

> Please file a new bug report in mailnews about this and CC: me.

I'll do that, if the problem with a new build still exists.

Comment 48

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8446198 [details] [diff] [review]
Folded patch from parts 1-5 including files outside of mailnews/mime

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

::: mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
@@ +22,5 @@
>  #include "mozilla/mailnews/MimeHeaderParser.h"
>  #include "nsIMimeConverter.h"
> +#include "ScopedNSSTypes.h"
> +#include "nsIX509Cert2.h"
> +#include "mozilla/RefPtr.h"

Move nsIX509Cert2.h above ScopedNSSTypes.h and mozilla/RefPtr.h above mozilla/Serices.h.

@@ +810,1 @@
>      

Nit: remove this trailing whitespace.

::: mailnews/mime/src/nsCMS.cpp
@@ +20,5 @@
> +#include "nsServiceManagerUtils.h"
> +#include "nsIPrompt.h"
> +#include "nsIWindowWatcher.h"
> +#include "CryptoTask.h"
> +#include "mozilla/RefPtr.h"

Nit: try to keep these headers sort of sorted...

@@ +389,5 @@
> +    nsresult rv;
> +    if (mDigestData.IsEmpty()) {
> +      rv = mMessage->VerifyDetachedSignature(
> +                       reinterpret_cast<uint8_t*>(const_cast<char *>(mDigestData.get())),
> +                       mDigestData.Length());

Nit: indent these lines only two spaces.

@@ +977,5 @@
> +  nsCOMPtr<nsIPrompt> prompt;
> +  rv = wwatch->GetNewPrompter(nullptr, getter_AddRefs(prompt));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return CallQueryInterface(prompt, aRequestor);

I'm highly skeptical that this actually works. The prompt you're probably going to get back from this will be here: <http://dxr.mozilla.org/comm-central/source/mozilla/toolkit/components/prompts/src/nsPrompter.js#497>, which is notably not an nsIInterfaceRequestor. You need to make a new class that inherits from nsIInterfaceRequestor and actually handles the request for an nsIPrompt properly.

::: mailnews/mime/src/nsCMS.h
@@ +17,5 @@
>  #include "nsICMSDecoder.h"
>  #include "sechash.h"
>  #include "cms.h"
>  #include "nsNSSShutDown.h"
> +#include "nsIInterfaceRequestor.h"

Don't need this include, see about 9 lines earlier.

::: mailnews/mime/src/nsCMSSecureMessage.cpp
@@ +376,5 @@
> +  nsCOMPtr<nsIPrompt> prompt;
> +  rv = wwatch->GetNewPrompter(nullptr, getter_AddRefs(prompt));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return CallQueryInterface(prompt, aRequestor);

And ditto here.

Comment 49

[[e-mail]]

(In reply to Joshua Cranmer from comment #48)
> (From update of attachment 8446198 [details] [diff] [review])
> > +  nsCOMPtr<nsIPrompt> prompt;
> > +  rv = wwatch->GetNewPrompter(nullptr, getter_AddRefs(prompt));
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  return CallQueryInterface(prompt, aRequestor);
> 
> I'm highly skeptical that this actually works.

(Doesn't seem to matter; I couldn't find anyone actually using the value.)

Comment 50

[Joe Sabash [:JoeS1]]

So, I'm running with a self-built with these patches applied.
SSL connections seem to work OK
I did see a fleeting error of "unable to decrypt string" at some point.
Anything in particular that needs testing..my email addy is valid.

I normally get Zarro encrypted or s/mime signed mail.

Comment 51

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

Comment on attachment 8446198 [details] [diff] [review]
Folded patch from parts 1-5 including files outside of mailnews/mime

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

::: mailnews/mime/src/nsCMS.cpp
@@ +977,5 @@
> +  nsCOMPtr<nsIPrompt> prompt;
> +  rv = wwatch->GetNewPrompter(nullptr, getter_AddRefs(prompt));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return CallQueryInterface(prompt, aRequestor);

Since this code is already adding private PSM directories to LOCAL_INCLUDES to access other private PSM stuff, it is OK--preferable, even--to revert back to using PipUIContext and avoid this new code at all. However, I recommend filing a follow-up bug for replacing the use of PipUIContext with something else. AFAICT, even in Gecko, PipUIContext is a messy hack for code that wasn't properly written to accept the UI context as a parameter from its caller.

Comment 53

[Ian Neal]

Attachment: Folded without GetUIContext changes

As suggested by Brian Smith, reverting PipUIContext changes.

Comment 54

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8447750 [details] [diff] [review]
Folded without GetUIContext changes

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

S/MIME signature verification is broken...

... actually, on further testing, it fails to correctly notate that the S/MIME signature is correct, marking a valid signature as invalid!

[Also, complaining after the fact: these files were imported into mailnews/mime and not mailnews/extensions/smime!?]

::: mailnews/mime/src/nsCMS.cpp
@@ +21,5 @@
>  #include "nsIArray.h"
>  #include "nsArrayUtils.h"
>  #include "nsCertVerificationThread.h"
> +#include "nsServiceManagerUtils.h"
> +#include "mozilla/RefPtr.h"

No need to include this header twice.

@@ +387,5 @@
> +  {
> +    MOZ_ASSERT(!NS_IsMainThread());
> +
> +    nsresult rv;
> +    if (mDigestData.IsEmpty()) {

This ought to be if (!mDigestData.IsEmpty()).

@@ +402,5 @@
> +  {
> +    MOZ_ASSERT(NS_IsMainThread());
> +
> +    nsCOMPtr<nsICMSMessage2> m2;
> +    do_QueryInterface(mMessage);

I think you meant m2 = do_QueryInterface(mMessage) here?

Although, honestly, you could just make mMessage be an nsCOMPtr<nsICMSMessage2> and avoid the need for a QI altogether.

Comment 55

[Joshua Cranmer [:jcranmer]]

I made the changes I mentioned above and pushed them so we could at least get the tree reopened:
https://hg.mozilla.org/comm-central/rev/a900b3a64007

Comment 56

[Joe Sabash [:JoeS1]]

Looks good with self built Built from http://hg.mozilla.org/mozilla-central/rev/afa67a2f7905