Closed Bug 1058933 Opened 5 years ago Closed 5 years ago

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

Categories

(NSS :: Libraries, defect)

3.16.4
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
3.17.1

People

(Reporter: KaiE, Assigned: KaiE)

Details

Attachments

(2 files, 1 obsolete file)

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?
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?
Yes, it is fine to change the default signature hash algorithm
to SHA-256.
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
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?
Attached patch patch (a) (obsolete) — Splinter Review
This patch implements:

(a) Change certutil to use SEC_OID_SHA256 by default.
Assignee: nobody → kaie
Attached patch patch (b)Splinter Review
This patch implements:

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

    nss/lib/cryptohi/secsign.c
Attachment #8479763 - Flags: review?(rrelyea)
Attachment #8479766 - Flags: review?(rrelyea)
Attachment #8479766 - Flags: review?(rrelyea) → review+
Attachment #8479763 - Flags: review?(rrelyea) → review+
Comment on attachment 8479763 [details] [diff] [review]
patch (a)

I think you attached the wrong patch. This one is the same as 'b'.
Attachment #8479763 - Flags: review+ → review-
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
Attached patch real patch (a)Splinter Review
sorry, here is the real patch (a)
Attachment #8479763 - Attachment is obsolete: true
Attachment #8479959 - Flags: review?(rrelyea)
Comment on attachment 8479959 [details] [diff] [review]
real patch (a)

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

bob
Attachment #8479959 - Flags: review?(rrelyea) → review+
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.
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.
Component: Tools → Libraries
Summary: Change the default of certutil -R (create a CSR) to use SHA256 → Change library signature algorithm default to SHA256 (affects SEC_GetSignatureAlgorithmOidTag and certutil cert/CSR creation)
Summary: Change library signature algorithm default to SHA256 (affects SEC_GetSignatureAlgorithmOidTag and certutil cert/CSR creation) → Change signature algorithm default to SHA256 (affects SEC_GetSignatureAlgorithmOidTag and certutil cert/CSR creation)
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?
(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?
Summary: Change signature algorithm default to SHA256 (affects SEC_GetSignatureAlgorithmOidTag and certutil cert/CSR creation) → Change library's signature algorithm default to SHA256 (affects SEC_GetSignatureAlgorithmOidTag, certutil cert/CSR creation, CRMF code)
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.
Attachment #8479959 - Flags: feedback-
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.17.1
You need to log in before you can comment on or make changes to this bug.