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)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.31
People
(Reporter: ueno, Unassigned)
Details
Attachments
(1 file, 1 obsolete file)
1.97 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | 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.
Reporter | ||
Updated•8 years ago
|
Attachment #8854848 -
Flags: review?(rrelyea)
Comment 1•8 years ago
|
||
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-
Reporter | ||
Comment 2•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8862858 -
Flags: review?(rrelyea) → review+
Reporter | ||
Comment 3•8 years ago
|
||
Thank you for the review, Bob.
Kai, could you push this when you have time?
Flags: needinfo?(kaie)
Comment 4•8 years ago
|
||
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.
Description
•