1353325 - pkcs12: don't encode password into Unicode if non-PKCS#12 PBE is used

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs12-unicode.patch

PBEs defined in PKCS#12 expect BMPString encoded password, while other PBEs (such as PKCS#5 v2) expect an octet string.

We had to add a hack to differentiate those in bug 1268141, but it was not complete: a similar treatment is also necessary for wrapping/unwrapping private keys:

# 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 aes-128-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.

I am attaching a patch to fix this, for importing and exporting.  The tools.sh script already has a coverage for this change.

Comment 1

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs12-unicode-v2.patch

Now that the same Unicode coding logic is used in multiple places, refactored it into sec_pkcs12_{decode,encode}_password.

Comment 2

[Robert Relyea]

Comment on attachment 8854412 [details] [diff] [review]
nss-pkcs12-unicode-v2.patch

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

r+ rrelyea

Comment 3

[Kai Engert (:KaiE:)]

Please needinfo me, if this is ready for landing and you want me to help.
Have you run the test suite?

Comment 4

[Daiki Ueno [:ueno]]

Yes, I have confirmed that.  Could you push this?

Comment 5

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/7e62307b5cf413080b399a45edb18082a9b71f6e

Comment 6

[Kai Engert (:KaiE:)]

Reopening, because it didn't work with the clang compiler:
  https://treeherder.mozilla.org/#/jobs?repo=nss&revision=7e62307b5cf413080b399a45edb18082a9b71f6e

Backed out:
  https://hg.mozilla.org/projects/nss/rev/7228445b43ac095ebc0eee330d6a351b898ebbdd

Daiki, could you please work on getting nss-try server access,
so you can test patches yourself, prior to submitting them for checkin?
  https://wiki.mozilla.org/NSS:TryServer

Comment 7

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs12-unicode-v3.patch

Sorry for the trouble, I have just requested L1 commit access.

So far, I am attaching an amended patch that should fix the problem (tested locally with CC=clang CCC=clang++).

Comment 8

[Kai Engert (:KaiE:)]

No problem, while we're waiting for your access, I've pushed a try build with your v3 patch:
  https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=b0fdbaae2714c265139bcddca8d9f7f1d2e5d1f5

Comment 9

[Daiki Ueno [:ueno]]

I was able to push a try build by myself and confirmed that it works:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=513e07186c5c0aa233843548cb73600329ba1143

Could you re-land the v3 patch when you have time?

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 8855690 [details] [diff] [review]
nss-pkcs12-unicode-v3.patch

r=kaie for the incremental change

Comment 11

[Kai Engert (:KaiE:)]

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