Last Comment Bug 507359 - Implement TLS keying material exporters in NSS
: Implement TLS keying material exporters in NSS
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
: P1 enhancement (vote)
: 3.14
Assigned To: Douglas Stebila
:
Mentors:
http://tools.ietf.org/html/rfc5705
Depends on:
Blocks: 508184
  Show dependency treegraph
 
Reported: 2009-07-30 04:09 PDT by Douglas Stebila
Modified: 2012-07-12 17:55 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch implementing draft-ietf-tls-extractor-06 (14.70 KB, patch)
2009-07-30 04:09 PDT, Douglas Stebila
no flags Details | Diff | Review
Patch implementing draft-ietf-tls-extractor-06 (take 2) (15.87 KB, patch)
2009-08-03 20:39 PDT, Douglas Stebila
nelson: review-
Details | Diff | Review
Patch implementing RFC 5705 (18.14 KB, patch)
2011-10-16 01:04 PDT, Douglas Stebila
no flags Details | Diff | Review
Patch implementing RFC 5705, v2, by Douglas Stebila (25.93 KB, patch)
2012-03-01 16:23 PST, Wan-Teh Chang
no flags Details | Diff | Review
Patch from the Chromium source tree (checked in) (9.85 KB, patch)
2012-03-06 17:37 PST, Wan-Teh Chang
no flags Details | Diff | Review
Add SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION (3.02 KB, patch)
2012-03-06 18:17 PST, Wan-Teh Chang
brian: review+
rrelyea: superreview+
Details | Diff | Review
Add the hasContext parameter to SSL_ExportKeyingMaterial, from Douglas Stebila's patch (checked in) (4.11 KB, patch)
2012-03-09 18:41 PST, Wan-Teh Chang
no flags Details | Diff | Review
Remove a superfluous "if" in SSL_ExportKeyingMaterial (1.20 KB, patch)
2012-03-13 17:59 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Add SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION, v2 (2.55 KB, patch)
2012-07-12 17:54 PDT, Wan-Teh Chang
no flags Details | Diff | Review

Description Douglas Stebila 2009-07-30 04:09:34 PDT
Created attachment 391565 [details] [diff] [review]
Patch implementing draft-ietf-tls-extractor-06

TLS keying material exporters (http://tools.ietf.org/html/draft-ietf-tls-extractor-06) allow additional keying material to be derived ("exported") from the master secret; this exported keying material can then be used at the application layer for a variety of purposes.  It would be good if NSS had support for this.

FYI, I have also implemented this for OpenSSL, and a patch for that is available at http://www.douglas.stebila.ca/code/keying-material-exporters/ in case interoperability testing is desired.
Comment 1 Douglas Stebila 2009-08-03 20:39:52 PDT
Created attachment 392413 [details] [diff] [review]
Patch implementing draft-ietf-tls-extractor-06 (take 2)

Minor revisions to first patch.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2009-08-04 21:23:31 PDT
Comment on attachment 392413 [details] [diff] [review]
Patch implementing draft-ietf-tls-extractor-06 (take 2)

I presume Douglas wanted me to review this.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2009-10-28 19:46:34 PDT
Comment on attachment 392413 [details] [diff] [review]
Patch implementing draft-ietf-tls-extractor-06 (take 2)

I apologize for the long delay for this review.
Most of these comments are minor matters that can be easily and 
mechanically fixed with an editor, but some require some real work.

1. Lines longer than 80 columns need to be wrapped to look pretty. 
One example (which occurs in several places)

>+	    result = SSL_ExportKeyingMaterial(fd, keymatexportlabel, NULL, 0, keymat, keymatexportlen);

could become 

>+	    result = SSL_ExportKeyingMaterial(fd, keymatexportlabel, NULL, 
>+                                            0, keymat, keymatexportlen);


2.  This will need to be 3.12.5 now.

>+;+NSS_3.12.4 {       # NSS 3.12.4 release
>+;+    global:
>+SSL_ExportKeyingMaterial;
>+;+    local:
>+;+*;
>+;+};

3. A number of issues in new function SSL_ExportKeyingMaterial

>+    ss = ssl_FindSocket(fd);
>+    if (!ss) {
>+	SSL_DBG(("%d: SSL[%d]: bad socket in ExportKeyingMaterial",
>+		 SSL_GETPID(), fd));
>+	return SECFailure;
>+    }
>+
>+    if (ss->version < SSL_LIBRARY_VERSION_3_1_TLS) {
>+	PORT_SetError(SEC_ERROR_TLS_EXPORT_KEY_MAT_VERSION_ERROR);
>+	return SECFailure;
>+    }
>+
>+    // construct PRF arguments

a. We cannot use c++/c99 style comment delimiters in NSS because 
the c compilers on some of the platforms do not recognize them.
We must use the older c comment delimiters /* and */ exclusively.

b. I believe that testing the version number is necessary but not 
sufficient to ensure that the socket is in the proper state.  It 
would be possible to call this function on a server, after the 
server had sent the server hello sequence (meaning the version 
negotiation was complete) but before the client had sent the 
messages from which the master secret is derived, in which case 
the logic below would fail.  So, it is necessary to ensure that 
the handshake has progressed at least to the point where the 
master secret has been locally derived before proceeding.
This is the only comment here that will be more than 5 minutes
worth of work.

>+    vallen = SSL3_RANDOM_LENGTH * 2 + 2 + contextlen;

c) probably should establish some reasonable upper bound on contextlen.

>+    val = PORT_Alloc(vallen);
>+    if (val == NULL) goto done;

d) please separate if statements and their "then" clauses onto 
separate lines.  There are many examples of this.

>+    val[currentvalpos] = (contextlen << 8) & 0xff;

e) I'm pretty sure that expression is always zero. 
Also, casting is preferable to anding. try:
      val[currentvalpos] = (unsigned char)(contextlen >> 8);

>+    if ((contextlen > 0) || (context != NULL)) {
>+	PORT_Memcpy(val + currentvalpos, context, contextlen);

f) I think you want && instead of ||

>+    // disallow prohibited labels
>+    // we have to ensure that the concatenation of values does not create 
>+    // a prohibited label
>+    // note that SSL3_RANDOM_LENGTH > max(prohibited label len) = 15, so
>+    // prohiblen > max(prohibited label len) = 15 and the comparisons won't
>+    // have buffer overflow

g) c99 comments again

>+    prohiblen = strlen(label) + vallen;
>+    prohib = PORT_Alloc(prohiblen);
>+    if (prohib == NULL) goto done;
>+    PORT_Memcpy(prohib, (unsigned char *) label, strlen(label));
>+    PORT_Memcpy(prohib+strlen(label), val, vallen);

h) I see 3 calls to strlen(label) there.  Please factor it out so 
there's just one call on that string.

>+    if ((bcmp(prohib, "client finished", 15) == 0)
>+        || (bcmp(prohib, "server finished", 15) == 0)
>+        || (bcmp(prohib, "master secret", 13) == 0)
>+        || (bcmp(prohib, "key expansion", 13) == 0)

i) please move the conjunctions to the end of the preceding lines,
then the bcmp statements can all be aligned vertically. 


>+    ) {
>+	if (val != NULL) PORT_ZFree(val, vallen);
>+	if (prohib != NULL) PORT_ZFree(prohib, prohiblen);

Please split all the if statements.

>+SEC_ERROR_TLS_EXPORT_KEY_MAT_VERSION_ERROR =	(SEC_ERROR_BASE + 170),
>+SEC_ERROR_TLS_EXPORT_KEY_MAT_LABEL_ERROR   =	(SEC_ERROR_BASE + 171),

You've added new SEC error codes in secerr.h, but since they are only 
set by functions in libSSL, I think these should be new SSL error codes 
defined in nss/lib/ssl/sslerr.h, and don't forget to add corresponding
error strings to nss/cmd/lib/SSLerrs.h
Comment 4 Douglas Stebila 2010-03-29 20:01:42 PDT
(In reply to comment #3)

Nelson, I'm reviewing your comments so I can submit a new patch and close this bug.  One question:

> >+    ss = ssl_FindSocket(fd);
> >+    if (!ss) {
> >+	SSL_DBG(("%d: SSL[%d]: bad socket in ExportKeyingMaterial",
> >+		 SSL_GETPID(), fd));
> >+	return SECFailure;
> >+    }
> >+
> >+    if (ss->version < SSL_LIBRARY_VERSION_3_1_TLS) {
> >+	PORT_SetError(SEC_ERROR_TLS_EXPORT_KEY_MAT_VERSION_ERROR);
> >+	return SECFailure;
> >+    }
> 
> b. I believe that testing the version number is necessary but not 
> sufficient to ensure that the socket is in the proper state.  It 
> would be possible to call this function on a server, after the 
> server had sent the server hello sequence (meaning the version 
> negotiation was complete) but before the client had sent the 
> messages from which the master secret is derived, in which case 
> the logic below would fail.  So, it is necessary to ensure that 
> the handshake has progressed at least to the point where the 
> master secret has been locally derived before proceeding.
> This is the only comment here that will be more than 5 minutes
> worth of work.

Based on what's done on lines 81-90 of lib/ssl/sslinfo.c, my guess is that I should check the flags
    ss->opt.useSecurity
    ss->firstHsDone
    ss->ssl3.initialized

Does that sound right?
Comment 5 Wan-Teh Chang 2011-07-20 14:27:14 PDT
The Internet-Draft has become RFC 5705: http://tools.ietf.org/html/rfc5705.

The differences between draft -06 and the RFC are all editorial changes
except for one change: the RFC allows the context value to be NOT provided.

   If no context is provided, it then computes:

           PRF(SecurityParameters.master_secret, label,
               SecurityParameters.client_random +
               SecurityParameters.server_random
               )[length]

   If context is provided, it computes:

           PRF(SecurityParameters.master_secret, label,
               SecurityParameters.client_random +
               SecurityParameters.server_random +
               context_value_length + context_value
               )[length]

Douglas's patch can be updated to support the no-context
construction by putting the following code of SSL_ExportKeyingMaterial
in an if (contextlen > 0) block:

+    val[currentvalpos] = (contextlen << 8) & 0xff;
+    currentvalpos++;
+    val[currentvalpos] = contextlen & 0xff;
+    currentvalpos++;
+    if ((contextlen > 0) || (context != NULL)) {
+	PORT_Memcpy(val + currentvalpos, context, contextlen);
+    }
Comment 6 Wan-Teh Chang 2011-10-06 16:01:19 PDT
Douglas's patch is being used in Chrome now, so I raised the priority
to P1.
Comment 7 Douglas Stebila 2011-10-12 05:18:50 PDT
(In reply to Wan-Teh Chang from comment #5)
> Douglas's patch can be updated to support the no-context
> construction by putting the following code of SSL_ExportKeyingMaterial
> in an if (contextlen > 0) block:

It's not clear (to me) from the RFC if a 0 length context should be excluded or whether it should included but with length 0.  If a length 0 context is included, that's a different input to the PRF than if the context is excluded.  I've sent an email to Eric Rescorla asking for clarification.
Comment 8 Eric Rescorla (:ekr) 2011-10-12 07:52:26 PDT
IMO the "best" interface would be to allow both nonexistent and empty context, e.g., with a "is_present" boolean. As a practical matter, I doubt anyone is going to use zero length context.
Comment 9 Douglas Stebila 2011-10-16 01:04:59 PDT
Created attachment 567321 [details] [diff] [review]
Patch implementing RFC 5705

I have made the changes from Nelson's review in comment #3 and have updated the patch to implement RFC 5705, rather than draft -06.  

The main change is that the context is now optional.  Based on RFC author Eric Rescorla's comment #8 and private email, a zero length context is treated as present and there is a separate flag to indicate an absent context. This should preserve compatibility with how Chrome is using keying material exporters for Chromoting.

Successfully tested interoperability with corresponding patch for OpenSSL.
Comment 10 Wan-Teh Chang 2011-10-17 18:36:02 PDT
Comment on attachment 567321 [details] [diff] [review]
Patch implementing RFC 5705

Douglas: thanks for the patch.  Have you already incorporated
the changes we made to your old patch in the Chromium tree?

http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/secret_exporter.patch?revision=103728&view=markup
Comment 11 Douglas Stebila 2011-10-18 16:56:47 PDT
(In reply to Wan-Teh Chang from comment #10)
> Have you already incorporated the changes we made to your old patch in the Chromium tree?

No, I wasn't aware of those changes.
Comment 12 Wan-Teh Chang 2012-03-01 16:23:16 PST
Created attachment 602188 [details] [diff] [review]
Patch implementing RFC 5705, v2, by Douglas Stebila

I updated Douglas's patch to NSS 3.13.3.
Comment 13 Wan-Teh Chang 2012-03-06 17:37:54 PST
Created attachment 603532 [details] [diff] [review]
Patch from the Chromium source tree (checked in)

After comparing Douglas's latest patch and the patch used in Chromium
carefully, I decided to check in Chromium's patch first because it has
been improved to support SSL False Start.  The code review for that
patch was done in the Chromium Code Review tool:
http://codereview.chromium.org/7464031
http://codereview.chromium.org/7493056

Note: nonexistent context and empty context are disambiguated
as follows:
- context == NULL && contextLen == 0: nonexistent context
- context != NULL && contextLen == 0: empty context

Patch checked in on the NSS trunk (NSS 3.13.4).

Checking in ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.30; previous revision: 1.29
done
Checking in ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.50; previous revision: 1.49
done
Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.168; previous revision: 1.167
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.95; previous revision: 1.94
done
Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.26; previous revision: 1.25
done
Comment 14 Wan-Teh Chang 2012-03-06 18:17:41 PST
Created attachment 603547 [details] [diff] [review]
Add SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION

The TLS keying material exporter cannot be used with SSL 3.0
and SSL 2.0, so we need a new error code.

We already have SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SSL2.
Rather than adding SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_SSL3,
I suggest that we add a protocol-version-neutral error code
SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION, which can be
used for any SSL/TLS protocol version.

I also changed the error message for SSL_ERROR_HANDSHAKE_NOT_COMPLETED
so that it can be used more generally.  The current error message
is specific to the functions that initiate a renegotiation, but
I want to report this error if someone tries to export keying
material before the handshake is completed.

This patch also has some small changes I took from Douglas's
patch.
Comment 15 Wan-Teh Chang 2012-03-06 18:27:08 PST
Comment on attachment 602188 [details] [diff] [review]
Patch implementing RFC 5705, v2, by Douglas Stebila

Review of attachment 602188 [details] [diff] [review]:
-----------------------------------------------------------------

Douglas,

I have a question and a suggested change for your code that
checks for prohibited labels.

::: mozilla/security/nss/lib/ssl/sslinfo.c
@@ +419,5 @@
> +    }
> +
> +    /* disallow prohibited labels
> +     * we have to ensure that the concatenation of values does not create 
> +     * a prohibited label

1. Why do you check the concatenation of label and value?
It seems that checking the label should be enough.

2. Since you are examining at most 15 bytes of the concatenation, you
can use a stack buffer of size 15 or 16 to avoid heap allocation.
Comment 16 Douglas Stebila 2012-03-06 19:21:01 PST
1. The keying material exporter computes its values as PRF(master secret, label, client_random+server_random+context_len+context_val).  If you look at how the PRF is computed in RFC 5246 Section 5, you see that PRF(secret, label, seed) := Phash(secret, label+seed).  So label and seed are just concatenated, and there is no length field from label incorporated.  If label consists of the string "client finishe" and the client_random starts with "d" then the first bytes of label+seed in Phash are indeed "client finished".  I haven't written out an attack, but I think a creative adversary could construct label and context values such that an input to the keying material exporter where the input label is not prohibited but the overall combination has the same effect as computation using a prohibited label.  The RFC doesn't say that this should be checked for, but I think it should be.

2. Sure.
Comment 17 Wan-Teh Chang 2012-03-09 18:41:33 PST
Created attachment 604589 [details] [diff] [review]
Add the hasContext parameter to SSL_ExportKeyingMaterial, from Douglas Stebila's patch (checked in)

After playing with the API a bit, I decided that adding a boolean
hasContext parameter is a better idea than using a NULL/non-NULL
'context' argument to distinguish between nonexistent and zero-length
contexts.

Patch checked in on the NSS trunk (NSS 3.13.4).

Checking in ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.51; previous revision: 1.50
done
Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.27; previous revision: 1.26
done
Comment 18 Wan-Teh Chang 2012-03-13 17:59:04 PDT
Created attachment 605619 [details] [diff] [review]
Remove a superfluous "if" in SSL_ExportKeyingMaterial

Suggested by Adam Langley.

Patch checked in on the NSS trunk (NSS 3.13.4).

Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.28; previous revision: 1.27
done
Comment 19 Robert Relyea 2012-04-13 15:33:14 PDT
Comment on attachment 603547 [details] [diff] [review]
Add SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION

r+ rrelyea
Comment 20 Robert Relyea 2012-04-13 15:41:21 PDT
Hmm, Either the name of the function is wrong or the interface is wrong.

What is returned is a signed mac. If that signed mac is just a signed mac, then the interface is fine. If it's input for key material (which the function name implies), then we need something that returns a symkey.

It's already part of the ABI, so changes are limited, but I would like to know how it's meant to be used.
Comment 21 Douglas Stebila 2012-04-17 06:10:41 PDT
(In reply to Robert Relyea from comment #20)

> What is returned is a signed mac. If that signed mac is just a signed mac,
> then the interface is fine. If it's input for key material (which the
> function name implies), then we need something that returns a symkey.

I wouldn't say that what's returned is necessarily "input for key material". It's binary data derived from the key that can be used for whatever purpose an application desires, including as key material.
Comment 22 Wan-Teh Chang 2012-04-17 10:11:08 PDT
Bob: the keying material exporter is a way for applications to invoke
the TLS PRF with the master secret as the first argument.  The output
can be used for several purposes, as Douglas said:
- as keying material, similar to how TLS itself generates the key_block.
- as a shared secret for mutual authentication.

Therefore, it is most flexible to return the output as an array of bytes.
If we return the output as a PK11SymKey, I believe most applications will
still be able to use PK11_Derive* or PK11_DigestKey to operate on the
actual output, but they may be constrained by the lack of a PKCS #11
mechanism, so I suspect some of them would still resort to calling
PK11_GetKeyData.

Do you think returning the output as a PK11SymKey would be more consistent
with the style of the NSS API?
Comment 23 Wan-Teh Chang 2012-07-12 17:54:24 PDT
Created attachment 641678 [details] [diff] [review]
Add SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION, v2

I updated the patch to the current NSS trunk and checked it in
(NSS 3.14).

Checking in SSLerrs.h;
/cvsroot/mozilla/security/nss/lib/ssl/SSLerrs.h,v  <--  SSLerrs.h
new revision: 1.23; previous revision: 1.22
done
Checking in sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.25; previous revision: 1.24
done
Checking in sslinfo.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslinfo.c,v  <--  sslinfo.c
new revision: 1.30; previous revision: 1.29
done

NOTE: none of the patches in this bug are in any NSS 3.13.x release
because we changed the contents of the NSS 3.13.4+ releases to be based
on the NSS_3_13_4_BRANCH.

Note You need to log in before you can comment on or make changes to this bug.