1028643 - nsNSSComponent::VerifySignature is using (indirectly) CERT_VerifyCert

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

nsNSSComponent::VerifySignature
  SEC_PKCS7VerifyDetachedSignature
    sec_pkcs7_verify_signature
      CERT_VerifyCert

This function needs to be rewritten to work like VerifySignature in AppSignatureVerification.cpp.

Comment 1

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

Attachment: Use same CMS detached signature processing as AppSignatureVerification and clarify code

This:
1. Moves the CMS signature decoding logic from AppSignatureVerification to nsDataSignatureVerifier.
2. Removes nsISignatureVerifier
3. Moves nsISignatureVerifier.VerifySignature to nsIDataSignatureVerifier.
4. Generalizes the logic from #1 so that it is shared by AppSignatureVerification and nsIDataSignatureVerification.

The extension signature verification (the stuff that uses nsJAR.cpp) is tested in mochitest-browser using tests under toolkit/mozapps/extensions/test/xpinstall/ and also in some mochitest-other tests. The app signature verification code is tested using xpcshell tests in security/manager/ssl/tests/unit/test_signed_apps-* and in some mochitests under dom/apps or similar.

Please avoid giving style and naming nits for nsJAR.cpp. My goal in that file was to minimize the changes I need to make to that file to avoid the need for a separate review.

It would probably be good to rename ns[I]DataSignatureVerifier to ns[I]ExtensionSignatureVerifier. That should be done in a follow-up bug.

tryserver:
https://tbpl.mozilla.org/?tree=Try&rev=63020dd0407e

Here is my previous *bad* tryserver run so you can see that the tests actually do find bugs:
https://tbpl.mozilla.org/?tree=Try&rev=3b7f59d3cfd6

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8444218 [details] [diff] [review]
Use same CMS detached signature processing as AppSignatureVerification and clarify code

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

LGTM. Just style nits in non-nsJAR code. Also, I think nsDataSignatureVerifier needs to implement nsNSSShutDownObject (see below).

::: security/apps/AppSignatureVerification.cpp
@@ +531,2 @@
>  nsresult
> +VerifyCertificate(CERTCertificate *signerCert, void* voidContext, void* pinArg)

nit: CertCertificate*

@@ +540,2 @@
>  
> +  AppTrustDomain trustDomain(pinArg); 

nit: trailing space

::: security/manager/ssl/public/nsIDataSignatureVerifier.idl
@@ +30,5 @@
> +   /* Sig Verification Error Codes */
> +  const long VERIFY_OK = 0;
> +  const long VERIFY_ERROR_UNKNOWN_ISSUER = 1;
> +  const long VERIFY_ERROR_OTHER = 2;
> +  

nit: trailing whitespace

::: security/manager/ssl/src/nsDataSignatureVerifier.cpp
@@ +117,5 @@
> +
> +nsresult
> +VerifyCMSDetachedSignatureIncludingCertificate(
> +  const SECItem& buffer, const SECItem& detachedDigest,
> +  nsresult (*verifyCertificate)(CERTCertificate* cert, void* context,

nit: maybe make this a typedef so the method signature is easier to read?

@@ +192,5 @@
> +  NSSCMSSignerInfo* signer = NSS_CMSSignedData_GetSignerInfo(signedData, 0);
> +  if (NS_WARN_IF(!signer)) {
> +    return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
> +  }
> +  // cert is signerCert

nit: I'm not sure what useful information this comment is imparting, but I guess it was there in the original code, so no big deal

@@ +200,5 @@
> +    return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
> +  }
> +
> +  nsresult rv = verifyCertificate(signerCert, verifyCertificateContext, pinArg);
> +  if (NS_WARN_IF(rv != NS_OK)) {

NS_WARN_IF(NS_FAILED(rv)) ?

@@ +259,5 @@
> +    return rv;
> +  }
> +
> +  context->principal =
> +    new nsCertificatePrincipal(NS_ConvertUTF16toUTF8(fingerprint),

handle out-of-memory?

@@ +262,5 @@
> +  context->principal =
> +    new nsCertificatePrincipal(NS_ConvertUTF16toUTF8(fingerprint),
> +                                NS_ConvertUTF16toUTF8(subjectName),
> +                                NS_ConvertUTF16toUTF8(orgName),
> +                                xpcomCert);

nit: indentation

@@ +293,5 @@
> +
> +  *aErrorCode = VERIFY_ERROR_OTHER;
> +  *aPrincipal = nullptr;
> +
> +  nsNSSShutDownPreventionLock locker;

Check for shutdown and return early if so?
Actually, this lock here is useless as is. I think nsDataSignatureVerifier needs to implement nsNSSShutDownObject so it can call isAlreadyShutDown().

@@ +299,5 @@
> +  Digest digest;
> +  nsresult rv = digest.DigestBuf(SEC_OID_SHA1,
> +                                 reinterpret_cast<const uint8_t*>(aPlaintext),
> +                                 aPlaintextLen);
> +  if (NS_WARN_IF(rv != NS_OK)) {

NS_WARN_IF(NS_FAILED(rv))

::: security/manager/ssl/src/nsDataSignatureVerifier.h
@@ +38,5 @@
> +namespace mozilla {
> +
> +nsresult VerifyCMSDetachedSignatureIncludingCertificate(
> +  const SECItem& buffer, const SECItem& detachedDigest,
> +  nsresult (*verifyCertificate)(CERTCertificate* cert, void* context,

again, maybe use a typedef here to make the method signature easier to read?

@@ +40,5 @@
> +nsresult VerifyCMSDetachedSignatureIncludingCertificate(
> +  const SECItem& buffer, const SECItem& detachedDigest,
> +  nsresult (*verifyCertificate)(CERTCertificate* cert, void* context,
> +                                void* pinArg),
> +  void *verifyCertificateContext, void* pinArg);

nit: void*

Comment 3

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

Comment on attachment 8444218 [details] [diff] [review]
Use same CMS detached signature processing as AppSignatureVerification and clarify code

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

Thanks for the quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1aa8745a6f

::: security/manager/ssl/src/nsDataSignatureVerifier.cpp
@@ +117,5 @@
> +
> +nsresult
> +VerifyCMSDetachedSignatureIncludingCertificate(
> +  const SECItem& buffer, const SECItem& detachedDigest,
> +  nsresult (*verifyCertificate)(CERTCertificate* cert, void* context,

I didn't do this typedef thing because I didn't want to create confusion between the real CertVerifier class and whatever I would name the typedef.

@@ +192,5 @@
> +  NSSCMSSignerInfo* signer = NSS_CMSSignedData_GetSignerInfo(signedData, 0);
> +  if (NS_WARN_IF(!signer)) {
> +    return NS_ERROR_CMS_VERIFY_ERROR_PROCESSING;
> +  }
> +  // cert is signerCert

Removed.

@@ +259,5 @@
> +    return rv;
> +  }
> +
> +  context->principal =
> +    new nsCertificatePrincipal(NS_ConvertUTF16toUTF8(fingerprint),

operator new never returns null in Gecko, except for new (nothrow). If memory allocation fails, the process is aborted. Search for "infallible" in the documentation.

@@ +293,5 @@
> +
> +  *aErrorCode = VERIFY_ERROR_OTHER;
> +  *aPrincipal = nullptr;
> +
> +  nsNSSShutDownPreventionLock locker;

That was an existing problem with the code that is outside the scope of this bug. I filed bug 1029173 for fixing it.

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3a1aa8745a6f

Comment 5

[Kaspar]

Attachment: Use signingTime attribute for XPI VerifyCertificate

This will introduce a regression in terms of XPI signature verification, actually. SEC_PKCS7VerifyDetachedSignature (sec_pkcs7_verify_signature) uses the PKCS#9 signingTime attribute when verifying the cert, but the current implementation of  VerifyCMSDetachedSignatureIncludingCertificate does not take the signingTime into account.

I hope my comment doesn't turn into a discussion of the pros and cons of relying on the signingTime attribute (bug 835105 et. al), I would simply prefer to see the current behavior being retained as long as bug 1047239 isn't fully addressed.

The attached patch will restore the original behavior of SEC_PKCS7VerifyDetachedSignature. As a real-world test, installing the XPI from https://addons.mozilla.org/firefox/downloads/file/219550/noscript_security_suite-2.6.6.8-fx+fn+sm.xpi will show the difference - up to Firefox 32, the dialog will state "NoScript (InformAction)", while with Firefox 33 and later, it says "NoScript (Author not verified)".

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Kaspar Brand from comment #5)
Kaspar, I would file a new bug for this.

Comment 7

[Kaspar]

Comment on attachment 8466785 [details] [diff] [review]
Use signingTime attribute for XPI VerifyCertificate

(In reply to David Keeler (:keeler) [use needinfo?] from comment #6)
> Kaspar, I would file a new bug for this.

Ok, I have done so: bug 1049686 (Cc'ed you and Brian, not sure if I should also have used needinfo).