New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optionally log master secrets for TLS connections #2363

Closed
jsha opened this Issue Aug 12, 2015 · 20 comments

Comments

Projects
None yet
@jsha
Copy link
Contributor

jsha commented Aug 12, 2015

Sometimes it's necessary to decrypt your own TLS connections to debug their contents. Wireshark supports this quite nicely with its decryption feature. For non-DH key agreement, you simply provide the private key of the server. However, for DH key agreement, or when you are acting only as a client, that doesn't work. Firefox and Chrome support the environment variable SSLKEYLOGFILE to write the master secrets used to a file, for decryption by Wireshark. It would be great to support this or a similar mechanism for logging master secrets in Node.

Key log format: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/Key_Log_Format
Helpful Stack Exchange howto: https://security.stackexchange.com/questions/35639/decrypting-tls-in-wireshark-when-using-dhe-rsa-ciphersuites/42350#42350
Wireshark decryption docs: https://wiki.wireshark.org/SSL

(reposted from nodejs/node-convergence-archive#59).

@silverwind

This comment has been minimized.

Copy link
Contributor

silverwind commented Aug 12, 2015

As this would basically throw out all security, the only way I see for a feature like that would be a compile-time option.

@jsha

This comment has been minimized.

Copy link
Contributor

jsha commented Aug 12, 2015

I think a compile-time option would satisfy the use case for this reasonably well, though it would be less convenient than the environment variable used by Firefox and Chrome.

I don't understand why you say this would throw out all security, though. The contents of the TLS connection are known to both endpoints, including the Node endpoint on which you would add this debugging variable when needed. This just allows the developer who runs the Node endpoint to store extra data about that connection. The idea is not that you would leave this on permanently in any production system.

@silverwind

This comment has been minimized.

Copy link
Contributor

silverwind commented Aug 12, 2015

I'm thinking more of a scenario where a compromised server could be modified to provide these secrets to the attacker, while still serving the application. An attacker could for example modify the environment variables and force a restart on the node process.

I'm not really sure the benefits outweigth the risks here. Couldn't you just run a MITM proxy for your debugging needs?

@jsha

This comment has been minimized.

Copy link
Contributor

jsha commented Aug 12, 2015

compromised server could be modified to provide these secrets to the attacker, while still serving the application

This is entirely possible today. Anyone with access to the Node process' memory can exfiltrate key material.

I'm not really sure the benefits outweigth the risks here. Couldn't you just run a MITM proxy for your debugging needs?

For debugging of HTTP applications this is generally sufficient. But for debugging HTTP protocol issues, or especially HTTP/2 protocol issues, a proxy would not show the necessary information. The need for this flag came up for me specifically when trying to debug issues using HTTP/2 as a client against a remote server. The client knows the session key by definition, so allowing it to be (optionally) logged merely enables better debugging.

@silverwind

This comment has been minimized.

Copy link
Contributor

silverwind commented Aug 12, 2015

Well, maybe a runtime flag could be done. I'm a bit more comfortable with a flag than a mere environment variable ;)

@jsha

This comment has been minimized.

Copy link
Contributor

jsha commented Aug 12, 2015

Yep, a runtime flag would be great too! I only suggested the env variable for similarity with Firefox and Chrome.

@jorangreef

This comment has been minimized.

Copy link
Contributor

jorangreef commented Aug 13, 2015

Sorry, but this is a terrible idea. It's a massive accident just waiting to happen. It opens so many possible attack vectors. If anyone needs to do this kind of thing they should just modify Node themselves.

@jsha

This comment has been minimized.

Copy link
Contributor

jsha commented Aug 13, 2015

@jorangreef: Can you go into more detail about the attack vectors or accidents you forsee? Keep in mind that this has been an option in Firefox and Chrome for many years and I have not heard a single example of anyone being compromised by it.

If I may guess a little: It sounds like you and @silverwind are most concerned by server operators misusing this flag. I think the most important debugging is is in TLS client code: if you control the server, you can force negotiation with a non-forward secret cipher, and since you also possess the private key, that allows the necessary analysis. However, for the client, there is no such option: the server controls the cipher negotiation and the private key.

Would you be less worried about accidental misuse of a flag that affects TLS clients only, and not servers?

@jorangreef

This comment has been minimized.

Copy link
Contributor

jorangreef commented Aug 14, 2015

@jsha there's probably an endless combination of attack vectors that an option to log the master key would support. But even if there's just one vector, that's enough, and @silverwind has already given a few.

Chrome and Firefox are desktop applications so the security threats are different.

I think your use-case would be solved if your Javascript could access the key from the actual TLS client instance directly, i.e. it's not necessary to ask Node to log it (via env flag or compile flag). But even then, it should only be exposed as a property by the instance if an option is explicitly provided.

Something like this should err on the side of being very difficult to do.

@lxe

This comment has been minimized.

Copy link
Contributor

lxe commented Jan 5, 2016

+1 This feature would immensely help with debugging.

I'm not really sure the benefits outweigth the risks here. Couldn't you just run a MITM proxy for your debugging needs?

Not at all. There are heisenbugs that require delicate and specific environment, and altering network flow with MITM proxies makes debugging impossible.

@larryboymi

This comment has been minimized.

Copy link

larryboymi commented May 6, 2016

+1 ...
Realize it seems like more could happen server-side with exposure of client secrets, but this would immensely help debugging encrypted traffic.

If an attacker is able to get to the running process and manipulate it, the possibilities range widely regardless.

Java exposes via logging key parts which can be consumed by wireshark with a little manipulation. I know node is not java.

@silverwind silverwind added the security label Oct 20, 2016

@tgvarik

This comment has been minimized.

Copy link

tgvarik commented Feb 8, 2017

Wireshark >=1.6.0 supports NSS-format log files giving the session ID and master key in the following format:

RSA Session-ID:<32-byte session ID, hex encoded> Master-Key:<48-byte master key, hex encoded>

Both the session ID and master key can be obtained with TLSSocket.prototype.getSession(), which returns a DER-encoded ASN.1 structure described in ssl.h.

So just do something like this:

https.request(opts, cb)
.once('socket', (s) => {
  s.once('secureConnect', () =>
    let session = parseSession(s.getSession());
    // session.sessionId and session.masterKey should be hex strings
    fs.appendFileSync('sslkeylog.log', `RSA Session-ID:${session.sessionId} Master-Key:${session.masterKey}\n`);
  });
});

This works very well for me in Wireshark 2.2.4. How thoroughly to parse the session buffer is up to you, but even something as barebones as this will work in most cases:

function parseSession(buf) {
  return {
    sessionId: buf.slice(17, 17+32).toString('hex'),
    masterKey: buf.slice(51, 51+48).toString('hex')
  };
}
@jhford

This comment has been minimized.

Copy link

jhford commented Feb 14, 2017

@tgvarik I found your comment really useful and wrote a simple library to implement that. I was wondering if you'd be OK with me publishing it to npm. If so, how would you like attribution?

https://github.com/jhford/node-https-wireshark

I'm also happy to delete the code :)

I changed it so that it should play nicely with older versions of node.

@tgvarik

This comment has been minimized.

Copy link

tgvarik commented Feb 14, 2017

@jhford Thanks for writing that up. Please feel free to publish! No attribution needed, but a link to my comment above would be appreciated.

@joshperry

This comment has been minimized.

Copy link

joshperry commented Feb 21, 2017

We've been working to debug a binary, non-HTTP, TLS-secured protocol implementation with a custom wireshark dissector; we would find this feature extremely useful.

Even simple properties that gave these values without opaque buffer parsing would be very useful. I'm not sure what the negative security implications would be with this key material already available in-process from the session with no flags.

Thanks for the code, @tgvarik! Going to give it a shot here.

@silverwind

This comment has been minimized.

Copy link
Contributor

silverwind commented Feb 23, 2017

I've been looking into extracting the keys from a server socket, and found that parsing the SSL_SESSION structure is insufficient with modern browser clients at least. For these type of connections, Wireshark supports the CLIENT_RANDOM <client_random> <master-secret> format (full details on supported formats).

SSL_SESSION only contains the master secret in these cases, so I assume SSL_get_client_random would need to be exposed as well to enable decryption of these type of sessions.

At this point, I think out-of-band decryption is something we should enable userland to do. It won't have to be easy or convenient, but we should provide access to the necessary data structures to make it possible.

cc: @nodejs/crypto

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Feb 24, 2017

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jul 26, 2017

Is this being worked on? Should this stay open at this point?

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Jul 28, 2017

I'll close it out. It's almost its 2nd anniversary and no one has put up a PR so far. If someone still wants to pursue this, open a PR and we'll take it from there.

@bnoordhuis bnoordhuis closed this Jul 28, 2017

@yattamax yattamax referenced this issue Sep 3, 2017

Open

how to use #1

@kolontsov

This comment has been minimized.

Copy link

kolontsov commented Dec 11, 2018

Recently I published https://github.com/kolontsov/node-sslkeylog to implement SSLKEYLOG. It uses little hacky approach to get access to SSL_get_client_random(), but so far it works well for my debugging purporses.

I was thinking about submitting PR (to use SSL_get_client_random() without tricks), but I'm not yet sure how implementation should look like: we can write sslkeylog.txt if some env var exists, we can expose get_session_key() to be used with TLSSocket, etc..

OR we can wait until openssl 1.1.1 will be fully integrated and then just start using SSL_CTX_set_keylog_callback..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment