Closed Bug 95987 Opened 24 years ago Closed 23 years ago

Crash visiting https server that implements DHE ciphersuites

Categories

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

Product:
Core Graveyard
Component:
Security: UI
Version:
1.0 Branch
Platform:
x86
Windows 2000
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
psm2.1

People

(Reporter: inactive-mailbox, Assigned: KaiE)

References

(
URL
)

Details

(Keywords: crash)

Attachments

(2 files)

Suggested fix
951 bytes, patch
Details | Diff | Splinter Review
Reenable the Diffie Hellmann ciphers
1.06 KB, patch
ssaux
: review+
blizzard
: superreview+
Details | Diff | Splinter Review
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
Keywords: crash, nsenterprise
Priority: -- → P1
Target Milestone: --- → 2.1
CC'ing lord. -> Kai
Assignee: ssaux → kai.engert
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
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
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?
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?
Status: NEW → ASSIGNED
One more note, I did not use SSL_PolicySet, but the new function name, SSL_CipherPolicySet.
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
sr=sfraser
a=asa on behalf of drivers@mozilla.org
Patch checked in
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Crash visiting page with optional client auth. → Crash visiting https server that implements DHE ciphersuites
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.
Depends on: 97887
marking future. The workaround is in place. The bug was reopened and made to depend on the underlying bug 97887.
Target Milestone: 2.1 → Future
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.
To fix PSM's user interface as Nelson suggested, I created bug 102633.
r=relyea
Comment on attachment 51628 [details] [diff] [review] Reenable the Diffie Hellmann ciphers sr=blizzard
Attachment #51628 - Flags: superreview+
Changing my prefered e-mail address.
Assignee: kai.engert → kaie
Status: REOPENED → NEW
Status: NEW → ASSIGNED
QA > junruh
QA Contact: ckritzer → junruh
marking nsenterprise-; will be reevaluated for nsenterprise in future release.
Keywords: nsenterprise-
Comment on attachment 51628 [details] [diff] [review] Reenable the Diffie Hellmann ciphers has-review as per Bob Relyea.
Attachment #51628 - Flags: review+
We should check this on the trunk.
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago → 23 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Version: unspecified → 2.1
Changing target to 2.1
Really changing target to 2.1
Target Milestone: Future → 2.1
Product: PSM → Core
Version: psm2.1 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: