335481 - crash importing a PKCS#12 file [@ PK11_UnwrapPrivKey]

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Victor Sudakov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Thunderbird 1.5.0.2 (Windows/20060308)

Importing a .pfx file obtained from the Communigate Pro Web User interface crashes ThunderBird.

Reproducible: Always

Steps to Reproduce:
1. Download the sample PFX file from http://vas.tomsk.ru/test.pfx or use the attached one.
2. Try to import it into "Personal certificates", the password is "test"
3. See ThunderBird crash


Actual Results:  
Thunderbird crashes and is closed by the OS.

Expected Results:  
The certificate and key should have been added to the storage.

Files created with "openssl pkcs12" are imported just fine.

Comment 1

[Victor Sudakov]

Attachment: A testcase certificate&key file that can crash ThunderBird when imported

Comment 2

[Daniel Veditz [:dveditz]]

This also crashes Firefox in the shared NSS library. Talkback TB18046833 says PK11_UnwrapPrivKey but that's all it's got. Bogus stack or talkback glitch?

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

See if this can be reproduced with pk12util.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I tried to reproduce this using pk12util.  Got a crash with this stack:

PK11_UnwrapPrivKey(... SECItemStr * label 0x00000000, ... ) line 954 + 15 bytes

    PK11_SETATTRS(attrs, CKA_LABEL, label->data, label->len); attrs++;

PK11_ImportEncryptedPrivateKeyInfo( nickname 0x00000000 ) line 1254 + 60 bytes
sec_pkcs12_add_key()             line 2464 + 50 bytes

    nickName = sec_pkcs12_get_nickname(key);
-> No NULL test here.
    switch(SECOID_FindOIDTag(&key->safeBagType))

sec_pkcs12_install_bags()        line 2845 + 31 bytes
SEC_PKCS12DecoderImportBags()    line 2923 + 22 bytes
P12U_ImportPKCS12Object()        line 507 + 9 bytes
main(int 6, char * * 0x004d1990) line 953 + 21 bytes


In a nutshell, this private key doesn't have a PKCS#9 friendly name
(a.k.a "nickname").  

See also:  bug 321584  and bug 335019

The fix to this bug should not only avoid the crash, but also succesfully
import the cert and key, even if that means making up a nickname.

Comment 5

[Julien Pierre]

Is this bug a regression ? In oter words, has this test case ever worked with any version of NSS ?

Comment 6

[Neil Williams]

This failure occurs on NSS 3.10 SunOS x86. Import only fails. The list option of pk12util works just fine.

Comment 7

[Neil Williams]

This also fails under NSS3.9.5.

Comment 8

[Julien Pierre]

Not a regression. Changing target to 33.11.3 .

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1

I got hit by this bug again twice this week, and decided to fix it.
This patch eliminates the crash, but doesn't make the cert import work.
I have developed a much later patch to p12d.c that solves the underlying
problem of not recovering from missing nicknames.  I will attach that to
a different bug.

Comment 10

[Adam Guthrie]

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

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 238751 [details] [diff] [review]
patch v1

best 2 out of 3 ?  :)

Comment 12

[Julien Pierre]

Nelson,

attachment 238751 [details] [diff] [review] looks like it will fix the crash when label is NULL. But the function will also always fail in this case. Do we ever want to support this case or is this precluded in the PKCS#12 specification ?

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 238751 [details] [diff] [review]
patch v1

I'm going to write antoher patch that takes a somewhat different approach, making labels mandatory only when unwrapping a key that will be a a "perm" (token) object.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2, require label for perm key

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 240510 [details] [diff] [review]
patch v2, require label for perm key

As I think about this patch some more, I'm not sure it's right, and so I'm asking Bob Relyea to also review it.  
I'm not sure it's ever correct to require a non-NULL CKA_LABEL on a key, not even for a token object.  
Maybe the correct fix is just the second "if" added in this patch.  
Bob?

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 240510 [details] [diff] [review]
patch v2, require label for perm key

This patch was bad.  

>-    PK11_SETATTRS(attrs, CKA_LABEL, label->data, label->len); attrs++;
>+    if (label && label->data) 
>+	PK11_SETATTRS(attrs, CKA_LABEL, label->data, label->len); attrs++;

There needs to be braces around that last line, because that line
is not a single statement.  In fact, it's not 2 statements.  It's 4!

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v3

The previous patches were bogus.  
There's no need to require a CKA_LABEL for perm certs.  
We just need to avoid copying NULL label pointers into the PKCS#11 
attribute array.  

This patch eliminates the crash, AND causes the p12 file attached to 
this bug to be properly imported.

We really should harden softoken against NULL attribute pointers!

Comment 18

[Robert Relyea]

Comment on attachment 240802 [details] [diff] [review]
Patch v3

r+, This is definately the correct fix.

Re: Softoken hardening. I'm of two minds about that. On the one hand it would be good for softoken itself to be tolerant of this kind of abuse, on the other, PKCS #11 does not make any guarrentees about that happens when a NULL pointer is passed in a template during a C_Create or Set attribute call, so the wrapper needs to be robust against passing null pointers in. I'd say we fix softoken, but only after we have another test module which is less accepting to catch these things in pk11wrap.

bob

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Adding requested reviewers to CC list

Comment 20

[Julien Pierre]

Bob,

I would expect CKR_ARGUMENTS_BAD or CKR_ATTRIBUTE_VALUE_INVALID to be returned by PKCS#11 modules for NULL pointer arguments to C_CreateObject or C_SetXXX. We didn't test what actually happens with softoken here - the crash was in the wrapper dereferencing a SECItem .

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

> Fix crash when importing (unwrapping) private key with no label.
> Bug 335481. r=julien,rrelyea

Checking in pk11obj.c; new revision: 1.11.2.3; previous revision: 1.11.2.2
Checking in pk11obj.c; new revision: 1.15; previous revision: 1.14