+++ This bug was initially created as a clone of Bug #1169532 +++ The new AMO addon mass signing frenzy is upon us, but the signatures aren't exactly bleeding edge. Signed extension manifest.rf files contain MD5 & SHA1 digests. Finding an MD5 collision now takes half a day via cloud computing, at the cost of less than a dollar. SHA1 is considered weak and is being phased out by everyone in favor of SHA2. The combination of the two together, if both checked, can be greater than the sum of their parts, but they're still old deprecated parts. I did a little digging, and near as I can tell, there is currently no support for better hash algorithms here. I'm also not entirely sure the MD5 hash is checked at all, but there's multiple different manifest parsers in the codebase and I'm not sure which gets used. Support needs to be added for SHA2 at some point and SHA3 might be desired in the not too distant future as well. SHA1 can be kept for backwards compatibility for now, but MD5 needs to be nuked from orbit as soon as better options are available. While we're on the subject of obsolete crypto, transitioning from RSA to some form of ECC signature would significantly reduce the key size and possibly improve performance. See also the application update signing equivalent of this: bug 1105689

If we're going to modernize XPI signatures, could we move away from the jarsigner format as well? This may be a good fit for TUF [1], if we can adapt it to work with the AMO PKI. [1] https://github.com/theupdateframework/tuf/blob/develop/docs/tuf-spec.txt

New research has produced a SHA-1 collision that in theory means a malicious actor could submit a benign version of add-on code for review and then swap out one of the included files with a malicious counterpart with the same hash. Currently we only review add-ons that are hosted on AMO. Malicious actors could submit the malicious version and get it signed without this computationally intensive subterfuge, although at a small risk we might find the bad code after the fact. For add-ons that are reviewed and hosted on AMO we will host the reviewed version and there wouldn't be the opportunity for the bad actor to replace the component that had the hash collision.

Dan, what do you think the best approach here is - have a watershed version where we switch immediately from sha-1 to sha-256 or have one (or more) versions where we support sha-1 and sha-256?

We need to keep supporting sha-1 for a while, because we can't resign all add-ons to have sha-256 right away (many aren't hosted by us).

The manifest format supports multiple hashes. In the past (Java days until recently) it supported MD5 and SHA1. Signers put both hashes in every item and the clients consumed the one they thought was most secure. We could simply add SHA-256 and/or SHA-512 support, and the signing tool could add both sha2/sha1 hashes for each manifest item. Old clients will ignore the unknown SHA-2 hashes, and new clients will ignore the insecure SHA-1 hashes. We might even have new clients fall-back to SHA1 if no sha-2 is found so we don't have to re-sign all the old packages. We'd have to think through if that means an attacker with a sha-1 collision could strip the SHA-2 hashes and then tweak the content to match their colliding sha-1 hash. Could they? I guess, but it does provide a transition strategy. Not sure what to do about the PKCS7 signatures themselves. IIRC currently the code barfs if it finds multiple signatures. We could change that and support both? but old clients will be broken so it's still not much of a transition. We have a deadline of Firefox 59, because if we miss that then we'll have to continue supporting SHA-1-signed (or double-signed) packages for the life of that ESR. If 59 has SHA-2 support then we can transition after 52ESR goes EOL (~fx 61).

Here's the patch to the backend signing library that adds SHA256-Digest to the manifest: https://github.com/mozilla/signing-clients/pull/22/files

Comment on attachment 8920709 [details] bug 1180826 - add support for sha256 digests in add-on signature manifests https://reviewboard.mozilla.org/r/191710/#review197010 Looks good, minor things noted. ::: security/apps/AppSignatureVerification.cpp:50 (Diff revision 1) > extern mozilla::LazyLogModule gPIPNSSLog; > > namespace { > > +// A convenient way to pair the bytes of a digest with the algorithm that > +// purportedly produced those bytes. Only SHA-1 and SHA-256 are supported. Should we add more algorithmic flexibility here so we can switch by re-signing everything immediately if sha-256 starts looking weak? ::: security/apps/AppSignatureVerification.cpp:541 (Diff revision 1) > - // reject just by adding additional SHA1-Digest-Manifest > + // reject just by adding additional SHA*-Digest-Manifest > // attributes. > - break; > + return NS_OK; > + } > + > + if (attrName.LowerCaseEqualsLiteral("sha1-digest-manifest")) { Shouldn't this be if (attrName.LowerCaseEqualsLiteral("sha1-digest-manifest") && savedSHA1Digest.IsEmpty()) ? I suppose it doesn't matter too much if we take the last or the first, but it does mean that if we encounter a JAR with two different SHA-1 hashes that we will make a different decision about whether it's valid or not compared to an older Firefox. Since we're the only ones signing these I'm not worried in practice, just getting a fingernails-on-blackboard feeling from having a big comment justifying why we're doing it one way for sha256, followed by the next check that does the opposite. ::: security/apps/AppSignatureVerification.cpp:557 (Diff revision 1) > } > > // Parses MANIFEST.MF. The filenames of all entries will be returned in > // mfItems. buf must be a pre-allocated scratch buffer that is used for doing > // I/O. > nsresult Maybe should add to the comment here that we will only look for the hash algorithm that was used in the .SF file. We support either-or-both in the .sf file, but the .mf file must match. ::: security/apps/AppSignatureVerification.cpp:818 (Diff revision 1) > // Signature (SF) file > nsAutoCString sfFilename; > ScopedAutoSECItem sfBuffer; > Digest sfCalculatedDigest; > rv = FindAndLoadOneEntry(zip, NS_LITERAL_CSTRING(JAR_SF_SEARCH_STRING), > - sfFilename, sfBuffer, &sfCalculatedDigest); > + sfFilename, sfBuffer, SEC_OID_SHA1, Does this explicit instance of SHA1 get fixed in bug 1357815? ::: security/manager/ssl/tests/unit/sign_app.py:125 (Diff revision 1) > + default=[]) > parsed = parser.parse_args(args) > - signZip(appPath, outputFile, parsed.issuer, not parsed.no_sign) > + if len(parsed.manifest_hash) == 0: > + parsed.manifest_hash.append('sha1') > + if len(parsed.signature_hash) == 0: > + parsed.signature_hash.append('sha1') Could you make this utility default to sha256? Or did that require changing lots of existing tests you didn't want to mess with?

Comment on attachment 8920709 [details] bug 1180826 - add support for sha256 digests in add-on signature manifests https://reviewboard.mozilla.org/r/191710/#review197010 Thanks! > Should we add more algorithmic flexibility here so we can switch by re-signing everything immediately if sha-256 starts looking weak? My thinking here is that we want to keep this as an explicit whitelist, so we would have to list any such hash algorithms we would accept. The problem is that we basically don't accept any others. My understanding is that NSS doesn't even have a SHA-3 implementation. (And supporting SHA-2 384 / SHA-2 512 wouldn't really help as a weakness in SHA-2 256 is likely to apply to the others as well.) > Shouldn't this be > > if (attrName.LowerCaseEqualsLiteral("sha1-digest-manifest") && savedSHA1Digest.IsEmpty()) ? > > I suppose it doesn't matter too much if we take the last or the first, but it does mean that if we encounter a JAR with two different SHA-1 hashes that we will make a different decision about whether it's valid or not compared to an older Firefox. > > Since we're the only ones signing these I'm not worried in practice, just getting a fingernails-on-blackboard feeling from having a big comment justifying why we're doing it one way for sha256, followed by the next check that does the opposite. Yep - good call. > Maybe should add to the comment here that we will only look for the hash algorithm that was used in the .SF file. We support either-or-both in the .sf file, but the .mf file must match. Ok - I updated the documentation. > Does this explicit instance of SHA1 get fixed in bug 1357815? Yep - that's the plan. > Could you make this utility default to sha256? Or did that require changing lots of existing tests you didn't want to mess with? Not many - fixed.

Comment on attachment 8920709 [details] bug 1180826 - add support for sha256 digests in add-on signature manifests https://reviewboard.mozilla.org/r/191710/#review197462

Thanks for the reviews! https://treeherder.mozilla.org/#/jobs?repo=try&revision=171ea77eb133

Pushed by dkeeler@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/34861ae44db0 add support for sha256 digests in add-on signature manifests r=dveditz,jcj