277334 - pk12util can't import a pfx file encrypted with an empty password

Status: RESOLVED FIXED

(NSS :: Tools, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Neil, I'm giving the pk12util bugs to you. 

Comment 3

[Wan-Teh Chang]

PSM bug 265991 is not yet fixed but there is already
a working patch attached to the bug report.

Comment 4

[Neil Williams]

Attachment: patch to retry pfx import created with zero-length passwords

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.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

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);

Comment 6

[Neil Williams]

Attachment: better fix for the problem

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.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

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?

Comment 8

[Neil Williams]

Attachment: previous patch with fixed glitch in unicode conversion

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.

Comment 9

[Neil Williams]

Attachment: Previous patch plus fix to return proper error code (checked in)

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.)

Comment 10

[Neil Williams]

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.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Targetting to 3.12
Julien, do you think we need this fix in 3.11.1 ?

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 210404 [details] [diff] [review]
Previous patch plus fix to return proper error code (checked in)

r=nelson.bolyard for the trunk.

Comment 13

[Neil Williams]

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

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

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