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

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Kai Engert (:KaiE:)]

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 )

Comment 1

[Kai Engert (: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.

Comment 2

[Kai Engert (:KaiE:)]

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.

Comment 3

[Kai Engert (:KaiE:)]

Attachment: backup of work-in-progress

Comment 4

[Kai Engert (:KaiE:)]

Attachment: 1345106-v4.patch

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.

Comment 5

[Kai Engert (:KaiE:)]

(I'd like to handle signtool separately in bug 1345528.)

Comment 6

[Robert Relyea]

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.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: 1345106-v5.patch

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?

Comment 8

[Robert Relyea]

Comment on attachment 8845602 [details] [diff] [review]
1345106-v5.patch

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

r+ rrelyea
Thanks Kai.

Comment 9

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/848abc2061a4