Closed
Bug 277334
Opened 20 years ago
Closed 18 years ago
pk12util can't import a pfx file encrypted with an empty password
Categories
(NSS :: Tools, defect, P3)
Tracking
(Not tracked)
RESOLVED
FIXED
3.11.1
People
(Reporter: nelson, Assigned: neil.williams)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
12.88 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
This bug reports a problem for pk12util that is also reported against PSM in bug 265991. When a user "exports" a cert and private key from Microsoft Windows' key store, the "Certificate Export Wizard" asks the user for a password for the PFX file being created. The dialog says: Password To maintan security, you must protect the private key by using a password. Type and confirm a password. Password: [ ] Confirm password: [ ] [ <Back ] [ Next> ] [Cancel ] However, the user can merely click Next> without entering any passwords. In this case, the Wizard creates a pfx file encrypted with an empty password. However, IMO, it does not strictly follow the PKCS12 standard for that empty password. The PKCS12 standard says: In this specification however, all passwords are created from BMPStrings with a NULL terminator. This means that each character in the original BMPString is encoded in 2 bytes in big-endian format (most-significant byte first). There are no Unicode byte order marks. The 2 bytes produced from the last character in the BMPString are followed by two additional bytes with the value 0x00. To illustrate with a simple example, if a user enters the 6-character password "Beavis", the string that PKCS #12 implementations should treat as the password is the following string of 14 bytes: 0x00 0x42 0x00 0x65 0x00 0x61 0x00 0x76 0x00 0x69 0x00 0x73 0x00 0x00 According to that standard, the UCS2 password from which the key is computed includes a trailing 2-byte NULL character, and is one character longer than the number of characters in the user's password. So, when the entered password is empty (zero characters long), the UCS2 password still should contain a single 2-byte UCS2 NULL character. That's how NSS handles an empty password. But when Microsoft Window's certificate export wizard handles an empty password, it computes the key from a zero-byte string, one that does not include the trailing 2-byte UCS2 NULL character. So, it computes a different key for an empty password than NSS does, and that is why NSS cannot import a PFX file created by MS's wizard with an empty password. Note that MS's wizard does include the trailing NULL when the password is not empty, so only empty passwords are handled in this exceptional manner. MS and NSS compute UCS2 passwords the same way, except for empty passwords. I verified (in the debugger) that by detecting the empty password case, and eliminating the trailing 2-byte NULL character from the UCS2 password, then NSS was able to import the PFX file generated by MS's wizard with an empty password. So, if we want to be able to import PFX files created by MS's wizard with an empty password, we must drop the trailing NULL character when computing the PBE key. But if we do that, we will no longer be able to import P12 files created with mozilla or pk12util with an empty password, because those tools create PKCS12 files with the trailing NULL character always intact. I suspect that MS's cert import wizard also cannot import P12 files created by mozilla or pk12util with empty passwords, for essentially the same reason, namely that empty passwords are encoded differently. Some possible options include: a) look at the suffix of the file name being imported or exported. If it is "pfx" and the user enters an empty password, drop the trailing NULL. Otherwise, do not drop the trailing NULL. PFX files with empty passwords are then importable by both NSS and MS, but P12 files with empty passwords are not. b) when importing with an empty password, try it with the trailing NULL, and if that fails, try it again without the trailing NULL. Unfortunately, this means doing the entire PKCS12 import operation twice. Continue to create PKCS12 files with empty passwords as we have done before. Solves the problem for import by NSS only, not for import by MS Wizard. c) Document that a non-empty PFX file password is always required when using p12/pfx files to transport keys between windows and mozilla products. Solves problem in both directions. d) Like b above, trying both ways of encoding empty passwords for import, but for export, abandon the way NSS has previously handled empty passwords, and switch to doing it the MS way. Solves exchange of PFX/p12 files between MS Wizard and new NSS, but creates problem for old NSS products that would import a p12 file with an empty password from the new NSS. PSM duplicates a large part of NSS's PKCS12 code. If it duplicates the code that converts passwords to UCS2 for PBE computation, then whatever we do to NSS (if anything) must also be done to PSM.
Reporter | ||
Comment 1•19 years ago
|
||
Note, a sample PKCS12 file that demonstrates this problem is attached to bug 265991, which is now reportedly fixed in PSM. The attachment is https://bugzilla.mozilla.org/attachment.cgi?id=174323
Priority: -- → P3
Reporter | ||
Comment 2•19 years ago
|
||
Neil, I'm giving the pk12util bugs to you.
Assignee: wtchang → neil.williams
Comment 3•19 years ago
|
||
PSM bug 265991 is not yet fixed but there is already a working patch attached to the bug report.
Reporter | ||
Updated•19 years ago
|
QA Contact: bishakhabanerjee → jason.m.reid
Assignee | ||
Comment 4•19 years ago
|
||
This is the sort of patch that looks substantial because it shifts a bunch of code right so as to retry through the import code block in the case of a null password. The code to convert the supplied password to Unicode ensures that the Unicode version has a (UCS-2) NULL at the end. So a null unipw will have length 2 which is changed to 0 for the retry.
Attachment #202457 -
Flags: review?(nelson)
Reporter | ||
Comment 5•19 years ago
|
||
Comment on attachment 202457 [details] [diff] [review] patch to retry pfx import created with zero-length passwords I think this patch is correct, and so (assuming that it has been tested with the provided test .p12 file), I give this r=nelson, but I'd like to request one change. Instead of >+ for (trypw=PR_TRUE; trypw;) { >+ trypw = PR_FALSE; /* normally we do this once */ [...] > } I suggest >+ do { >+ trypw = PR_FALSE; /* normally we do this once */ [...] >- } >+ } while (trypw);
Attachment #202457 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 6•19 years ago
|
||
Incorporated your suggestion, Nelson. This looks more complicated than the last patch because the code to get the password and read the PKCS12 file was carved out of Import() and placed in a new function. This way list and import use the same function. Also a small fix to cmd/lib/secpwd.c to initialize the pw buffer to null so that if the input is at EOF the utitilty doesn't do funny things.
Attachment #202457 -
Attachment is obsolete: true
Attachment #203860 -
Flags: review?(nelson)
Reporter | ||
Comment 7•19 years ago
|
||
Comment on attachment 203860 [details] [diff] [review] better fix for the problem Let's discuss this. I have two sets of issues: 1) I found several paths in the code (both in the patched code and the orginal) that goto loser without setting rv to SECFailure, causing the function to return SECSuccess when it should return SECFailure. 2) The elimination of the swapunicode variable. Is this code correct on little-endian systems?
Attachment #203860 -
Attachment description: better fix for the problem → better fix for the problem
Attachment #203860 -
Flags: review?(nelson) → review-
Assignee | ||
Comment 8•19 years ago
|
||
The only change here is to restore the way the program handled unicode conversion. Thanks for picking up on that. We may want to change the way this does unicode conversion someday but that would need some more study. Nelson, I believe the places you mention not setting rv are all taking advantage of the fact that it's initialized to SECFailure.
Attachment #203860 -
Attachment is obsolete: true
Attachment #209937 -
Flags: review?(nelson)
Assignee | ||
Comment 9•19 years ago
|
||
Nelson, besides the return code problem you pointed out this patch incorporates your suggestion for ...ReadFile() to return the decode context pointer on success and NULL on failure. This makes sense in this case because the function prints error messages itself rather than merely passing success/fail and a reason code. (It still sets the reason code though.)
Attachment #209937 -
Attachment is obsolete: true
Attachment #210404 -
Flags: review?(nelson)
Attachment #209937 -
Flags: review?(nelson)
Assignee | ||
Comment 10•19 years ago
|
||
These patches affect the list and import options by creating a new function that reads and verifies the PKCS12 file for both. This patch also fixes a problem in cmd/lib/secpwd.c when end-of-file is encountered while reading a password. It was returning uninitialized data. This change affects modutil as well as pk12util.
Status: NEW → ASSIGNED
Reporter | ||
Comment 11•19 years ago
|
||
Targetting to 3.12 Julien, do you think we need this fix in 3.11.1 ?
Target Milestone: --- → 3.12
Reporter | ||
Comment 12•19 years ago
|
||
Comment on attachment 210404 [details] [diff] [review] Previous patch plus fix to return proper error code (checked in) r=nelson.bolyard for the trunk.
Attachment #210404 -
Flags: review?(nelson) → review+
Assignee | ||
Comment 13•19 years ago
|
||
Checking in cmd/pk12util/pk12util.c; /cvsroot/mozilla/security/nss/cmd/pk12util/pk12util.c,v <-- pk12util.c new revision: 1.33; previous revision: 1.32 done Checking in cmd/lib/secpwd.c; /cvsroot/mozilla/security/nss/cmd/lib/secpwd.c,v <-- secpwd.c new revision: 1.14; previous revision: 1.13
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 14•18 years ago
|
||
This fix should go into 3.11.1. There needs to be a nightly QA test that verifies that this is still working. part of tools.sh. Attachment 174323 [details] is a PFX file with no password, for testing. https://bugzilla.mozilla.org/attachment.cgi?id=174323
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: 3.12 → 3.11.1
Reporter | ||
Comment 15•18 years ago
|
||
Comment on attachment 210404 [details] [diff] [review] Previous patch plus fix to return proper error code (checked in) Backported to trunk lib/secpwd.c; new revision: 1.13.28.1; previous revision: 1.13 pk12util/pk12util.c; new revision: 1.32.2.1; previous revision: 1.32
Attachment #210404 -
Attachment description: Previous patch plus fix to return proper error code → Previous patch plus fix to return proper error code (checked in)
Reporter | ||
Updated•18 years ago
|
Status: REOPENED → RESOLVED
Closed: 19 years ago → 18 years ago
Depends on: 335200
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•