Closed
Bug 507359
Opened 15 years ago
Closed 12 years ago
Implement TLS keying material exporters in NSS
Categories
(NSS :: Libraries, enhancement, P1)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.14
People
(Reporter: douglas, Assigned: douglas)
References
()
Details
Attachments
(5 files, 4 obsolete files)
25.93 KB,
patch
|
Details | Diff | Splinter Review | |
9.85 KB,
patch
|
Details | Diff | Splinter Review | |
4.11 KB,
patch
|
Details | Diff | Splinter Review | |
1.20 KB,
patch
|
Details | Diff | Splinter Review | |
2.55 KB,
patch
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•15 years ago
|
||
Minor revisions to first patch.
Assignee | ||
Updated•15 years ago
|
Attachment #391565 -
Attachment is obsolete: true
Attachment #391565 -
Flags: review?(nelson)
Updated•15 years ago
|
Priority: -- → P3
Target Milestone: --- → 3.12.5
Version: unspecified → trunk
Comment 2•15 years ago
|
||
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 3•15 years ago
|
||
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-
Updated•14 years ago
|
Target Milestone: 3.12.5 → 3.12.7
Assignee | ||
Comment 4•14 years ago
|
||
(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?
Updated•14 years ago
|
Target Milestone: 3.12.7 → 3.13
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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
Assignee | ||
Comment 7•13 years ago
|
||
(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•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
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 10•13 years ago
|
||
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
Assignee | ||
Comment 11•13 years ago
|
||
(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.
Updated•13 years ago
|
Target Milestone: 3.13.1 → 3.13.2
Comment 12•12 years ago
|
||
I updated Douglas's patch to NSS 3.13.3.
Attachment #567321 -
Attachment is obsolete: true
Attachment #567321 -
Flags: review?(wtc)
Comment 13•12 years ago
|
||
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•12 years ago
|
||
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 15•12 years ago
|
||
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.
Assignee | ||
Comment 16•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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
Updated•12 years ago
|
Target Milestone: 3.13.2 → 3.13.4
Updated•12 years ago
|
Target Milestone: 3.13.4 → 3.13.5
Comment 19•12 years ago
|
||
Comment on attachment 603547 [details] [diff] [review] Add SSL_ERROR_FEATURE_NOT_SUPPORTED_FOR_VERSION r+ rrelyea
Attachment #603547 -
Flags: superreview?(rrelyea) → superreview+
Comment 20•12 years ago
|
||
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.
Assignee | ||
Comment 21•12 years ago
|
||
(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•12 years ago
|
||
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?
Updated•12 years ago
|
Target Milestone: 3.13.5 → 3.14
Updated•12 years ago
|
Attachment #603547 -
Flags: review?(bsmith) → review+
Comment 23•12 years ago
|
||
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
Updated•12 years ago
|
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.
Description
•