tls: expose Finished messages in TLSSocket #19102

codedot · 2018-03-03T00:30:59Z

Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished() and tlsSocket.getPeerFinished(), respectively.

Fixes: #19055
Refs: XRPLF/rippled#2413

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

tls, crypto, doc, test

addaleax

The implementation seems fine to me, thanks!

/cc @nodejs/crypto

addaleax · 2018-03-03T09:06:54Z

src/node_crypto.cc

+  char* buf = Malloc(EVP_MAX_MD_SIZE * 2);
+  size_t len = SSL_get_finished(w->ssl_, buf, EVP_MAX_MD_SIZE * 2);
+  if (!len)
+    return;


This leaks memory, buf is not free’d here (the same applies below)

Amended commit, now free()ing the buffers in both functions. Thanks.

addaleax · 2018-03-03T09:08:18Z

test/parallel/test-tls-finished.js

+  const bob = tls.connect({
+    port: server.address().port,
+    rejectUnauthorized: false
+  }, () => {


Can you wrap the functions that should be called exactly once in common.mustCall()?

I wrapped all callbacks in common.mustCall(), except for the exit event.

addaleax · 2018-03-03T09:08:48Z

test/parallel/test-tls-finished.js

+  };
+});
+
+process.on('exit', () => {


Is there maybe a more suitable event to listen for, like the finished event of bob or similar?

I considered other ways to catch the moment when both alice and bob had saved their versions of Finished messages, however the code gets messy compared to just listening to the exit event.

addaleax · 2018-03-03T09:10:00Z

doc/api/tls.md

+* Returns: {undefined|Buffer} The latest `Finished` message that was sent
+to the socket, or `undefined` if no `Finished` message has been sent yet.
+
+Corresponds to the `SSL_get_finished` routine in OpenSSL.


What are the actual about the contents of these buffers? How does the peer generate them? Should Node provide support for that too?

I added "as part of a SSL/TLS handshake" phrase to the documentation in order to emphasize that the Finished messages are internal to OpenSSL implementation and are not provided by application.

@codedot If these are internal to OpenSSL, how is the returned buffer meaningful to the caller?

@addaleax An article called Why is it unlikely to have a complete and alternative implementation of Ripple? describes in detail how the Finished messages are used to generate Session-Signature in rippled using the fact that both Finished messages are available on both sides of an SSL/TLS socket. Also, see XRPLF/rippled#2413 and the comment by @JoelKatz.

However, I would not suggest including this motivation into Node.js documentation because I do not think it would be wise to endorse such a questionable method.

@codedot All of these only resources only describe that these functions are necessary to implement that protocol, but make no statements about what guarantees can be made about the returned contents of these buffers.

I realize that the OpenSSL documentation and source code don’t provide much more information either, but since apparently people found a use case for these functions, there must be sensible statements that we could make here in the documentation.

@addaleax Well, I think Node.js should not provide more documentation for these routines than OpenSSL itself does. The modern versions do not include any man(1) pages for them like they used to. However, even the old documentation was not descriptive.

Well, I think Node.js should not provide more documentation than OpenSSL itself does.

I’d agree, but the place to start fixing this would be the OpenSSL documentation then ;)

I’m just really not a fan of having documentation that doesn’t really describe what something does…

I’d agree, but the place to start fixing this would be the OpenSSL documentation then ;)

I'm afraid that's the best I can do :(

@codedot I guess there’s no harm in opening an issue: openssl/openssl#5509

@addaleax Thank you very much for asking OpenSSL team for help. Documentation updated, please review.

indutny

LGTM with a nit.

indutny · 2018-03-04T00:07:15Z

src/node_crypto.cc

+
+  char* buf = Malloc(EVP_MAX_MD_SIZE * 2);
+  size_t len = SSL_get_finished(w->ssl_, buf, EVP_MAX_MD_SIZE * 2);
+  if (!len) {


I think it might be worth verifying that len is less than EVP_MAX_MD_SIZE * 2.

@indutny This test is done in the routines themselves.

Yes, it is done there. However, what I'm asking to check is that the data is complete and not partial.

@indutny Oh, I see now what you mean. In order to ensure that the message is not being cut off, I provide the buffer of the same size that the corresponding arrays have in <openssl/ssl3.h>. Also, the comment there suggests that EVP_MAX_MD_SIZE * 2 may possibly be an overstatement:

/* actually only need to be 16+20 for SSLv3 and 12 for TLS */ unsigned char finish_md[EVP_MAX_MD_SIZE * 2]; int finish_md_len; unsigned char peer_finish_md[EVP_MAX_MD_SIZE * 2]; int peer_finish_md_len;

assert then?

I suggest to add an assert here anyway. We've little control over OpenSSL codebase, and it'd be great to have a check for partial data even if such can't be returned right now.

@indutny I am sorry, but I still cannot understand what assertion exactly do you mean?

@indutny Nevermind, I amended the commit to call SSL_get_finished and SSL_get_peer_finished twice: first just to get the actual length, then to copy the message to the buffer.

Unfortunately, this is undefined behavior:

If an argument to a function has an invalid value (such as a value outside the domain of the function, or a pointer outside the address space of the program, or a null pointer, or a pointer to non-modifiable storage when the corresponding parameter is not const-qualified) or a type (after promotion) not expected by a function with variable number of arguments, the behavior is undefined.

See: https://www.imperialviolet.org/2016/06/26/nonnull.html

I'm fine with what it looked like before. Could you please revert it to that form and add:

CHECK_LT(len, EVP_MAX_MD_SIZE * 2);

@indutny Oops, I did not know about section 7.1.4, my bad. Still, I would prefer avoiding the EVP_MAX_MD_SIZE * 2 constant because it is unclear how exactly the latter is related to the Finished messages. So, I further amended the commit to use a dummy array instead of nullptr not to cause undefined behavior in memcpy().

indutny

LGTM

indutny · 2018-03-04T18:49:29Z

src/node_crypto.cc

+  Base* w;
+  ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
+
+  char dummy[1];


This probably needs a comment, explaining that nullptr can't be passed to SSL_get_finished.

@indutny Indeed, using a dummy byte without a comment is not readily understandable. Comments added, please review.

bnoordhuis

I read the linked issue but I couldn't quite figure out from the discussion what function this has in ripple. MitM or replay protection?

edit: didn't feel like a particularly good solution and I guess my spidey sense wasn't completely off: openssl/openssl#5509

bnoordhuis · 2018-03-04T20:05:45Z

doc/api/tls.md

+be used for external authentication procedures when the authentication
+provided by SSL/TLS is not desired or is not enough.
+
+Corresponds to the `SSL_get_peer_finished` routine in OpenSSL.


I think the docs could be more explicit that it's for implementing tls-unique channel binding from RFC 5929.

(That is the intended use case, right?)

@bnoordhuis While RFC 5929 might be a use case for the introduced tlsSocket.getFinished() and tlsSocket.getPeerFinished() methods, that is not exactly what rippled does. Should I still reference RFC 5929 in documentation?

I would lean towards 'yes'. Outside of rippled that seems like the primary (and possibly only) use case.

Definitely agree that the docs could include more context. Code impl looks fine tho.

It looks like the pull request is stalling. The link to RFC 5929 has been added two days ago.

Not stalling at all :-) ... just busy people working on lots of different things :-) I've started the CI on this, things are looking good so far.

bnoordhuis · 2018-03-04T20:06:43Z

src/node_crypto.cc

+  // Thus, we use a dummy byte.
+  char dummy[1];
+  size_t len = SSL_get_finished(w->ssl_, dummy, sizeof dummy);
+  if (!len)


Style: tiniest of nits but can you use explicit size checks, i.e., if (len == 0)? Likewise on line 2139.

Substance: couldn't you pass in a buffer of size EVP_MAX_MD_SIZE? The result value is (essentially) the HMAC of the message. You can then drop the Malloc() call and switch to Buffer::Copy().

Style: tiniest of nits but can you use explicit size checks, i.e., if (len == 0)? Likewise on line 2139.

Will do.

Substance: couldn't you pass in a buffer of size EVP_MAX_MD_SIZE? The result value is (essentially) the HMAC of the message. You can then drop the Malloc() call and switch to Buffer::Copy().

I would prefer avoiding referencing the EVP_MAX_MD_SIZE constant if I may.

That's fine. Didn't see until afterwards that you discussed the same thing with @indutny.

bnoordhuis · 2018-03-04T20:11:51Z

src/node_crypto.cc

+    return;
+
+  char* buf = Malloc(len);
+  SSL_get_finished(w->ssl_, buf, len);


Can you CHECK_EQ(size, SSL_get_finished(...)) here and on line 2143?

@bnoordhuis Done in the amended commit.

jasnell · 2018-03-06T16:49:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/13533/

BridgeAR · 2018-03-06T18:21:54Z

doc/api/tls.md

@@ -583,6 +583,20 @@ if called on a server socket. The supported types are `'DH'` and `'ECDH'`. The

 For Example: `{ type: 'ECDH', name: 'prime256v1', size: 256 }`

+### tlsSocket.getFinished()
+


Please add:



The same for the other function.

@BridgeAR Done in the amended commit.

BridgeAR · 2018-03-06T18:26:13Z

doc/api/tls.md

+### tlsSocket.getFinished()
+
+* Returns: {Buffer|undefined} The latest `Finished` message that has been
+sent to the socket as part of a SSL/TLS handshake, or `undefined` if


The current implementation actually returns null in case there is no handle. Should this also be reflected here? And it would be good to add a test for that as well.

@BridgeAR Actually, it might be more consistent to return undefined in case there is no handle. What do you think?

@BridgeAR Done in the amended commit.

Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively. Fixes: #19055 Refs: XRPLF/rippled#2413

BridgeAR · 2018-03-06T19:17:23Z

CI https://ci.nodejs.org/job/node-test-pull-request/13540/

codedot · 2018-03-09T19:17:51Z

So, what is the next step?

addaleax · 2018-03-11T17:33:18Z

Landed in 98a14e0 🎉

@codedot Thanks for the contribution!

Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively. PR-URL: #19102 Fixes: #19055 Refs: XRPLF/rippled#2413 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable changes: * assert: - From now on all error messages produced by `assert` in strict mode will produce a error diff. (Ruben Bridgewater) #17615 - From now on it is possible to use a validation object in throws instead of the other possibilities. (Ruben Bridgewater) #17584 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * fs: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * tty: - Add getColorDepth function to determine if terminal supports colors (Ruben Bridgewater) #17615 * util: - add util.inspect compact option (Ruben Bridgewater) #17576 * **Added new collaborators** - [watson](https://github.com/watson) Thomas Watson PR-URL: #19428

Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively. PR-URL: nodejs#19102 Fixes: nodejs#19055 Refs: XRPLF/rippled#2413 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>

Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively. PR-URL: #19102 Fixes: #19055 Refs: XRPLF/rippled#2413 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) nodejs#18633 - remove runtime deprecation (Ali Ijaz Sheikh) nodejs#19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) nodejs#18513 * cluster: - add cwd to cluster.settings (cjihrig) nodejs#18399 - support windowsHide option for workers (Todd Wong) nodejs#17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) nodejs#18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) nodejs#21592 - upgrade libuv to 1.19.2 (cjihrig) nodejs#18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) nodejs#21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) nodejs#18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) nodejs#19408 * http, http2: - add options to http.createServer() (Peter Marton) nodejs#15752 - add 103 Early Hints status code (Yosuke Furukawa) nodejs#16644 - add http fallback options to .createServer (Peter Marton) nodejs#15752 * n-api: - take n-api out of experimental (Michael Dawson) nodejs#19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) nodejs#18087 * src: - add public API for managing NodePlatform (Cheng Zhao) nodejs#16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) nodejs#17600 - node internals' postmortem metadata (Matheus Marchini) nodejs#14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) nodejs#19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) nodejs#18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) nodejs#18186 PR-URL: nodejs#21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.4.1 (Kat Marchán) #22591 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

Notable Changes: * async_hooks: - rename PromiseWrap.parentId (Ali Ijaz Sheikh) #18633 - remove runtime deprecation (Ali Ijaz Sheikh) #19517 - deprecate unsafe emit{Before,After} (Ali Ijaz Sheikh) #18513 * cluster: - add cwd to cluster.settings (cjihrig) #18399 - support windowsHide option for workers (Todd Wong) #17412 * crypto: - allow passing null as IV unless required (Tobias Nießen) #18644 * deps: - upgrade npm to 6.2.0 (Kat Marchán) #21592 - upgrade libuv to 1.19.2 (cjihrig) #18918 - Upgrade node-inspect to 1.11.5 (Jan Krems) #21055 * fs,net: - support as and as+ flags in stringToFlags() (Sarat Addepalli) #18801 - emit 'ready' for fs streams and sockets (Sameer Srivastava) #19408 * http, http2: - add options to http.createServer() (Peter Marton) #15752 - add 103 Early Hints status code (Yosuke Furukawa) #16644 - add http fallback options to .createServer (Peter Marton) #15752 * n-api: - take n-api out of experimental (Michael Dawson) #19262 * perf_hooks: - add warning when too many entries in the timeline (James M Snell) #18087 * src: - add public API for managing NodePlatform (Cheng Zhao) #16981 - allow --perf-(basic-)?prof in NODE\_OPTIONS (Leko) #17600 - node internals' postmortem metadata (Matheus Marchini) #14901 * tls: - expose Finished messages in TLSSocket (Anton Salikhmetov) #19102 * **trace_events**: - add file pattern cli option (Andreas Madsen) #18480 * util: - implement util.getSystemErrorName() (Joyee Cheung) #18186 PR-URL: #21593

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 3, 2018

MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 3, 2018

addaleax reviewed Mar 3, 2018

View reviewed changes

indutny approved these changes Mar 4, 2018

View reviewed changes

addaleax added tls Issues and PRs related to the tls subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Mar 4, 2018

addaleax mentioned this pull request Mar 4, 2018

What do SSL_get_finished/SSL_get_peer_finished return? openssl/openssl#5509

Closed

indutny approved these changes Mar 4, 2018

View reviewed changes

bnoordhuis reviewed Mar 4, 2018

View reviewed changes

jasnell approved these changes Mar 5, 2018

View reviewed changes

BridgeAR reviewed Mar 6, 2018

View reviewed changes

tls: expose Finished messages in TLSSocket

a2d06f5

Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively. Fixes: #19055 Refs: XRPLF/rippled#2413

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 6, 2018

addaleax closed this Mar 11, 2018

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

MylesBorins added the land-on-v8.x label Aug 7, 2018

MylesBorins mentioned this pull request Aug 16, 2018

v8.12.0 proposal #21593

Merged

		@@ -583,6 +583,20 @@ if called on a server socket. The supported types are `'DH'` and `'ECDH'`. The

		For Example: `{ type: 'ECDH', name: 'prime256v1', size: 256 }`

		### tlsSocket.getFinished()

tls: expose Finished messages in TLSSocket #19102

tls: expose Finished messages in TLSSocket #19102

Conversation

codedot commented Mar 3, 2018 • edited Loading

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedot Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedot Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedot Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codedot Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny Mar 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

indutny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Mar 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented Mar 6, 2018

codedot commented Mar 9, 2018

addaleax commented Mar 11, 2018

codedot commented Mar 3, 2018 •

edited

Loading

codedot Mar 3, 2018 •

edited

Loading

codedot Mar 3, 2018 •

edited

Loading

codedot Mar 4, 2018 •

edited

Loading

codedot Mar 4, 2018 •

edited

Loading

indutny Mar 4, 2018 •

edited

Loading

bnoordhuis left a comment •

edited

Loading