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

RESOLVED FIXED in 3.30

Status

NSS
Tools
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: Hubert Kario, Assigned: ueno)

Tracking

({good-first-bug, regression})

3.23
3.30
good-first-bug, regression

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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

2 years ago
Is this an enhancement request, or a regression report?
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

2 years ago
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.

Updated

2 years ago
Keywords: regression
Keywords: good-first-bug
(Assignee)

Comment 3

2 years ago
Created attachment 8801049 [details] [diff] [review]
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
(Assignee)

Comment 4

2 years ago
(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.
(Assignee)

Comment 5

2 years ago
Created attachment 8801100 [details] [diff] [review]
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.
Attachment #8801049 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8801100 - Flags: review?(rrelyea)

Comment 6

2 years ago
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.
Attachment #8801100 - Flags: review?(rrelyea) → review-
Assignee: nobody → dueno
(Assignee)

Comment 7

2 years ago
Created attachment 8810346 [details] [diff] [review]
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()
Attachment #8801100 - Attachment is obsolete: true
Attachment #8810346 - Flags: review?(rrelyea)

Comment 8

2 years ago
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.
Attachment #8810346 - Flags: review?(rrelyea) → review+
Is this ready for checkin, or will there be another patch?
If you want me to help with checkin, please ensure that you have executed the test suite, and set needinfo for me.
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.
(Assignee)

Comment 12

a year ago
Created attachment 8835535 [details] [diff] [review]
nss-pkcs12-aes-v4.patch
Attachment #8810346 - Attachment is obsolete: true
(Assignee)

Comment 13

a year ago
(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.
(Assignee)

Comment 14

a year ago
(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)?
Flags: needinfo?(kaie)
(Assignee)

Updated

a year ago
See Also: → bug 452464
(Assignee)

Comment 16

a year ago
Created attachment 8836629 [details] [diff] [review]
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.
Attachment #8835535 - Attachment is obsolete: true
checked in:
https://hg.mozilla.org/projects/nss/rev/09d1a0757431
Status: NEW → RESOLVED
Last Resolved: a year ago
Flags: needinfo?(kaie)
Resolution: --- → FIXED
Target Milestone: --- → 3.30

Updated

6 months ago
Blocks: 1416265
You need to log in before you can comment on or make changes to this bug.