Closed Bug 668397 Opened 13 years ago Closed 6 years ago

Crash when verifying certificate chain containing Fortezza certificates

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX
3.13.2

People

(Reporter: taviso, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: crash, Whiteboard: Crash fixed in NSS 3.12.11 and 3.13)

Crash Data

Attachments

(9 files, 6 obsolete files)

384 bytes, application/octet-stream
Details
369 bytes, application/octet-stream
Details
335 bytes, application/octet-stream
Details
179.71 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
65.73 KB, patch
Details | Diff | Splinter Review
64.42 KB, patch
Details | Diff | Splinter Review
30.60 KB, patch
Details | Diff | Splinter Review
64.36 KB, patch
Details | Diff | Splinter Review
60.92 KB, patch
Details | Diff | Splinter Review
I thought it might be fun to flame conspiracy theories by studying the SkipJack and Fortezza support in NSS. I don't know the justification for adding support for Skipjack (the US government's attempt to push for mandatory key escrow, i.e. law enforcement backdoors in all crypto systems), because it was implemented before the Netscape source release in 1998, so there is no development log.

This was during a time when the US Government was terrified of widespread adoption of crypto, and were busy trying to reclassify it as a weapon. I imagine the thought of SSL being distributed in something as popular as Netscape would have seemed very scary, so I suspect they "suggested" SkipJack be implemented.

SkipJack was (understandably) a disaster, but bizarrely you're still shipping it along with Fortezza support in NSS. There are some very strange codepaths that looked worth investigating, but it turns out that it's been broken since at least 1998 (earliest I can check), so I can't reach any of the interesting code :-)

At least on platforms using NSS where NULL cannot be mapped, it can only cause a crash.

Here is bug 1:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/cryptohi/seckey.c&rev=1.61#943

	/* shared key */
	if (rawptr >= end) {
	    pubk->u.fortezza.DSSKey.len = pubk->u.fortezza.KEAKey.len;
	    /* this depends on the fact that we are going to get freed with an
	     * ArenaFree call. We cannot free DSSKey and KEAKey separately */
	    pubk->u.fortezza.DSSKey.data=
					pubk->u.fortezza.KEAKey.data;
	    pubk->u.fortezza.DSSprivilege.len = 
				pubk->u.fortezza.KEAprivilege.len;
	    pubk->u.fortezza.DSSprivilege.data =
			pubk->u.fortezza.DSSprivilege.data;
	    goto done;
	}
		
DSSprivilege.data was clearly intended to be set to KEAKey.data, but it's just set to itself, which is always NULL. So you have a NULL dereference here: 

    /* get the privilege mask */
    if (key->u.fortezza.DSSprivilege.len > 0) {
	priv = key->u.fortezza.DSSprivilege.data[0];
    }

Because len > 0, and data == NULL. See Example attatched.

You can bypass this bug by just appending the DSSPrivilege, however, then you run into this bug:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certhigh/certvfy.c&rev=1.72#445

    /*
     * make sure the CA's keys are OK
     */
            
    rv = SEC_CheckKRL(handle, key, NULL, t, wincx);
    SECKEY_DestroyPublicKey(key);
    if (rv != SECSuccess) {
	return rv;
    }

It is not legal to call SEC_CheckKRL() with a NULL rootCert, because then it does this:


    /* first look up the KRL */
    crl = SEC_FindCrlByName(handle,&rootCert->derSubject, SEC_KRL_TYPE);
    if (crl == NULL) {
	PORT_SetError(SEC_ERROR_NO_KRL);
	goto done;
    }

Which crashes during some hash table operation. See attached example.

As there is absolutely no way anybody could be using this code since prior to 1998 due to major implementation flaws, and nobody with any sanity could be using Skipjack since the late 90s, perhaps it's time to remove this attack surface?

Crash 1:

Program received signal SIGSEGV, Segmentation fault.
SECITEM_Hash (key=0x54) at secitem.c:303
303	    for( i = 0; i < item->len; i++ ) {
(gdb) bt
#0  SECITEM_Hash (key=0x54) at secitem.c:303
#1  0x4539c8b9 in PL_HashTableLookup (ht=0x8aa7b38, key=0x54) at ../../mozilla/nsprpub/lib/ds/plhash.c:364
#2  0x4598a322 in CRLCache_GetIssuerCache (returned=<synthetic pointer>, subject=0x54, cache=0x45a6c1d8) at crl.c:2425
#3  AcquireDPCache (issuer=0x0, subject=0x54, dp=0x0, t=0, wincx=0x0, dpcache=0xb3abec98, writeLocked=0xb3abec9c) at crl.c:2525
#4  0x4598b42d in SEC_FindCrlByName (handle=0x8b95d20, crlKey=0x54, type=0) at crl.c:2842
#5  0x459475af in SEC_CheckKRL (handle=0x8b95d20, key=0x822f470, rootCert=0x0, t=1309391844461397, wincx=0x8cd9c10) at certvfy.c:175
#6  0x459477b6 in cert_VerifyFortezzaV1Cert (handle=0x8b95d20, cert=<optimized out>, next_type=0xb3abedec, last_type=cbd_None, t=1309391844461397, wincx=0x8cd9c10) at certvfy.c:449
#7  0x45947d9b in cert_VerifyCertChainOld (revoked=0x0, log=0x0, wincx=0x8cd9c10, t=1309391844461397, certUsage=certUsageSSLServer, sigerror=0x0, checkSig=1, cert=0x8d3efc8, handle=0x8b95d20) at certvfy.c:572
#8  cert_VerifyCertChain (handle=0x8b95d20, cert=0x8d3efc8, checkSig=1, sigerror=0x0, certUsage=certUsageSSLServer, t=1309391844461397, wincx=0x8cd9c10, log=0x0, revoked=0x0) at certvfy.c:885
#9  0x459489c4 in CERT_VerifyCertChain (handle=0x8b95d20, cert=0x8d3efc8, checkSig=1, certUsage=certUsageSSLServer, t=1309391844461397, wincx=0x8cd9c10, log=0x0) at certvfy.c:894
#10 0x4594975b in CERT_VerifyCert (handle=0x8b95d20, cert=0x8d3efc8, checkSig=1, certUsage=certUsageSSLServer, t=1309391844461397, wincx=0x8cd9c10, log=0x0) at certvfy.c:1490
#11 0x45949971 in CERT_VerifyCertNow (handle=0x8b95d20, cert=0x8d3efc8, checkSig=1, certUsage=certUsageSSLServer, wincx=0x8cd9c10) at certvfy.c:1541
#12 0x45c0d8c5 in SSL_AuthCertificate (arg=0x8b95d20, fd=0x8ced8e8, checkSig=1, isServer=0) at sslauth.c:261
#13 0x00e809f7 in AuthCertificateCallback (client_data=0x0, fd=0x8ced8e8, checksig=1, isServer=0) at nsNSSCallbacks.cpp:1029
#14 0x45c0aa95 in ssl3_HandleCertificate (length=0, b=0x8cddff2 "\315\b", ss=<optimized out>) at ssl3con.c:7902
#15 ssl3_HandleHandshakeMessage (ss=<optimized out>, b=0xb3abf07c "\362\337\315\b", length=390) at ssl3con.c:8601
#16 0x45c0c188 in ssl3_HandleHandshake (origBuf=0x8ce8818, ss=0x8ce85c8) at ssl3con.c:8725
#17 ssl3_HandleRecord (ss=0x8ce85c8, cText=0xb3abf1f4, databuf=0x8ce8818) at ssl3con.c:9064
#18 0x45c0d198 in ssl3_GatherCompleteHandshake (ss=0x8ce85c8, flags=0) at ssl3gthr.c:209
#19 0x45c0ed91 in ssl_GatherRecord1stHandshake (ss=0x8ce85c8) at sslcon.c:1258
#20 0x45c162a5 in ssl_Do1stHandshake (ss=0x8ce85c8) at sslsecur.c:151
#21 0x45c17b40 in ssl_SecureSend (ss=0x8ce85c8, buf=0x8c61da0 "GET / HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0\r\nAccept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8\r\nAccept-Lang"..., len=328, flags=0) at sslsecur.c:1222
#22 0x45c17d44 in ssl_SecureWrite (ss=0x8ce85c8, buf=0x8c61da0 "GET / HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0\r\nAccept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8\r\nAccept-Lang"..., len=328) at sslsecur.c:1267
#23 0x45c1be85 in ssl_Write (fd=0x8ced8e8, buf=0x8c61da0, len=328) at sslsock.c:1654
#24 0x00e7d1ea in nsSSLThread::Run (this=0x84a9900) at nsSSLThread.cpp:1046
#25 0x00e7c753 in nsPSMBackgroundThread::nsThreadRunner (arg=0x84a9900) at nsPSMBackgroundThread.cpp:45
#26 0x45912c22 in _pt_root (arg=0x8c5c0f0) at ../../../mozilla/nsprpub/pr/src/pthreads/ptthread.c:187
#27 0x4457f9fe in start_thread (arg=0xb3abfb70) at pthread_create.c:305
#28 0x444c634e in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:133
(gdb) p item
$25 = (const SECItem *) 0x54

Note that offsetof(derSubject, rootCert) is 0x54.

Crash 2:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xb1f88b70 (LWP 14138)]
0x45947803 in cert_VerifyFortezzaV1Cert (handle=0x8b963e8, cert=<optimized out>, next_type=0xb1f87dec, last_type=cbd_None, t=1309391963127242, wincx=0x8c64ef0) at certvfy.c:442
442		priv = key->u.fortezza.DSSprivilege.data[0];
(gdb) bt
#0  0x45947803 in cert_VerifyFortezzaV1Cert (handle=0x8b963e8, cert=<optimized out>, next_type=0xb1f87dec, last_type=cbd_None, t=1309391963127242, wincx=0x8c64ef0) at certvfy.c:442
#1  0x45947d9b in cert_VerifyCertChainOld (revoked=0x0, log=0x0, wincx=0x8c64ef0, t=1309391963127242, certUsage=certUsageSSLServer, sigerror=0x0, checkSig=1, cert=0x8dd4bf0, handle=0x8b963e8) at certvfy.c:572
#2  cert_VerifyCertChain (handle=0x8b963e8, cert=0x8dd4bf0, checkSig=1, sigerror=0x0, certUsage=certUsageSSLServer, t=1309391963127242, wincx=0x8c64ef0, log=0x0, revoked=0x0) at certvfy.c:885
#3  0x459489c4 in CERT_VerifyCertChain (handle=0x8b963e8, cert=0x8dd4bf0, checkSig=1, certUsage=certUsageSSLServer, t=1309391963127242, wincx=0x8c64ef0, log=0x0) at certvfy.c:894
#4  0x4594975b in CERT_VerifyCert (handle=0x8b963e8, cert=0x8dd4bf0, checkSig=1, certUsage=certUsageSSLServer, t=1309391963127242, wincx=0x8c64ef0, log=0x0) at certvfy.c:1490
#5  0x45949971 in CERT_VerifyCertNow (handle=0x8b963e8, cert=0x8dd4bf0, checkSig=1, certUsage=certUsageSSLServer, wincx=0x8c64ef0) at certvfy.c:1541
#6  0x45c0d8c5 in SSL_AuthCertificate (arg=0x8b963e8, fd=0x8b88d80, checkSig=1, isServer=0) at sslauth.c:261
#7  0x00e809f7 in AuthCertificateCallback (client_data=0x0, fd=0x8b88d80, checksig=1, isServer=0) at nsNSSCallbacks.cpp:1029
#8  0x45c0aa95 in ssl3_HandleCertificate (length=0, b=0x8c95663 "", ss=<optimized out>) at ssl3con.c:7902
#9  ssl3_HandleHandshakeMessage (ss=<optimized out>, b=0xb1f8807c "cV\311\b", length=375) at ssl3con.c:8601
#10 0x45c0c188 in ssl3_HandleHandshake (origBuf=0x8ccc7b8, ss=0x8ccc568) at ssl3con.c:8725
#11 ssl3_HandleRecord (ss=0x8ccc568, cText=0xb1f881f4, databuf=0x8ccc7b8) at ssl3con.c:9064
#12 0x45c0d198 in ssl3_GatherCompleteHandshake (ss=0x8ccc568, flags=0) at ssl3gthr.c:209
#13 0x45c0ed91 in ssl_GatherRecord1stHandshake (ss=0x8ccc568) at sslcon.c:1258
#14 0x45c162a5 in ssl_Do1stHandshake (ss=0x8ccc568) at sslsecur.c:151
#15 0x45c17b40 in ssl_SecureSend (ss=0x8ccc568, buf=0x8457a10 "GET / HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0\r\nAccept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8\r\nAccept-Lang"..., len=328, flags=0) at sslsecur.c:1222
#16 0x45c17d44 in ssl_SecureWrite (ss=0x8ccc568, buf=0x8457a10 "GET / HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0\r\nAccept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8\r\nAccept-Lang"..., len=328) at sslsecur.c:1267
#17 0x45c1be85 in ssl_Write (fd=0x8b88d80, buf=0x8457a10, len=328) at sslsock.c:1654
#18 0x00e7d1ea in nsSSLThread::Run (this=0x84fb3d8) at nsSSLThread.cpp:1046
#19 0x00e7c753 in nsPSMBackgroundThread::nsThreadRunner (arg=0x84fb3d8) at nsPSMBackgroundThread.cpp:45
#20 0x45912c22 in _pt_root (arg=0x8c5c348) at ../../../mozilla/nsprpub/pr/src/pthreads/ptthread.c:187
#21 0x4457f9fe in start_thread (arg=0xb1f88b70) at pthread_create.c:305
#22 0x444c634e in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:133
(gdb) p key->u.fortezza.DSSprivilege.data
There is no member named DSSprivilege.
(gdb) wut
Undefined command: "wut".  Try "help".
(gdb) list keythi.h:177
172	    SECItem DSSKey;
173	    SECKEYPQGParams params;
174	    SECKEYPQGParams keaParams;
175	};
176	typedef struct SECKEYFortezzaPublicKeyStr SECKEYFortezzaPublicKey;
177	#define KEAprivilege KEApriviledge /* corrected spelling */
178	#define DSSprivilege DSSpriviledge /* corrected spelling */
179	
180	struct SECKEYDiffPQGParamsStr {
181	    SECKEYPQGParams DiffKEAParams;
(gdb) lol
Undefined command: "lol".  Try "help".
(gdb) p key->u.fortezza.DSSpriviledge.data
$26 = (unsigned char *) 0x0
(gdb) 

I've been able to create Fortezza certificates that demonstrate these flaws on a default firefox installation.
Keywords: crash
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xb2efeb70 (LWP 8817)]
SECKEY_FortezzaDecodeCertKey (arena=0xac1989d0, pubk=0xac1aa010, rawkey=0xb2efdd28, params=0xac1a90f4) at seckey.c:859
859		pubk->u.fortezza.KEAversion = *rawptr++;
(gdb) bt
#0  SECKEY_FortezzaDecodeCertKey (arena=0xac1989d0, pubk=0xac1aa010, rawkey=0xb2efdd28, params=0xac1a90f4) at seckey.c:859
#1  0x0697bf16 in seckey_ExtractPublicKey (spki=<value optimized out>) at seckey.c:1102
#2  0x069b07f3 in cert_GetKeyID (cert=0xac1a9010) at certdb.c:751
#3  0x069b26fd in CERT_DecodeDERCertificate (derSignedCert=0xb2efde24, copyDER=1, nickname=0x0) at certdb.c:931
#4  0x069c791d in nssDecodedPKIXCertificate_Create (arenaOpt=0x0, encoding=0xac19f078) at pki3hack.c:483
#5  0x069c7f3b in stan_GetCERTCertificate (c=0xac19f048, forceUpdate=<value optimized out>) at pki3hack.c:834
#6  0x069bafbb in CERT_NewTempCertificate (handle=0xb3741018, derCert=0xb2efdfe8, nickname=0x0, isperm=0, copyDER=1) at stanpcertdb.c:439
#7  0x068da963 in ssl3_HandleCertificate (ss=0xadbf8000, b=0xb2efdff4 "", length=341) at ssl3con.c:7848
#8  ssl3_HandleHandshakeMessage (ss=0xadbf8000, b=0xb2efdff4 "", length=341) at ssl3con.c:8601
#9  0x068dd4d7 in ssl3_HandleHandshake (ss=0xadbf8000, cText=0xb2efe1b4, databuf=0xadbf8250) at ssl3con.c:8725
#10 ssl3_HandleRecord (ss=0xadbf8000, cText=0xb2efe1b4, databuf=0xadbf8250) at ssl3con.c:9064
#11 0x068de680 in ssl3_GatherCompleteHandshake (ss=0xadbf8000, flags=0) at ssl3gthr.c:209
#12 0x068dfe8c in ssl_GatherRecord1stHandshake (ss=0xadbf8000) at sslcon.c:1258
#13 0x068e7cf5 in ssl_Do1stHandshake (ss=0xadbf8000) at sslsecur.c:151
#14 0x068e9567 in ssl_SecureSend (ss=0xadbf8000, buf=0xaddcc600 "GET / HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.6.17-1.fc14 Firefox/3.6.17\r\nAccept: text/html,application/xhtml+xml,appli"..., len=383, flags=0) at sslsecur.c:1222
#15 0x068e9764 in ssl_SecureWrite (ss=0xadbf8000, buf=0xaddcc600 "GET / HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.17) Gecko/20110428 Fedora/3.6.17-1.fc14 Firefox/3.6.17\r\nAccept: text/html,application/xhtml+xml,appli"..., len=383) at sslsecur.c:1267
#16 0x068ed7a7 in ssl_Write (fd=0xadbdea60, buf=0xaddcc600, len=383) at sslsock.c:1654
#17 0x042d0342 in nsSSLThread::Run (this=0xb373f700) at nsSSLThread.cpp:1045
#18 0x042cf84f in nsPSMBackgroundThread::nsThreadRunner (arg=0xb373f700) at nsPSMBackgroundThread.cpp:44
#19 0x067e7965 in _pt_root (arg=0xb2477030) at ../../../mozilla/nsprpub/pr/src/pthreads/ptthread.c:187
#20 0x00bd6e99 in start_thread (arg=0xb2efeb70) at pthread_create.c:301
#21 0x00b1cd2e in clone () at ../sysdeps/unix/sysv/linux/i386/clone.S:133
IIUC, NSS team has no resources to remove Fortezza code. See bug 327664 comment 4, bug 239960, etc.
I don't think there is much choice, if the chosen solution is to just fix the crashes, then it will expose some very suspicious codepaths into the very sensitive SSL certificate chain verification code.

So the options are:

- Leave the crashes, and let servers terminate clients forever.
- Just fix the crashes, thus allowing active attackers to defeat SSL.
- Remove Fortezza support, speeding up and simplifying a very sensitive and hot codepath.

The choice seems obvious :-)
I propose that we finish the work in bug 239960 to resolve this bug. It seems easier to do that than analyze all Fortezza code paths. I am cleaning up the patch for doing so now.
Assignee: nobody → bsmith
Summary: Remove Skipjack and Fortezza Support → Crash when verifying certificate chain containing Fortezza certificates
Because one of the crashes was in cryptohi, I decided to remove the fortezza/kea members from SECKEYPublicKey. Doing so necessitated the large number of changes in this patch as there were many dependencies on these members. I think it is better to bite the bullet and remove all of this code to ensure we haven't overlooked any other possible crash sites or other bugs.

The patch consists almost entirely of code deletion and comment rewriting. Some things to pay attention to:

* I removed CERT_KMIDPublicKey. It was declared in a public header file, but it was never exported in the .def file.

* In PK11_PubDerive, the case for dsaKey looked wrong. In particular, it was referencing pubKey->u.fortezza.KEAKey instead of the u.dsa members. I assume then that PK11_PubDerive isn't used for DSA keys. If it is, then we should file a follow-up bug to implement the dsaKey case correctly.

* In NSS_CMSUtil_EncryptSymKey_ESDH, all the code I removed is #if 0'd out. I removed the code because it was a copy/paste of Fortezza code. I filed another bug about cleaning the rest of this up: Bug 671060
Attachment #545464 - Flags: review?(rrelyea)
Attachment #545464 - Flags: feedback?(wtc)
Attachment #545464 - Flags: feedback?(alvolkov.bgs)
Comment on attachment 545464 [details] [diff] [review]
[patch v1] Remove all fortezza support except for Softoken and FreeBL

r+ except the changes to cryptohi/keythi.h.

Unfortunately the public key structure is a public structure, so we need to keep the deprecated definitions.

Everything else looks good,

bob
Attachment #545464 - Flags: review?(rrelyea) → review+
Comment on the conspiracy theory;).

I added FORTEZZA support to NSS before it was even called NSS (back when it was libsec). The purpose of the FORTEZZA support was to sell products to the U.S. Government, who had put a lot of resources trying to make FORTEZZA and SKIPJACK the next encryption standards. The initial version was ifdef's in NSS itself and called directly into the FORTEZZA library provided by the government FORTEZZA vendors.

It became obvious the the government that getting one off versions of Netscape products was neither sustainable nor was likely to get FORTEZZA to catch one, so we added PKCS #11 support and used PKCS #11 to access FORTEZZA. This didn't help FORTEZZA much, but it gave the early PKCS #11 specs a boost.

FORTEZZA's last gasp was when the government release the SKIPJACK spec so that you could implement FORTEZZA is software. We implemented a software version that went with the our hardware FORTEZZA driver. If you really cared about the actual SKIPJACK implementation, It's still in the cvs repository. With the spec released, it was not long until Shamir broke a reduced round version (reduced by only a few rounds). That was pretty much the end of FORTEZZA.

From that time on, the code pretty much rotted, so what you found here is mostly neglect, certainly not some sort of government conspiracy;).

This patch should be the next to last step of expunging the FORTEZZA code from NSS (we mostly ejected it from SSL a quite a while ago).


bob
Comment on attachment 545464 [details] [diff] [review]
[patch v1] Remove all fortezza support except for Softoken and FreeBL

In secmodti.h you remove a member from struct PK11Context. This type is used in multiple NSS modules.

Are we worried about scenarios where users combine e.g. [nss3.dll <= 3.12.10] with [ssl3.dll >= 3.12.11] , and the libraries have different expectations of structure size?

Could the removal of the member cause any other binary compatibility issues?

If yes, should the member be kept, and renamed to "not_in_use_was_fortezza"?
Attachment #545464 - Attachment description: Remove all fortezza support except for Softoken and FreeBL → [patch v1] Remove all fortezza support except for Softoken and FreeBL
Attached patch Patch v2 (obsolete) — Splinter Review
This is a version of Brian's patch, with most of Brian's changes to cryptohi/keythi.h removed - in other words - it keeps most of the existing cryptohi/keythi.h as is.

However, I kept Brian's /* deprecated */ comment in that header.


I believe this is what Bob gave r+ to.
(In reply to comment #10)
> In secmodti.h you remove a member from struct PK11Context. This type is used
> in multiple NSS modules.
> 
> Are we worried about scenarios where users combine e.g. [nss3.dll <=
> 3.12.10] with [ssl3.dll >= 3.12.11] , and the libraries have different
> expectations of structure size?

Withing NSS, PK11Contexts are only ever allocated by pk11wrap, so removing it seems OK to me. Note that secmodti.h is not exported so code outside of NSS cannot depend on these members.

Thank you for fixing my change to keythi.h.
Attachment #546207 - Attachment is obsolete: true
Attached patch Patch v3 (obsolete) — Splinter Review
I have reviewed everything in patch v2 except the changes to
lib/pkcs7/secmime.c and lib/smime/*.

1. I have made some cosmetic changes.  You can review my changes
with patch interdiffs.

2. I found only one minor bug: we cannot blindly remove a case
from a switch statement if the switch statement doesn't have a
default case.  I saw two switch statements that list all the
key types exhaustively, so simply removing the fortezzaKey
and keaKey cases could introduce unhandled cases.  At least
some compilers may warn about that.

3. Brian, you removed the dhKey (Diffie-Hellman key) case from
PK11_ImportPublicKey.  Is that intentional?

4. pkcs7t.h is an exported header, so it is risky to remove type
definitions from the header.  We can give it a try.

5. Bob, is the nullKey case in PK11_PubDeriveWithKDF correct?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11skey.c&rev=1.122&mark=1898,1905,1907,1912-1913,1918#1897

Why isn't nullKey handled as an error?

6. Bob, can we remove PK11_SaveContext and PK11_RestoreContext?  The
comments say they're required to make FORTEZZA work.  I wonder if they
have since found new uses.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11cxt.c&rev=1.8&mark=490-491,494#489
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11cxt.c&rev=1.8&mark=553-554,557#552

7. The "deprecated" comment for the PK11Origin enum type requires
more explanation.  I verified that all the PK11Origin fields will
be ignored after this patch, but many public functions take a
PK11Origin argument, so people who write new code still need guidance
on what PK11Origin values should be passed to those functions.
> 3. Brian, you removed the dhKey (Diffie-Hellman key) case from
> PK11_ImportPublicKey.  Is that intentional?

No, I did not intend to do so. 

> 6. Bob, can we remove PK11_SaveContext and PK11_RestoreContext?  The
> comments say they're required to make FORTEZZA work.  I wonder if they
> have since found new uses.

These are exported from the library. Also, PK11_SaveContextAlloc and PK11_RestoreContext are used by libssl.
> 5. Bob, is the nullKey case in PK11_PubDeriveWithKDF correct?

I checked this before. The nullKey case will call PK11_PubDerive, which will report an error.  I filed did notice bug 672828 though.
I added a comment to the public functions that take a PK11Origin parameter.
I added back the mistakenly-deleted code for DH key import and the inappropriately-deleted type definitions in pkcs7t.h.
Attachment #545464 - Attachment is obsolete: true
Attachment #547100 - Attachment is obsolete: true
Attachment #547111 - Attachment is obsolete: true
Attachment #547187 - Flags: review?(rrelyea)
Attachment #545464 - Flags: feedback?(wtc)
Attachment #545464 - Flags: feedback?(alvolkov.bgs)
This is a much smaller patch that fixes all the
problems Tavis reported.  Only lib/cryptohi/seckey.c
(the decoding of public key and PQG parameters) and
lib/certhigh/certvfy.c (certificate verification)
are modified.

It would be more appropriate for the NSS_3_12_BRANCH
and much easier for NSS package owners in Linux
distributions to review.
Attachment #547571 - Flags: review?(bsmith)
(In reply to comment #19)
> 
> It would be more appropriate for the NSS_3_12_BRANCH
> and much easier for NSS package owners in Linux
> distributions to review.
I am going to review and check in Brian's patch in
three parts.  Here is part one of Brian's patch v5,
provided here for patch interdiffs with what I checked
in.
Brian, when you update your NSS source tree, you will get
a merge conflict in pk11cert.c.  Just delete the
KEAPQGCompare function to resolve the merge conflict.

I made some changes to Brian's patch.

1. I restored the ability to generate SEC_OID_MISSI_DSS
signatures with DSA keys.  SEC_OID_MISSI_DSS signatures
are not DER encoded.  Bug 561598 shows the unencoded format
can be useful.

This requires reverting Brian's changes to seckey_GetKeyType
and sec_DecodeSigAlg so that they can handle SEC_OID_MISSI_DSS,
etc., and reverting the changes to SGN_NewContext so that it
allows using DSA keys for SEC_OID_MISSI_DSS signatures.

2. Call PORT_SetError in the 'default' cases that return a
failure status.

3. I adjusted the comments related to Fortezza's wrapped
format of PQG parameters to better match the code.
SECKEY_DSADecodePQG still has a check for Fortezza's
non-standard format.  When we delete that check, we can
also delete the comment mentioning Fortezza's wrapped
format.

Patch checked in on the NSS trunk (NSS 3.13).

Checking in lib/certdb/cert.h;
/cvsroot/mozilla/security/nss/lib/certdb/cert.h,v  <--  cert.h
new revision: 1.86; previous revision: 1.85
done
Checking in lib/certdb/certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.114; previous revision: 1.113
done
Checking in lib/certdb/crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.72; previous revision: 1.71
done
Checking in lib/certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.74; previous revision: 1.73
done
Checking in lib/cryptohi/keyhi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keyhi.h,v  <--  keyhi.h
new revision: 1.18; previous revision: 1.17
done
Checking in lib/cryptohi/keythi.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/keythi.h,v  <--  keythi.h
new revision: 1.15; previous revision: 1.14
done
Checking in lib/cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.62; previous revision: 1.61
done
Checking in lib/cryptohi/secsign.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/secsign.c,v  <--  secsign.c
new revision: 1.26; previous revision: 1.25
done
Checking in lib/pk11wrap/pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.179; previous revision: 1.178
done
Target Milestone: --- → 3.13
Comment on attachment 547571 [details] [diff] [review]
Smaller patch for NSS_3_12_BRANCH

Elio: if you think NSS package owners are likely to apply this
patch to NSS 3.12.x, please review this patch for NSS_3_12_BRANCH.
Otherwise we'll just fix the bug in NSS 3.13.  Thanks.
Attachment #547571 - Flags: superreview?(emaldona)
(In reply to comment #23)
As the NSS packager owner in Fedora would rebase to 3.12.11. The thing is that at this point in time would have to do it in the Rawhide (the devel branch) only. Fedora-16 is about to enter the Alpha stage where rebases aren't allowed - unless there is a compelling reason such as security fixes and we don't have any. That forces me to pospone rebasing on the stable brances as well. On top of that we don't know if NSS 3.13 could end up coming out in August 16 (so FF7 picks it) or later in the year.  I could very well end up skipping 3.12.11 and going straight to 3.13.
Comment on attachment 547193 [details] [diff] [review]
Patch v5: Remove all fortezza support except for Softoken and FreeBL

r+, though I think wtc has already checked it in.

bob
Attachment #547193 - Flags: review?(rrelyea) → review+
Comment on attachment 547193 [details] [diff] [review]
Patch v5: Remove all fortezza support except for Softoken and FreeBL

Bob: I plan to check in this patch in three installments.
I have only checked in one installment.  Two more to go.
I checked in the smaller patch on the NSS_3_12_BRANCH (NSS 3.12.11).

Checking in certhigh/certvfy.c;
/cvsroot/mozilla/security/nss/lib/certhigh/certvfy.c,v  <--  certvfy.c
new revision: 1.71.2.1; previous revision: 1.71
done
Checking in cryptohi/seckey.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/seckey.c,v  <--  seckey.c
new revision: 1.54.2.4; previous revision: 1.54.2.3
done
Attachment #547571 - Attachment is obsolete: true
Attachment #547571 - Flags: superreview?(emaldona)
Attachment #547571 - Flags: review?(bsmith)
Here is part two of Brian's patch v5, provided here for
patch interdiffs with what I checked in.
I made some changes to Brian's patch.

1. I removed the obsolete type definitions from the
public headers lib/pkcs7/pkcs7t.h and lib/smime/cmst.h.
They were only used by internal functions that have
been removed.  They should not have been defined in
public headers.

2. I removed the includeFortezzaCiphers argument from
the NSS_SMIMEUtil_CreateSMIMECapabilities function,
which is not exported.

3. I didn't remove the commented-out incomplete implementation
of Ephemeral Static Diffie-Hellman because there is another
bug for the removal of that code, and because I think we should
simply remove those two functions.

4. Other minor changes.

Patch checked in on the NSS trunk (NSS 3.13).

Checking in lib/pkcs7/p7decode.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/p7decode.c,v  <--  p7decode.c
new revision: 1.26; previous revision: 1.25
done
Checking in lib/pkcs7/p7encode.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/p7encode.c,v  <--  p7encode.c
new revision: 1.14; previous revision: 1.13
done
Checking in lib/pkcs7/p7local.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/p7local.c,v  <--  p7local.c
new revision: 1.15; previous revision: 1.14
done
Checking in lib/pkcs7/p7local.h;
/cvsroot/mozilla/security/nss/lib/pkcs7/p7local.h,v  <--  p7local.h
new revision: 1.3; previous revision: 1.2
done
Checking in lib/pkcs7/pkcs7t.h;
/cvsroot/mozilla/security/nss/lib/pkcs7/pkcs7t.h,v  <--  pkcs7t.h
new revision: 1.7; previous revision: 1.6
done
Checking in lib/pkcs7/secmime.c;
/cvsroot/mozilla/security/nss/lib/pkcs7/secmime.c,v  <--  secmime.c
new revision: 1.5; previous revision: 1.4
done
Checking in lib/smime/cmsasn1.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsasn1.c,v  <--  cmsasn1.c
new revision: 1.10; previous revision: 1.9
done
Checking in lib/smime/cmsencode.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsencode.c,v  <--  cmsencode.c
new revision: 1.12; previous revision: 1.11
done
Checking in lib/smime/cmslocal.h;
/cvsroot/mozilla/security/nss/lib/smime/cmslocal.h,v  <--  cmslocal.h
new revision: 1.7; previous revision: 1.6
done
Checking in lib/smime/cmspubkey.c;
/cvsroot/mozilla/security/nss/lib/smime/cmspubkey.c,v  <--  cmspubkey.c
new revision: 1.8; previous revision: 1.7
done
Checking in lib/smime/cmsrecinfo.c;
/cvsroot/mozilla/security/nss/lib/smime/cmsrecinfo.c,v  <--  cmsrecinfo.c
new revision: 1.21; previous revision: 1.20
done
Checking in lib/smime/cmssiginfo.c;
/cvsroot/mozilla/security/nss/lib/smime/cmssiginfo.c,v  <--  cmssiginfo.c
new revision: 1.35; previous revision: 1.34
done
Checking in lib/smime/cmst.h;
/cvsroot/mozilla/security/nss/lib/smime/cmst.h,v  <--  cmst.h
new revision: 1.14; previous revision: 1.13
done
Checking in lib/smime/smime.h;
/cvsroot/mozilla/security/nss/lib/smime/smime.h,v  <--  smime.h
new revision: 1.11; previous revision: 1.10
done
Checking in lib/smime/smimeutil.c;
/cvsroot/mozilla/security/nss/lib/smime/smimeutil.c,v  <--  smimeutil.c
new revision: 1.22; previous revision: 1.21
done
Checking in tests/ssl/sslcov.txt;
/cvsroot/mozilla/security/nss/tests/ssl/sslcov.txt,v  <--  sslcov.txt
new revision: 1.15; previous revision: 1.14
done
The crashes have been fixed in NSS 3.12.11 and 3.13.

Unfortunately we are using this bug report for the removal of
all obsolete Fortezza code, and that work isn't done yet (my
fault).
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: Crash fixed in NSS 3.12.11 and 3.13
Target Milestone: 3.13 → 3.13.1
Target Milestone: 3.13.1 → 3.13.2
Severity: major → critical
Crash Signature: [@ cert_VerifyFortezzaV1Cert]
Assignee: brian → nobody
Closing because no crash reported since 12 weeks.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.