213084 - crash when reading signed messages with some certs

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Kai Engert [:KaiE:]]

After having received several complaints from people crashing with received
S/Mime messages sent from Outlook users, I was able to reproduce the problem on
my system.

Reproducable: Every time, but only with some messages and some certificate
databases.

Seen with Mozilla 1.4 final (aka Netscape 7.1), and also a special Mozilla 1.3.1
build that uses a recent snapshot of NSS_3_7_branch.

The crash is in SECITEM_Hash (key=0xd8) at secitem.c:259

The problem probably starts in seckey_UpdateCertPQGChain,
it gets called with a NULL subjectCert,
and no non-NULL test is made in that function.

The null parameter can be traced back to 
CERT_FilterCertListByUsage, where a call to
CERT_CheckKeyUsage(node->cert, ...)
is made, with node being != NULL,
and node->cert being NULL.

#6  0x42dc01ec in SECITEM_Hash (key=0xd8) at secitem.c:259
	item = (SECItem *) 0xd8
	rv = 0
	data = (PRUint8 *) 0x80c9830 ""
	i = 1079354304
	rvc = (PRUint8 *) 0x40150158 "лж\211ц\213\203М\001"
#7  0x400e71ec in PL_HashTableLookupConst (ht=0x896fe38, key=0xd8) at
/home/kaie/140/mozilla/nsprpub/lib/ds/plhash.c:404
	keyHash = 1078650184
	he = (PLHashEntry *) 0xbfffeae4
	hep = (PLHashEntry **) 0x40559fa0
#8  0x42dc0939 in SECOID_FindOID (oid=0xd8) at secoid.c:1614
	ret = (SECOidData *) 0xbfffeaf4
	rv = 1123284412
#9  0x42d7214f in seckey_UpdateCertPQGChain (subjectCert=0x0, count=1) at
seckey.c:454
	rv = SECSuccess
	rvCompare = 1121719511
	oid = (SECOidData *) 0x0
	tag = 0
	subjectSpki = (CERTSubjectPublicKeyInfo *) 0x0
	issuerSpki = (CERTSubjectPublicKeyInfo *) 0x0
	issuerCert = (CERTCertificate *) 0x0
#10 0x42d72353 in SECKEY_UpdateCertPQG (subjectCert=0x0) at seckey.c:556
No locals.
#11 0x42d732c4 in CERT_ExtractPublicKey (cert=0x0) at seckey.c:1152
	rv = 1121927604
#12 0x42da4440 in CERT_CheckKeyUsage (cert=0x0, requiredUsage=16384) at
certdb.c:1209
	key = (SECKEYPublicKey *) 0x8617418
#13 0x42da6687 in CERT_FilterCertListByUsage (certList=0x8616b08,
usage=certUsageEmailRecipient, ca=0) at certdb.c:2596
	requiredKeyUsage = 16384
	requiredCertType = 32
	node = (CERTCertListNode *) 0x8616b28
	savenode = (CERTCertListNode *) 0x0
	bad = 0
	rv = SECSuccess
	certType = 224
	dummyret = 144776328
#14 0x42d39323 in NSS_CMSSignedData_ImportCerts (sigd=0x89c48e8,
certdb=0x8845dd8, certusage=certUsageEmailRecipient, keepcerts=1) at
cmssigdata.c:496
	certcount = 2
	certArray = (CERTCertificate **) 0x8a12048
	certList = (CERTCertList *) 0x8616b08
	node = (CERTCertListNode *) 0x8845dd8
	rv = SECSuccess
	rawArray = (SECItem **) 0xbfffec18
	i = 2
	now = 621809595118526316
#15 0x42f3f835 in nsCMSMessage::CommonVerifySignature (this=0x8a11c88,
aDigestData=0x8a12000 "щIK\227Хn*\211NBr\004t\027!_\n\037-Є1",
aDigestDataLen=20) at /home/kaie/140/mozilla/security/manager/ssl/src/nsCMS.cpp:362
	this = (nsCMSMessage *) 0x8a11c88
	locker = {<No data fields>}
	cinfo = (NSSCMSContentInfo *) 0x89c4870
	sigd = (NSSCMSSignedData *) 0x89c48e8
	si = (NSSCMSSignerInfo *) 0xbfffec20
	nsigners = -1073746885
	rv = 2147500037
#16 0x42f3f643 in nsCMSMessage::VerifyDetachedSignature (this=0x8a11c88,
aDigestData=0x8a12000 "щIK\227Хn*\211NBr\004t\027!_\n\037-Є1",
aDigestDataLen=20) at /home/kaie/140/mozilla/security/manager/ssl/src/nsCMS.cpp:315
	this = (nsCMSMessage *) 0x8a11c88
	locker = {<No data fields>}
#17 0x4253801e in MimeMultCMS_generate (crypto_closure=0x87d26a8) at
/home/kaie/140/mozilla/mailnews/mime/src/mimemcms.cpp:480
	data = (MimeMultCMSdata *) 0x87d26a8
	signed_p = 1
	good_p = 0
	encrypted_p = 0
	unverified_p = 0
	rv = 0
	signature_status = 1
	signerCert = {mRawPtr = 0x0}
	aRelativeNestLevel = 1
	maxNestLevel = -1073746756
#18 0x42502550 in MimeMultipartSigned_emit_child (obj=0x87d2130) at
/home/kaie/140/mozilla/mailnews/mime/src/mimemsig.cpp:617
	html = 0x42510c64 "U\211е\203м\030VSи"
	sig = (MimeMultipartSigned *) 0x87d2130
	mult = (MimeMultipart *) 0x87d2130
	cont = (MimeContainer *) 0x87d2130
	status = 0
	body = (MimeObject *) 0x42f40ba0
#19 0x4250182f in MimeMultipartSigned_parse_eof (obj=0x87d2130, abort_p=0) at
/home/kaie/140/mozilla/mailnews/mime/src/mimemsig.cpp:161
	sig = (MimeMultipartSigned *) 0x87d2130
	status = 0
#20 0x424f0327 in MimeContainer_parse_eof (object=0x894d7f8, abort_p=0) at
/home/kaie/140/mozilla/mailnews/mime/src/mimecont.cpp:141
	lstatus = 1075133906
	kid = (MimeObject *) 0x87d2130
	i = 0
	cont = (MimeContainer *) 0x894d7f8
	status = 0
#21 0x42500a13 in MimeMessage_parse_eof (obj=0x894d7f8, abort_p=0) at
/home/kaie/140/mozilla/mailnews/mime/src/mimemsg.cpp:544
	status = -1073746512
	outer_p = 1112607844
	msg = (MimeMessage *) 0x894d7f8
#22 0x42510d06 in mime_display_stream_complete (stream=0x894d878) at
/home/kaie/140/mozilla/mailnews/mime/src/mimemoz2.cpp:928
	status = 142882620
	abortNow = 0
	msd = (mime_stream_data *) 0x897a848
	obj = (MimeObject *) 0x894d7f8
#23 0x42524cff in nsStreamConverter::OnStopRequest (this=0x88436e0,
request=0x86efa8c, ctxt=0x828bfd0, status=0) at
/home/kaie/140/mozilla/mailnews/mime/src/nsStreamConverter.cpp:1059
	tSession = (nsMIMESession *) 0x894d878
#24 0x408e8f58 in nsDocumentOpenInfo::OnStopRequest (this=0x8864e08,
request=0x86efa8c, aCtxt=0x828bfd0, aStatus=0) at
/home/kaie/140/mozilla/uriloader/base/nsURILoader.cpp:251
	listener = {mRawPtr = 0x88436e0}
	this = (nsDocumentOpenInfo *) 0x8864e08
#25 0x422eaf43 in nsMsgProtocol::OnStopRequest (this=0x86efa88,
request=0x89320d0, ctxt=0x828bfd0, aStatus=0) at
/home/kaie/140/mozilla/mailnews/base/util/nsMsgProtocol.cpp:363
	rv = 0
	msgUrl = {mRawPtr = 0x422eaea8}
#26 0x42c36bdc in nsMailboxProtocol::OnStopRequest (this=0x86efa88,
request=0x89320d0, ctxt=0x828bfd0, aStatus=0) at
/home/kaie/140/mozilla/mailnews/local/src/nsMailboxProtocol.cpp:394
	rv = 143859948
	stopped = 0
#27 0x40cc04dc in nsInputStreamPump::OnStateStop (this=0x89320d0) at
/home/kaie/140/mozilla/netwerk/base/src/nsInputStreamPump.cpp:483
No locals.
#28 0x40cbfdb6 in nsInputStreamPump::OnInputStreamReady (this=0x89320d0,
stream=0x8826254) at
/home/kaie/140/mozilla/netwerk/base/src/nsInputStreamPump.cpp:324
	nextState = 3
	this = (nsInputStreamPump *) 0x89320d0
#29 0x4069ef3f in nsInputStreamReadyEvent::EventHandler (plevent=0x8864e98) at
/home/kaie/140/mozilla/xpcom/io/nsStreamUtils.cpp:116
	plevent = (PLEvent *) 0x8864e98
	ev = (nsInputStreamReadyEvent *) 0x8864e98
#30 0x406c2fdb in PL_HandleEvent (self=0x8864e98) at
/home/kaie/140/mozilla/xpcom/threads/plevent.c:659
	result = (void *) 0x8864e98
#31 0x406c377f in PL_ProcessEventsBeforeID (aSelf=0x81262c0, aID=6119) at
/home/kaie/140/mozilla/xpcom/threads/plevent.c:1673
	event = (PLEvent *) 0x8864e98
	count = 0
	fullCount = 2
#32 0x41bdb888 in processQueue (aElement=0x81262c0, aData=0x17e7) at
/home/kaie/140/mozilla/widget/src/gtk/nsAppShell.cpp:443
	queue = (struct PLEventQueue *) 0x81262c0
	id = 6119
#33 0x406757f0 in nsVoidArray::EnumerateForwards (this=0x821f3c0,
aFunc=0x41bdb85c <processQueue(void *, void *)>, aData=0x17e7) at
/home/kaie/140/mozilla/xpcom/ds/nsVoidArray.cpp:648
	this = (nsVoidArray *) 0x821f3c0
	index = 0
	running = 1
#34 0x41bdb8d2 in nsAppShell::ProcessBeforeID (aID=6119) at
/home/kaie/140/mozilla/widget/src/gtk/nsAppShell.cpp:451
No locals.
#35 0x41beacda in handle_gdk_event (event=0x8292788, data=0x0) at
/home/kaie/140/mozilla/widget/src/gtk/nsGtkEventHandler.cpp:904
	eventObject = (GtkObject *) 0x8190348
	event_time = 1981283859
	serial = 6119
#36 0x402a7d6f in gdk_event_dispatch () from /usr/lib/libgdk-1.2.so.0
No symbol table info available.
#37 0x402d9773 in g_main_dispatch () from /usr/lib/libglib-1.2.so.0
No symbol table info available.
#38 0x402d9d39 in g_main_iterate () from /usr/lib/libglib-1.2.so.0
No symbol table info available.
#39 0x402d9eec in g_main_run () from /usr/lib/libglib-1.2.so.0
No symbol table info available.
#40 0x401f52e3 in gtk_main () from /usr/lib/libgtk-1.2.so.0
No symbol table info available.
#41 0x41bdb50a in nsAppShell::Run (this=0x821f3a8) at
/home/kaie/140/mozilla/widget/src/gtk/nsAppShell.cpp:328
	this = (nsAppShell *) 0x821f3a8
#42 0x41b81004 in nsAppShellService::Run (this=0x81e2240) at
/home/kaie/140/mozilla/xpfe/appshell/src/nsAppShellService.cpp:478
	this = (nsAppShellService *) 0x81e2240
#43 0x08066a7b in main1 (argc=4, argv=0xbffff794, nativeApp=0x814e6b8) at
/home/kaie/140/mozilla/xpfe/bootstrap/nsAppRunner.cpp:1268
	rv = 0
	nativeAppOwner = {<nsCOMPtr_base> = {mRawPtr = 0x0}, <No data fields>}
	eventQService = {mRawPtr = 0x814df40}
	obsService = {mRawPtr = 0x81238f8}
	registrar = {mRawPtr = 0x0}
	startupNotifier = {mRawPtr = 0x814ee10}
	cmdLineArgs = {mRawPtr = 0x821ea20}
	appShell = {mRawPtr = 0x81e2240}
	windowOpened = 1
	defaultStartup = 0
	remoteService = {mRawPtr = 0x8291af8}
#44 0x080676da in main (argc=4, argv=0xbffff794) at
/home/kaie/140/mozilla/xpfe/bootstrap/nsAppRunner.cpp:1647
	rv = 0
	nativeApp = (nsINativeAppSupport *) 0x814e6b8
	splash = (nsISplashScreen *) 0x0
	dosplash = 0
	remoterv = 0
	argused = 0
	mainResult = 134897544
#45 0x404511c4 in __libc_start_main () from /lib/libc.so.6

Comment 1

[Kai Engert [:KaiE:]]

The official Mozilla 1.3.1 release does not crash with the same certificate
database.

Comment 2

[Kai Engert [:KaiE:]]

Summary:

- Mozilla 1.3.1 works
- Mozilla 1.3.1 with recent NSS_3_7_BRANCH crashes
- Mozilla 1.4 crashes
- Mozilla trunk / 1.5a code crashes


I'm attaching a patch that skips a cert if node->cert == NULL is detected.
It masks the crash for me.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Patch

Comment 4

[Kai Engert [:KaiE:]]

As a final test, I applied this patch to the special Mozilla 1.3.1 build that
uses the latest snapshot of the NSS_3_7_BRANCH.

The patch also fixes the crash in that combination.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Except for the indentation (or lack thereof) this patch seems like a good
and proper change to make.  It is true that it masks the real cause of 
this problem, and we should not mark this bug fixed until we have fixed
the real cause.  But even when the real cause is fixed, this patch is 
appropriate and desirable, IMO (once the indentation is better).

The real cause is a node with a NULL cert pointer.  That shouldn't happen.
Some code that constructed this chain apparently failed to notice the NULL
cert pointer.  That should be found and fixed.  

Even that may be a penultimate cause.  Another question to ask is how/why
the null cert pointer arose in the first place.  My *guess* is that it was
caused by two different certs with the same issuer name and serial number
(the usual cause of problems with OpenSSL certs.)

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

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

Comment 7

[Calum Mackay]

It's amusing that only today Thawte have announced a problem with some of their
certs being issued with duplicate serial numbers. Probably just a coincidence...

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Ok, here's the story.  After the message signature has been parsed, the client
calls NSS_CMSSignedData_ImportCerts to import the signature's certs into the
token.  NSS_CMSSignedData_ImportCerts calls CERT_ImportCerts to import the 
entire collection of certs in one call.  

CERT_ImportCerts returns an array of cert pointers, which may have fewer 
members than the number of raw certs passed in.  The cert pointers in the
returned array may not correspond 1:1 with the input raw DER certs.  
If any of the raw certs cannot be imported for any reason, the returned 
list of pointers simply will not contain any pointer for that cert.  So, 
fewer non-NULL cert pointers may be returned than the number of raw DER certs
that were passed in.  The array of returned pointers will contain NULL values 
at the end of the array if any certs could not be imported.

NSS_CMSSignedData_ImportCerts constructs the linked list of CERTCertListNode 
nodes from the array of cert pointers returned by CERT_ImportCerts, and takes
no notice of the NULL pointer in the array.  Instead, it assumes that it gets
back as many non-NULL pointers as the number of raw certs it passed in.  
That's the first bug.

Then NSS_CMSSignedData_ImportCerts calls CERT_FilterCertListByUsage, which 
calls CERT_CheckKeyUsage, which calls  CERT_ExtractPublicKey, which calls
SECKEY_UpdateCertPQG, NONE of which check for NULL input arguments.  
They all should check.  That's bugs 2, 3, 4 and 5.  

Unfortunately, there's no way for the caller of CERT_ImportCerts to know the 
specific reason why any of the certs failed to import.  The API doesn't 
provide individual error codes for the different raw certs passed in.  

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

taking bug

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

In the case of bug 212945 (a duplicate of this bug) the cause of the null
cert pointer was that one of the certs had a validity date encoded as a
GeneralizedTime.  It was a date in the year 2053, so according to the RFC,
it was proper to encode it as a GeneralizedTime.  But NSS doesn't expect
GeneralizedTimes in certificates (which is bug 143334). 

In any case, the problem has little to do with the message being sent by 
any particular email client, and instead has to do with signatures containing
certs that are invalid or that NSS simply can't handle.  So, I'm changing the
Summary to reflect this.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch for NSS trunk, add missing NULL pointer checks

I expect there will be objections that this patch changes too much for 
the branches.  But I believe it is the right change for the trunk.

Comment 12

[Kai Engert [:KaiE:]]

Comment on attachment 128052 [details] [diff] [review]
patch for NSS trunk, add missing NULL pointer checks

r=kaie

Comment 13

[Wan-Teh Chang]

Nelson, was this bug unmasked by the fix for bug 193367
(NSS stores too many certs from an S/MIME message) in
NSS 3.7.2?

Kai, could you try backing out the fix for bug 193367
and see if that eliminates the crash?  You can do that
by reverting to rev. 1.15 of mozilla/security/nss/lib/smime/cmssigdata.c
(from either rev. 1.17 or rev. 1.15.30.2).

Comment 14

[Kai Engert [:KaiE:]]

I confirm removing the change from bug 193367 makes the crash go away.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: rest of patch (above patch was incomplete)

Before attaching the above patch, I changed 3 files, but for some reason, the
patch file above only shows the changes to two of them.  This patch includes 
the third part, which IMO is the most important part.  Sorry I didn't notice it
was missing before now.

This patch corrects the new code that was added to cmssigdata.c in rev 1.16 
to fix bug 193367.  Yes, this crash was a regression introduced by that change.

Comment 16

[Wan-Teh Chang]

Comment on attachment 128178 [details] [diff] [review]
rest of patch (above patch was incomplete)

>+	if (cert)
>+	    cert = CERT_DupCertificate(cert);
>+	if (cert)
>+	    CERT_AddCertToListTail(certList,cert);

Can these be combined into one "if (cert)" block?

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

With the present implementation of CERT_DupCertificate, we know that it will
always return the value it is passed as input, because all it does is bump a
ref count.  With a different implementation, it might conceivably return 
NULL, or some other value.  The question is: is the present behavior part of 
the definition of the function?  If so, then yes, we can get rid of the test.
Otherwise, if a future implementation could change this, then this code's
correctness depends on that test, I believe.

Comment 18

[Mike Kaply [:mkaply]]

Is there a smaller fix that should be done for the 1.4 branch, or do you want
this one?

Comment 19

[Kai Engert [:KaiE:]]

Nelson would a cleaned up version (indenting) of the initial patch be a
sufficient solution for the Mozilla 1.4 branch?

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

In the course of investigating this bug, we found numerous functions (all 
shown on the stack above) that used a NULL cert pointer argument without
checking it first.  The two patches above fix all of those functions.
The two patches above have been checked in on the NSS trunk for NSS 3.9.

If you would prefer to check in the smallest possible change that fixes 
only the one immediate cause of this crash, the above patch entitled 
"rest of patch" accomplishes that.  It corrects the regression that was 
directly responsible for the appearance of this particular crash. 

Comment 21

[Mike Kaply [:mkaply]]

a=mkaply to checkin the entire fix into 1.4.1

Comment 22

[Kai Engert [:KaiE:]]

The entire fix (attachm. 2 and 3) have been checked in to the 1.4 branch for 1.4.1

Comment 23

[Kai Engert [:KaiE:]]

Could you please add this fix (attachm. 2 and 3) to NSS_CLIENT_TAG for Mozilla 1.5b?

Comment 24

[Asa Dotzler [:asa]]

a=asa for checkin to Mozilla 1.5 beta. Can you get this in quickly?

Comment 25

[Wan-Teh Chang]

Fix checked into the NSS_3_8_BRANCH (NSS 3.8.2) and
NSS_CLIENT_TAG (Mozilla 1.5 Beta).