Closed Bug 1353325 Opened 3 years ago Closed 3 years ago

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


(NSS :: Libraries, enhancement)

Not set


(Not tracked)



(Reporter: ueno, Unassigned)



(1 file, 2 obsolete files)

Attached patch nss-pkcs12-unicode.patch (obsolete) — Splinter Review
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 script already has a coverage for this change.
Attachment #8854388 - Flags: review?(rrelyea)
Attached patch nss-pkcs12-unicode-v2.patch (obsolete) — Splinter Review
Now that the same Unicode coding logic is used in multiple places, refactored it into sec_pkcs12_{decode,encode}_password.
Attachment #8854388 - Attachment is obsolete: true
Attachment #8854388 - Flags: review?(rrelyea)
Attachment #8854412 - Flags: review?(rrelyea)
Comment on attachment 8854412 [details] [diff] [review]

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

r+ rrelyea
Attachment #8854412 - Flags: review?(rrelyea) → review+
Please needinfo me, if this is ready for landing and you want me to help.
Have you run the test suite?
Yes, I have confirmed that.  Could you push this?
Flags: needinfo?(kaie)
Closed: 3 years ago
Flags: needinfo?(kaie)
Resolution: --- → FIXED
Target Milestone: --- → 3.31
Reopening, because it didn't work with the clang compiler:

Backed out:

Daiki, could you please work on getting nss-try server access,
so you can test patches yourself, prior to submitting them for checkin?
Resolution: FIXED → ---
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++).
Attachment #8854412 - Attachment is obsolete: true
Attachment #8855690 - Flags: review?(kaie)
No problem, while we're waiting for your access, I've pushed a try build with your v3 patch:
I was able to push a try build by myself and confirmed that it works:

Could you re-land the v3 patch when you have time?
Flags: needinfo?(kaie)
Comment on attachment 8855690 [details] [diff] [review]

r=kaie for the incremental change
Flags: needinfo?(kaie)
Attachment #8855690 - Flags: review?(kaie) → review+
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.