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

Categories

(NSS :: Libraries, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ueno, Unassigned)

Details

Attachments

(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 tools.sh 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]
nss-pkcs12-unicode-v2.patch

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)
https://hg.mozilla.org/projects/nss/rev/7e62307b5cf413080b399a45edb18082a9b71f6e
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(kaie)
Resolution: --- → FIXED
Target Milestone: --- → 3.31
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
Status: RESOLVED → REOPENED
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:
  https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=b0fdbaae2714c265139bcddca8d9f7f1d2e5d1f5
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?
Flags: needinfo?(kaie)
Comment on attachment 8855690 [details] [diff] [review]
nss-pkcs12-unicode-v3.patch

r=kaie for the incremental change
Flags: needinfo?(kaie)
Attachment #8855690 - Flags: review?(kaie) → review+
https://hg.mozilla.org/projects/nss/rev/ef11922df67881332f1fa200c7ae21b9c30cec76
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.