Closed Bug 507359 Opened 15 years ago Closed 12 years ago

Implement TLS keying material exporters in NSS

Categories

(NSS :: Libraries, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: douglas, Assigned: douglas)

References

()

Details

Attachments

(5 files, 4 obsolete files)

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.
Attachment #391565 - Flags: review?(nelson)
Minor revisions to first patch.
Attachment #391565 - Attachment is obsolete: true
Attachment #391565 - Flags: review?(nelson)
Blocks: 508184
Priority: -- → P3
Target Milestone: --- → 3.12.5
Version: unspecified → trunk
Comment on attachment 392413 [details] [diff] [review]
Patch implementing draft-ietf-tls-extractor-06 (take 2)

I presume Douglas wanted me to review this.
Attachment #392413 - Flags: review?(nelson)
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
Attachment #392413 - Flags: review?(nelson) → review-
Target Milestone: 3.12.5 → 3.12.7
(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?
Target Milestone: 3.12.7 → 3.13
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);
+    }
Douglas's patch is being used in Chrome now, so I raised the priority
to P1.
Status: NEW → ASSIGNED
Priority: P3 → P1
Target Milestone: 3.13 → 3.13.1
(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.
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.
Attached patch Patch implementing RFC 5705 (obsolete) — Splinter Review
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.
Attachment #392413 - Attachment is obsolete: true
Attachment #567321 - Flags: review?(wtc)
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
(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.
Target Milestone: 3.13.1 → 3.13.2
I updated Douglas's patch to NSS 3.13.3.
Attachment #567321 - Attachment is obsolete: true
Attachment #567321 - Flags: review?(wtc)
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
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.
Attachment #603547 - Flags: superreview?(rrelyea)
Attachment #603547 - Flags: review?(bsmith)
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.
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.
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
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
Target Milestone: 3.13.2 → 3.13.4
Target Milestone: 3.13.4 → 3.13.5
Comment on attachment 603547 [details] [diff] [review]
Add SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION

r+ rrelyea
Attachment #603547 - Flags: superreview?(rrelyea) → superreview+
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.
(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.
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?
Target Milestone: 3.13.5 → 3.14
Attachment #603547 - Flags: review?(bsmith) → review+
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.
Attachment #603547 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: