160424 - NSS crashes when some smartcards return invalid or corrupt certificates

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[bill]

This is somewhat related to bug #157730 but affects all platforms.

The problem is apparent when using MUSCLE drivers for instances where the CAC
card has 2 (instead of 3) certificates stored on it.  The driver is trying to
report back the 3rd cert on the card when NSS queries the driver for certs on
the token.

We need a way to filter out corrupt structures when PKCS#11 drivers report back
apparently invalid certificates.

Comment 1

[Robert Relyea]

Attachment: Handle the case where we can't decode the certificate.

This patch fixes a couple of issues.
1) nssPKIObjectCollection_GetObjects() is supposed to return no more than the
requested number of objects. The patch makes this happen. (currently none of
the callers ask for less than the available Objects).
2) When we Convert a PKIObject to it's decoded Object format, if we fail we
need to remove that object from the collection. In many places the current code
just fails (meaning 1) you will now have an object with a NULL pointer in the
collection, and 2) you won't see the other objects in the collection if you
have one bad one.

bob

Comment 2

[Robert Relyea]

Attachment: Don't crash if the cert you are looking up by name is not valid.

We find a cert, but it doesn't have a valid encoding. This shows up when you
do S/MIME (which Bill didn't get to in his earlier tests).

Comment 3

[Wan-Teh Chang]

Comment on attachment 93515 [details] [diff] [review]
Don't crash if the cert you are looking up by name is not valid.

r=wtc.

Comment 4

[Wan-Teh Chang]

Assigned the bug to Bob.  Ian, please review Bob's patch.

Comment 5

[Ian McGreer]

The first patch is slightly different than the one that was checked in, but what
was checked in was more correct.  So I'll r= the patch that was checked in.

Comment 6

[Ian McGreer]

One note, on a later checkin you added a change to detect when the encoding of a
cert is returned as NULL, but then reversed the sense of the test to handle that
case.  I made the following change in pkibase.c:

@@ -987,7 +987,7 @@
     derCert = nssCertificate_GetEncoding(c);
     uid[0].data = NULL; uid[0].size = 0;
     uid[1].data = NULL; uid[1].size = 0;
-    if (derCert == NULL) {
+    if (derCert != NULL) {
 	uid[0] = *derCert;
     }
 #else

Comment 7

[Robert Relyea]

Ian, which differences are you refering to in comment 5 ? The only difference
should be that in the above patch I collected everything you need to remove a
link in a single function. On the tip I inlined the differences. I was
personally thinking that the function idea was better and was planning to
advance the tipe to using the function.

comment 6 : Thanks. The crash this 'fixes' only happenned on the tip, not on
3.5. 3.5 always tries to decode the certificate immediately and would fail out
at that point. I made two changes to the tip, this one (which was supposed to
protect against a bad encoding), and one that rejects NULL cert objects without
any CKA_VALUE associated with them (I think all the rest of the attributes could
be derived for the encoding if necessary).

bob

Comment 8

[Robert Relyea]

BTW the review for these patches is against NSS_3_5_BRANCH, not the tip.

thanks.

bob

Comment 9

[Ian McGreer]

Comment on attachment 93513 [details] [diff] [review]
Handle the case where we can't decode the certificate.

Okay, I was looking at the tip.  If this patch is for 3.5, I have one comment:

> static PRStatus
> nssPKIObjectCollection_GetObjects
> (
>@@ -859,11 +870,16 @@
>            /* Convert the proto-object to an object */
>            node->object = (*collection->createObject)(node->object);
>            if (!node->object) {
>-               return PR_FAILURE;
>+               /*remove bogus object from list*/

don't you need a link = PR_NEXT_LINK(link) here?

>+               nssPKIObjectCollection_RemoveNode(collection,node);
>+               continue;
>            }
>            node->haveObject = PR_TRUE;
>        }

Comment 10

[Robert Relyea]

Attachment: Handle the case where we can't decode the certificate. II

Incorporate Ian's note: good catch.

Comment 11

[Wan-Teh Chang]

Comment on attachment 93513 [details] [diff] [review]
Handle the case where we can't decode the certificate.

I reviewed this patch. Ian already pointed out the bug that
in nssPKIObjectCollection_GetObjects we are not advancing
the link before we remove the node.

I have one question: does nssPKIObjectCollection_GetObjects
allow the caller to specify a rvSize that is smaller than the
number of objects in the collection?  Or should it fail with
a "buffer too small" error?

I verified that this patch fixes all the call sites of
    node->object = (*collection->createObject)(node->object);

Comment 12

[Ian McGreer]

Wan-Teh, the answer to your question is yes.

Comment 13

[Wan-Teh Chang]

Comment on attachment 93584 [details] [diff] [review]
Handle the case where we can't decode the certificate. II

> 	rvObjects[i++] = nssPKIObject_AddRef(node->object);
>+	if (i >= rvSize) {
>+	   break;
>+	}

Instead of this, I suggest we do this check in the loop's
control:
    while (i < rvSize && link != &collection->head) {

This will handle the pathological case that the caller passes
in a rvSize of 0.

Another problem with this function is that it probably should
fail if the collection has fewer objects than requested.  We
may want to add something like this at the end of the function:

+    if (i < rvSize) {
+	 PORT_SetError(/* too few objects */);
+	 return PR_FAILURE;
+    }
     return PR_SUCCESS;

Or

+    if (i < rvSize) {
+	 rvObjects[i] = NULL;  /* mark end of data in array */
+    }
     return PR_SUCCESS;

Comment 14

[Ian McGreer]

Wan-Teh, I agree with both suggestions.  For the second, I would prefer the
latter method of NULL-terminating the array.  Perhaps the caller has a static
array of N objects, while the collection only has M and M < N.  The caller
doesn't want to fail, he just wants at most N objects.

Comment 15

[Robert Relyea]

When I made the change, I looked at all the callers of this static function, in
which all the callers know the size of the collection to begin with and are
purposefully asking for less objects than are in the collection so it's clear
that is not an error case. If it were the original function would have simply
failed and returned to the caller.

The loop change seems reasonable.

NOTE: This is not a patch to fix an existing bug, but a problem I saw when
reviewing the code to make the previous patch.

Ian, the code does this already. It does not return a length, but a NULL
terminated array. It currently assmes the caller has passed srcSize+1 elements
pre-zero'ed'

Comment 16

[Wan-Teh Chang]

Bob,

You are right.  The callers have passed rvSize+1 elements
pre-zeroed.  I didn't realize that nssPKIObjectCollection_GetObjects
is a static function.

Please go ahead and check in both patches on the
NSS_3_5_BRANCH and request drivers' approval for mozilla1.1.
Thanks.

Comment 17

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 1.1

Comment 18

[Robert Relyea]

Check-in.

Comment 19

[bill]

With my cert on my smartcard I tested SMIME signing and SSL-client-auth via
HTTPS, SSL/IMAP, and SSL/SMTP under Mozilla 1.1b for Linux.  Works.

Comment 20

[Wan-Teh Chang]

Marked the bug verified per shadow's comment #19.

Comment 21

[bill]

Shouldn't the 2002080303 Mac OS X build have this fix?  I think it's missing
this patch and only has the #157730 patch.

I can see the certs on my card without crashing but sites that request cert-auth
don't prompt me for my cert (even with "ask everytime set") or allow me to
perform the client-auth.

Comment 22

[Wan-Teh Chang]

The Mozilla *trunk* builds should have this fix.  Did
you test a 1.0 branch build?

Comment 23

[bill]

I just tried again.  Build 2002080503, the trunk build, did the following:
- prompt me for the smartcard password
- think for a few seconds
- then prompt me for the LDAP basic-auth credentials

cert-auth continues to fail on Mac OS X with the CAC card.  I'll test the Linux
build later.

Comment 24

[bill]

comment 23 was for the Mac OS X build.

I just downloaded the 2002080504 trunk build for Linux and the patch appears to
be in there because the CAC card functions properly with SSL client-auth sites.

We noticed that the patch to inject new root CA certs took longer on the Mac as
well; perhaps Mac builds are several days behind in code pulls?

Comment 25

[bill]

I'm unable to perform SSL client-authentication using Mozilla 2002080611-trunk
build with the certs on my smartcard with mac OS X.

What happens is:
- I got a website that requests ssl client-auth
- I'm prompted for my smartcard password
- then I'm prompted for my userid/password, which indicates that client-auth as
failed.

I'm not asked to choose a certificate even though "ask every time" is chosen.

Comment 26

[bill]

just for the record, this is also affecting Mozilla 1.01 rc1 builds.

Comment 27

[Wan-Teh Chang]

Reopened the bug because we are still crashing on the Mac.
Bob, please use the Mozilla 1.1 debug build on Javi's Mac
to debug this.

shadow: Mozilla 1.0.1 will not have this fix. This fix is
only in Mozilla 1.1.

Comment 28

[Wan-Teh Chang]

Correction: in comment #27 I said we were still crashing
on the Mac.  That is incorrect.  I read shadow's comments
again and realized that the problem was not a crash but
rather the inability to perform SSL client auth (see
comment #21). 

Anyways, shadow just reported that Mozilla 1.1 works with
the latest MUSCLE pkcs11.bundle on Mac.  So I'm marking
the bug fixed.