net: add NSS support for RFC 5705

net: add NSS support for RFC 5705

(Keying Material Exporters for TLS).

This is a reworked version of the patch from
https://bugzilla.mozilla.org/show_bug.cgi?id=507359.

BUG=none
TEST=none yet


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

[agl, 2011-07-21 15:46:30]

wtc: please review
wez: fyi

[wtc, 2011-07-21 19:56:16]

LGTM.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/README.chro...
File net/third_party/nss/README.chromium (right):

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/README.chro...
net/third_party/nss/README.chromium:60: patches/secret_extractor.patch
Nit: please name the patch secret_exporter.patch.

You forgot to include the patch in the CL.

List the upstream NSS bug
https://bugzilla.mozilla.org/show_bug.cgi?id=507359

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl.h#n...
net/third_party/nss/ssl/ssl.h:691: ** already be allocated.
We should say if contextlen is 0, the exporter will use the
no-context construction defined in the RFC.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:8455: unsigned int retLen;
Move retLen into the if block, at line 8458.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:8498: label = isServer ? "server finished" :
"client finished";
Nit: use an unsigned int len local variable and set it to 15
here, as the original code does.  This makes it more obvious
what 15 is.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo.c
File net/third_party/nss/ssl/sslinfo.c (right):

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo...
net/third_party/nss/ssl/sslinfo.c:42: #include "pk11func.h"
I don't see anything in the new code that requires
"pk11func.h".  Can this header be removed?

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo...
net/third_party/nss/ssl/sslinfo.c:321: /* Export keying material according to
draft-ietf-tls-extractor-06.
Change draft-ietf-tls-extractor-06 to RFC 5705.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo...
net/third_party/nss/ssl/sslinfo.c:345:
PORT_SetError(SSL_ERROR_UNSUPPORTED_VERSION);
This is a new meaning for the SSL_ERROR_UNSUPPORTED_VERSION
error code.  It's fine for now.  (It'll be confusing if someone
prints the error message for this error code:
"Peer using unsupported version of security protocol.")

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo...
net/third_party/nss/ssl/sslinfo.c:350:
PORT_SetError(SSL_ERROR_HANDSHAKE_NOT_COMPLETED);
In the other place where NSS sets this error code, the
test is much more complicated:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/ssl3...

Should we do the same here?  We can omit the
(ss->version >= SSL_LIBRARY_VERSION_3_0) test because it is
ensured by the test on line 344 above.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo...
net/third_party/nss/ssl/sslinfo.c:374: rv =
ssl3_TLSPRFWithMasterSecret(ss->ssl3.crSpec, label, strlen(label), val, valLen,
out, outLen);
Please fold this long line.

[agl, 2011-07-22 14:05:40]

Thanks. Setting the commit bit...

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/README.chro...
File net/third_party/nss/README.chromium (right):

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/README.chro...
net/third_party/nss/README.chromium:60: patches/secret_extractor.patch
On 2011/07/21 19:56:16, wtc wrote:
> Nit: please name the patch secret_exporter.patch.
> 
> You forgot to include the patch in the CL.
> 
> List the upstream NSS bug
> https://bugzilla.mozilla.org/show_bug.cgi?id=507359

Done.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl.h#n...
net/third_party/nss/ssl/ssl.h:691: ** already be allocated.
On 2011/07/21 19:56:16, wtc wrote:
> We should say if contextlen is 0, the exporter will use the
> no-context construction defined in the RFC.

Done.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl3con.c
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:8455: unsigned int retLen;
On 2011/07/21 19:56:16, wtc wrote:
> Move retLen into the if block, at line 8458.

Done.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/ssl3con...
net/third_party/nss/ssl/ssl3con.c:8498: label = isServer ? "server finished" :
"client finished";
On 2011/07/21 19:56:16, wtc wrote:
> Nit: use an unsigned int len local variable and set it to 15
> here, as the original code does.  This makes it more obvious
> what 15 is.

Done.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo.c
File net/third_party/nss/ssl/sslinfo.c (right):

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo...
net/third_party/nss/ssl/sslinfo.c:42: #include "pk11func.h"
On 2011/07/21 19:56:16, wtc wrote:
> I don't see anything in the new code that requires
> "pk11func.h".  Can this header be removed?

Done.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo...
net/third_party/nss/ssl/sslinfo.c:321: /* Export keying material according to
draft-ietf-tls-extractor-06.
On 2011/07/21 19:56:16, wtc wrote:
> Change draft-ietf-tls-extractor-06 to RFC 5705.

Done.

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo...
net/third_party/nss/ssl/sslinfo.c:350:
PORT_SetError(SSL_ERROR_HANDSHAKE_NOT_COMPLETED);
On 2011/07/21 19:56:16, wtc wrote:
> In the other place where NSS sets this error code, the
> test is much more complicated:

I've copied the referenced test for completeness (omitting the version clause.)

http://codereview.chromium.org/7464031/diff/1/net/third_party/nss/ssl/sslinfo...
net/third_party/nss/ssl/sslinfo.c:374: rv =
ssl3_TLSPRFWithMasterSecret(ss->ssl3.crSpec, label, strlen(label), val, valLen,
out, outLen);
On 2011/07/21 19:56:16, wtc wrote:
> Please fold this long line.

Done.

[commit-bot: I haz the power, 2011-07-22 15:41:47]

Change committed as 93635

[wtc, 2011-07-22 17:33:17]

http://codereview.chromium.org/7464031/diff/6001/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

http://codereview.chromium.org/7464031/diff/6001/net/third_party/nss/ssl/ssl....
net/third_party/nss/ssl/ssl.h:698: unsigned int outlen);
I suggest that we specify the label as
    const char *label,
    unsigned int labellen,

Not requiring the string be null-terminated is more
friendly to std::string and StringPiece.

Note: the RFC defines labels as:

   Labels here have the same definition as in TLS, i.e., an ASCII string
   with no terminating NULL.

The RFC also recommends that labels begin with "EXPORTER"
or "EXPERIMENTAL" (for private use).

[agl, 2011-07-22 17:35:40]

On Fri, Jul 22, 2011 at 1:33 PM,  <wtc@chromium.org> wrote:
> Not requiring the string be null-terminated is more
> friendly to std::string and StringPiece.

Wez also requested a change to the handshake test to allow the
function to be called before the handshake is finished. I'll roll up
both in a change next week.


Cheers

AGL