295754 - PK11_ListCertsInSlot crashes in subject_list_sort on a cert with unsupported critical extension

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

We have a smart card that contains a certificate with a
CRL Distribution Points extension that is marked critical.

When we call PK11_ListCertsInSlot, it crashes in subject_list_sort
because it deferences a null pointer:

From func PK11_ListCertsInSlot(slot);

Program received signal SIGSEGV, Segmentation fault.
0x00aeac35 in subject_list_sort (v1=0x9a037c0, v2=0x9a02ee8) at tdcache.c:157
157         if (dc1->isNewerThan(dc1, dc2)) {
Current language:  auto; currently c
(gdb) list
152     {
153         NSSCertificate *c1 = (NSSCertificate *)v1;
154         NSSCertificate *c2 = (NSSCertificate *)v2;
155         nssDecodedCert *dc1 = nssCertificate_GetDecoding(c1);
156         nssDecodedCert *dc2 = nssCertificate_GetDecoding(c2);
157         if (dc1->isNewerThan(dc1, dc2)) {
158             return -1;
159         } else {
160             return 1;
161         }
(gdb) print dc1
$1 = (nssDecodedCert *) 0x0
(gdb) pirnt dc2
Undefined command: "pirnt".  Try "help".
(gdb) print dc2
$2 = (nssDecodedCert *) 0x9a01858
(gdb) print dc1
$3 = (nssDecodedCert *) 0x0
(gdb) print v1
$4 = (void *) 0x9a037c0
(gdb) print v2
$5 = (void *) 0x9a02ee8
(gdb) print dc1
$6 = (nssDecodedCert *) 0x0
(gdb) print dc2
$7 = (nssDecodedCert *) 0x9a01858
(gdb) print c1
$8 = (NSSCertificate *) 0x9a037c0
(gdb) print c2
$9 = (NSSCertificate *) 0x9a02ee8
(gdb) print *c1;
Invalid character ';' in expression.
(gdb) print *c1
$10 = {object = {arena = 0x9a036c0, refCount = 1, lock = 0x9a03f90,
   instances = 0x9a037b0, numInstances = 1, trustDomain = 0x98b0050,
   cryptoContext = 0x0, tempName = 0x0}, type = NSSCertificateType_PKIX, id = {
   data = 0x9a03850, size = 1}, encoding = {data = 0x9a03860, size = 664},
 issuer = {data = 0x9a03b00, size = 62}, subject = {data = 0x9a03b58,
   size = 53}, serial = {data = 0x9a03b48, size = 4}, email = 0x0,
 decoding = 0x0}
(gdb) print *c2
$11 = {object = {arena = 0x9a26e80, refCount = 2, lock = 0x9a27010,
   instances = 0x9a02ed8, numInstances = 1, trustDomain = 0x98b0050,
   cryptoContext = 0x0, tempName = 0x0}, type = NSSCertificateType_PKIX, id = {
   data = 0x9a02f78, size = 1}, encoding = {data = 0x9a02f88, size = 596},
 issuer = {data = 0x9a031e8, size = 62}, subject = {data = 0x9a03240,
   size = 53}, serial = {data = 0x9a03230, size = 4}, email = 0x0,
 decoding = 0x9a01858}
(gdb)

Sample debug stack trace of crash instance:

(gdb) where
#0  0x008c8c35 in subject_list_sort (v1=0x854bb58, v2=0x854b2c0) at
tdcache.c:157
#1  0x008d88fc in nsslist_add_element (list=0x8549a28, data=0x854bb58)
    at list.c:225
#2  0x008d8abc in nssList_AddUnique (list=0x8549a28, data=0x854bb58)
    at list.c:272
#3  0x008c96a6 in add_subject_entry (arena=0x854c3f0, cache=0x840d958,
    cert=0x854bb58, nickname=0x856f370 "encryption key for testuser",
    subjectList=0xbfe4e894) at tdcache.c:578
#4  0x008c9c91 in add_cert_to_cache (td=0x840cf70, cert=0x854bb58)
    at tdcache.c:812
#5  0x008c9e77 in nssTrustDomain_AddCertsToCache (td=0x840cf70,
    certs=0xbfe4e8fc, numCerts=1) at tdcache.c:890
#6  0x008cce2a in cert_createObject (o=0x854bb20) at pkibase.c:1045
#7  0x008cca5a in nssPKIObjectCollection_GetObjects (collection=0x8549208,
    rvObjects=0x856efb8, rvSize=2) at pkibase.c:851
#8  0x008ccf74 in nssPKIObjectCollection_GetCertificates (collection=0x8549208,
    rvOpt=0x856efb8, maximumOpt=0, arenaOpt=0x0) at pkibase.c:1100
#9  0x0087dd21 in PK11_TraverseCertsInSlot (slot=0x8401c58,
    callback=0x87ec2c <listCertsCallback>, arg=0xbfe4e9dc) at pk11cert.c:2966
#10 0x0087ed9c in PK11_ListCertsInSlot (slot=0x8401c58) at pk11cert.c:3619

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

The CRL distribution points extension is an unsupported extension:
http://lxr.mozilla.org/security/source/security/nss/lib/util/secoid.c#782

An unsupported critical extension is enough to cause certificate decoding
to fail:
http://lxr.mozilla.org/security/source/security/nss/lib/certdb/certdb.c#863
CERT_DecodeDERCertificate fails with SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION
because
http://lxr.mozilla.org/security/source/security/nss/lib/certdb/certxutl.c#504
cert_HasUnknownCriticalExten returns PR_TRUE because
http://lxr.mozilla.org/security/source/security/nss/lib/util/secoid.c#1776
SECOID_KnownCertExtenOID on CRL dist points returns PR_FALSE.

We can fix this crash at two levels.  At the higher level, we
can prevent undecodable certs from being passed to this
subject_list_sort function.  At the lower level, we can prevent
the subject_list_sort function from crashing on undecodable certs.

This patch fixes the crash at the lower level.	If we consider an
undecodable cert older than a decodable cert, we can implement the
subject_list_sort function like this:

static PRIntn subject_list_sort(void *v1, void *v2)
{
    NSSCertificate *c1 = (NSSCertificate *)v1;
    NSSCertificate *c2 = (NSSCertificate *)v2;
    nssDecodedCert *dc1 = nssCertificate_GetDecoding(c1);
    nssDecodedCert *dc2 = nssCertificate_GetDecoding(c2);
    if (!dc1) {
	return dc2 ? 1 : 0;
    } else if (!dc2) {
	return -1;
    } else {
	return dc1->isNewerThan(dc1, dc2) ? -1 : 1;
    }
}

I verified that this patch fixed the crash.

I reviewed the other callers of nssCertificate_GetDecoding
and fixed a similar null pointer dereferencing in this patch.

This may dereference null pointers but it is dead code.  It
probably should be removed.
http://lxr.mozilla.org/security/ident?i=nssBestCertificate_Callback

This does not crash but should be reviewed for correctness.
http://lxr.mozilla.org/security/source/security/nss/lib/pki/certificate.c#443

I then looked at the callers of subject_list_sort more.
This function is used when we add certs to the cache.
So the question is whether we want to add undecodable
certs to the cache.  If so, then this patch is fine.
If not, then we need to fix this crash at a higher
level, probably in nssTrustDomain_AddCertsToCache.
nssTrustDomain_AddCertsToCache returns PRStatus but none
of its callers check the return value.	Also, all callers
of nssTrustDomain_AddCertsToCache call it to add only one
cert even though the function can add an array of certs
to the cache.

Note to self: pk11_fastCert has a memory leak.	It may
store an allocated string in *nickptr but return NULL.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 184725 [details] [diff] [review]
Proposed patch

I believe this patch is correct, but it reveals the existence of this function
duplicated entirely in several source files.	Please use this opportunity to
eliminate one of the duplicate bodies of code.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 184725 [details] [diff] [review]
Proposed patch

This is one of those situations where the patch is correct, and so deserves an
r+, but it reveals another pre-existing issue in the code that should also be
addressed.  So, I'm giving it r+, but asking that one of the duplicate
functions be eliminated if possible.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

This bug raises an issue that needs to be (re)visited.  
All the PKI cert standards agree that a "relying party" must not rely on
a cert that contains unknown critical extensions.  NSS's way of enforcing
that behavior is implemented in CERT_DecodeDERCertificate.  That function 
intentionally fails to return a non-NULL result (fails to return a
CERTCertificate) if the cert contains any unknown critical extension.
This approach ensures that NSS will never rely upon any such cert, because
NSS will never be able to access the parsed contents of such a cert.  

But as this bug illustrates, it has other side effects of making it difficult
to even SEE (e.g. display) the contents of such a cert.  I believe there 
are already numerous bugs against PSM that have this issue as their root 
cause.  

Another consequence of this issue is that today there are no (or too few) 
places in NSS, outside of CERT_DecodeDERCertificate, that check to see if
a cert has any unknown critical extensions.  Throughout NSS, the code 
assumes that if it has a non-NULL CERTCertificate address, the cert therein
must have no unknown critical extensions.  

We could change CERT_DecodeDERCertificate to succesfully decode a cert
with  unknown critical extensions, and (say) set a flag in the CERTCertificate
that says "do not rely on this cert, because it has unknown critical
extensions".  But every place in NSS that today depends on a NULL cert pointer
test to determine if a cert may be relied upon would have to be changed to 
check this other flag.  I'm sure that we do not know all the places in NSS
where such a check would be required.  

Suggestions?

Comment 5

[Robert Relyea]

Comment on attachment 184725 [details] [diff] [review]
Proposed patch

r+ I agree with Nelson's comments. About duplication

Comment 6

[Robert Relyea]

On question is what does it mean to 'rely' on a cert. Is it safe to say that we
don't 'rely' on a cert until we go to use the public key? If that is the case
then we can simply add the check in extract public key. Decoding the cert can
mark it as having 'unknown critical extensions' so we don't have to parse the
extensions everytime. That also gives applications an easy handle to display
that particular certs are bad because of unknown critical extensions.

bob

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 6, on the meaning of "rely", I think that when we send 
out a cert, such as part of a signature chain, or for a remote party to 
use to encrypt data to send to us, we are also relying on that cert to 
represent us.  In those circumstances, we tend not to use or check the 
public key from the cert.  So, a check at public key extraction time would
not suffice to prevent us from relying on the cert at those times.  

It may be that there is a small set of functions, including public key
extraction and others, which together suffice to prevent us from relying
on a cert with unknown critical extensions.  So, I don't think we should 
give up on this idea entirely.  

Perhaps the functions such as  CERT_CertChainFromCert, which build a chain
for transmitting, could check all the certs, including the one passed in,
to see if they have any unknown critical extensions.  There are several 
functions that perform similar functions (build cert chains).  They might 
all need to be changed.  

On the other hand, I worry that if NSS begins to check for unknown Critical
Extensions at different places than it has in the past, then programs may 
begin to experience failures in places never before seen.  IOW, such a 
change may be a binary compatibility issue.

Comment 8

[Wan-Teh Chang]

Comment on attachment 184725 [details] [diff] [review]
Proposed patch

Patch checked in on the tip for NSS 3.10.1.

Checking in pkistore.c;
/cvsroot/mozilla/security/nss/lib/pki/pkistore.c,v  <--  pkistore.c
new revision: 1.25; previous revision: 1.24
done
Checking in tdcache.c;
/cvsroot/mozilla/security/nss/lib/pki/tdcache.c,v  <--	tdcache.c
new revision: 1.41; previous revision: 1.40
done

I will submit another patch that eliminates the
duplicate code.  Thanks for the code review.

Comment 9

[Wan-Teh Chang]

Attachment: Eliminate duplicate code

Rename the subject_list_sort function as nssCertificate_SubjectListSort
and move it to certificate.c.  Declare it in pkim.h.

It's not obvious which .c and .h files are the right home for this sorting
function for NSSCertificate.  I picked certificate.c and pkim.h based
on my best judgement.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 187174 [details] [diff] [review]
Eliminate duplicate code

r=nelsonb

Comment 11

[Robert Relyea]

Comment on attachment 187174 [details] [diff] [review]
Eliminate duplicate code

r+ relyea; locations seem fine.

Comment 12

[Wan-Teh Chang]

I checked in the "Eliminate duplicate code" patch on the
NSS trunk for NSS 3.11 only.

Checking in certificate.c;
/cvsroot/mozilla/security/nss/lib/pki/certificate.c,v  <--  certificate.c
new revision: 1.54; previous revision: 1.53
done
Checking in pkim.h;
/cvsroot/mozilla/security/nss/lib/pki/pkim.h,v  <--  pkim.h
new revision: 1.27; previous revision: 1.26
done
Checking in pkistore.c;
/cvsroot/mozilla/security/nss/lib/pki/pkistore.c,v  <--  pkistore.c
new revision: 1.26; previous revision: 1.25
done
Checking in tdcache.c;
/cvsroot/mozilla/security/nss/lib/pki/tdcache.c,v  <--  tdcache.c
new revision: 1.42; previous revision: 1.41
done

Note that the crash is fixed in NSS 3.10.1 by the first patch.

Comment 13

[Wan-Teh Chang]

Attachment: Remove dead code nssBestCertificate_Callback

Using LXR, it is easy to verify nssBestCertificate_Callback
is dead code.

On an unrelated note, at the end of comment 1, I wrote the
following note to self:

  Note to self: pk11_fastCert has a memory leak.  It may
  store an allocated string in *nickptr but return NULL.

I reviewed the only caller of pk11_fastCert and found that
it frees the string returned in *nickptr if pk11_fastCert
returns NULL, so there is no memory leak.

Comment 14

[Julien Pierre]

Comment on attachment 187441 [details] [diff] [review]
Remove dead code nssBestCertificate_Callback

I wonder if somebody was calling this function before .

Comment 15

[Wan-Teh Chang]

Comment on attachment 187441 [details] [diff] [review]
Remove dead code nssBestCertificate_Callback

Julien, good question.	Indeed this function was called in NSS 3.4.
It was removed in bug 135521 for NSS 3.5.  (As far as
I can tell, the new code uses nssCertificateArray_FindBestCertificate.)
For some reason Ian removed the declaration and the callers of
the old function but did not remove its definition.

I found some other related dead code, so I will address this issue
in a new bug.