TLSv1.3: Update the state machine to be closer to TLSv1.3 by mattcaswell · Pull Request #1932 · openssl/openssl

mattcaswell · 2016-11-15T20:16:58Z

Checklist

tests are added or updated
CLA is signed

Description of change

This is a major overhaul of the TLSv1.3 state machine. Currently it still
looks like TLSv1.2. This commit changes things around so that it starts
to look a bit less like TLSv1.2 and bit more like TLSv1.3.

After this PR we will have:

ClientHello
+ key_share          ---->
                           ServerHello
                           +key_share
                           {CertificateRequest*}
                           {Certificate*}
                           {CertificateStatus*}
                     <---- {Finished}
{Certificate*}
{CertificateVerify*}
{Finished}           ---->
[ApplicationData]    <---> [Application Data]

Key differences between the intermediate position after this PR and the final TLSv1.3
position are:

No EncryptedExtensions message yet
No server side CertificateVerify message yet
CertificateStatus still exists as a separate message
A number of the messages are still in the TLSv1.2 format
Still running on the TLSv1.2 record layer (although it is using the new TLSv1.3 secret generation)

richsalz · 2016-11-15T22:24:35Z

#1848 has been reviewed and approved, so once you do that and rebase I will begin looking at this.

TLSv1.3 has a NewSessionTicket message, but it is *completely* different to the TLSv1.2 one and may as well have been called something else. This commit removes the old style NewSessionTicket from TLSv1.3. We will have to add the new style one back in later.

This is a major overhaul of the TLSv1.3 state machine. Currently it still looks like TLSv1.2. This commit changes things around so that it starts to look a bit less like TLSv1.2 and bit more like TLSv1.3. After this commit we have: ClientHello + key_share ----> ServerHello +key_share {CertificateRequest*} {Certificate*} {CertificateStatus*} <---- {Finished} {Certificate*} {CertificateVerify*} {Finished} ----> [ApplicationData] <---> [Application Data] Key differences between this intermediate position and the final TLSv1.3 position are: - No EncryptedExtensions message yet - No server side CertificateVerify message yet - CertificateStatus still exists as a separate message - A number of the messages are still in the TLSv1.2 format - Still running on the TLSv1.2 record layer

The previous commit had a dummy payload for the Finished data. This commit fills it in with a real value.

There is a set of miscellaneous processing for OCSP, CT etc at the end of the ServerDone processing. In TLS1.3 we don't have a ServerDone, so this needs to move elsewhere.

In one case we weren't always sending an unexpected message alert if we don't get what we expect.

After the client processes the server's initial flight in TLS1.3 it may respond with either an encrypted, or an unencrypted alert. We needed to teach TLSProxy about this so that it didn't issue spurious warnings.

Current s_server can only get an OCSP Response from an OCSP responder. This provides the capability to instead get the OCSP Response from a DER encoded file. This should make testing of OCSP easier.

Add various different handshake types that are possible.

mattcaswell · 2016-11-16T10:17:31Z

#1848 is now in, so I rebased. All commits in this PR are now for review.

FdaSilvaYY · 2016-11-16T23:08:38Z

ssl/t1_lib.c

 static int tls_use_ticket(SSL *s)
 {
-    if (s->options & SSL_OP_NO_TICKET)
+    if (s->options & SSL_OP_NO_TICKET || SSL_IS_TLS13(s))


Oups: missing parenthesis around binary & operands ...
EDIT: C operator precedence say it's ok.

yeah, but we tend to (not styleguide requirement) use those parens, so consider it. don't care much what you do.

I added parens for clarity.

richsalz

This is a first, lightly-read, review. After you respond and push I'll probably have more comments, hopefully substantive ones.

richsalz · 2016-11-17T02:06:21Z

apps/s_server.c

 } tlsextstatusctx;

-static tlsextstatusctx tlscstatp = { NULL, NULL, NULL, 0, -1, 0 };
+static tlsextstatusctx tlscstatp = { NULL, NULL, NULL, NULL, 0, -1, 0 };


consider re-ordering the fields in the structure so that the initializer can just be "-1" at the start, and everything else gets the zero default. More future-proof.

richsalz · 2016-11-17T02:07:26Z

apps/s_server.c

+ * simplified version. It examines certificates each time and makes one OCSP
+ * responder query for each request. A full version would store details such as
+ * the OCSP certificate IDs and minimise the number of OCSP responses by caching
+ * them until they were considered "expired".


Do we ever have plans to write the better one? Can we tie this into the external session caching mechanism to at least make it not stink?

This is just s_server, so I don't think we need a better one?

richsalz · 2016-11-17T02:09:18Z

apps/s_server.c

 done:
-    if (ret != SSL_TLSEXT_ERR_OK)
-        ERR_print_errors(bio_err);
    if (aia) {


do you need this test? since free of NULL is okay. and if you do need it, then "aia != NULL"

Yes, I think we need it to determine whether we should free "host", "path" and "port". Dependent on code path they may be set to something that needs freeing, or set to something that doesn't need freeing. Changed to "aia != NULL".

I don't understand that, aren't the always allocated or NULL?

In the case where aia is NULL, we set set host, path and port to point to srctx->host, srctx->path and srctx->port respectively. Those shouldn't be freed here.

richsalz · 2016-11-17T02:10:50Z

apps/s_server.c

+    int rspderlen;
+    int ret = SSL_TLSEXT_ERR_ALERT_FATAL;
+
+    if (srctx->verbose)


these should only be in debug builds; libraries shouldn't have print statements. so ifdef it or (better imo) just remove all of them.

Errr...this is in s_server, not in the library? It's not a debug thing. It implements the "-status_verbose" feature, and was done this way in the existing code.

right, forgot that. ok.

richsalz · 2016-11-17T02:11:30Z

apps/s_server.c

+        }
+        resp = d2i_OCSP_RESPONSE_bio(derbio, NULL);
+        BIO_free(derbio);
+        if (!resp) {


test against NULL and remove the printf.

Test against NULL added, but kept the printf for same reason as above.

richsalz · 2016-11-17T02:18:43Z

ssl/tls13_enc.c


+/*
+ * Generates the mac for the Finished message.
+ *


delete this "whitespace"

richsalz · 2016-11-17T02:19:17Z

ssl/tls13_enc.c

+ * There isn't really a key block in TLSv1.3, but we still need this function
+ * for initialising the cipher and hash.
+ *
+ * Returns 1 on success or 0 on failure.


put this "returns" statement inline.

Done. And other similar instances in this file.

richsalz · 2016-11-17T02:22:07Z

ssl/tls13_enc.c

+        }
+    } else if (EVP_CIPHER_mode(ciph) == EVP_CIPH_CCM_MODE) {
+        int taglen;
+        if (s->s3->tmp.new_cipher->algorithm_enc


blank line before decl.

richsalz · 2016-11-17T02:23:37Z

ssl/tls13_enc.c

+    return 1;
+}

+const unsigned char client_handshake_traffic[] =


static? and perhaps move into the function that uses them if it's only one function.

richsalz · 2016-11-17T02:25:57Z

test/asynciotest.c

+                wholebody = payload;
+                if (contenttype == SSL3_RT_HANDSHAKE
+                        && !PACKET_get_1(&wholebody, &msgtype))
+                    abort();


you sure abort is the right thing do do, even in a test?

It's what we do already in that test and in others. I think its ok.

The SSL_IS_TLS13() macro wasn't quite right. It would come back with true in the case where we haven't yet negotiated TLSv1.3, but it could be negotiated.

mattcaswell · 2016-11-21T13:45:06Z

New commits pushed addressing all feedback so far.

richsalz

assuming you address this, I approve.

richsalz · 2016-11-21T15:16:52Z

ssl/statem/statem_clnt.c

+ * server's initial flight. In TLS1.3 this is after the Server Finished message.
+ * In <=TLS1.2 this is after the ServerDone message.
+ *
+ * Returns 1 on success or 0 on failure.


richsalz · 2016-11-21T15:20:31Z

ssl/tls13_enc.c

+    /* TODO(size_t): convert me */
+    keylen = EVP_CIPHER_key_length(ciph);
+
+    if (EVP_CIPHER_mode(ciph) == EVP_CIPH_GCM_MODE)


consider using a switch on the cipher mode, for future expansion? not a requirement.

Not done. A later commit does some simplification in this area, so I'll leave it be.

I should have said a later commit in a later PR

richsalz · 2016-11-21T15:21:34Z

ssl/tls13_enc.c

+    }
+#endif
+
+    OPENSSL_cleanse(secret, sizeof(secret));


add a "ret" variable and remove the duplicate lines? it's consistent with what we do.

richsalz · 2016-11-21T15:22:46Z

test/clienthellotest.c

        ctx = SSL_CTX_new(TLS_method());
+
+        /*
+         * This test is testing session tickets for <= TLS1.2. It isn't relevant


"Testing for session tickets <=- TLS1.2; not relevant for 1.3" might fit into a single line. If not, never mind.

richsalz · 2016-11-21T15:34:58Z

apps/s_server.c

-    if (ret != SSL_TLSEXT_ERR_OK)
-        ERR_print_errors(bio_err);
-    if (aia) {
+    if (aia != NULL) {


Can you add a comment that says "if we parsed AIA, we need to free; otherwise they were copied and we don't" or something like that.,

mattcaswell · 2016-11-23T15:43:18Z

All required changed made, so I've taken your plus one and pushed. Thanks.

mattcaswell added 11 commits November 16, 2016 10:14

Implement tls13_change_cipher_state()

5016af1

Create the Finished message payload

a3eee5e

The previous commit had a dummy payload for the Finished data. This commit fills it in with a real value.

Fix the tests following the state machine changes for TLSv1.3

3e96cee

Ensure the end of first server flight processing is done

30d066c

There is a set of miscellaneous processing for OCSP, CT etc at the end of the ServerDone processing. In TLS1.3 we don't have a ServerDone, so this needs to move elsewhere.

Ensure unexpected messages are handled consistently

57d0940

In one case we weren't always sending an unexpected message alert if we don't get what we expect.

Fix some TLSProxy warnings

7c62c22

After the client processes the server's initial flight in TLS1.3 it may respond with either an encrypted, or an unencrypted alert. We needed to teach TLSProxy about this so that it didn't issue spurious warnings.

Add a test for the TLSv1.3 state machine

3ba6896

Add an s_server capability to read an OCSP Response from a file

edfae57

Current s_server can only get an OCSP Response from an OCSP responder. This provides the capability to instead get the OCSP Response from a DER encoded file. This should make testing of OCSP easier.

Extend test_tls13messages

4427701

Add various different handshake types that are possible.

mattcaswell force-pushed the tls1_3-use-secrets branch from 89dd6b9 to 4427701 Compare November 16, 2016 10:16

FdaSilvaYY reviewed Nov 16, 2016

View reviewed changes

richsalz suggested changes Nov 17, 2016

View reviewed changes

mattcaswell mentioned this pull request Nov 17, 2016

TLSv1.3: Use TLSv1.3 style nonce construction (new approach) #1947

Closed

2 tasks

mattcaswell added 2 commits November 21, 2016 13:34

Fix SSL_IS_TLS13(s)

b6defa2

The SSL_IS_TLS13() macro wasn't quite right. It would come back with true in the case where we haven't yet negotiated TLSv1.3, but it could be negotiated.

Fix some style issues identified during review

9f6efe4

richsalz approved these changes Nov 21, 2016

View reviewed changes

richsalz reviewed Nov 21, 2016

View reviewed changes

mattcaswell mentioned this pull request Nov 21, 2016

TLSv1.3: Record Construction #1975

Closed

2 tasks

mattcaswell closed this Nov 23, 2016

Uh oh!

Comments

Conversation

mattcaswell commented Nov 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description of change

Uh oh!

richsalz commented Nov 15, 2016

Uh oh!

mattcaswell commented Nov 16, 2016

Uh oh!

FdaSilvaYY Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Nov 21, 2016

Uh oh!

richsalz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Nov 15, 2016 •

edited

Loading

FdaSilvaYY Nov 16, 2016 •

edited

Loading