349966 - re-enable SSLTRACE for keys and (pre)master secrets

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

When all crypto work was moved into softoken in NSS 3.4, we lost the ability
to get the key values with SSLTRACE.  Now that we have the "bypass", we 
should get some of that ability back.  

I have a patch that does (some of) this.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: add back some SSL tracing, v1

Bob, if you can't get to this right away, please pass the review request
on to another NSS developer.  I particularly would like your review of 
the patch to ssl3con.c.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

To test this patch, apply it and build a "debug" build of NSS, 
then do the following:
a) use a test program for NSS that does not close stdout 
   (because trace output goes to stdout)
b) set these environment variables to these values
   SSLTRACE=127
   SSLDEBUG=127
   SSLBYPASS=1
c) if using tstclnt (or any program that manipulates the bypass feature)
   turn on the bypass feature (e.g. -B)
d) run the test.  trace output should appear whereever stdout goes.

Comment 3

[Robert Relyea]

Comment on attachment 235192 [details] [diff] [review]
add back some SSL tracing, v1

r+ rrelyea.

Question, in the case where you extract the PSM, is it useful to log that you couldn't extract the PSM? That would tell you that they system things it has a valid PSM value, even if the token won't give it up.

bob

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

On Trunk:
Checking in derive.c;  new revision: 1.4;  previous revision: 1.3
Checking in ssl3con.c; new revision: 1.96; previous revision: 1.95

Checking in derive.c;  new revision: 1.3.2.1; previous revision: 1.3
Checking in ssl3con.c; new revision: 1.76.2.17; previous revision: 1.76.2.16

This checkin is necessary but perhaps not sufficient.
I won't mark this fixed just yet.  More may be needed. 

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: also trace the DH(E) PMS

requesting review from any NSS team member

Comment 6

[Wan-Teh Chang]

Comment on attachment 235340 [details] [diff] [review]
also trace the DH(E) PMS

>Index: ssl3con.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v
>retrieving revision 1.76.2.17
>diff -u -9 -r1.76.2.17 ssl3con.c
>--- ssl3con.c	24 Aug 2006 22:31:54 -0000	1.76.2.17
>+++ ssl3con.c	25 Aug 2006 00:50:29 -0000
>@@ -68,19 +68,19 @@
> 
> #ifndef PK11_SETATTRS
> #define PK11_SETATTRS(x,id,v,l) (x)->type = (id); \
> 		(x)->pValue=(v); (x)->ulValueLen = (l);
> #endif
> 
> static void      ssl3_CleanupPeerCerts(sslSocket *ss);
> static PK11SymKey *ssl3_GenerateRSAPMS(sslSocket *ss, ssl3CipherSpec *spec,
>                                        PK11SlotInfo * serverKeySlot);
>-static SECStatus ssl3_DeriveMasterSecret(sslSocket *ss, const PK11SymKey *pms);
>+static SECStatus ssl3_DeriveMasterSecret(sslSocket *ss, PK11SymKey *pms);
> static SECStatus ssl3_DeriveConnectionKeysPKCS11(sslSocket *ss);
> static SECStatus ssl3_HandshakeFailure(      sslSocket *ss);
> static SECStatus ssl3_InitState(             sslSocket *ss);
> static sslSessionID *ssl3_NewSessionID(      sslSocket *ss, PRBool is_server);
> static SECStatus ssl3_SendCertificate(       sslSocket *ss);
> static SECStatus ssl3_SendEmptyCertificate(  sslSocket *ss);
> static SECStatus ssl3_SendCertificateRequest(sslSocket *ss);
> static SECStatus ssl3_SendFinished(          sslSocket *ss, PRInt32 flags);
> static SECStatus ssl3_SendServerHello(       sslSocket *ss);
>@@ -2528,19 +2528,19 @@
>     return SECSuccess;
> }
> 
> /* This method uses PKCS11 to derive the MS from the PMS, where PMS 
> ** is a PKCS11 symkey. This is used in all cases except the 
> ** "triple bypass" with RSA key exchange.
> ** Called from ssl3_InitPendingCipherSpec.   prSpec is pwSpec.
> */
> static SECStatus
>-ssl3_DeriveMasterSecret(sslSocket *ss, const PK11SymKey *pms)
>+ssl3_DeriveMasterSecret(sslSocket *ss, PK11SymKey *pms)
> {
>     ssl3CipherSpec *  pwSpec = ss->ssl3.pwSpec;
>     const ssl3KEADef *kea_def= ss->ssl3.hs.kea_def;
>     unsigned char *   cr     = (unsigned char *)&ss->ssl3.hs.client_random;
>     unsigned char *   sr     = (unsigned char *)&ss->ssl3.hs.server_random;
>     PRBool            isTLS  = (PRBool)(kea_def->tls_keygen ||
>                                 (pwSpec->version > SSL_LIBRARY_VERSION_3_0));
>     /* 
>      * Whenever isDH is true, we need to use CKM_TLS_MASTER_KEY_DERIVE_DH
>@@ -2578,18 +2578,30 @@
> 	master_params.RandomInfo.ulClientRandomLen = SSL3_RANDOM_LENGTH;
> 	master_params.RandomInfo.pServerRandom     = sr;
> 	master_params.RandomInfo.ulServerRandomLen = SSL3_RANDOM_LENGTH;
> 
> 	params.data = (unsigned char *) &master_params;
> 	params.len  = sizeof master_params;
>     }
> 
>     if (pms != NULL) {
>+#if defined(TRACE)
>+	if (ssl_trace >= 100) {
>+	    SECStatus extractRV = PK11_ExtractKeyValue(pms);
>+	    if (extractRV == SECSuccess) {
>+		SECItem * keyData = PK11_GetKeyData(pms);
>+		if (keyData && keyData->data && keyData->len) {
>+		    ssl_PrintBuf(ss, "Pre-Master Secret", 
>+				 keyData->data, keyData->len);
>+		}
>+	    }
>+	}
>+#endif
> 	pwSpec->master_secret = PK11_DeriveWithFlags((PK11SymKey *)pms, 
> 					master_derive, &params, key_derive, 
> 					CKA_DERIVE, 0, keyFlags);
> 	if (!isDH && pwSpec->master_secret && ss->opt.detectRollBack) {
> 	    SSL3ProtocolVersion client_version;
> 	    client_version = pms_version.major << 8 | pms_version.minor;
> 	    if (client_version != ss->clientHelloVersion) {
> 		/* Destroy it.  Version roll-back detected. */
> 		PK11_FreeSymKey(pwSpec->master_secret);

Comment 7

[Julien Pierre]

Comment on attachment 235340 [details] [diff] [review]
also trace the DH(E) PMS

Nelson,

1) PK11_ExtractKeyValue changes pms which was originally const. Can you tell me if this object is protected from access by other threads and how ?

2) I see there was already a cast of pms from non-const to const in the call to PK11_DeriveWithFlags . But that function might not actually change pms but just have the wrong prototype (requiring non-const argument). If it is determined above that the change of the pms argument to non-const is safe, then this cast should be removed because it is now unnecessary. r+ if 1) is OK and that change is made.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Julien,  The PMS is unknown to any thread except the one that created it.
So there cannot be any other threads manipulating it. There is no race.
I'll also get rid of the cast of pms in the call to PK11_DeriveWithFlags, 
if you'll give this patch r+.

Comment 9

[Julien Pierre]

Comment on attachment 235340 [details] [diff] [review]
also trace the DH(E) PMS

r+ with the removal of now unnecessary cast.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

DHE PMS patch committed on trunk and NSS 3.11 branch.
Checking in ssl/ssl3con.c; new revision: 1.97; previous revision: 1.96
Checking in ssl/ssl3con.c; new revision: 1.76.2.18; previous revision: 1.76.2.17
Log comment:
Also trace the DH(E) PMS.  bug 349966. r=julien.pierre, wtchang

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Marking fixed