95987 - Crash visiting https server that implements DHE ciphersuites

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[(Don't use, old account) - Kai Engert]

I'm using the source code from Friday.

Above URL requests for client authentication, but does not require it.

To make sure this bug isn't influenced by corrupt profiles or security database
files, I started with a clean profile, removing both Netscape Communicator and
Netscape 6 profiles.

To reproduce, make sure you have at least one certificate imported.
I can reproduce it every time.

To see the crash, just access the URL. Ignore the CA warning. The browser does
still crash if the CA is known. In case you want to see it, the CA cert is
available at http://www.kuix.de/ca

Here is the call stack. I looked at the variable contents that are checked by
the assertion code. These variables always have the same contents.

data->len == 128
blockType == 2 (RSA_BlockPublic)
modulusLen == 64
RSA_BLOCK_MIN_PAD_LEN == 8 

assert( 128 <= (64 - 3 + 8) );


NTDLL! 778a018c()
RSA_FormatBlock(SECItemStr * 0x0187f584, unsigned int 64, int 2, SECItemStr *
0x0187f570) line 371 + 44 bytes
RSA_EncryptBlock(SECKEYLowPublicKeyStr * 0x047604e8, unsigned char * 0x03dbb750,
unsigned int * 0x0187f5d0, unsigned int 64, unsigned char * 0x0397f9c8, unsigned
int 128) line 789 + 19 bytes
NSC_Encrypt(unsigned long 2147483651, unsigned char * 0x0397f9c8, unsigned long
128, unsigned char * 0x03dbb750, unsigned long * 0x0187f63c) line 911 + 33 bytes
NSC_WrapKey(unsigned long 2147483651, CK_MECHANISM * 0x0187f640, unsigned long
7, unsigned long 6, unsigned char * 0x03dbb750, unsigned long * 0x0187f63c) line
3720 + 31 bytes
PK11_PubWrapSymKey(unsigned long 1, SECKEYPublicKeyStr * 0x0477eed8,
PK11SymKeyStr * 0x047701b8, SECItemStr * 0x0187f6c4) line 2124 + 41 bytes
pk11_KeyExchange(PK11SlotInfoStr * 0x011fd6b8, unsigned long 2147484533,
unsigned long 268, PK11SymKeyStr * 0x047701b8) line 134 + 19 bytes
pk11_CopyToSlot(PK11SlotInfoStr * 0x011fd6b8, unsigned long 2147484533, unsigned
long 268, PK11SymKeyStr * 0x047701b8) line 1169 + 21 bytes
pk11_DeriveWithTemplate(PK11SymKeyStr * 0x047701b8, unsigned long 2147484533,
SECItemStr * 0x0187f9b8, unsigned long 2147484530, unsigned long 268, int 0,
CK_ATTRIBUTE * 0x0187f858, unsigned int 2) line 2429 + 25 bytes
PK11_DeriveWithFlags(PK11SymKeyStr * 0x047701b8, unsigned long 2147484533,
SECItemStr * 0x0187f9b8, unsigned long 2147484530, unsigned long 268, int 0,
unsigned long 10240) line 2354 + 40 bytes
ssl3_GenerateSessionKeys(sslSocketStr * 0x0390bc78, const PK11SymKeyStr *
0x047701b8) line 1961 + 32 bytes
ssl3_InitPendingCipherSpec(sslSocketStr * 0x0390bc78, PK11SymKeyStr *
0x047701b8) line 1045 + 13 bytes
sendDHClientKeyExchange(sslSocketStr * 0x0390bc78, SECKEYPublicKeyStr *
0x03da6058) line 3238 + 13 bytes
ssl3_SendClientKeyExchange(sslSocketStr * 0x0390bc78) line 3750 + 13 bytes
ssl3_HandleServerHelloDone(sslSocketStr * 0x0390bc78) line 4623 + 9 bytes
ssl3_HandleHandshakeMessage(sslSocketStr * 0x0390bc78, unsigned char *
0x03db4007, unsigned int 0) line 7027 + 9 bytes
ssl3_HandleHandshake(sslSocketStr * 0x0390bc78, sslBufferStr * 0x03935cd4) line
7114 + 25 bytes
ssl3_HandleRecord(sslSocketStr * 0x0390bc78, SSL3Ciphertext * 0x0187fc0c,
sslBufferStr * 0x03935cd4) line 7379 + 13 bytes
ssl3_GatherCompleteHandshake(sslSocketStr * 0x0390bc78, int 0) line 204 + 20 bytes
ssl3_GatherAppDataRecord(sslSocketStr * 0x0390bc78, int 0) line 234 + 13 bytes
DoRecv(sslSocketStr * 0x0390bc78, unsigned char * 0x03c74ee8, int 4096, int 0)
line 515 + 11 bytes
ssl_SecureRecv(sslSocketStr * 0x0390bc78, unsigned char * 0x03c74ee8, int 4096,
int 0) line 1024 + 21 bytes
ssl_SecureRead(sslSocketStr * 0x0390bc78, unsigned char * 0x03c74ee8, int 4096)
line 1033 + 19 bytes
ssl_Read(PRFileDesc * 0x038eb110, void * 0x03c74ee8, int 4096) line 1212 + 21 bytes
nsSSLIOLayerRead(PRFileDesc * 0x03d54ac8, void * 0x03c74ee8, int 4096) line 631
+ 26 bytes
PR_Read(PRFileDesc * 0x03d54ac8, void * 0x03c74ee8, int 4096) line 136 + 20 bytes
nsSocketIS::Read(nsSocketIS * const 0x038ee108, char * 0x03c74ee8, unsigned int
4096, unsigned int * 0x0187fd90) line 2284 + 21 bytes
nsHttpTransaction::Read(nsHttpTransaction * const 0x03980894, char * 0x03c74ee8,
unsigned int 4096, unsigned int * 0x0187fd90) line 730 + 38 bytes
nsReadFromInputStream(nsIOutputStream * 0x03c70d4c, void * 0x03980894, char *
0x03c74ee8, unsigned int 0, unsigned int 4096, unsigned int * 0x0187fd90) line 831
nsPipe::nsPipeOutputStream::WriteSegments(nsPipe::nsPipeOutputStream * const
0x03c70d4c, unsigned int (nsIOutputStream *, void *, char *, unsigned int,
unsigned int, unsigned int *)* 0x1005c700 nsReadFromInputStream(nsIOutputStream
*, void *, char *, unsigned int, unsigned int, unsigned int *), void *
0x03980894, unsigned int 16384, unsigned int * 0x0187fe28) line 704 + 29 bytes
nsPipe::nsPipeOutputStream::WriteFrom(nsPipe::nsPipeOutputStream * const
0x03c70d4c, nsIInputStream * 0x03980894, unsigned int 16384, unsigned int *
0x0187fe28) line 839
nsStreamListenerProxy::OnDataAvailable(nsStreamListenerProxy * const 0x03c70ab8,
nsIRequest * 0x03980890, nsISupports * 0x00000000, nsIInputStream * 0x03980894,
unsigned int 0, unsigned int 16384) line 288 + 38 bytes
nsHttpTransaction::OnDataReadable(nsIInputStream * 0x038ee108) line 178 + 72 bytes
nsHttpConnection::OnDataAvailable(nsHttpConnection * const 0x02c86d08,
nsIRequest * 0x02c87868, nsISupports * 0x00000000, nsIInputStream * 0x038ee108,
unsigned int 0, unsigned int 8192) line 698 + 15 bytes
nsSocketReadRequest::OnRead() line 2729 + 57 bytes
nsSocketTransport::doReadWrite(short 1) line 999 + 14 bytes
nsSocketTransport::Process(short 1) line 480 + 13 bytes
nsSocketTransportService::Run(nsSocketTransportService * const 0x0115c7e4) line
458 + 13 bytes
nsThread::Main(void * 0x0112f550) line 105 + 26 bytes
_PR_NativeRunThread(void * 0x0110b7c8) line 413 + 13 bytes
_threadstartex(void * 0x01156e98) line 212 + 13 bytes

Comment 1

[(Don't use, old account) - Kai Engert]

CC'ing lord.
-> Kai

Comment 2

[Robert Relyea]

This stake traceback generates lots of questions:

1) Why are we trying to move the key? Are we moving it from the database token
to the crypto token?
2) Why are we trying to use a key exchange to move the key? If we have only the
internal token loaded, we should be able to extract the key directly. What is
failing about the extract or insert code?
3) Why is the key we are generating so darn large?

I guess it is possible that #1 is happening all the time, so we should just
verify that it is copying from the database token to the crypto token. We can do
that by examining the slot structure in the symkey and in the target for
pk11_KeyExchange. Set a debugging assert there and examine which slot we are
copying from and which slot we are copying to.

We can figure out #2 by tracing through pk11_CopyToSlot. Find out 1) is the key
extract failing, or 2) is the create key failing. If it's #1 trace through the
extract code to find out why, if it's 3 trace through the create object code.
This one may be harder because pk11_CopyToSlot may be called a fair amount in a
given Netscape 6 session.

Finally, why is the key so big? Did the data get trashed somewhere? are we
trying to do a diffie helman key exchange? Ah Bingo We are look at the stack
traceback! That explains what could be going on. The first thing I'd do is turn
off the DH ciphers and see if everything is OK. If they are then we can 1) leave
them off as part this release, or 2) trace what is going wrong in the DH derive
code. This should be realtively easy since sendDHClientKeyExchange should only
be called in this case. The big thing to check is why the Master secret is 128
bytes rather than 48 bytes.

bob

Comment 3

[Robert Relyea]

OK I looked at the code some more and it looks like our implementation of DH in
pkcs11c.c is incomplete. We aren't even doing the derive step, so I suggest we
turn DH off until we fix that.

bob

Comment 4

[(Don't use, old account) - Kai Engert]

Bob, thanks for figuring this out. You suggest to turn off Diffie-Hellman
completely for now.

1) How can we do this, i.e. is it easy to provide a patch to turn it off?

2) Will the absence of DH cause disadvantages for the next release?

Comment 5

[(Don't use, old account) - Kai Engert]

Attachment: Suggested fix

Comment 6

[(Don't use, old account) - Kai Engert]

Bob Relyea wrote:
> 
> We should use the SetPolicy call so they don't even show up in the UI.
> Do it right after NSS_SetDomesticPolicy.
> 
> Longer term we should fix the DH ciphers, but right now we should just
> disable it.

Ok, I see the following ciphers in sslsock.c. 

static cipherPolicy ssl_ciphers[] = {	   /*   Export           France   */
 {  SSL_EN_RC4_128_WITH_MD5,		    SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_EN_RC4_128_EXPORT40_WITH_MD5,	    SSL_ALLOWED,     SSL_ALLOWED },
 {  SSL_EN_RC2_128_CBC_WITH_MD5,	    SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_EN_RC2_128_CBC_EXPORT40_WITH_MD5,   SSL_ALLOWED,     SSL_ALLOWED },
 {  SSL_EN_DES_64_CBC_WITH_MD5,		    SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_EN_DES_192_EDE3_CBC_WITH_MD5,	    SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_FORTEZZA_DMS_WITH_FORTEZZA_CBC_SHA, SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_FORTEZZA_DMS_WITH_RC4_128_SHA,      SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_RSA_WITH_RC4_128_MD5,		    SSL_RESTRICTED,  SSL_NOT_ALLOWED },
 {  SSL_RSA_WITH_RC4_128_SHA,		    SSL_RESTRICTED,  SSL_NOT_ALLOWED },
 {  SSL_RSA_FIPS_WITH_3DES_EDE_CBC_SHA,	    SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_RSA_WITH_3DES_EDE_CBC_SHA,	    SSL_RESTRICTED,  SSL_NOT_ALLOWED },
 {  SSL_RSA_FIPS_WITH_DES_CBC_SHA,	    SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_RSA_WITH_DES_CBC_SHA,		    SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_RSA_EXPORT_WITH_RC4_40_MD5,	    SSL_ALLOWED,     SSL_ALLOWED },
 {  SSL_RSA_EXPORT_WITH_RC2_CBC_40_MD5,	    SSL_ALLOWED,     SSL_ALLOWED },
 {  SSL_FORTEZZA_DMS_WITH_NULL_SHA,	    SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_DHE_RSA_WITH_DES_CBC_SHA,           SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_DHE_DSS_WITH_DES_CBC_SHA,           SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA,      SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA,      SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  TLS_DHE_DSS_WITH_RC4_128_SHA,           SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_RSA_WITH_NULL_MD5,		    SSL_ALLOWED,     SSL_ALLOWED },
 {  TLS_RSA_EXPORT1024_WITH_DES_CBC_SHA,    SSL_ALLOWED,     SSL_NOT_ALLOWED },
 {  TLS_RSA_EXPORT1024_WITH_RC4_56_SHA,     SSL_ALLOWED,     SSL_NOT_ALLOWED },
 {  0,			
	    SSL_NOT_ALLOWED, SSL_NOT_ALLOWED }
};

I assume I have to disable all ciphers containing the string "DH" or "DHE",
which are:

 {  SSL_DHE_RSA_WITH_DES_CBC_SHA,           SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_DHE_DSS_WITH_DES_CBC_SHA,           SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA,      SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA,      SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },
 {  TLS_DHE_DSS_WITH_RC4_128_SHA,           SSL_NOT_ALLOWED, SSL_NOT_ALLOWED },

I will do it this way:

SSL_SetPolicy(SSL_DHE_RSA_WITH_DES_CBC_SHA, SSL_NOT_ALLOWED);
SSL_SetPolicy(SSL_DHE_DSS_WITH_DES_CBC_SHA, SSL_NOT_ALLOWED);
SSL_SetPolicy(SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA, SSL_NOT_ALLOWED);
SSL_SetPolicy(SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA, SSL_NOT_ALLOWED);
SSL_SetPolicy(TLS_DHE_DSS_WITH_RC4_128_SHA, SSL_NOT_ALLOWED);

I tried it, and with this change, I'm able to connect to the site.

I have attached a patch. Can you please review it?

Comment 7

[(Don't use, old account) - Kai Engert]

One more note, I did not use SSL_PolicySet, but the new function name,
SSL_CipherPolicySet.

Comment 8

[Robert Relyea]

Ok, this looks good. It's the right function as well, the SetPolicy is
depricated. We probably should think of taking these out of NSS until we get a
fix in so the DH ciphers actually work.

bob

r=relyea

Comment 9

[Simon Fraser [no longer active]]

sr=sfraser

Comment 10

[Asa Dotzler [:asa]]

a=asa on behalf of [e-mail]

Comment 11

[(Don't use, old account) - Kai Engert]

Patch checked in

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

I would say this bug is not fixed, but rather is temporarily worked around
with a change that must be removed when the real problem is solved.  
The workaround disables desired functionality.  

I think that a new NSS bug should be created that reports the underlying 
problem in NSS, and this PSM bug should be marked as depending on that one.  
Then, when the NSS bug is fixed, the PSM workaround should be removed.  

I will attempt to reproduce this problem using one of the NSS test client
programs.  

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Bug http://bugzilla.mozilla.org/show_bug.cgi?id=97887 now reports the 
bug in the underrlying NSS libraries.  When that bug is fixed, the 
workaround that has been checked into PSM (see patch above) should be
removed.

Comment 14

[Stephane Saux]

marking future.
The workaround is in place. The bug was reopened and made to depend on the
underlying bug 97887.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Now that the underlying bug is fixed, the workaround that was checked in 
above (see attachment) should be backed out.

Also, PSM's UI for cipher suite selection does NOT allow the user to 
turn the DHE ciphersuites on or off.  That should be fixed.

Comment 16

[(Don't use, old account) - Kai Engert]

To fix PSM's user interface as Nelson suggested, I created bug 102633.

Comment 17

[(Don't use, old account) - Kai Engert]

Attachment: Reenable the Diffie Hellmann ciphers

Comment 18

[Robert Relyea]

r=relyea

Comment 19

[Christopher Blizzard (:blizzard)]

Comment on attachment 51628 [details] [diff] [review]
Reenable the Diffie Hellmann ciphers

sr=blizzard

Comment 20

[(Don't use, old account) - Kai Engert]

Changing my prefered e-mail address.

Comment 21

[John Unruh]

QA > junruh

Comment 22

[Hong Kwon]

marking nsenterprise-; will be reevaluated for nsenterprise in future release.

Comment 23

[Stephane Saux]

Comment on attachment 51628 [details] [diff] [review]
Reenable the Diffie Hellmann ciphers

has-review as per Bob Relyea.

Comment 24

[Stephane Saux]

We should check this on the trunk.

Comment 25

[Kai Engert (:KaiE:)]

Checked in to trunk.

Comment 26

[John Unruh]

Verified on trunk per kaie's comment. There is no crash on the 10/12 trunk 
build, and there is also no crash on the 10/12 branch Win98 or WinNT build.

Comment 27

[Kai Engert (:KaiE:)]

Changing target to 2.1

Comment 28

[Kai Engert (:KaiE:)]

Really changing target to 2.1