1058933 - Change library's signature algorithm default to SHA256 (affects SEC_GetSignatureAlgorithmOidTag, certutil cert/CSR creation, CRMF code)

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert [:KaiE:]]

When creating a CSR, certutil currently uses a signature algorithm with SHA1.
(The option to change that is currently undocumented, and thus few people will find it...)

This is a suggestion to change the default to SHA256.

Any objections?

Comment 1

[Kai Engert [:KaiE:]]

Changing this default would also cause other commands like -C and -S to use SHA256, too.

Can we change the default for all of them?

Comment 2

[Wan-Teh Chang]

Yes, it is fine to change the default signature hash algorithm
to SHA-256.

Comment 3

[Robert Relyea]

yes please change the default kai. We'll have to talk about whether we can do change the default in shipping RHEL, but we should make the upstream change now so future RHEL will definately have the new default.

bob

Comment 4

[Kai Engert [:KaiE:]]

Currently, certutil doesn't has a default embedded. It uses SEC_OID_UNKNOWN, and let's function SEC_GetSignatureAlgorithmOidTag() make the decision, which currently defaults to SEC_OID_SHA1.

We have two choices how to handle this bug.

(a) Change certutil to use SEC_OID_SHA256 by default.

(b) Change the default used by function SEC_GetSignatureAlgorithmOidTag() to 
    SEC_OID_SHA256.

Which one do you prefer?

Comment 5

[Kai Engert [:KaiE:]]

Attachment: patch (a)

This patch implements:

(a) Change certutil to use SEC_OID_SHA256 by default.

Comment 6

[Kai Engert [:KaiE:]]

Attachment: patch (b)

This patch implements:

(b) Change the default used by function SEC_GetSignatureAlgorithmOidTag() to 
    SEC_OID_SHA256.

    nss/lib/cryptohi/secsign.c

Comment 7

[Robert Relyea]

Comment on attachment 8479763 [details] [diff] [review]
patch (a)

I think you attached the wrong patch. This one is the same as 'b'.

Comment 8

[Robert Relyea]

I'm ok with either (a) or (b) with a slight preference for (b). If we have other objecters, I'm happy to yield to (a). I've rejected the (a) patch because it's really the (b) patch, so if you attach the real (a) patch I'll approve it as well.

bob

Comment 9

[Kai Engert [:KaiE:]]

Attachment: real patch (a)

sorry, here is the real patch (a)

Comment 10

[Robert Relyea]

Comment on attachment 8479959 [details] [diff] [review]
real patch (a)

r+ I prefer patch (b). Please do not apply both patches.

bob

Comment 11

[Kai Engert [:KaiE:]]

I'm OK with patch (b). If you do patch (b), we should change the summary of this bug, to indicate that we're changing the library default.

Comment 12

[Kai Engert [:KaiE:]]

I see only one place in the library, where the default seems to be used:
- crmfpop.c, crmf_get_key_sign_tag(), hardcoded to always use the default, 
  and which would therefore change to SHA256.

The following seems to be unaffected by a change of default:
- p7encode.c, sec_pkcs7_encoder_sig_and_certs():
  doesn't seem to use the default - the code queries the signer information.
- cmssiginfo.c, NSS_CMSSignerInfo_Sign():
  doesn't seem to use the default - the code queries the signer information.

Comment 13

[Adam Williamson]

A similar downstream bug:

https://bugzilla.redhat.com/show_bug.cgi?id=1062444

notes that as key length increases, a different hash function should be chosen (it doesn't make much sense to use a key that gives 192 bits of security but a hash function that only gives 128). http://csrc.nist.gov/publications/nistpubs/800-57/sp800-57_part1_rev3_general.pdf suggests that a 3072-bit asymmetric key provides 128 bits of security (and hence one should use a >256-bit hashing function when signing certs that use keys longer than 3072 bits), and a 7680-bit asymmetric key provides 192 bits of security (hence one should use a >384-bit hashing function when signing certs that use keys longer than 7680 bits). Basically, use SHA-384 if the key length is > 3072 bits, and SHA-512 if the key length is > 7680 bits.

Would it be a good idea to do this instead of / in addition to simply swapping SHA-256 in for SHA-1 as the default?

Comment 14

[Kai Engert [:KaiE:]]

(In reply to Adam Williamson from comment #13)
> Would it be a good idea to do this instead of / in addition to simply
> swapping SHA-256 in for SHA-1 as the default?

This might require more work, as it needs to be researched where are the right places to decide about the hash sizes. Also, it would be algorithm specific, because of the difference of key sizes between RSA/ECC.

Would you mind to file this as a separate bug?

Comment 15

[Kai Engert [:KaiE:]]

Comment on attachment 8479959 [details] [diff] [review]
real patch (a)

marking this one as feedback- because we prefer the other patch in the more general place.

Comment 16

[Kai Engert [:KaiE:]]

Comment on attachment 8479766 [details] [diff] [review]
patch (b)

checked in:
https://hg.mozilla.org/projects/nss/rev/1db83ff1b65b