Export key logging in normal builds.

Export key logging in normal builds.

This moves SSL keylogging (see [1]) out from behind the TRACE and DEBUG defines
and allows normal builds to use it when the correct environment variable is
set. It also adds a new CLIENT_RANDOM format to support ECDHE-RSA key-agreement
(and others).

[1] https://developer.mozilla.org/en/NSS_Key_Log_Format

BUG=none
TEST=none


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141029

[wtc, 2012-06-05 23:45:22]

Patch set 2 LGTM.  I can open an upstream NSS bug when this
patch is final.

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:8999: 32*2 /* client_random */ +

Nit: 32 => SSL3_RANDOM_LENGTH

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:9006: PORT_Assert( ss->opt.noLocks ||
ssl_HaveXmitBufLock(ss));

The XmitBufLock should be unnecessary for this function.

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:9014: rv =
PK11_ExtractKeyValue(ss->ssl3.cwSpec->master_secret);

Should this function call ssl_GetSpecReadLock?

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:9029: memcpy(buf, "CLIENT_RANDOM ", 14);

I guess the CLIENT_RANDOM is just for identification purposes
because the master secret alone should be enough to decrypt
the traffic, right?

Should we log both CLIENT_RANDOM and SERVER_RANDOM?

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:9052: return;

Nit: write the last few lines like this:

    if (fwrite(buf, sizeof(buf), 1, ssl_keylog_iob) != 1)
        return;
    fflush(ssl_keylog_iob);

[agl, 2012-06-06 19:17:13]

Have added hexEncode helper function as requested. Won't submit until the new
code has been reviewed.

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:8999: 32*2 /* client_random */ +
On 2012/06/05 23:45:22, wtc wrote:
> 
> Nit: 32 => SSL3_RANDOM_LENGTH

Done.

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:9006: PORT_Assert( ss->opt.noLocks ||
ssl_HaveXmitBufLock(ss));
On 2012/06/05 23:45:22, wtc wrote:
> 
> The XmitBufLock should be unnecessary for this function.

Done.

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:9014: rv =
PK11_ExtractKeyValue(ss->ssl3.cwSpec->master_secret);
On 2012/06/05 23:45:22, wtc wrote:
> 
> Should this function call ssl_GetSpecReadLock?

Done.

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:9029: memcpy(buf, "CLIENT_RANDOM ", 14);
On 2012/06/05 23:45:22, wtc wrote:
> I guess the CLIENT_RANDOM is just for identification purposes
> because the master secret alone should be enough to decrypt
> the traffic, right?

Yes, having the CLIENT_RANDOM simply allows Wireshark to avoid trial decryptions
of each possible key.

> Should we log both CLIENT_RANDOM and SERVER_RANDOM?

I think either is sufficient for identification, no? And we'll have the
CLIENT_RANDOM first which might be helpful for tools in the future.

https://chromiumcodereview.appspot.com/10509009/diff/1001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:9052: return;
On 2012/06/05 23:45:22, wtc wrote:
> 
> Nit: write the last few lines like this:
> 
>     if (fwrite(buf, sizeof(buf), 1, ssl_keylog_iob) != 1)
>         return;
>     fflush(ssl_keylog_iob);

Done.

[wtc, 2012-06-06 21:29:18]

Patch set 3 LGTM.  Thanks!

https://chromiumcodereview.appspot.com/10509009/diff/8001/net/third_party/nss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10509009/diff/8001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:4807: 

Nit: delete one blank line here.

Nit: use indentation level of 4 spaces inside the function.

I think *out++ is quite common in low-level C code, but the
parentheses help clarify the operator precedence.

[agl, 2012-06-07 15:31:42]

https://chromiumcodereview.appspot.com/10509009/diff/8001/net/third_party/nss...
File net/third_party/nss/ssl/ssl3con.c (right):

https://chromiumcodereview.appspot.com/10509009/diff/8001/net/third_party/nss...
net/third_party/nss/ssl/ssl3con.c:4807: 
On 2012/06/06 21:29:18, wtc wrote:
> 
> Nit: delete one blank line here.
> 
> Nit: use indentation level of 4 spaces inside the function.
> 
> I think *out++ is quite common in low-level C code, but the
> parentheses help clarify the operator precedence.

Done.

[commit-bot: I haz the power, 2012-06-07 15:32:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/10509009/15001

[commit-bot: I haz the power, 2012-06-07 15:40:51]

Try job failure for 10509009-15001 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-06-07 16:43:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/10509009/18002

[commit-bot: I haz the power, 2012-06-07 17:40:34]

Change committed as 141029

[wtc, 2012-06-08 02:34:31]

https://chromiumcodereview.appspot.com/10509009/diff/18002/net/third_party/ns...
File net/third_party/nss/ssl/sslsock.c (right):

https://chromiumcodereview.appspot.com/10509009/diff/18002/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:2917: fputs("# pre-master secret log file,
generated by NSS\n",

Should we update this comment?  You are now logging the
master secret, right?

[agl, 2012-06-11 19:16:35]

https://chromiumcodereview.appspot.com/10509009/diff/18002/net/third_party/ns...
File net/third_party/nss/ssl/sslsock.c (right):

https://chromiumcodereview.appspot.com/10509009/diff/18002/net/third_party/ns...
net/third_party/nss/ssl/sslsock.c:2917: fputs("# pre-master secret log file,
generated by NSS\n",
On 2012/06/08 02:34:31, wtc wrote:
> 
> Should we update this comment?  You are now logging the
> master secret, right?

updated in crrev.com/141456