1049686 - Switching from SEC_PKCS7VerifyDetachedSignature in nsNSSComponent::VerifySignature to VerifyCMSDetachedSignatureIncludingCertificate in nsDataSignatureVerifier leads to backwards-incompatible certificate checking results

Status: RESOLVED WORKSFORME

(Core :: Security: PSM, defect)

Description

[Kaspar]

Attachment: Use signingTime attribute for XPI VerifyCertificate

Originally filed as bug 1028643 comment 5:

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 1

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

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

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

Kaspar, I agree with Brian in bug 835105 comment 0. Basically, if we trust the signing time in the signature, code signing certificates effectively don't expire, because the signer can always specify a signing time within the validity of their cert.

Comment 2

[Andy McKay]

Is this something we need to keep open?

Comment 3

[Daniel Veditz [:dveditz]]

I think so: the current situation is that when the certs expire packages will no longer install, and installed packages will stop working.

If authors--and users--update more often than certs expire then people might not notice, mostly. Old versions will not be installable though. People could inspect/experiment with old versions by using Dev Edition and disabling the signing requirement I guess.

Lots of authors do NOT update that often; some simple tools NEVER update and don't need to. That means AMO will need to 1) keep track of when things were signed / when they expire, 2) regularly re-sign things, 3) artificially bumping the versions as it does so so that users will be sent updates. Users will have to install updates that don't do anything but refresh the signatures.

This bug is requesting that the certificate checks use the signing time (our old behavior), so that "valid when signed" packages stay valid. Brian and David rightly point out that the signer can easily lie about the signing time making this check useless in the general case. However, our code signing is no longer general case--we've removed support for the code-signing bit for our root store--the only place we now support code-signing is for Marketplace and AMO. In both those cases _we_ are the ones doing the signing -- we can promise not to lie to ourselves about the time.

This is more of an issue if we ever decide to issue certs and allow 3rd-party signing, but that seems unlikely at this point. And since expiring packages are untenable, at that point we'd have to implement some kind of timestamping service.

Comment 4

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

(In reply to Daniel Veditz [:dveditz] from comment #3)
> This bug is requesting that the certificate checks use the signing time (our
> old behavior), so that "valid when signed" packages stay valid. Brian and
> David rightly point out that the signer can easily lie about the signing
> time making this check useless in the general case. However, our code
> signing is no longer general case--we've removed support for the
> code-signing bit for our root store--the only place we now support
> code-signing is for Marketplace and AMO. In both those cases _we_ are the
> ones doing the signing -- we can promise not to lie to ourselves about the
> time.

That seems reasonable. On a related note, as far as I can tell, signed JAR verification is now basically dead code, since that uses nsIDataSignatureVerifier::VerifySignature, which doesn't use the AppTrustDomain verification but rather the default NSSCertDBTrustDomain for which there are no trusted code-signing roots. Can we just remove all that?

(See https://dxr.mozilla.org/mozilla-central/rev/02398f2be72b9bbf5ee79348b73ef122c915aae0/modules/libjar/nsJAR.cpp#616 and https://dxr.mozilla.org/mozilla-central/rev/02398f2be72b9bbf5ee79348b73ef122c915aae0/security/manager/ssl/nsDataSignatureVerifier.cpp#249 )

Comment 5

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

(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> That seems reasonable. On a related note, as far as I can tell, signed JAR
> verification is now basically dead code, since that uses
> nsIDataSignatureVerifier::VerifySignature, which doesn't use the
> AppTrustDomain verification but rather the default NSSCertDBTrustDomain for
> which there are no trusted code-signing roots. Can we just remove all that?

Yes, other Barry, we can (bug 1382749, bug 805305).
In terms of this bug, though, the only add-ons that can be installed in 57 are Mozilla-signed add-ons, and we don't include the signing time attribute anyway, and when the signing certificate is expired we just say it's good (yaaaay...), so I think this bug is wfm now.