561577 - Softoken crashes when cert8.db file contains cert with invalid extension encodings [@ SECITEM_Hash ]

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Aaron Burgemeister]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100317 SUSE/3.5.9-0.1.1 Firefox/3.5.9
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100317 SUSE/3.5.9-0.1.1 Firefox/3.5.9

A coworker was having a problem getting to www.gmail.com with an error mentioned in various already-reported bugs (Bug# 410622, Bug# 435013, Bug# 435778).  While trying to find out which certificate may be bad through Firefox's interface the browser crashes (completely gone in a fraction of a second) when clicking on 'View Certificates' (Edit: Preferences: Advanced: Encryption: View Certificates).  This happened on her Linux box with Firefox 3.5.x and I can reproduce it on mine (FF 3.5.9 x86_64 on Linux) by copying out just her cert8.db and putting it in my profile directory leaving everything else the same.  I can also reproduce the sec_error_reused_issuer_and_serial error she saw by copying in this one database file.  As this is a non-trivial site (gmail.com) I'm interested in hearing why this error would occur for them.  Thawte should, obviously, know better than to reissue serial numbers so I assume something else is wrong.

This bug is specifically for the Firefox crashing issue (the browser shouldn't crash no matter how munged up the certificate file is).  If the cert8.db helps resolve other issues with the sec_error_reused_issuer_and_serial error then that's great, but at this point I cannot tell what the root cause of that is due to the Firefox bug until I get up the energy to open the file another way.

My apologies if I chose the wrong component.

Reproducible: Always

Steps to Reproduce:
1.  Copy attached cert8.db file into user's profile directory, overwriting previous version.
2.  Start Firefox.
3.  Edit: Preferences: Advanced: Encryption: View Certificates

Actual Results:  
Boom

Expected Results:  
No Boom.  Show a warning about a problem with the file, or auto-correct the error and let the user know, or recreate the database, or something else.

Comment 1

[Aaron Burgemeister]

Attachment: cert8.db file which leads to the crash within Firefox as described.

Comment 2

[Tyler Downer [He/Him]]

https://developer.mozilla.org/En/How_to_get_a_stacktrace_for_a_bug_report
Does this happen with an official build of firefox 3.6.3 or later?

Comment 3

[Aaron Burgemeister]

Yes, I just pulled down the .tar.bz2, extracted it, ran from there with the cert8.db (from above) copied in, and it crashed.  It also tried to submit a bug report which I didn't mean to do since I already had this bug open so sorry about that.

OpenSUSE 11.2 x86_64 with current patches for my testing, btw, though the coworker was on an earlier version.  I can try to reproduce on windows too if needed, though I'll need to find a windows box for that.

Comment 4

[Tyler Downer [He/Him]]

Did it try to submit a stacktrace? We need that. Please read the link in comment 2.

Comment 5

[Aaron Burgemeister]

Crash ID: bp-7cf9e83d-6519-4f06-9418-97bec2100425

Comment 6

[Aaron Burgemeister]

From a windows 2003 SP2 server (freshly updated to FF 3.6.3):

Crash ID: bp-dbac1e27-5866-47ad-992f-b98fe2100425

Comment 7

[timeless]

Signature	SECITEM_Hash
UUID	dbac1e27-5866-47ad-992f-b98fe2100425
Time 	2010-04-25 19:34:50.453804
Uptime	45
Product	Firefox
Version	3.6.3
Build ID	20100401080539
Branch	1.9.2
OS	Windows NT
OS Version	5.2.3790 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 15 model 2 stepping 7
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x5000000
User Comments	Bug# 561577, crashing with the same cert8.db from windows.
Processor Notes 	
Related Bugs

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	nssutil3.dll 	SECITEM_Hash 	security/nss/lib/util/secitem.c:304
1 	plds4.dll 	PL_HashTableLookupConst 	nsprpub/lib/ds/plhash.c:381
2 	nssutil3.dll 	SECOID_FindOID_Util 	security/nss/lib/util/secoid.c:1972
3 	nssutil3.dll 	SECOID_FindOIDTag_Util 	security/nss/lib/util/secoid.c:1988
4 	nssdbm3.dll 	nsslowcert_EmailAltName 	security/nss/lib/softoken/legacydb/lowcert.c:516
5 	nssdbm3.dll 	nsslowcert_GetCertificateEmailAddress 	security/nss/lib/softoken/legacydb/lowcert.c:590
6 	nssdbm3.dll 	nsslowcert_DecodeDERCertificate 	security/nss/lib/softoken/legacydb/lowcert.c:664
7 	nssdbm3.dll 	DecodeACert 	security/nss/lib/softoken/legacydb/pcertdb.c:4295
8 	nssdbm3.dll 	certcallback 	security/nss/lib/softoken/legacydb/pcertdb.c:4412
9 	nssdbm3.dll 	nsslowcert_TraverseDBEntries 	security/nss/lib/softoken/legacydb/pcertdb.c:4273
10 	nssdbm3.dll 	nsslowcert_TraversePermCerts 	security/nss/lib/softoken/legacydb/pcertdb.c:4464
11 	nssdbm3.dll 	lg_searchCertsAndTrust 	security/nss/lib/softoken/legacydb/lgfind.c:560
12 	nssdbm3.dll 	lg_searchTokenList 	security/nss/lib/softoken/legacydb/lgfind.c:843
13 	nssdbm3.dll 	lg_FindObjectsInit 	security/nss/lib/softoken/legacydb/lgfind.c:892
14 	softokn3.dll 	sftkdb_FindObjectsInit 	security/nss/lib/softoken/sftkdb.c:1220
15 	softokn3.dll 	sftk_searchDatabase 	security/nss/lib/softoken/pkcs11.c:4140
16 	softokn3.dll 	sftk_searchTokenList 	security/nss/lib/softoken/pkcs11.c:4265
17 	softokn3.dll 	NSC_FindObjectsInit 	security/nss/lib/softoken/pkcs11.c:4318
18 	nss3.dll 	nssToken_TraverseCertificates 	security/nss/lib/dev/devtoken.c:1545
19 	nss3.dll 	NSSTrustDomain_TraverseCertificates 	security/nss/lib/pki/trustdomain.c:1079
20 	nss3.dll 	PK11_ListCerts 	security/nss/lib/pk11wrap/pk11cert.c:2365
21 	xul.dll 	nsNSSCertCache::CacheAllCerts 	security/manager/ssl/src/nsNSSCertCache.cpp:88
22 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
23 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2722
24 	xul.dll 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1740
25 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1360
26 	js3250.dll 	js_Interpret 	js/src/jsops.cpp:2240
27 	js3250.dll 	js_Invoke 	js/src/jsinterp.cpp:1368
28 	js3250.dll 	js_InternalInvoke 	js/src/jsinterp.cpp:1423
29 	js3250.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5112
30 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:2134

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I can reproduce it with this cert DB.

The relevant portion of my stack is:

#0  0x0010a7d9 in SECITEM_Hash (key=0xbfffe9e0) at secitem.c:304
#1  0x0012a93f in PL_HashTableLookupConst (ht=0x404730, key=0xbfffe9e0) at ../../../lib/ds/plhash.c:381
#2  0x0010b5aa in SECOID_FindOID_Util (oid=0xbfffe9e0) at secoid.c:1972
#3  0x0010b5ed in SECOID_FindOIDTag_Util (oid=0xbfffe9e0) at secoid.c:1988
#4  0x006bef54 in nsslowcert_EmailAltName (cert=0x82f200, space=0x82f360 "", len=200) at lowcert.c:516
#5  0x006bf1cb in nsslowcert_GetCertificateEmailAddress (cert=0x82f200) at lowcert.c:589
#6  0x006bf3aa in nsslowcert_DecodeDERCertificate (derSignedCert=0x82e830, nickname=0x82e840 "DigiCert High Assurance CA-3") at lowcert.c:664
#7  0x006c6c72 in DecodeACert (handle=0x422f20, entry=0x82e810) at pcertdb.c:4295
#8  0x006c6f6c in certcallback (dbdata=0xbfffeb94, dbkey=0xbfffeb88, type=certDBEntryTypeCert, data=0xbfffebe0) at pcertdb.c:4412
#9  0x006c6bf4 in nsslowcert_TraverseDBEntries (handle=0x422f20, type=certDBEntryTypeCert, callback=0x6c6e96 <certcallback>, udata=0xbfffebe0) at pcertdb.c:4273
#10 0x006c700b in TraversePermCertsNoLocking (handle=0x422f20, certfunc=0x6bb732 <lg_cert_collect2>, udata=0xbfffec4c) at pcertdb.c:4445
#11 0x006c7041 in nsslowcert_TraversePermCerts (handle=0x422f20, certfunc=0x6bb732 <lg_cert_collect2>, udata=0xbfffec4c) at pcertdb.c:4464
#12 0x006bbb80 in lg_searchCertsAndTrust (sdb=0x423070, derCert=0xbfffed6c, name=0xbfffed54, derSubject=0xbfffed60, issuerSN=0xbfffed0c, email=0xbfffed48, classFlags=2, handles=0x426e70, pTemplate=0x426e60, ulCount=1) at lgfind.c:560
#13 0x006bc568 in lg_searchTokenList (sdb=0x423070, search=0x426e70, pTemplate=0x426e60, ulCount=1) at lgfind.c:841
#14 0x006bc6d3 in lg_FindObjectsInit (sdb=0x423070, pTemplate=0x426e60, ulCount=1, retSearch=0xbfffee80) at lgfind.c:892
#15 0x00595559 in sftkdb_FindObjectsInit (handle=0x423a90, template=0xbfffeed0, count=1, find=0xbfffee80) at sftkdb.c:1219

The crash occurrs in SECITEM_Hash (key=0xbfffe9e0) at secitem.c:304
304	        rvc[ i % sizeof(rv) ] ^= *data;
because data points to the beginning of an unmapped page.

SECItem_Hash was called with a SECItem containing these values:

$5 = {
  type = 0x1, 
  data = 0x82ee3d, 
  len = 0x50ea7389
}

At the time of crash, it has incremented data up to 0x010ab000, which means
it has read in 0x87c1c3 or 8,896,963 bytes from that cert, bytes which are 
ostensibly the OID value of the extension.  

Function nsslowcert_EmailAltName (seen in the stack) repeatedly calls 
function nsslowcert_dataStart to parse the cert in the DB, and doesn't 
sanity check the values it gets back.  I think this is a case where, 
if we used a real DER parser, such as our QuickDerDecoder, it might 
have caught this.  In any case, the bug is in those two functions.

I will attempt to pull the offending cert out of the DB.  From the stack 
backtrace, we can see that its nickname appears to be 
"DigiCert High Assurance CA-3" 
It may be that there is nothing wrong with this cert, and there is merely 
a bug in one of those nsslowcert_ functions named above.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I think someone is doing "security research" on NSS (read: trying to break it).
This cert DB contains two copies of an intermediate CA cert, one of which has
been (seemingly) carefully modified.  I will soon attach copies of both of them to this bug. The modified bytes include one byte of the serial number, the 
subject's public key, and several bytes within the encoded extensions have 
been changed.  All were changed to '?' (0x3f).  

The modified cert doesn't decode correctly.  Our QuickDER decoder would certainly not decode it without error, and so it's not clear to me how it 
got into our cert DB in the first place.  MAYBE this is a case of "cert DB corruption", although in the past, such cases generally resulted in much 
more gross corruption than this.  

There is definitely an NSS bug here, and it needs to be fixed.  I'll work on 
a patch ASAP.  

This DB can be repaired if the bug reporter desires that.  (Do you?)

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Attachment: The bad cert (binary DER), extracted from the cert8.db

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: The good cert (Binary DER) for comparison

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Further, it appears that this DB is empty, except for these certs, so 
I think no repair is necessary.  Agreed?

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v1 for NSS 3.13 branch

This patch makes nsslowcert_dataStart somewhat more robust in detecting 
invalid encodings, and makes two of its 26 callers similarly more robust.
Bob, please review.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Very strange.  With my patch applied, 
   certutil -L -d .
shows NO nicknames in this DB, but 
   certutil -L -n "DigiCert High Assurance CA-3" -a -d .
outputs one of the two intermediate CA certs.  
Offhand, I don't know HOW a DB can get into that state.
Corrupt DB index, I suppose.
I'll look at this DB with dbck when I have more time.

Comment 16

[Aaron Burgemeister]

I appreciate the offer for a fix to the DB, but no it is not necessary.  We already blew away the profile to get past it (before isolating just the cert8.db file later).  Thanks for your work on this.

On an unrelated sidenote if you have some tips/tricks you've used for analyzing this (tools, maybe, and steps used with them like dbck and certutil) I wouldn't mind learning those tricks.  Not critical obviously; just curious.

Comment 17

[Robert Relyea]

Comment on attachment 441622 [details] [diff] [review]
patch v1 for NSS 3.13 branch

r-  Well sort of.

The patch itself isn't wrong, It's just a lot more than necessary.

nsslowcert_dataStart already checks that the returned data length fits within the passed in buffer, so none of the extra checks in the second half of the patch is necessary.

The only truly necessary patch is the 

if (len_count > length)
     return NULL;

Though the code to remove returning a truncated length doesn't hurt.

I've verified that all callers of nsslowcert_dataStart() check for a NULL return except 2 places both in  nsslowcert_GetValidityFields. 

The first is derSN->data. I check probably be here to simplify reading the code, but it's not strictly necessary because because the following line does exactly the same call except it doesn't include the tag. Why it is safe it too sublte, so including the check is probably worth it.

The second is when loading the extensions->data. In that same function. Again, we should probably check, but the code checks extension.data for Non-NULL before it's used (which it needs to because extensions->data can be NULL legitimately -- no extensions included.


I did find a problem that it appears nsslowcert_DecodeDERCertificate doesn't check the return code from nsslowcert_GetFields!

bob

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

> The only truly necessary patch is the 
>
> if (len_count > length)
>     return NULL;

Ok, fair enough.  That WAS the part of the patch that caught the error 
that caused the crash.  And, BTW, that test has an "off by 1" error in it.
It should be:
  if (len_count + 1 > length)
to account for the fact that we've already fetched the tag byte.

I'll let you carry this from here.

Comment 19

[Robert Relyea]

Attachment: Tighten up overflow checks in nsslowcert_dataStart. Make sure errors are handled.

I'm not going to obsolute Nelson's patch. There are parts that we probably want to pick up in the future (like dropping the whole truncated length stuff).

Besides what I commented in in the review, I found 2 more areas where we need to check the length, plus another off-by-one error (well it's really the same one, but it was off by 2;).

bob

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch that actually sanity checks the extensions

Bob, your patch is good, as far as it goes, but it doesn't actually check 
any of the extension for valid lengths while its parsing the certificate.
The original code has a loop that appears to be for that purpose, but it 
doesn't work.  This patch is your patch plus some additional lines to fix 
that.  Please have a look at it.

Comment 21

[Robert Relyea]

Comment on attachment 441930 [details] [diff] [review]
patch that actually sanity checks the extensions

r+ rrelyea

Now that's a colaborative patch;).

bob

Comment 22

[Nelson Bolyard (seldom reads bugmail)]

Checking in lowcert.c; new revision: 1.5.4.2; previous revision: 1.5.4.1

Now, how are we really going to shake this out before we go into revalidation?