Closed Bug 1345106 Opened 7 years ago Closed 7 years ago

Don't use SHA1 by default for signatures in the NSS library and in certutil, crlutil and cmsutil

Categories

(NSS :: Libraries, enhancement)

3.30
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: KaiE, Assigned: KaiE)

Details

Attachments

(3 files)

NSS should be changed to use something better than SHA1 when creating signatures, by default, if the application hasn't requested a specific hash.

At least our tools should be changed, but when possible, the library defaults, too.

(See also: https://bugzilla.redhat.com/show_bug.cgi?id=1309781 )
Assignee: nobody → kaie
Three years ago we made a first step in bug 1058933, but that change was very limited, it only affected when signing with certutil and with RSA.
certutil and crlutil don't request a specific hash, unless it's requested with a command line parameter. Changing the library default for all algorithms, like we did in bug 1058933, should cover them.

Looking at signtool (specific for signing JAR files), it already uses a syntax that creates multiple signatures, currently for MD5 and SHA1. I don't know if we break anything if we remove one of the existing ones. It might work, and be safest, to extend it to create SHA256 in addition.
Depends on: 1345528
Attached patch 1345106-v4.patchSplinter Review
This patch should effectively change certutil, crlutil and cmsutil to no longer use SHA1 for signatures.

In addition, it changes the library default hash function used for EC and DSA signatures.
Attachment #8845083 - Flags: review?(rrelyea)
(I'd like to handle signtool separately in bug 1345528.)
No longer depends on: 1345528
Summary: Don't use SHA1 by default for signatures → Don't use SHA1 by default for signatures in the NSS library and in certutil, crlutil and cmsutil
Comment on attachment 8845083 [details] [diff] [review]
1345106-v4.patch

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

I'm a little worried about the DSA case in SEC_DerSignData, but not too worried. There are some caveats for DSA:
The Hash oid value is not encoded with the signature itself, and the length of the hash is truncated or expanded to match the length of the underlying dsa subgroup group (length of q). When signing with a 1024 bit key (which has q=20 bytes), we would still use 20 bytes of the 32 bytes in SHA256. An attacker could still use that value and claim it's a SHA-1 signature if he can find a collision between SHA1 and SHA256.

OTOH, The few  out there DSA users should already be avoiding SHA-1 anyway. The same issue also applies to ECC as well, so we are just biting the bullet. Currently SHA1 is still strong against 2nd preimage attack, which you would really need to make all this work.

bob

::: lib/cryptohi/secsign.c
@@ +325,1 @@
>                          algID = SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST;

Hmm I wonder if we shouldn't just change default dsa key we generate. It seems that maybe we should still use sha1 when q is 320 bits (20*8*2), but move the default tag.
Attachment #8845083 - Flags: review?(rrelyea) → review+
Attached patch 1345106-v5.patchSplinter Review
IIUC, Bob suggests that in SEC_DerSignData we add

                 switch (PK11_SignatureLen(pk)) {
+                    case 320:
+                        algID = SEC_OID_ANSIX9_DSA_SIGNATURE_WITH_SHA1_DIGEST;
+                        break;

This is the only change in this patch v5.
Bob, is this correct?
Attachment #8845602 - Flags: review?(rrelyea)
Comment on attachment 8845602 [details] [diff] [review]
1345106-v5.patch

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

r+ rrelyea
Thanks Kai.
Attachment #8845602 - Flags: review?(rrelyea) → review+
https://hg.mozilla.org/projects/nss/rev/848abc2061a4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: