1268141 - pk12util can't import PKCS#12 files encrypted with AES-128-CBC

Status: RESOLVED FIXED

(NSS :: Tools, defect)

Description

[Hubert Kario]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20160304160129

Steps to reproduce:

1. openssl req -x509 -newkey rsa -keyout localhost.key -out localhost.crt -subj /CN=localhost -nodes -batch
2. openssl pkcs12 -export -out bundle.p12 -in localhost.crt -caname server-cert -nokeys -passout pass:RedHatEnterpriseLinux7.1 -certpbe aes-128-cbc
3. pk12util -l bundle.p12 -W RedHatEnterpriseLinux7.1 -v


Actual results:

No output


Expected results:

Certificate printed

Comment 1

[Kai Engert (:KaiE:)]

Is this an enhancement request, or a regression report?

Comment 2

[Hubert Kario]

Robert Relyea said that this functionality worked in the past, so based on his word it would be a regression.

I haven't tested it with very old versions of NSS though.

Comment 3

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs12-aes.patch

Since this is marked as good-first-bug, I took some time to track it down.
As far as I can see, PKCS #5 2.1 AES-CBC-Pad encryption scheme[1] is not supported.  The attached patch tries to add support for it (both encoding and decoding).

To test, do:
$ echo RedHatEnterpriseLinux7.1 > pwfile
$ mkdir nssdb
$ certutil -N -f pwfile -d sql:nssdb
$ certutil -S -n ca -s 'CN=localhost' -t 'C,C,C' -x -w -3 -v 99 -1 -2 -5 -f pwfile -d sql:nssdb
$ pk12util -o bundle.p12 -n ca -w pwfile -k pwfile -d sql:nssdb -C AES-128-CBC
$ pk12util -l bundle.p12 -w pwfile -v

Unfortunately, it doesn't work with the .p12 file generated by OpenSSL (at least version 1.0.2j installed here).  The reason is, with AES-CBC-Pad, OpenSSL apparently uses an ASCII passphrase instead of BMPString-encoded one for encryption, while it uses BMPString passphrase for computing MAC.  I am not sure if this is a bug or for some compatibility reasons.

1. https://tools.ietf.org/html/draft-moriarty-pkcs5-v2dot1-04#appendix-B.2.5

Comment 4

[Daiki Ueno [:ueno]]

(In reply to Daiki Ueno [:ueno] from comment #3)

> Unfortunately, it doesn't work with the .p12 file generated by OpenSSL (at
> least version 1.0.2j installed here).  The reason is, with AES-CBC-Pad,
> OpenSSL apparently uses an ASCII passphrase instead of BMPString-encoded one
> for encryption, while it uses BMPString passphrase for computing MAC.  I am
> not sure if this is a bug or for some compatibility reasons.

Sorry, it seems that I was reading PKCS #12 wrongly, as if all passwords shall be encoded as BMPString even if the encryption scheme is defined in other standards.  In this case, since the scheme is PBES2, the password shouldn't be encoded to BMPString.  I will update the patch soon.

Comment 5

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs12-aes-v2.patch

I have changed it to use the original password for encryption/decryption with PBES2, by decoding it back from the BMPString form.  It's not clean but I thought that we cannot change SEC_PKCS12DecoderStart() to accept the original password for compatibility reasons.

I have confirmed that it is now interoperable with OpenSSL and GnuTLS.

Comment 6

[Robert Relyea]

Comment on attachment 8801100 [details] [diff] [review]
nss-pkcs12-aes-v2.patch

Review of attachment 8801100 [details] [diff] [review]:
-----------------------------------------------------------------

good job running this to ground Daiki. My only quibble is the code in p12e.c I'd rather see a mix in this patch where we have a SEC_PKCS7CreateEncryptedDataWithPBE5v2() function that has the parameters to handle pkcs #5v2 oids.

bob

::: lib/pkcs12/p12d.c
@@ +204,5 @@
> +        sec_pkcs12_convert_item_to_unicode(NULL, &decodedPwitem,
> +                                           p12dcx->pwitem,
> +                                           PR_TRUE, PR_FALSE, PR_FALSE)) {
> +        pwitem = &decodedPwitem;
> +    }

Sigh, they changes the pw encoding from unicode to utf8? OK.

::: lib/pkcs12/p12e.c
@@ +435,5 @@
> +            break;
> +        default:
> +            break;
> +        }
> +    }

It might be preferable to create a new PKCS7CreateEncryptedData. The existing interface was created so that we could create PKCS #5v2 with minimal changes. If those changes aren't sufficient, then we should adjust the PKCS7 code.

This code is assuming a lot of how PKCS7CreateEncryptedData works internally. I'd rather create a new PKCS7CreateEncryptedData that handle PKCS #5v2 and then call it from here.

Comment 7

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs12-aes-v3.patch

Thank you for the review.  I am attaching a new patch with the following changes:
- added SEC_PKCS7CreateEncryptedDataWithPBEV2() for creating encryptedData from PKCS #5 v2 PBE algorithms
- consolidated AES key length calculation into a new function sec_pkcs5v2_cipher_key_length()
- guess the correct key length of SEC_OID_AES_*_CBC, if 0 is passed to PK11_CreatePBEV2AlgorithmID()

Comment 8

[Robert Relyea]

Comment on attachment 8810346 [details] [diff] [review]
nss-pkcs12-aes-v3.patch

Review of attachment 8810346 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Daiki. My comments are more railing against the hacks in the protocol introduced by others rather than your code.

bob

::: lib/pk11wrap/pk11pbe.c
@@ +302,5 @@
>      return SEC_OID_UNKNOWN;
>  }
>  
> +static int
> +sec_pkcs5v2_cipher_key_length(SECOidTag algorithm)

I'd like this to be more general. We should have a general key length from oid function in pk11mech.c, so was we add new algorithms we don't have to muck around with this code again.

OK, this is fine because it's dealing with a pkcs 12 specific issue where new oids were invented because someone (openSSL? windows?) didn't use the standard SECOID_AES_CBC and encode the key length in the pkcs5 param block like the pkcs5 spec intended

@@ +353,5 @@
>          length = DER_GetInteger(&p5_param.keyLength);
> +    } else if (cipherAlgId) {
> +        SECOidTag cipherAlg;
> +        cipherAlg = SECOID_GetAlgorithmTag(cipherAlgId);
> +        length = sec_pkcs5v2_cipher_key_length(cipherAlg);

Sigh, the whole point of pkcs 5 was to encode this length in the parameter. Not your foult we have to hack our code because some other applications can't read the spec.

@@ +639,4 @@
>              if (hashAlg != SEC_OID_UNKNOWN) {
>                  keyLength = HASH_ResultLenByOidTag(hashAlg);
>              } else {
> +                keyLength = sec_pkcs5v2_cipher_key_length(cipherAlgorithm);

OK, so the issue here is that we already have a 'get the key length' from the mechanism, but the mechanism supports more than one length, and that length is encoded in an oid rather than the the pkcs5 param block because someone didn't understand the purpose of the param block.

Comment 9

[Kai Engert (:KaiE:)]

Is this ready for checkin, or will there be another patch?

Comment 10

[Kai Engert (:KaiE:)]

If you want me to help with checkin, please ensure that you have executed the test suite, and set needinfo for me.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Please push this to nss-try before landing, turnaround time shouldn't be more than an hour.

AFAICT this patch doesn't have any tests? I don't think we should land new code without any kind of test. Writing gtests is super easy these days and we can have tests similar to the ones I added in bug 1295121.

Comment 12

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs12-aes-v4.patch

Comment 13

[Daiki Ueno [:ueno]]

(In reply to Robert Relyea from comment #8)

Thank you for the review.

> > +static int
> > +sec_pkcs5v2_cipher_key_length(SECOidTag algorithm)
> 
> I'd like this to be more general. We should have a general key length from
> oid function in pk11mech.c, so was we add new algorithms we don't have to
> muck around with this code again.

Yes that would be nice.

> OK, this is fine because it's dealing with a pkcs 12 specific issue where
> new oids were invented because someone (openSSL? windows?) didn't use the
> standard SECOID_AES_CBC and encode the key length in the pkcs5 param block
> like the pkcs5 spec intended

In the the PKCS#5 spec, the AES based encryption scheme (AES-CBC-Pad) is defined differently to others.  That is, the key length is always calculated from the algorithm ID.  To make it more explicit, I changed the function name to sec_pkcs5v2_aes_key_length() and added checks whether the cipher algorithm is AES to the callers.

(In reply to Tim Taubert [:ttaubert] from comment #11)

> AFAICT this patch doesn't have any tests? I don't think we should land new
> code without any kind of test. Writing gtests is super easy these days and
> we can have tests similar to the ones I added in bug 1295121.

Thank you for pointing that.  There was actually some code checking AES support in tests/tools/tools.sh, which was commented out.  I just enabled it and confirmed that it runs properly.

Comment 14

[Daiki Ueno [:ueno]]

(In reply to Kai Engert (:kaie) from comment #10)
> If you want me to help with checkin, please ensure that you have executed
> the test suite, and set needinfo for me.

The patch attached to comment #12 should be ready for check-in, though it has a minor modification from the previous version attached to comment #7.  Could you push it to nss-try (I am not sure if I can do that by myself)?

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Pushed it to try:

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=af803706521d13bb6b5a66425bbb35248c0a7590

Comment 16

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs12-aes-v5.patch

Thank you.  It seems to have detected clang-format issues.  I am attaching an amended patch with the style fixes.

Comment 17

[Kai Engert (:KaiE:)]

checked in:
https://hg.mozilla.org/projects/nss/rev/09d1a0757431