Add support for logging out TLSv1.3 secrets. #2287

Lukasa · 2017-01-25T16:32:44Z

Checklist

tests are added or updated
CLA is signed

Description of change

Follows on from #1646.

This change adds support for logging out several of the secrets from the TLSv1.3 key schedule for debugging purposes. These secrets are the same as the ones logged by BoringSSL, and are logged in the same format. They are used to continue to allow tools like Wireshark to decrypt TLS traffic when needed.

This change also fixes a bug from the previous code. This slipped past because the tests did not adequately validate the RSA key exchange logging actually occurred: they would pass even if that log never fired. This patch updates the test to correctly exercise that code, and then fixes the underlying bug as well.

I strongly recommend at least one TLSv1.3 expert validates this code. I did my best to read the draft specification and log at the appropriate point, but it is not yet possible to sensibly validate that these logs are actually ok. We should confirm that the TLSv1.3 logs are actually logging the correct secret before we merge this.

Lukasa · 2017-01-25T16:34:18Z

ssl/ssl_lib.c

+                          client_random_len,
+                          secret,
+                          secret_len);
+}


This function is basically identical to the one above it, except that the one above doesn't fire for TLSv1.3. I suspect we should collapse these two and hoist up the check for TLSv1.3 into the calling scope of the above function, but I wanted a code reviewer to confirm that was a good idea.

An alternative would be to have the function above call this one: either would work.

I agree that it could be merged with the function above. Have a TLSv1.3 section and a non-TLSv1.3 one.

levitte · 2017-01-25T17:45:10Z

ssl/ssl_lib.c

@@ -4413,15 +4413,20 @@ int ssl_log_rsa_client_key_exchange(SSL *ssl,
                                    const uint8_t *premaster,
                                    size_t premaster_len)
 {
+    uint8_t premaster_tag[8];


It seems that you don't use this for anything useful. Isn't it enough that the length parameter for the encrypted premaster was changed to 8 in the call to nss_keylog_int?

Uh...yes, yes it is. Honestly not sure what I was thinking there.

levitte · 2017-01-25T17:50:31Z

ssl/ssl_lib.c

+    if (client_random_len != 32) {
+        SSLerr(SSL_F_SSL_LOG_MASTER_SECRET, ERR_R_INTERNAL_ERROR);
+        return 0;
+    }


I'm starting to think this might do better as an assertion. After all, it is a length that we specify internally and that we should get correctly at all times.

Works for me.

levitte · 2017-01-25T17:51:38Z

ssl/ssl_lib.c

+                          client_random_len,
+                          secret,
+                          secret_len);
+}


I agree that it could be merged with the function above. Have a TLSv1.3 section and a non-TLSv1.3 one.

levitte · 2017-01-25T17:53:03Z

ssl/tls13_enc.c

        } else {
            insecret = s->session->master_key;
            label = server_application_traffic;
            labellen = sizeof(server_application_traffic) - 1;
+            log_label = "SERVER_TRAFFIC_SECRET_0";


Maybe these strings should be made into macros in ssl_locl.h...

richsalz · 2017-01-25T19:31:30Z

Can you split the bugfix into a separate PR? No need to hold that up for the TLS 1.3 review part. Thanks!

Lukasa · 2017-01-25T19:45:44Z

@richsalz My gift to you: #2288.

Lukasa · 2017-01-25T20:13:47Z

Ok, I've updated a bunch of stuff.

Right now the tests will fail because the RSA log doesn't come out correctly, so the tests won't pass until #2288 is merged. At that point, I'll rebase onto a new master and we should be good to go.

levitte · 2017-01-25T20:53:45Z

ssl/statem/statem_lib.c

+                                             s->s3->client_random,
+                                             SSL3_RANDOM_SIZE,
+                                             s->session->master_key,
+                                             s->session->master_key_length))


The indentation of the ssl_log_secret parameters is off by one space. If you used tabs, convert them to spaces, please.

levitte · 2017-01-25T21:03:29Z

#2288 just got merged, so this should build as soon as it's rebased

Lukasa · 2017-01-25T21:21:16Z

Alrighty, updated.

Lukasa · 2017-01-26T15:46:14Z

Hrm, what's up with that Windows build? It seems like there's a problem with the linkage on Windows.

levitte · 2017-01-26T16:27:37Z

In test/build.info, you need to add ../ssl/ssl_lib.c to the source files for tls13secretstest

levitte · 2017-01-26T16:33:56Z

Hold that thought, you did guard it against building when configured shared... I'll have a closer look

levitte · 2017-01-26T16:35:48Z

Found it. You got the condition in test/build.info wrong:

diff --git a/test/build.info b/test/build.info
index c11623894..b1a69da24 100644
--- a/test/build.info
+++ b/test/build.info
@@ -371,7 +371,7 @@ IF[{- !$disabled{tests} -}]
   # We disable this test completely in a shared build because it deliberately
   # redefines some internal libssl symbols. This doesn't work in a non-shared
   # build
-  IF[{- !$disabled{shared} -}]
+  IF[{- $disabled{shared} -}]
     PROGRAMS_NO_INST=tls13secretstest
     SOURCE[tls13secretstest]=tls13secretstest.c testutil.c test_main.c
     SOURCE[tls13secretstest]= ../ssl/tls13_enc.c ../ssl/packet.c

levitte · 2017-01-26T16:37:08Z

As a matter of fact, you can remove one of the source lines as well:

diff --git a/test/build.info b/test/build.info
index c11623894..579cf7562 100644
--- a/test/build.info
+++ b/test/build.info
@@ -371,10 +371,9 @@ IF[{- !$disabled{tests} -}]
   # We disable this test completely in a shared build because it deliberately
   # redefines some internal libssl symbols. This doesn't work in a non-shared
   # build
-  IF[{- !$disabled{shared} -}]
+  IF[{- $disabled{shared} -}]
     PROGRAMS_NO_INST=tls13secretstest
     SOURCE[tls13secretstest]=tls13secretstest.c testutil.c test_main.c
-    SOURCE[tls13secretstest]= ../ssl/tls13_enc.c ../ssl/packet.c
     INCLUDE[tls13secretstest]=.. ../include
     DEPEND[tls13secretstest]=../libcrypto ../libssl
   ENDIF

Lukasa · 2017-01-26T16:37:36Z

I mean, I didn't touch test/build.info at all. ;) But I'm certainly happy to add that delta to my patch.

mattcaswell · 2017-01-26T16:37:59Z

So why was this working before?

levitte · 2017-01-26T16:40:04Z

So why was this working before?

I have no clue. did we fail to pay attention to Windows back in #1646?

mattcaswell · 2017-01-26T16:40:16Z

Reading the comment (which I wrote) it doesn't make any sense:

   # We disable this test completely in a shared build because it deliberately
   # redefines some internal libssl symbols. This doesn't work in a non-shared
   # build

We disable the shared build because it doesn't work in non-shared??? Huh? I really have no idea what I was thinking at the time.

Anyway - it would be a shame to lose this from testing in shared builds because that is the default.

richsalz · 2017-01-26T16:40:33Z

FYI, from a posting in the TLS mailing list:

This is indeed work in progress, the current state can be tracked at:
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=12779

Note for TLS implementers: Wireshark supports decryption when provided with the master secret (TLS 1.2 and before), but with TLS 1.3 there are more secrets. The current plan is to accept the client/server handshake/application traffic secrets (as opposed to the more sensitive Handshake/Master secrets) following the format proposed by BoringSSL:
https://code.wireshark.org/review/19801

If everything goes well, Wireshark 2.4 should be the first stable version with TLS 1.3 support.

Lukasa · 2017-01-26T16:41:09Z

No, we didn't support TLSv1.3 in #1646. So I don't think these functions would have needed to be linked in. The change here is that we now log from tls13_enc.c.

levitte · 2017-01-26T16:44:19Z

Ok, I'll experiment a bit more...

mattcaswell · 2017-01-26T16:44:42Z

No, we didn't support TLSv1.3 in #1646. So I don't think these functions would have needed to be linked in. The change here is that we now log from tls13_enc.c.

Right but the test existed and works fine in master now. So why should we need to change it to work only on non-shared because you added a function call?

levitte · 2017-01-26T16:47:53Z

Right but the test existed and works fine in master now. So why should we need to change it to work only on non-shared because you added a function call?

Because ssl/tls13_enc.c, which is mentioned as a specific source module for tls13secretstest, now calls ssl_log_secret, which is a private symbol in ssl/ssl_lib.c

levitte · 2017-01-26T16:49:34Z

Unfortunately, ssl/ssl_lib.c has dependencies to a number of libssl modules...

Lekensteyn · 2017-01-26T16:50:01Z

Matt, this PR (and some other implementations) are mentioned in the commit message of https://code.wireshark.org/review/19801

levitte · 2017-01-26T16:51:58Z

@Lukasa, I think that the best you can do to resolve this is to move the key logging functions to a separate source file, so tls13secretstest doesn't have to depend on ssl/ssl_lib.c...

Lukasa · 2017-01-26T16:52:59Z

Yeah, we can do that. ssl_log.c acceptable? (I considered calling it ssl_keylog.c but I figured that'd give the tinfoil-hat brigade heart attacks.)

Lukasa · 2017-01-31T14:57:26Z

Ok, I've squashed this all down to two commits. I'm pretty happy with this at the moment, though as always better testing would be better. Should be good for another round of code review.

mattcaswell · 2017-01-31T15:02:17Z

ssl/tls13_enc.c

@@ -370,6 +375,11 @@ int tls13_change_cipher_state(SSL *s, int which)
    keylen = EVP_CIPHER_key_length(ciph);
    ivlen = EVP_CIPHER_iv_length(ciph);

+    if (!ssl_log_secret(s, log_label, secret, hashlen)) {
+        SSLerr(SSL_F_TLS13_CHANGE_CIPHER_STATE, ERR_R_INTERNAL_ERROR);
+        return 0;


Should be "goto err" to make sure secret is cleansed.

mattcaswell · 2017-01-31T15:04:26Z

test/sslapitest.c

+                return 0;
+            }
+
+            /* TODO: Can I get this key out? */


Not sure what this means?

Essentially it means: can I not just confirm that the structure is right, but actually validate the secret? Is it hung off an object that I can use to compare it against, or can I derive it?

I think the only way to do that would be to reach inside the SSL object. That might not be an unreasonable thing to do in a test.

So all four of the derived secrets we're logging out here are stored on the SSL object after the connection is established?

Well not exactly. We store the handshake_secret and the master_secret. We also store the server finished hash. That should be enough to derive the two application traffic secrets. We don't store the hash of ClientHello..ServerHello which is necessary for the handshake traffic secrets. It would be possible to calculate the hash using a custom BIO to watch the communication between client and server and pick out the first two messages, because those messages are sent in the clear....but that is definitely harder.

Hrm.

This is ultimately a balance between the effort needed to write this test code and risk of this code breaking or being wrong. BoringSSL has no tests for this functionality because ultimately it's not that important to them.

The breakage we're risking here is a breakage where the secret we log out is not right. This kind of breakage is of low likelihood once the code is written, but unless we test that it's right we don't really stand a chance of preventing it. Of course, the amount of code required to get it right is...high.

Ultimately this is a question for the maintenance team: all patches should be written with the assumption that the contributor will be hit by a bus 20 seconds after it's merged and the maintenance team will have to live with it with no assistance for the rest of time. So it's your call, not mine. 😁

I'm thinking it might be useful to do at least the application traffic secrets bit - it has value as a test beyond the logging functionality. However, thinking about it more you would need to have access to the internal tls13_hkdf_expand() function which, wile doable, probably would mean moving the test to the tls13secretstest.c file. Perhaps a separate PR? For now just leave the TODO in, make it something like TODO(TLS1.3): test that application traffic secrets are what we expect

Ok, let's clarify the TODO and consider approaching it in a further PR.

Lekensteyn · 2017-01-31T17:21:16Z

ssl/statem/statem_lib.c

+     */
+    if (!SSL_IS_TLS13(s) && !ssl_log_secret(s, MASTER_SECRET_LABEL,
+                                              s->session->master_key,
+                                              s->session->master_key_length))


indentation is not aligned

Lekensteyn · 2017-01-31T17:21:39Z

ssl/tls13_enc.c

@@ -261,6 +261,7 @@ int tls13_change_cipher_state(SSL *s, int which)
    unsigned char *hash = hashval;
    unsigned char *insecret;
    unsigned char *finsecret = NULL;
+    char *log_label = NULL;


Shouldn't this be const char?

Lekensteyn · 2017-01-31T17:23:45Z

test/sslapitest.c

            }
            token = strtok(NULL, " \n");
            if (!token) {
                printf("Unexpectedly short premaster secret log.\n");
-                return -1;
+                return 0;
            }
            /* TODO: Can I check this sensibly? */


what does this old comment refer to? maybe it can be removed now after you check the count?

It's a similar question about whether I can extract the RSA premaster secret once the test has run to validate the secret. An OpenSSL core dev would probably be able to tell me this.

No, the pre-master is held only transiently. Once we've generated the master secret it is removed. In theory you could probably derive the master secret independently from the logged pre-master and check that it is equal to the master in the session...but that is non-trivial.

Ok then, in that case I'll remove the comment as written and replace it with one that notes that we can't easily validate the log. =)

Lekensteyn · 2017-01-31T17:38:20Z

@Lukasa Without early data the secret would not be very useful (nothing to test). BoringSSL also does not implement early data as far as I could see, so that did not help either. I should probably look into Tris (Go implementation from Cloudflare) for a more complete 0-RTT implementation for testing.

My change requests have been answered

Lukasa · 2017-02-01T10:11:50Z

Ok, I've updated in response to code review.

mattcaswell

Looks good. I spotted some style nits for multi-line comments that I hadn't noticed before. Approved subject to those being fixed.

mattcaswell · 2017-02-01T10:28:28Z

ssl/statem/statem_lib.c

-                               s->session->master_key_length))
+    /* Log the master secret, if logging is enabled. We don't log it for
+     * TLSv1.3: there's a different key schedule for that.
+     */


Nit: Multi-line comments should look like this:

/* * Log the master secret, if logging is enabled. We don't log it for * TLSv1.3: there's a different key schedule for that. */

mattcaswell · 2017-02-01T10:28:46Z

test/sslapitest.c

@@ -41,6 +41,18 @@ static X509 *ocspcert = NULL;

 #define NUM_EXTRA_CERTS 40

+/* This structure is used to validate that the correct number of log messages
+ * of various types are emitted when emitting secret logs.
+ */


Nit: multi-line comment style

mattcaswell · 2017-02-01T10:29:02Z

test/sslapitest.c

-            /* TODO: Can I check this sensibly? */
+            /* We can't sensibly check the log because the premaster secret is
+             * transient, and OpenSSL doesn't keep hold of it once the master
+             * secret is generated. */


Nit: multi-line comment style

mattcaswell · 2017-02-01T10:29:11Z

test/sslapitest.c

+             * client random, and then the hex-encoded secret. In this case,
+             * we treat all of these secrets identically and then just
+             * distinguish between them when counting what we saw.
+             */


Nit: multi-line comment style

mattcaswell · 2017-02-01T10:29:54Z

test/sslapitest.c

+            }
+
+            /* TODO(TLS1.3): test that application traffic secrets are what
+             * we expect */


Nit: multi-line comment style

mattcaswell · 2017-02-01T10:30:08Z

test/sslapitest.c

@@ -271,15 +348,20 @@ static int test_keylog(void) {

    /* Now we want to test that our output data was vaguely sensible. We
     * do that by using strtok and confirming that we have more or less the
-     * data we expect.
+     * data we expect. For both client and server, we expect to see one master
+     * secret. The client should also see a RSA key exchange.


Nit: multi-line comment style

mattcaswell · 2017-02-01T10:30:16Z

test/sslapitest.c

@@ -355,15 +438,20 @@ static int test_keylog_no_master_key(void) {
    }

    /* Now we want to test that our output data was vaguely sensible. For this
-     * test, we expect no CLIENT_RANDOM entry.
+     * test, we expect no CLIENT_RANDOM entry becuase it doesn't make sense for
+     * TLSv1.3, but we do expect both client and server to emit keys.


Nit: multi-line comment style

Lukasa · 2017-02-01T10:33:57Z

Updated the multiline comments.

mattcaswell

Great...still missed a couple though. Also needs a second team review...@levitte?

mattcaswell · 2017-02-01T10:36:28Z

ssl/ssl_locl.h

-/* ssl_log_master_secret logs |master| to the SSL_CTX associated with |ssl|, if
- * logging is enabled. It returns one on success and zero on failure. The entry
- * is identified by |client_random|.
+/* ssl_log_secret logs |secret| to the SSL_CTX associated with |ssl|, if


Missed one.

mattcaswell · 2017-02-01T10:36:49Z

test/sslapitest.c

+            /*
+             * We can't sensibly check the log because the premaster secret is
+             * transient, and OpenSSL doesn't keep hold of it once the master
+             * secret is generated. */


And this one.

levitte

Yup

Lekensteyn

LGTM, I have tested it with a modified s_client version and am able to decrypt the handshake and application data in both direction with Wireshark. Will put a PR for that as well.

mattcaswell · 2017-02-02T09:43:18Z

Pushed. Thanks!!

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2287)

Lekensteyn · 2017-03-22T11:11:13Z

What is the first public version where support for this API can be expected? 1.1.1, 1.2.0 or something else?

What is the suggested method to detect the API availability? There is no macro, so either there will be ifdef checks against the version or there needs to be a configure-time check.

NSS and GnuTLS support the SSLKEYLOGFILE method to configure the TLS keylog file, but OpenSSL requires application to configure it explicitly. This patch adds an opt-in feature that enables keylogging for OpenSSL. libcurl applications linked with OpenSSL that would like to disable this feature can add this to their CURLOPT_SSL_CTX_FUNCTION callback: SSL_CTX_set_keylog_callback((SSL_CTX *)sslctx, NULL) TODO: feature detection, openssl/openssl#2287 (comment) TODO: update docs? Where to mention this? Ref curl#1030

levitte · 2017-03-22T11:17:56Z

Our aim is to release a 1.1.1 that includes TLS 1.3. Either way, the following would be safe to check for the availability:

#if OPENSSL_VERSION_NUMBER >= 0x10101000L

The reasoning is that TLSv1.3 can only appear in the next feature release, which must at least be 1.1.1.

mattcaswell · 2017-03-22T11:21:09Z

In addition to @levitte's comment, the key logging API will also be in 1.1.1

Lukasa force-pushed the tlsv13-keylogs branch from 001b5ec to 2c321a5 Compare January 25, 2017 16:32

Lukasa commented Jan 25, 2017

View reviewed changes

levitte requested changes Jan 25, 2017

View reviewed changes

Lukasa mentioned this pull request Jan 25, 2017

Limit the length of the encrypted premaster key. #2288

Closed

2 tasks

levitte previously requested changes Jan 25, 2017

View reviewed changes

Lukasa force-pushed the tlsv13-keylogs branch 2 times, most recently from e62f9f6 to ac6a3fd Compare January 25, 2017 21:21

Lukasa force-pushed the tlsv13-keylogs branch from 75f8c2a to c4fcf1b Compare January 31, 2017 14:56

mattcaswell reviewed Jan 31, 2017

View reviewed changes

Lekensteyn reviewed Jan 31, 2017

View reviewed changes

Lukasa force-pushed the tlsv13-keylogs branch from c4fcf1b to daa317c Compare February 1, 2017 10:11

Lukasa force-pushed the tlsv13-keylogs branch from daa317c to f906a5b Compare February 1, 2017 10:26

mattcaswell approved these changes Feb 1, 2017

View reviewed changes

mattcaswell added the reviewed label Feb 1, 2017

Lukasa force-pushed the tlsv13-keylogs branch from f906a5b to 0e042db Compare February 1, 2017 10:33

mattcaswell approved these changes Feb 1, 2017

View reviewed changes

Lukasa added 2 commits February 1, 2017 10:38

Add support for logging out TLSv1.3 secrets

706d2c0

Test logging TLSv1.3 secrets.

b64db11

Lukasa force-pushed the tlsv13-keylogs branch from 0e042db to b64db11 Compare February 1, 2017 10:39

levitte approved these changes Feb 1, 2017

View reviewed changes

Lekensteyn approved these changes Feb 1, 2017

View reviewed changes

Lekensteyn mentioned this pull request Feb 1, 2017

apps: Add support for writing a keylog file #2343

Closed

2 tasks

mattcaswell closed this Feb 2, 2017

levitte pushed a commit that referenced this pull request Feb 2, 2017

Add support for logging out TLSv1.3 secrets

2c7bd69

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2287)

levitte pushed a commit that referenced this pull request Feb 2, 2017

Test logging TLSv1.3 secrets.

f1a5939

Reviewed-by: Richard Levitte <levitte@openssl.org> Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #2287)

Lekensteyn mentioned this pull request Mar 22, 2017

Enable SSLKEYLOGFILE feature for OpenSSL/BoringSSL curl/curl#1346

Closed

yan12125 mentioned this pull request Aug 30, 2019

Write TLS session keys to $SSLKEYLOGFILE ytdl-org/youtube-dl#11614

Closed

Add support for logging out TLSv1.3 secrets. #2287

Add support for logging out TLSv1.3 secrets. #2287

Conversation

Lukasa commented Jan 25, 2017

Checklist

Description of change

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

richsalz commented Jan 25, 2017

Lukasa commented Jan 25, 2017

Lukasa commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte commented Jan 25, 2017

Lukasa commented Jan 25, 2017

Lukasa commented Jan 26, 2017

levitte commented Jan 26, 2017

levitte commented Jan 26, 2017

levitte commented Jan 26, 2017

levitte commented Jan 26, 2017

Lukasa commented Jan 26, 2017

mattcaswell commented Jan 26, 2017

levitte commented Jan 26, 2017

mattcaswell commented Jan 26, 2017

richsalz commented Jan 26, 2017

Lukasa commented Jan 26, 2017

levitte commented Jan 26, 2017

mattcaswell commented Jan 26, 2017

levitte commented Jan 26, 2017

levitte commented Jan 26, 2017

Lekensteyn commented Jan 26, 2017

levitte commented Jan 26, 2017

Lukasa commented Jan 26, 2017

Lukasa commented Jan 31, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lekensteyn commented Jan 31, 2017

Lukasa commented Feb 1, 2017

mattcaswell 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

Lukasa commented Feb 1, 2017

mattcaswell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levitte left a comment

Choose a reason for hiding this comment

Lekensteyn left a comment

Choose a reason for hiding this comment

mattcaswell commented Feb 2, 2017

Lekensteyn commented Mar 22, 2017

levitte commented Mar 22, 2017

mattcaswell commented Mar 22, 2017