550041 - PFX file with empty friendly name crashes NSS import code

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Daniel Trebbien]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.6) Gecko/20091216 Iceweasel/3.5.8 (like Firefox/3.5.8)
Build Identifier: 

I have a PFX certificate that I wish to import into Iceweasel (Firefox). When I try to import the certificate using Edit -> Preferences -> Advanced -> Encryption -> View Certificates -> Your Certificates -> Import, Iceweasel asks me for the certificate password, then immediately crashes (with no message). I read Ubuntu bug #198841 (https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/198841) and tried all of the suggestions. One of them is to try using `pk12util -i BLAH.pfx -d PATH_TO_MOZILLA_FIREFOX_PROFILE` and while that command worked for the one person, I get a segmentation fault.

I am running Debian Squeeze (testing) amd64. Iceweasel is v. 3.5.8 and libnss3-1d is v. 3.12.5-2.

I installed the Debian libnss3-1d debug package, `libnss3-1d-dbg`, as well as downloaded the source package that libnss3-1d was built from (http://ftp.de.debian.org/debian/pool/main/n/nss/nss_3.12.5.orig.tar.gz) so that I could step through with `gdb`. What I found was that `pk12util` is segfault'ing when attempting to read the "nickname":

----------------------------------------------------------------------------
~$ gdb `which pk12util`
GNU gdb (GDB) 7.0.1-debian
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /usr/bin/pk12util...Reading symbols from /usr/lib/debug/usr/bin/pk12util...done.
(no debugging symbols found)...done.
(gdb) break p12local.c:961
No source file named p12local.c.
Make breakpoint pending on future shared library load? (y or [n]) y

Breakpoint 1 (p12local.c:961) pending.
(gdb) set args -i daniel.trebbien.pfx -d ~/.mozilla/firefox/vxivnlxy.default/
(gdb) run
The program being debugged has been started already.
Start it from the beginning? (y or n) y

Starting program: /usr/bin/pk12util -i daniel.trebbien.pfx -d ~/.mozilla/firefox/vxivnlxy.default/
[Thread debugging using libthread_db enabled]
Enter Password or Pin for "NSS Certificate DB":
Enter password for PKCS12 file: 

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff799de6e in sec_pkcs12_convert_item_to_unicode (arena=0x0, 
    dest=0x66ce30, src=0x6735c8, zeroTerm=0, asciiConvert=0, toUnicode=0)
    at p12local.c:961
961	p12local.c: No such file or directory.
	in p12local.c
(gdb) bt
#0  0x00007ffff799de6e in sec_pkcs12_convert_item_to_unicode (arena=0x0, 
    dest=0x66ce30, src=0x6735c8, zeroTerm=0, asciiConvert=0, toUnicode=0)
    at p12local.c:961
#1  0x00007ffff79a1891 in sec_pkcs12_get_nickname (bag=0x66d348) at p12d.c:1692
#2  0x00007ffff79a197c in sec_pkcs12_get_nickname_for_cert (cert=0x66d348, 
    key=0x678c20, wincx=0x0) at p12d.c:1872
#3  0x00007ffff79a2b09 in sec_pkcs12_validate_cert_nickname (cert=0x66d348, 
    key=0x678c20, nicknameCb=<value optimized out>, 
    wincx=<value optimized out>) at p12d.c:2176
#4  sec_pkcs12_validate_cert (cert=0x66d348, key=0x678c20, 
    nicknameCb=<value optimized out>, wincx=<value optimized out>)
    at p12d.c:2305
#5  0x00007ffff79a2f6a in sec_pkcs12_validate_bags (p12dcx=0x66d010, 
    nicknameCb=<value optimized out>) at p12d.c:2711
#6  SEC_PKCS12DecoderValidateBags (p12dcx=0x66d010, 
    nicknameCb=<value optimized out>) at p12d.c:2760
#7  0x0000000000405dce in P12U_ImportPKCS12Object (
    in_file=0x61a4d0 "daniel.trebbien.pfx", slot=<value optimized out>, 
    slotPw=0x7fffffffe310, p12FilePw=<value optimized out>) at pk12util.c:520
#8  0x00000000004062f4 in main (argc=<value optimized out>, 
    argv=<value optimized out>) at pk12util.c:1102
(gdb) quit
~$ 
~$ 
~$ 
~$ 
~$ gdb `which pk12util`
GNU gdb (GDB) 7.0.1-debian
Copyright (C) 2009 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /usr/bin/pk12util...Reading symbols from /usr/lib/debug/usr/bin/pk12util...done.
(no debugging symbols found)...done.
(gdb) set args -i daniel.trebbien.pfx -d ~/.mozilla/firefox/vxivnlxy.default/
(gdb) break p12local.c:961
No source file named p12local.c.
Make breakpoint pending on future shared library load? (y or [n]) y

Breakpoint 1 (p12local.c:961) pending.
(gdb) run
Starting program: /usr/bin/pk12util -i daniel.trebbien.pfx -d ~/.mozilla/firefox/vxivnlxy.default/
[Thread debugging using libthread_db enabled]
Enter Password or Pin for "NSS Certificate DB":
Enter password for PKCS12 file: 

Breakpoint 1, sec_pkcs12_convert_item_to_unicode (arena=0x0, dest=0x66ce30, 
    src=0x6735c8, zeroTerm=0, asciiConvert=0, toUnicode=0) at p12local.c:961
961	p12local.c: No such file or directory.
	in p12local.c
----------------------------------------------------------------------------


Note: For reference, here is the relevant part of `p12local.c` from the source package:
----------------------------------------------------------------------------
/* this function converts a password to unicode and encures that the 
 * required double 0 byte be placed at the end of the string
 */
PRBool
sec_pkcs12_convert_item_to_unicode(PRArenaPool *arena, SECItem *dest,
				   SECItem *src, PRBool zeroTerm,
				   PRBool asciiConvert, PRBool toUnicode)
{
    PRBool success = PR_FALSE;
    if(!src || !dest) {
	PORT_SetError(SEC_ERROR_INVALID_ARGS);
	return PR_FALSE;
    }

    dest->len = src->len * 3 + 2;
    if(arena) {
	dest->data = (unsigned char*)PORT_ArenaZAlloc(arena, dest->len);
    } else {
	dest->data = (unsigned char*)PORT_ZAlloc(dest->len);
    }

    if(!dest->data) {
	dest->len = 0;
	return PR_FALSE;
    }

    if(!asciiConvert) {
	success = PORT_UCS2_UTF8Conversion(toUnicode, src->data, src->len, dest->data,
					   dest->len, &dest->len);
    } else {
#ifndef IS_LITTLE_ENDIAN
	PRBool swapUnicode = PR_FALSE;
#else
	PRBool swapUnicode = PR_TRUE;
#endif
	success = PORT_UCS2_ASCIIConversion(toUnicode, src->data, src->len, dest->data,
					    dest->len, &dest->len, swapUnicode);
    }

    if(!success) {
	if(!arena) {
	    PORT_Free(dest->data);
	    dest->data = NULL;
	    dest->len = 0;
	}
	return PR_FALSE;
    }

    if((dest->data[dest->len-1] || dest->data[dest->len-2]) && zeroTerm) {
----------------------------------------------------------------------------

The last line is #961. Continuing with my `gdb` session...

----------------------------------------------------------------------------
(gdb) print dest->len
$1 = 0
----------------------------------------------------------------------------

And there's the problem. `dest->len` is 0, so `dest->data[dest->len-1]` is a bad access. `dest->len` is set to at least 2 early on, which would be fine, but it seems that the call to `PORT_UCS2_ASCIIConversion` sets `dest->len` to 0. Resetting `dest->len` to 2 each time it is less than 2 allows the key to be imported successfully:

----------------------------------------------------------------------------
(gdb) print dest->len=2
$2 = 2
(gdb) condition 1 dest->len < 2
(gdb) c
Continuing.

Breakpoint 1, sec_pkcs12_convert_item_to_unicode (arena=0x0, dest=0x66b220, 
    src=0x6735c8, zeroTerm=0, asciiConvert=0, toUnicode=0) at p12local.c:961
961	in p12local.c
(gdb) print dest->len=2
$3 = 2
(gdb) c
Continuing.

Breakpoint 1, sec_pkcs12_convert_item_to_unicode (arena=0x0, dest=0x66b220, 
    src=0x6735c8, zeroTerm=0, asciiConvert=0, toUnicode=0) at p12local.c:961
961	in p12local.c
(gdb) print dest->len=2
$4 = 2
(gdb) c
Continuing.
pk12util: PKCS12 IMPORT SUCCESSFUL

Program exited normally.
(gdb) quit
----------------------------------------------------------------------------

The PFX certificate that I have has an empty "nickname" or "friendly name".


Reproducible: Always

Steps to Reproduce:
(I haven't tested this because I don't know how to generate a PKCS#12 PFX file, but I believe that this will work.)

1. Generate a PFX certificate (e.g. `test.pfx`) with an empty "nickname" or "friendly name"
2. Locate your Mozilla Firefox profile directory (e.g.: `~/.mozilla/firefox/vxivnlxy.default/`)
3. Open a terminal, `cd` to the directory that contains the PFX file.
4. Run `pk12util -i test.pfx -d ~/.mozilla/firefox/vxivnlxy.default/`
5. Enter the password.

You should get a segmentation fault.
Actual Results:  
Segmentation Fault

Expected Results:  
I expected the certificate to be imported successfully.

Comment 1

[Daniel Trebbien]

I am positive that the empty "nickname" or "friendly name" is causing problems. I just received a newly-generated test certificate that has a non-empty "nickname" (the `-name` flag to `openssl` was followed by a non-empty value), and I was able to import this into Iceweasel successfully.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

We can fix this so it doesn't crash, but without a valid nickname, it's 
still not going to import satisfactorily.  So, the only solution that will
make it import is to give it a valid friendly name in the PFX file. 

library function sec_pkcs12_get_nickname checks the return value from 
sec_pkcs12_get_attribute_value for NULL.  This detects a completely missing
friendly name, but not an empty friendly name.  The test should be expanded
to also detect an empty name.

Comment 3

[Shailen]

Attachment: Patch V 1

D. Trebbien

Could you please provide me PFX file (test certificate) so I can test the patch ?

Thanks,
Shailendra

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Shailendra,
With your patch, what happens if src->data is NULL?

It would be nice to have a PFX file with an empty nickname to test with.
Let's see if the reporter can supply one for us.  If not, we can certainly
hack NSS to make one.  (We wouldn't commit the hack, just use it once.)

Comment 5

[Daniel Trebbien]

Attachment: A PKCS#12 certificate with an empty friendly name

The certificate password is `test` (without the backticks).

I generated and tested it with the following:
------------------------------------------------------------------------
~/test$ openssl req -x509 -newkey rsa:1024 -keyout hostkey.pem -nodes -out hostcert.pem
Generating a 1024 bit RSA private key
......++++++
.................++++++
writing new private key to 'hostkey.pem'
-----
You are about to be asked to enter information that will be incorporated
into your certificate request.
What you are about to enter is what is called a Distinguished Name or a DN.
There are quite a few fields but you can leave some blank
For some fields there will be a default value,
If you enter '.', the field will be left blank.
-----
Country Name (2 letter code) [AU]:US
State or Province Name (full name) [Some-State]:Arizona
Locality Name (eg, city) []:Tucson
Organization Name (eg, company) [Internet Widgits Pty Ltd]:Test      
Organizational Unit Name (eg, section) []:QA 
Common Name (eg, YOUR name) []:Alice
Email Address []:[e-mail]
~/test$ ls
hostcert.pem  hostkey.pem
~/test$ openssl pkcs12 -export -in hostcert.pem -inkey hostkey.pem -out alice.pfx -name ""
Enter Export Password:
Verifying - Enter Export Password:
~/test$ ls
alice.pfx  hostcert.pem  hostkey.pem
~/test$ mkdir .pki
~/test$ pk12util -i alice.pfx -d .pki
Enter a password which will be used to encrypt your keys.
The password should be at least 8 characters long,
and should contain at least one non-alphabetic character.

Enter new password: 
Re-enter password: 
Enter password for PKCS12 file: 
Segmentation fault
------------------------------------------------------------------------

You can use any password for the key database password, but the input that is required for the `Enter password for PKCS12 file:` line is `test`.

Comment 6

[Daniel Trebbien]

Attachment: A PKCS#12 certificate with friendly name "Bob"

The certificate password is `test` (without the backticks). It was generated exactly the same as `alice.pfx`, except that the shell line which created `bob.pfx` was:
---------------------------------------------------------------------
~/test$ openssl pkcs12 -export -in hostcert.pem -inkey hostkey.pem -out bob.pfx -name "Bob"
---------------------------------------------------------------------

The result of importing `bob.pfx` was:
---------------------------------------------------------------------
~/test$ pk12util -i bob.pfx -d .pki
Enter a password which will be used to encrypt your keys.
The password should be at least 8 characters long,
and should contain at least one non-alphabetic character.

Enter new password: 
Re-enter password: 
Enter password for PKCS12 file: 
pk12util: PKCS12 IMPORT SUCCESSFUL
---------------------------------------------------------------------

Comment 7

[Shailen]

Hi Nelson,

  You are correct. I need to include the check for src->data being NULL.  I tested this fix with the certificate provided by Daniel and works just fine. Hence attaching the new patch which includes the check for 
src OR src->data being NULL

Also I had to exclude the check for src->data[0] == '\0' as this was resulting in unexpected behavior.

Regards,
Shailendra

Comment 8

[Shailen]

Attachment: Patch V 2

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

> Also I had to exclude the check for src->data[0] == '\0' as this was 
> resulting in unexpected behavior.

Please elaborate.  Isn't that test the whole point of this bug?

Comment 10

[Shailen]

Hi Nelson,

   If I include the condition src->data[0] == '\0', then I get the following outcomes.

1) ./pk12util -i ~/alice.pfx -d /home/firefox/.mozilla/firefox/pndcnxaw.default

outcome :

pk12util: no nickname for cert in PKCS12 file.
pk12util: using nickname: Alice - Test
pk12util: PKCS12 decode import bags failed: Certificate nickname already in use.


2) pk12util -i ~/bob.pfx -d /home/firefox/.mozilla/firefox/pndcnxaw.default

outcome :

pk12util: no nickname for cert in PKCS12 file.
pk12util: using nickname: Bob - Test
pk12util: PKCS12 decode import bags failed: Certificate nickname already in use.

After running te above 2 commands(with src->data[0] == '\0'), I checked the nss database and the certificates were not imported.

If I exclude the condition src->data[0] == '\0', then I get the following outcomes which seem to correct one.

1) pk12util -i ~/alice.pfx -d /home/firefox/.mozilla/firefox/pndcnxaw.default

outcome :

pk12util: no nickname for cert in PKCS12 file.
pk12util: using nickname: Alice - Test
pk12util: PKCS12 IMPORT SUCCESSFUL

2) pk12util -i ~/bob.pfx -d /home/firefox/.mozilla/firefox/pndcnxaw.default

pk12util: PKCS12 IMPORT SUCCESSFUL

After running the above 2 commands (without src->data[0] == '\0'), I checked the nss database and the certificates were found this time with nicknames "Alice - Test" and "Bob".


Also I debugged the code to see what condition is causing failure reported by Daniel. It happens to be "src being not NULL but src->data being NULL".

Thanks and regards,
Shailendra

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

The problem you encountered is because the characters in the data array 
returned by sec_pkcs12_get_attribute_value are 16-bit Unicode characters, 
in big-endian format, not 8-bit characters.  If you only check the first
byte, you may find it is zero, and yet the character is not zero, because
the second byte of the character is not zero.  

Let me suggest this test:

if (!src || !src->data || src->len < 2 || (!src->data[0] && !src->data[1]))

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 430911 [details] [diff] [review]
Patch V 2

see comment 11.

Comment 13

[Shailen]

Attachment: Patch Version 3

Nelson :

   As usual you are correct. I have tested the changes you suggested and it works fine. I am attaching the new patch.

Thanks,
Shailendra

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 431078 [details] [diff] [review]
Patch Version 3

r=nelson

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Checking in p12d.c; new revision: 1.45; previous revision: 1.44

Thanks, Shailendra