Closed Bug 1357815 Opened 7 years ago Closed 7 years ago

support SHA-256 when verifying PKCS7 signatures on addons

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- verified
firefox59 --- verified

People

(Reporter: jvehent, Assigned: keeler)

References

Details

(Keywords: sec-moderate, Whiteboard: [psm-assigned][adv-main58-])

Attachments

(5 files)

bug 1357815 - 1/4: move VerifyCMSDetachedSignatureIncludingCertificate to where it's used
59 bytes, text/x-review-board-request
jcj
: review+
Details
bug 1357815 - 2/4: refactor away unnecessary parts of certificate verification in add-on signature verification
59 bytes, text/x-review-board-request
jcj
: review+
Details
bug 1357815 - 3/4: support SHA256 in PKCS#7 signatures on add-ons
59 bytes, text/x-review-board-request
dveditz
: review+
jcj
: review+
Details
bug 1357815 - 4/4: go a bit overboard on testcases for SHA-256 support in add-on signatures
59 bytes, text/x-review-board-request
jcj
: review+
Details
Bug1357815.gif
358.82 KB, image/gif
Details 
+++ This bug was initially created as a clone of Bug #1169532 +++

Addons use PKCS7 signatures in the "mozilla.rsa" file of each addon. This signature currently uses the SHA1 algorithm and should be migrated to use SHA2 functions (either 256 or 384).

When trying to sign an addon with digest algorithm SHA256:

> OBJECT IDENTIFIER 2.16.840.1.101.3.4.2.1 sha-256 (NIST Algorithm)

Firefox returns "Add-on <ADDONID> is not correctly signed.". The same addon with a SHA1 signature works fine.
This looks like a change to be made in PSM: https://dxr.mozilla.org/mozilla-central/rev/6b98953213350d847f2d1c8165922af47b96eee4/security/manager/ssl/nsDataSignatureVerifier.cpp#181
Component: Security → Security: PSM
Priority: -- → P2
Product: addons.mozilla.org → Core
Whiteboard: [psm-backlog]
Can this be made future-proof and take the digest algorithm from the pkcs7 signer info?
We met today with Keeler & Dan to discuss the backward compatibility story around moving the PKCS7 signature to SHA-256. The main problem is migrating to a newer hash algorithm (SHA-256, but could be anything) while allowing older clients to still verify the signatures.

Current versions of Firefox hardcode the digest algorithm to SHA-1, so we must keep a SHA-1 signature around until we stop supporting them.

The first, and preferred, option is to add a SHA-256 signature in the PKCS7 document right after the SHA-1 signature. Older clients would use SHA-1, and newer clients would use SHA-256 (and blacklist SHA-1). This is only possible if the PKCS7 code in NSS supports multiple SignerInfo sections [1]. Keeler is verifying this.

The second option is to add a new set of manifests and signatures to the XPI. Every file in the XPI must be signed, so we would first issue {manifest-sha256.mf, signature-sha256.mf, signature-sha256.rsa}, then issue the current set of SHA1 files that includes the SHA-256 set. This requires a bit of backend signing complexity, but should be backward compatible in older clients, that will just verify the hashes of the SHA-256 set and ignore the files.

The third option is to issue two versions of add-ons, one signed with SHA-1 and one signed with SHA-256, and use AMO compatibility lists to serve the right version of the add-on to the right client. The downside of this approach is that hosting cost of storing two version of each add-on (several GB of EFS storage).

[1] https://tools.ietf.org/html/rfc2315#section-9.2
> From: David Keeler <dkeeler@mozilla.com>
> 
> Unfortunate news: without modification, Firefox will not support add-ons that have multiple signatures in the pkcs#7 blob: https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/security/manager/ssl/nsDataSignatureVerifier.cpp#214
> 
> Seems like we might have to do something more like serving different versions of add-ons to different versions of Firefox (which is unfortunate for unlisted add-ons...)

Given this, I'll take a look at option 2 and 3, which are now blocking to move Firefox over to SHA256.
Blocks: 1169532
No longer depends on: 1169532, 1180826
Assignee: nobody → franziskuskiefer
Summary: PKCS7 signatures of addons should support SHA2 → support SHA-256 when verifying PKCS7 signatures on addons
Assignee: franziskuskiefer → dkeeler
Priority: P2 → P1
Whiteboard: [psm-backlog] → [psm-assigned]
Comment on attachment 8922977 [details]
bug 1357815 - 2/4: refactor away unnecessary parts of certificate verification in add-on signature verification

https://reviewboard.mozilla.org/r/194078/#review200014

::: security/apps/AppSignatureVerification.cpp:768
(Diff revision 1)
> -  void* verifyCertificateContext, void* pinArg,
> -  const nsNSSShutDownPreventionLock& /*proofOfLock*/)
>  {
> -  // XXX: missing pinArg is tolerated.
> +  // Currently, this function is only called within the CalculateResult() method
> +  // of CryptoTasks. As such, NSS should not be shut down at this point and the
> +  // CryptoTask implementation should already hold a nsNSSShutDownPreventionLock.

I know this just moved, but it feels a bit dangerous though; this method is not declared static, this could be called by someone else who decides they want to verify a signature in the future.

Maybe this should get the proofOfLock treatment in a follow-on?
Attachment #8922977 - Flags: review?(jjones) → review+
Comment on attachment 8922977 [details]
bug 1357815 - 2/4: refactor away unnecessary parts of certificate verification in add-on signature verification

https://reviewboard.mozilla.org/r/194078/#review200080

::: security/apps/AppSignatureVerification.cpp:768
(Diff revision 1)
> -  void* verifyCertificateContext, void* pinArg,
> -  const nsNSSShutDownPreventionLock& /*proofOfLock*/)
>  {
> -  // XXX: missing pinArg is tolerated.
> +  // Currently, this function is only called within the CalculateResult() method
> +  // of CryptoTasks. As such, NSS should not be shut down at this point and the
> +  // CryptoTask implementation should already hold a nsNSSShutDownPreventionLock.

Well, hopefully the lock shenanigans will be removed in the nearish future. Also, this is in an unnamed namespace (unless I messed up), so other code shouldn't be able to call this.
Comment on attachment 8922976 [details]
bug 1357815 - 1/4: move VerifyCMSDetachedSignatureIncludingCertificate to where it's used

https://reviewboard.mozilla.org/r/194076/#review200106
Attachment #8922976 - Flags: review?(jjones) → review+
Comment on attachment 8922978 [details]
bug 1357815 - 3/4: support SHA256 in PKCS#7 signatures on add-ons

https://reviewboard.mozilla.org/r/194080/#review200036

::: security/apps/AppSignatureVerification.cpp:145
(Diff revision 1)
>    return NS_OK;
>  }
>  
>  // Finds exactly one (signature metadata) JAR entry that matches the given
> -// search pattern, and then load it. Fails if there are no matches or if
> -// there is more than one match. If bugDigest is not null then on success
> +// search pattern, and then loads it. Fails if there are no matches or if
> +// there is more than one match. If bufDigest is not null then on success

bug I loved `bugDigest`....

::: security/manager/ssl/tests/unit/pycms.py:171
(Diff revision 1)
>              asn1Spec=rfc2459.Certificate())[0]
>          extendedCertificateOrCertificate['certificate'] = certificate
>          certificates[0] = extendedCertificateOrCertificate
>          signedData['certificates'] = certificates
>  
>          signerInfos = rfc2315.SignerInfos()

univ.SetOf implements the Python list class methods [1], so I'd rather you use `signerInfos.append()` here for clarity and pythoniness.

1: https://github.com/etingof/pyasn1/blob/136f9e71f50ed8938037e5641b19d857f11b13e4/pyasn1/type/univ.py#L1699

::: security/manager/ssl/tests/unit/sign_app.py:72
(Diff revision 1)
>              base64hash = b64encode(hashFunc(mfContents).digest())
>              sfContents += '%s-Digest-Manifest: %s\n' % (name, base64hash)
>  
> -        cmsSpecification = 'hash:%s\nsigner:\n' % sha1(sfContents).hexdigest() + \
> +        cmsSpecification = ''
> +        for name in pkcs7Hashes:
> +            hashFunc = hashNameToFunction(name)

why not use `hashFunc, _ = hashNameToFunctionAndIdentifier(name)` instead of defining a new function?
Attachment #8922978 - Flags: review?(jjones) → review+
Comment on attachment 8922979 [details]
bug 1357815 - 4/4: go a bit overboard on testcases for SHA-256 support in add-on signatures

https://reviewboard.mozilla.org/r/194082/#review200204

LGTM
Attachment #8922979 - Flags: review?(jjones) → review+
Comment on attachment 8922978 [details]
bug 1357815 - 3/4: support SHA256 in PKCS#7 signatures on add-ons

https://reviewboard.mozilla.org/r/194080/#review200036

Thanks!

> univ.SetOf implements the Python list class methods [1], so I'd rather you use `signerInfos.append()` here for clarity and pythoniness.
> 
> 1: https://github.com/etingof/pyasn1/blob/136f9e71f50ed8938037e5641b19d857f11b13e4/pyasn1/type/univ.py#L1699

Unfortunately the version we have in-tree doesn't :(
In place of that, I used the [] syntax with length of the signerInfos - let me know what you think.

> why not use `hashFunc, _ = hashNameToFunctionAndIdentifier(name)` instead of defining a new function?

'cause, uh... that'd be too easy
Fixed.
Comment on attachment 8922978 [details]
bug 1357815 - 3/4: support SHA256 in PKCS#7 signatures on add-ons

https://reviewboard.mozilla.org/r/194080/#review200036

> Unfortunately the version we have in-tree doesn't :(
> In place of that, I used the [] syntax with length of the signerInfos - let me know what you think.

:sadface:
Comment on attachment 8922978 [details]
bug 1357815 - 3/4: support SHA256 in PKCS#7 signatures on add-ons

https://reviewboard.mozilla.org/r/194080/#review200788

pour one out for bugDigest

r=dveditz
Attachment #8922978 - Flags: review?(dveditz) → review+
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0104efea23b
1/4: move VerifyCMSDetachedSignatureIncludingCertificate to where it's used r=jcj
https://hg.mozilla.org/integration/autoland/rev/89b22be1685c
2/4: refactor away unnecessary parts of certificate verification in add-on signature verification r=jcj
https://hg.mozilla.org/integration/autoland/rev/3b40cb8a242e
3/4: support SHA256 in PKCS#7 signatures on add-ons r=dveditz,jcj
https://hg.mozilla.org/integration/autoland/rev/c5f4d4a711f9
4/4: go a bit overboard on testcases for SHA-256 support in add-on signatures r=jcj
Whiteboard: [psm-assigned] → [psm-assigned][adv-main58-]
Attached image Bug1357815.gif — 
I used an extension that has SHA256 signature on Firefox 57.0.4(20180103231032), 58.0b15(20180108140638) and 59.0a1(20180111220102) under Win 7 64-bit and Mac OS X 10.13.

The extension can be installed starting from Firefox 58 and up.

Is this the expected outcome?

Please see the attached video.
Flags: needinfo?(dkeeler)
Yep - thanks!
Flags: needinfo?(dkeeler)
Based on comment 25 and comment 26, this issue is verified as fixed.

Thanks David!
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
status-firefox59: --- → verified
See Also: → 1473674
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: