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

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Daiki Ueno [:ueno]]

Attachment: nss-pbes2-des-ede3-cbc-pad.patch

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.

Comment 1

[Robert Relyea]

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.

Comment 2

[Daiki Ueno [:ueno]]

Attachment: nss-pbes2-des-ede3-cbc-pad-v2.patch

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.)

Comment 3

[Daiki Ueno [:ueno]]

Thank you for the review, Bob.

Kai, could you push this when you have time?

Comment 4

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/f161a5fb1bf1