265991 - Can't import a pfx file encrypted with an empty password

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[JD Gamache]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20041001 Firefox/0.10.1

When importing a certificate (.PFX file)

If the certificate has no encrypt password It fails adding the certificate 
(invalid password).


Reproducible: Always
Steps to Reproduce:
1. Open certificate manager
2. In the 'Your certificate' tab, click on 'import'
3. Choose a PFX file with no encryption password.

Actual Results:  
Error: invalid password

Expected Results:  
Add certificate

Comment 1

[Josh Birnbaum]

->PSM

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

How does one make a PFX file with no password?

Comment 3

[Frank Wein [:mcsmurf]]

*** Bug 272813 has been marked as a duplicate of this bug. ***

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

The PKCS12 specification permits PKCS12 files to be created that are
- encrypted with as password, or
- encrypted with a public key, or
- not encrypted at all.

When this bug was filed, describing a "certificate with no password", 
it was not immediately clear whether this meant an unencrypted file, 
or a file encrypted with an empty password.  I knew the submittors of this
bug (and its duplicates) would not want to send us their PFX files for us 
to test them, so I asked them how they created their PFX files with no 
passwords, but got no answer.

Now I believe I have figured it out.

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

The password from which the key is computed includes a trailing UCS2 NULL
character, and is one 2-byte UCS2 character longer than the number of 
characters in the user's password.  So, when the entered password is empty
(zero characters long), the 2-byte UCS2 password still should contain the
single 2-byte UCS2 NULL character.  That's how NSS handles an empty password.

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

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.  If it is "pfx" and
the user enters an empty password, drop the trailing NULL.  Otherwise, do
not drop the trailing NULL.

b) when importing with an empty password, try it with the trailing NULL, and
if that fails, try it again without the trailing NULL.

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.  
Personally, I like that solution the best.

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 5

[Wan-Teh Chang]

I like solution b the best.  A variation of it is
to only do that on files with the "pfx" suffix.

Do PKCS12/PFX files contain information on what
programs generated them?

Comment 6

[Julien Pierre]

Wan-Teh, from a quick glance of the spec, I don't think there is any standard
attribute about who created the PKCS#12 file.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

I have filed bug 277334 against NSS to report this issue for pk12util.
I believe both products have the same issue, and it will need to be 
addressed separately in both, unless we choose option c, which is 
essentially WONTFIX.

Comment 8

[Kai Engert [:KaiE:]]

I also like b) best, should be simple to do.

Should this bug be fixed at the application level (PSM, doing multiple
decryption calls) or could it be simply handled inside NSS (detecting empty
password, trying again automatically)?

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Kaie,  IIRC, NSS's PKCS12 API requires the appliction to supply the 
password in the first call, the one that establishes the PKCS12 
context.  If the operation fails, the application must repeat the
entire operation from the beginning with a different password.
(The application need not read the file into memory again, if it 
has done so, but it must start a new PKCS12 content, IIRC).

Comment 10

[Kai Engert [:KaiE:]]

Could somebody please me such a certificate, created on a Windows machine,
having such an empty password, for testing?

Comment 11

[Kai Engert [:KaiE:]]

Attachment: patch v1 (ignores whitespace)

If I understood everything correctly, this patch should make it work.

I will test it, if you supply me with an appropriate p12 file.

Comment 12

[Kai Engert [:KaiE:]]

Attachment: patch v1 (including whitespace changes)

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: pkcs12 file with empty password

This file should do.  
Contains a test root CA cert and an email cert issued by that test CA.	
First I created the certs/keys with NSS's QA test script, then exported
into a p12 file with pk12util.	Then I imported the p12 file into windows 
and then re-exported the certs from there into a pfx file with an empty
password.  I verified that I could not import the pfx file with mozilla.

Comment 14

[Kai Engert [:KaiE:]]

Thanks Nelson, and: Success :-)

With standard Mozilla, I can not import it.
With the patch attached to this bug, it gets imported successfully.

(Cert issued to Alice, BOGUS NSS, issued by NSS Test CA, BOGUS NSS.)

Comment 15

[Kai Engert [:KaiE:]]

Attachment: patch v1 for moz 1.7 branch

Comment 16

[Wan-Teh Chang]

Comment on attachment 174214 [details] [diff] [review]
patch v1 (ignores whitespace)

Kai, thanks a lot for writing this patch.

Two suggestions.

1. In manager/ssl/src/nsPKCS12Blob.cpp, we have

>-  int wantRetry;
>+  RetryReason wantRetry;
>   
>   do {
>-    rv = ImportFromFileHelper(file, wantRetry);
>+    rv = ImportFromFileHelper(file, im_standard_prompt, wantRetry);
>+    
>+    if (NS_SUCCEEDED(rv) && wantRetry == rr_auto_retry_empty_password_flavors)
>+    {
>+      rv = ImportFromFileHelper(file, im_try_zero_byte_password, wantRetry);
>+    }
>   }
>   while (NS_SUCCEEDED(rv) && wantRetry);

Now that wantRetry is no longer a Boolean, in
the last line, we should replace wantRetry by
wantRetry != rr_do_not_retry.

2. In manager/ssl/src/nsPKCS12Blob.h, document
rr_auto_retry_empty_password_flavors (possibly
in conjunction with im_try_zero_byte_password).
Its meaning is not obvious to someone who just
reads the code.

Comment 17

[Ben Stoner]

I to am having the same problem now i am not a compatent programmer so i have 
no idea how to apply the patch.

If someone could please explain i would be greatful.

I am needing this as i use the certificate in question to access my works 
intranet. I am currently forced to use IE untill i am able to fix this i would 
prefer to be able to use firefox for the security. 

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Ben, export your private key from IE with a non-empty password.

Comment 19

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Thanks Wan-Teh, good catch.
Fixed and comment added.

Comment 20

[Derek Meek]

*** Bug 309480 has been marked as a duplicate of this bug. ***

Comment 21

[Derek Meek]

I definantly concur that option b is the best solution as will create
transparent compatability with internet explorer's "feature"

My coworker who was suffering from this bug, upon my explaining what was
happening, promptly quipped "I wonder if microsoft does this on purpose to make
it look like their competition is having problems"

Comment 22

[Wan-Teh Chang]

Comment on attachment 184247 [details] [diff] [review]
Patch v2

r=wtc.	Sorry about the delay in reviewing this patch.
It fell through the cracks.

On second review, I think I would do the automatic retry of
zero-byte password entirely inside ImportFromFileHelper, so
that we don't need to define the RetryReason and ImportMode
enum types or change anything outside ImportFromFileHelper.
But your patch is correct.

I do want to suggest some comment changes.

>+      if (unicodePw.len == sizeof(PRUnichar))
>+      {
>+        // no password chars available, 
>+        // unicodeToItem allocated space for trailing zeros only

I think it's better to say "the trailing zero" (singular)
because there is only one trailing zero PRUnichar.

>+  // RetryReason and ImportMode are used when importing a PKCS12 file.
>+  // There are two reasons that cause us to retry:
>+  // - When the password entered by the user is incorrect.
>+  //   The user will be prompted to try again.
>+  // - When the user entered a zero length password.
>+  //   There are some applications that use a zero byte, while others use the empty string.
>+  //   We try both variations, empty string or zero byte,
>+  //   without giving a user prompt when trying the different empty password flavors.

First, "a zero byte" is very confusing.  To me, it means "a byte with
the value zero".

Second, I think "zero byte" should be "zero bytes" (meaning "the number
of bytes is zero").  I'm not a native English speaker, so someone please
correct me if I'm wrong.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #22)


> >+  // - When the user entered a zero length password.
> >+  //   There are some applications that use a zero byte, while others use
the empty string.
> >+  //   We try both variations, empty string or zero byte,
> >+  //   without giving a user prompt when trying the different empty password
flavors.
> 
> First, "a zero byte" is very confusing.  To me, it means "a byte with
> the value zero".

I believe the intent of the expression "empty string" is to convey that it
has zero length, that is zero characters in the string.  I believe the
intent of "zero byte" is to convey "a character whose value is zero".  
However, in this case, IINM, the character is not a byte (this is UCS2, right?)
So, I think the expression "zero byte" should be changed to "zero character"
or perhaps "NUL character".  I think that should be fixed throughout this
comment.

Comment 24

[Wan-Teh Chang]

Nelson: what Kai meant by "zero byte" is a SECItem whose
len field is 0.  That's why I find it confusing and suggest
"zero bytes" instead.  What's the best term to describe
that thing?

An "empty string" is a SECItem that contains a single
terminating NULL UTF16 character (PRUnichar).

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #24)
> Nelson: what Kai meant by "zero byte" is a SECItem whose
> len field is 0.  That's why I find it confusing and suggest
> "zero bytes" instead.  What's the best term to describe
> that thing?

"Zero length" seems the most concise way to express that.

Comment 26

[Kai Engert [:KaiE:]]

Attachment: Patch v3 - comment and identifier name changes only

This new patch no longer uses the term "byte".

I renamed "zero_byte_password" to "zero_length_secitem".

I changed "for trailing zeros only" 
       to "for the trailing zero character only".

I changed

    //   There are some applications that use a zero byte, while others use the empty string.
    //   We try both variations, empty string or zero byte,

to

  //   There are some applications that use a zero length SECItem, 
  //   while others use the empty string (a SECItem that contains a single
  //                                      terminating NULL UTF16 character).
  //   We try both variations, zero length item and empty string,

Comment 27

[Wan-Teh Chang]

Comment on attachment 202391 [details] [diff] [review]
Patch v3 - comment and identifier name changes only

r=wtc.

If you do the retry of the alternate flavor of empty
password inside ImportFromFileHelper, you can avoid
the need for rr_auto_retry_empty_password_flavors and
the ImportMode enum type.

>+  //   There are some applications that use a zero length SECItem, 
>+  //   while others use the empty string (a SECItem that contains a single
>+  //                                      terminating NULL UTF16 character).

It would be nice to reword this to indicate which
empty password flavor is the proper one.  For example,
you can say (assuming NSS's flavor is the proper one):

  An empty password should be represented as an empty
  string (a SECItem that contains a single terminating
  NULL UTF16 character), but some applications use a
  zero length SECItem.

Comment 28

[Kai Engert [:KaiE:]]

> If you do the retry of the alternate flavor of empty
> password inside ImportFromFileHelper, you can avoid
> the need for rr_auto_retry_empty_password_flavors and
> the ImportMode enum type.

I see no harm in introducing the new enum types.
We already have a mechanism in place, that wraps an "retry layer" around our real NSS code.
If I follow your proposal, we will have to start from scratch and the resulting patch will look completely different from the current one. The point is, retrying is not just a simple call to NSS, but a series of calls, which is implemented inside ImportFromFileHelper.

I would like to avoid repeating the NSS calls for the zero secitem retry.
Therefore I want a loop around the code inside ImportFromFileHelper.

However, if I changed ImportFromFileHelper, we'd effectively have two levels of retries nested inside each other, I think that's confusing.

And I'd have to wrap around a loop around the code that even use gotos. In order to avoid mixing loops with gotos, I'd either want to rewrite the whole method to no longer uses gotos, or introduce an additional helper function that encapsulates the retry.

What you suggest is doable. But it will result in a completely different patch, a much bigger one, that changes more of the current logic.
My motivation for the current patch was to minimize the amount of changes to the code, and to have a single point where we trigger a retry.

Please consider my arguments. However, if you still think I should implement the patch using the alternative approach you have suggested, please let me know.

However, you have already granted me r= on the patch, so I conclude your statement was a suggestion, but not a requirement.

Comment 29

[Kai Engert [:KaiE:]]

Attachment: Patch v4

Wan-Teh, this new patch contains the new comment you have proposed.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 202488 [details] [diff] [review]
Patch v4

sr=dveditz

Comment 31

[Kai Engert [:KaiE:]]

fixed on trunk

Comment 32

[:Gavin Sharp [email: [e-mail]]]

*** Bug 285926 has been marked as a duplicate of this bug. ***