Closed Bug 1353724 Opened 8 years ago Closed 8 years ago

fix key length calculation for PKCS#5 PBES2 DES-EDE3-CBC-Pad

Categories

(NSS :: Libraries, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ueno, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch nss-pbes2-des-ede3-cbc-pad.patch (obsolete) — Splinter Review
In PKCS#5 v2.1, DES-EDE3-CBC-Pad encryption scheme is defined as it uses the predefined key length (= 24 octets) instead of the length value read from the parameter, like AES-CBC-Pad: https://tools.ietf.org/html/rfc8018#appendix-B.2.2 This is currently causing interoperability issue with OpenSSL: # Create a PKCS#12 bundle with AES-CBC-Pad encrypted private key $ openssl req -x509 -newkey rsa -keyout localhost.key -out localhost.crt -subj /CN=localhost -nodes -batch $ openssl pkcs12 -export -out bundle.p12 -in localhost.crt -inkey localhost.key -caname server-cert -passout pass:'Red Hat Enterprise Linux 7.4' -keypbe des-ede3-cbc # Try to import the bundle into NSS database $ mkdir nssdb $ certutil -N --empty-password -d sql:./nssdb $ pk12util -d sql:./nssdb -i bundle.p12 -W 'Red Hat Enterprise Linux 7.4' pk12util: PKCS12 decode import bags failed: SEC_ERROR_PKCS12_UNABLE_TO_IMPORT_KEY: Unable to import. Error attempting to import private key. The attached patch should fix this.
Attachment #8854848 - Flags: review?(rrelyea)
Comment on attachment 8854848 [details] [diff] [review] nss-pbes2-des-ede3-cbc-pad.patch Review of attachment 8854848 [details] [diff] [review]: ----------------------------------------------------------------- r- I'm OK with this as a temp patch to an older branch like RHEL-7, or and older version of NSS, but I would prefer a fix that prevents this if statement from exploding as we add new fixed length algorithms in the future. ::: lib/pk11wrap/pk11pbe.c @@ +369,5 @@ > if (sec_pkcs5_is_algorithm_v2_aes_algorithm(cipherAlg)) { > length = sec_pkcs5v2_aes_key_length(cipherAlg); > + } else if (cipherAlg == SEC_OID_DES_EDE3_CBC) { > + /* The key length for PKCS#5 DES-EDE3-CBC-Pad is predefined */ > + length = 24; Sigh, In theory SECOID_DES_EDE3_CBC could be 16 bytes. So 1) we should put this test after the else if (p5_param.keyLength.data != NULL) text bellow. 2) instead of checking for SEC_OID_EDE3_CBC we should call one of the pk11wrap functions to get a key length from an oid or mechanism (there are functions to map between them). Perhaps call PK11_GetMaxKeyLength(), inside PK11_GetMaxKeyLength() we could fall back to calling PK11_GetKeyType(mech,0) and then pk11_GetPredefinedKeyLength(keytype). This will guarrentee things work for fixed key size algorithms. @@ +659,5 @@ > } else if (sec_pkcs5_is_algorithm_v2_aes_algorithm(cipherAlgorithm)) { > keyLength = sec_pkcs5v2_aes_key_length(cipherAlgorithm); > + } else if (cipherAlgorithm == SEC_OID_DES_EDE3_CBC) { > + /* The key length for PKCS#5 DES-EDE3-CBC-Pad is predefined */ > + keyLength = 24; This shouldn't be necessary. The block below should return the correct value key length for SEC_OID_DES_EDE3_CBC. If there is a bug there, please identify it so we can fix it. If there's a bug in softoken, then we should 1) fix the softoken bug, and 2) use the same fallback idea presented above.
Attachment #8854848 - Flags: review?(rrelyea) → review-
Thank you for the review. I have updated the patch along those lines and confirmed the tests still work. Try build: https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=c9af3144ac8cd7e2203817a334a9f814649e86b0 (There is a test failure on aarch64, but I suppose it's a system error and unrelated to this change.)
Attachment #8854848 - Attachment is obsolete: true
Attachment #8862858 - Flags: review?(rrelyea)
Attachment #8862858 - Flags: review?(rrelyea) → review+
Thank you for the review, Bob. Kai, could you push this when you have time?
Flags: needinfo?(kaie)
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(kaie)
Resolution: --- → FIXED
Target Milestone: --- → 3.31
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: