Closed Bug 1353724 Opened 6 years ago Closed 6 years ago

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


(NSS :: Libraries, enhancement)

Not set


(Not tracked)



(Reporter: ueno, Unassigned)



(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:

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]

Review of attachment 8854848 [details] [diff] [review]:


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 ( != 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:
(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)
Closed: 6 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.