Closed
Bug 265991
Opened 20 years ago
Closed 19 years ago
Can't import a pfx file encrypted with an empty password
Categories
(Core Graveyard :: Security: UI, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jdgamache, Assigned: KaiE)
References
Details
(Keywords: fixed1.8.1)
Attachments
(2 files, 5 obsolete files)
2.49 KB,
application/octet-stream
|
Details | |
4.58 KB,
patch
|
KaiE
:
review+
dveditz
:
superreview+
KaiE
:
approval-branch-1.8.1+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
->PSM
Assignee: firefox → kaie
Component: Preferences → Client Library
Product: Firefox → PSM
QA Contact: mconnor
Comment 2•20 years ago
|
||
How does one make a PFX file with no password?
Comment 3•20 years ago
|
||
*** Bug 272813 has been marked as a duplicate of this bug. ***
Comment 4•20 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P2
Summary: Can't add a certificate with no password → Can't import a pfx file encrypted with an empty password
Version: unspecified → 2.1
Comment 5•20 years ago
|
||
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•20 years ago
|
||
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•20 years ago
|
||
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.
Assignee | ||
Comment 8•20 years ago
|
||
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)?
OS: Windows XP → All
Hardware: PC → All
Comment 9•20 years ago
|
||
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).
Assignee | ||
Comment 10•20 years ago
|
||
Could somebody please me such a certificate, created on a Windows machine, having such an empty password, for testing?
Assignee | ||
Comment 11•20 years ago
|
||
If I understood everything correctly, this patch should make it work. I will test it, if you supply me with an appropriate p12 file.
Assignee | ||
Updated•20 years ago
|
Attachment #174214 -
Attachment description: patch v1 → patch v1 (ignores whitespace)
Assignee | ||
Comment 12•20 years ago
|
||
Comment 13•20 years ago
|
||
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.
Assignee | ||
Comment 14•20 years ago
|
||
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.)
Status: NEW → ASSIGNED
Assignee | ||
Comment 15•20 years ago
|
||
Comment 16•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
||
Ben, export your private key from IE with a non-empty password.
Assignee | ||
Comment 19•19 years ago
|
||
Thanks Wan-Teh, good catch. Fixed and comment added.
Attachment #174214 -
Attachment is obsolete: true
Attachment #174220 -
Attachment is obsolete: true
Attachment #174328 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #184247 -
Flags: review?(wtchang)
Comment 20•19 years ago
|
||
*** Bug 309480 has been marked as a duplicate of this bug. ***
Comment 21•19 years ago
|
||
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•19 years ago
|
||
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.
Attachment #184247 -
Flags: review?(wtchang) → review+
Comment 23•19 years ago
|
||
(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•19 years ago
|
||
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•19 years ago
|
||
(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.
Assignee | ||
Comment 26•19 years ago
|
||
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,
Attachment #184247 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #202391 -
Flags: review?(wtchang)
Comment 27•19 years ago
|
||
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.
Attachment #202391 -
Flags: review?(wtchang) → review+
Assignee | ||
Comment 28•19 years ago
|
||
> 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.
Assignee | ||
Comment 29•19 years ago
|
||
Wan-Teh, this new patch contains the new comment you have proposed.
Attachment #202391 -
Attachment is obsolete: true
Attachment #202488 -
Flags: superreview?(dveditz)
Attachment #202488 -
Flags: review+
Comment 30•19 years ago
|
||
Comment on attachment 202488 [details] [diff] [review] Patch v4 sr=dveditz
Attachment #202488 -
Flags: superreview?(dveditz) → superreview+
Assignee | ||
Comment 31•19 years ago
|
||
fixed on trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 32•19 years ago
|
||
*** Bug 285926 has been marked as a duplicate of this bug. ***
Assignee | ||
Updated•19 years ago
|
Attachment #202488 -
Flags: branch-1.8.1+
Assignee | ||
Updated•19 years ago
|
Keywords: fixed1.8.1
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•