support SHA-256 when verifying PKCS7 signatures on addons

VERIFIED FIXED in Firefox 58

Status

()

Core
Security: PSM
P1
normal
VERIFIED FIXED
10 months ago
a month ago

People

(Reporter: ulfr, Assigned: keeler)

Tracking

(Blocks: 2 bugs, {sec-moderate})

unspecified
mozilla58
sec-moderate
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 verified, firefox59 verified)

Details

(Whiteboard: [psm-assigned][adv-main58-])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

10 months ago
+++ 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.
(Assignee)

Comment 1

8 months ago
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]
(Reporter)

Comment 2

8 months ago
Can this be made future-proof and take the digest algorithm from the pkcs7 signer info?
(Reporter)

Comment 3

8 months ago
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
(Reporter)

Comment 4

8 months ago
> 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
(Assignee)

Updated

5 months ago
Summary: PKCS7 signatures of addons should support SHA2 → support SHA-256 when verifying PKCS7 signatures on addons
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Assignee: franziskuskiefer → dkeeler
Priority: P2 → P1
Whiteboard: [psm-backlog] → [psm-assigned]

Comment 9

4 months ago
mozreview-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/#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+
(Assignee)

Comment 10

4 months ago
mozreview-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 11

4 months ago
mozreview-review
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 12

4 months ago
mozreview-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 13

4 months ago
mozreview-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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

4 months ago
mozreview-review-reply
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 19

4 months ago
mozreview-review-reply
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 20

4 months ago
mozreview-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/#review200788

pour one out for bugDigest

r=dveditz
Attachment #8922978 - Flags: review?(dveditz) → review+

Comment 22

4 months ago
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-]
(Assignee)

Updated

a month ago
Duplicate of this bug: 1273740

Comment 25

a month ago
Created attachment 8942128 [details]
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)

Comment 27

a month ago
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
You need to log in before you can comment on or make changes to this bug.