231698 - crash after typing master password

Status: VERIFIED FIXED

(NSS :: Libraries, defect, P1)

Description

[Andrew Schultz]

this crash occurs with Mozilla linux trunk build 2004012008 and also 1.6 release

after upgrading from 1.5 to 1.6, I attempted to check my mail.  Mozilla asked me
for my master password, but after I typed that in, Mozilla crashed.  If I go
back to 1.5, Mozilla still works ok.

I'll attach a stacktrace and some debugging info.  I can collect more as needed.

Comment 1

[Andrew Schultz]

Attachment: stacktrace

this is Mozilla1.6 source.  the line numbers on pkcs11c.c are a bit off:

0xb5c20562 in pk11_InitGeneric (session=0x8bfe848, contextPtr=0x0,
ctype=PK11_DECRYPT, keyPtr=0xbfffadb8, 
    hKey=3087007744, keyTypePtr=0xbfffadbc, pubKeyType=0, operation=261) at
pkcs11c.c:369
369		if (att->attrib.ulValueLen < size) {
(gdb) p att
$1 = (PK11Attribute *) 0x0
(gdb) p key
$2 = (PK11Object *) 0x8b434b8
(gdb) p *key
$3 = {next = 0x0, prev = 0x0, objclass = 4, handle = 3087007744, refCount = 1,
refLock = 0x8b45d30, slot = 0x8b2a700, objectInfo = 0x8bd27b0, infoFree =
0xb5c0f5a7 <nsslowkey_DestroyPrivateKey>}

from my brief look into the code and printing a few things out, it looks like
key->handle=3087007744 is "bad".

Comment 2

[Andrew Schultz]

this regressed on the Mozilla trunk between nightly builds 2003111707 and
2003111807.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=11%2F17%2F2003+06%3A00%3A00&maxdate=11%2F18%2F2003+07%3A00%3A00&cvsroot=%2Fcvsroot

I don't see anything there that seems related to this bug.  Is it possible the
NSS client branch tag changed in that window?

Comment 3

[Wan-Teh Chang]

Assigned the bug to Bob.

I landed NSS 3.9 Beta on the Mozilla trunk on 2003-11-17.
The upgrade was from NSS 3.8.3.  There were a lot of
changes between NSS 3.8.3 and NSS 3.9 Beta.  (Bonsai
does not track the changes to NSS_CLIENT_TAG.)  Could you
confirm this by running the following command on libnss3.so
in nightly builds 2003111707 and 2003111807?

    % ident libnss3.so | grep NSS

The crash occurs here:

        /* get the key type */
        att = pk11_FindAttribute(key,CKA_KEY_TYPE);
        PORT_Assert(att != NULL);
        size = sizeof(CK_KEY_TYPE);
        if (att->attrib.ulValueLen < size) {

So the first thing we need to do is to replace the
assertion by proper error handling.

Comment 4

[Wan-Teh Chang]

Comment on attachment 139554 [details]
stacktrace

Do you know why only stack frames #0, #1, and #2 (all
in libsoftokn3.so) have debug info?  #3-#6 are functions
in another NSS shared library (libnss3.so), but they
don't have debug info.

Comment 5

[Andrew Schultz]

> There were a lot of changes between NSS 3.8.3 and NSS 3.9 Beta.

Is there any way to query what was different between those branches at that time?

> % ident libnss3.so | grep NSS

I don't have an ident command, but "nm -D libnss3.so | grep NSS_3" gives me 3.2
to 3.8 with 2003111707 and 3.2 to 3.9 with 2003111807

> Do you know why only stack frames #0, #1, and #2 (all in libsoftokn3.so) have 
> debug info? 

I was starting from a build without symbol info and rebuilding the top of the
stack with symbols.  I'll attach another stack with more symbols.

Comment 6

[Andrew Schultz]

Attachment: stacktrace with more symbols

Comment 7

[Andrew Schultz]

Attachment: stacktrace with more symbols

oops.  *this* is the the stack wth more symbols.

Comment 8

[Wan-Teh Chang]

You can try
    cvs diff -kk -u -r NSS_3_8_BRANCH -r NSS_3_9_BETA1 mozilla/security/nss/lib

Comment 9

[Andrew Schultz]

I cvs-pulled NSS HEAD by date within my Mozilla 1.6 release build (NSS HEAD in
early november became the NSS_CLIENT branch for 1.6 after Nov 15, right?), and
narrowed the regression down to this checkin:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fsecurity%2Fnss%2Flib&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=11%2F13%2F2003+9%3A00&maxdate=11%2F14%2F2003+12%3A00&cvsroot=%2Fcvsroot

Comment 10

[Andrew Schultz]

> became the NSS_CLIENT branch for 1.6 after Nov 15

Nov 17, not 15

Comment 11

[Robert Relyea]

OK, I see a couple of issues here:

1) we went from 'could never return NULL' for the get of KEY_TYPE, to 'it's
possible to return null on bad database data. That will fix the crash. You still
won't be able to read mail, however, since your SDR key would still have some
problems. I have no idea why linux would have different storage for the SDR key
than Windows (since they have the same 'endianness'. Could you set a breakpoint
at line 742 of pkcs11u.c (that would be insice the function
pk11_FindSecretKeyAttribute(), the line is:
        if ((keyTypeLen != sizeof(keyType)) && (keyTypeLen != 1)) {

and print out the following:
keyTypeLen
keyString[0]
keyString[1]
keyString[2]
keyString[3]

Thanks,

bob

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Is the problem endianness?  Or is it perhaps alignment?  

Comment 13

[Andrew Schultz]

> You still won't be able to read mail, however, since your SDR key would still
> have some problems.

Note that without the checkin mentioned in comment 9, Mozilla not only did not
crash, but successfully decrypted my passwords -- I was able to check my mail.


Breakpoint 1, pk11_FindSecretKeyAttribute (object=0x8, type=256) at pkcs11u.c:741
741             if ((keyTypeLen != sizeof(keyType)) && (keyTypeLen != 1)) {
(gdb) p keyTypeLen
$1 = 8
(gdb) p keyString[0]
$2 = 21 '\025'
(gdb) p keyString[1]
$3 = 0 '\0'
(gdb) p keyString[2]
$4 = 0 '\0'
(gdb) p keyString[3]
$5 = 0 '\0'

Comment 14

[Robert Relyea]

OK, it looks like your SDR key was originally created on a 64 bit platform.

Yes, there *was* an endian problem in the original pkcs11 module. This looks
like a 64-bit issue.

I should be able to come up with a fix from this information (thanks Andrew).

bob

Comment 15

[Andrew Schultz]

> OK, it looks like your SDR key was originally created on a 64 bit platform.

Yes.  I created the key on an Alpha.  I guess that's why I'm the only one
running into this problem...

Comment 16

[Robert Relyea]

Attachment: Fix both Crash symptoms and underlying problem

This patch does the following:
1) fix our storage put the same bytes in the database independed of platform
size (32 bit or 64 bit).
2) remove redundant endian specific code which only handled Generic_init case
anyway.
3) fix generic_init code to handle the case if we can't get the keyType.
4) fix FindSecretKeyAttribute to correctly return the keyType 1) on 64 bit
platforms, and 2) with data written by 64 bit plaforms.

Comment 17

[Robert Relyea]

Andrew, can you check this against your database?
Nelson could you review?

Thanks,  bob

Comment 18

[Andrew Schultz]

Comment on attachment 139772 [details] [diff] [review]
Fix both Crash symptoms and underlying problem

the patch works great except that these lines 

>+    // on 64 bit platforms, we still want to store 32 bits of keyType (This is
>+    // safe since the PKCS #11 defines for all types are 32 bits or less).

won't compile.	:)
need to use /*	*/

thanks!!!

Comment 19

[Andrew Schultz]

Comment on attachment 139772 [details] [diff] [review]
Fix both Crash symptoms and underlying problem

>+	/*
>+ 	 * this key was written on a 64 bit platform with a using NSS 3.9
>+	 * or earlier. Reduce the 64 bit possibilities above. We were are
>+	 * through, we will only have:
>+	 * 
>+	 * Big Endian, pre-3.9, 32 & 64 bits:   1   (kt)
>+	 * Little Endian, pre-3.9 all lengths:  4   (kt) 0  0  0
>+	 * All platforms 3.9 all lengths:       4     0  0  0 (kt)
>+	 * All platforms > 3.9.1 all lengths:   4    (a) k1 k2 k3
	 */  <========= need to also close this comment block

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Review comments:

This code depends on key->u.rsa.coefficient.data and att->attrib.pValue 
being properly aligned to be cast to (CK_KEY_TYPE *).  I guess it happens 
to be true now, but it's not clear that it will necessarily always be true.
The patch relies on this property, as does the unpatched code, so the patch
is no worse in this respect.  

I'd like to see an assertion that att->attrib.ulValueLen == sizeof(CK_KEY_TYPE)
just after the pk11_FindAttribute call in pkcs11c.c, just to catch us if we ever
blow it.

Having said that, I believe this code is correct, or will be once the problems
with comments are fixed.  In addition to the comment issues Andrew noted above,
there are some others.  I think it's quite important that the comments
accurately describe the facts.  So, here are some items.

1) There is a large block comment in the source, at line 737 in pkcs11u.c, 
just above the beginning of the patch, that says: 

         * 0x1f and bytes[1]-bytes[3] equal to '0' (little endian). All other
         * values are assumed to be from the new database, which is always 4
         * bytes in host order */

The new database is always in NETWORK byte order, not host.  The code is 
right, but the comment is exactly backwards.

2) The new comments in the patch consistently use the expression "> 3.9.1"
where you seem to mean "beginning in 3.9.1".  The expression seems to say
that releases AFTER 3.9.1 are this way, but 3.9.1 is not.  Perhaps they 
should read " > 3.9.0 "  or " >= 3.9.1 " or something else unambiguous.

3) You lined up the right side of the comments as a nice table.  Why not
make a table out of the left side too?  IMO

!        *                                    length  data
!        * Big Endian,    pre-3.9 all lengths:  1   (kt)
!        * Little Endian, pre-3.9 32 bits:      4   (kt) 0  0  0
!        * Little Endian, pre-3.9 64 bits:      8   (kt) 0  0  0   0  0  0  0
!        * All platforms      3.9 32 bits:      4     0  0  0 (kt)
!        * Big Endian         3.9 64 bits:      8     0  0  0 (kt) 0  0  0  0
!        * Little  Endian     3.9 64 bits:      8     0  0  0  0   0  0  0 (kt)
!        * All platforms >= 3.9.1 all lengths:  4    (a) k1 k2 k3

is easier to read and follow than

!        *                                    length  data
!        * Big Endian, pre-3.9, all lengths:    1   (kt)
!        * Little Endian, pre-3.9 32 bits:      4   (kt) 0  0  0
!        * Little Endian, pre-3.9 64 bits:      8   (kt) 0  0  0   0  0  0  0
!        * All platforms 3.9 32 bits:           4     0  0  0 (kt)
!        * Big Endian 3.9 64 bits:              8     0  0  0 (kt) 0  0  0  0
!        * Little  Endian 3.9 64 bits:          8     0  0  0  0   0  0  0 (kt)
!        * All platforms > 3.9.1 all lengths:   4    (a) k1 k2 k3

Please make another patch to address all these issues.  

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139772 [details] [diff] [review]
Fix both Crash symptoms and underlying problem

r-, see comments above.

Comment 22

[Robert Relyea]

Thanks Nelson & Andrew.

RE: alignment problem. This should be trivial to fix in the next patch. I'll
just use a PORT_Memcpy into the target variable, rather than the dereferenced
casted pointer.

RE: assert. PKCS #11 defines an error code for this case, I believe
(CKR_ATTRIBUTE_LENGTH_INVALID), which I will return after the assert.

I'll also adjust the comment (an wrong comment is worse than no comment), and
fix the c++ ism. I'll get a new patch out monday when I get back to work.

bob

Comment 23

[Robert Relyea]

Attachment: Incorporate review comments.

Comment 24

[Robert Relyea]

Attachment: Patch that actually compiles...

Comment 25

[Robert Relyea]

Comment on attachment 139922 [details] [diff] [review]
Patch that actually compiles...

This patch is flawed... I'll get a new one...

Comment 26

[Robert Relyea]

Attachment: second patch for review

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 139923 [details] [diff] [review]
second patch for review

Looks right to me.

Comment 28

[Robert Relyea]

Tip checkin log.

/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.89; previous revision: 1.88
done
Checking in pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.57; previous revision: 1.56
done
Checking in pkcs11u.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.55; previous revision: 1.54
done

Comment 29

[Wan-Teh Chang]

Comment on attachment 139923 [details] [diff] [review]
second patch for review

sr=wtc.  There are some minor problems that I'm not
sure if it is worth the time to fix them.

1. PORT_Memcpy vs. dereferenced casted pointer:
dereferenced casted pointers are used in many other
places (just search for "*(CK_KEY_TYPE" in an
editor).  So either they are safe or we need to
change them all to PORT_Memcpy.

This comment applies to the

>+	PORT_Memcpy(keyTypePtr, att->attrib.pValue, sizeof(CK_KEY_TYPE));

in pkcs11c.c.  In pkcs11u.c, we are using both
dereferenced casted pointer and PORT_Memcpy on
keyString within the same function:

+	    keyTypeStorage = *(PRUint32 *) keyString

+	    PORT_Memcpy(&keyTypeStorage, keyString, sizeof(keyTypeStorage));

2. Spelling errors in pkcs11u.c:
    "lengths The Database" => "lengths the database"
    "We were are" => "When we are"?

3. In pkcs11c.c,

>+	/*
>+	 * Now Handle:
>+	 *
>+	 * All platforms,      3.9, all lengths: 4    0  0  0 (kt)
>+	 * All platforms, => 3.9.1, all lengths: 4   (a) k1 k2 k3
>+	 *
>+	 * NOTE: if  kt == 0 or ak1k2k3 == 0, the test fails and
>+	 * we handle it as:
>+	 *
>+	 * Little Endian,  pre-3.9, all lengths: 4  (kt) 0  0  0
>+	 */
>+	if (keyTypeLen == sizeof(keyTypeStorage) &&
>+	     (((keyString[0] & 0x80) == 0x80) ||
>+		!((keyString[1] == 0) && (keyString[2] == 0)
>+	   				    && (keyString[3] == 0))) ) {

I think it is easier to understand if the last test is
		 ((keyString[0] == 0) && (keyString[1] == 0)
					    && (keyString[2] == 0))
which matches the comment.

Comment 30

[Wan-Teh Chang]

Comment on attachment 139923 [details] [diff] [review]
second patch for review

Another comment: in pkcs11u.c, in the final else block:

> 	} else {
>+	/*
>+	 * Now Handle:
>+	 *
>+	 * Big Endian,     pre-3.9, all lengths: 1  (kt)
>+	 * Little Endian,  pre-3.9, all lengths: 4  (kt) 0  0  0
>+	 *  -- KeyType == 0 all other cases ---: 4    0  0  0  0
>+	 */
>+	    keyType = (CK_KEY_TYPE) keyString[0] ;
>+        }

Would be nice to move the original key length test here:

-	/* only length of 1 or 4 are valid */
-	if ((keyTypeLen != 4) && (keyTypeLen != 1)) {
-	    PORT_SetError(SEC_ERROR_eyTypeBAD_DATABASE);
-	    return NULL;

Comment 31

[Robert Relyea]

"/tmp/cvsAAA6faywr" 13 lines, 441 characters 
Checking in lib/softoken/pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.88.4.1; previous revision: 1.88
done
Checking in lib/softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v  <--  pkcs11c.c
new revision: 1.56.14.1; previous revision: 1.56
done
Checking in lib/softoken/pkcs11u.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11u.c,v  <--  pkcs11u.c
new revision: 1.54.2.1; previous revision: 1.54
done


Branch checkin log... This checking includes fixed spellings identified by wan-teh.

Comment 32

[Wan-Teh Chang]

I checked Bob's fix into the NSS_CLIENT_TAG (Mozilla 1.7a).
Andrew, please verify this bug with tomorrow's (2004-01-28)
Mozilla nightly build.

Comment 33

[Andrew Schultz]

verified fixed in linux trunk 2004012808.
Mozilla did not crash and successfully retrieved my encrypted passwords.

thanks all!