Add TLS v1.3 as an option #661

SparkiDev · 2016-12-12T05:17:01Z

No description provided.

JacobBarthelmeh · 2016-12-28T19:06:06Z

retest this please Jenkins, was updating Windows jobs.

kaleb-himes · 2016-12-28T21:59:54Z

Jenkins, something went wrong, could you please re-run windows PR test

ejohnstown

I pulled this PR against commit 483e461, as it won't merge cleanly with master.

There are a few build issues on macOS with clang that I'm not seeing on Ubuntu with GCC. I think there is something glitchy with the configure.ac for that.

I'll dig into that, and the code in general, a little more over the week.

ejohnstown · 2017-01-09T19:01:15Z

src/tls13.c

+ * c    The opaque data.
+ * u32  Unsigned 32-bit value.
+ */
+static INLINE void ato32(const byte* c, word32* u32)


When building on macOS-clang without session tickets enabled, this function is unused.

ejohnstown · 2017-01-09T19:06:22Z

src/tls13.c

+        if (gettimeofday(&now, 0) < 0)
+            return GETTIME_ERROR;
+        /* Convert to milliseconds number. */
+        return now.tv_sec * 1000 + now.tv_usec / 1000;


Building with macOS-clang, this has a loss-of-precision error for the tv values.

Casting to word32 now

ejohnstown · 2017-01-09T19:08:26Z

src/tls13.c

+    return 0;
+}
+
+/* Handle a TLS v1.3 Certif


Building on macOS-clang, session doesn't have cipherSuite0 and cipherSuite when session certs isn't enabled.

ejohnstown · 2017-01-09T19:11:33Z

src/tls.c

+                milli = TimeNowInMilliseconds() - sess->ticketSeen +
+                        sess->ticketAdd;
+                /* Pre-shared key is mandatory extension for resumption. */
+                ret = TLSX_PreSharedKey_Use(ssl, sess->ticket, sess->ticketLen,


Building on macOS-clang, TLSX_PreSharedKey_Use() needs PSK enabled.

ejohnstown · 2017-01-09T19:12:55Z

src/tls.c

+                modes = 1 << PSK_KE;
+            #if !defined(NO_DH) || defined(HAVE_ECC)
+                if (!ssl->options.noPskDheKe)
+                    modes |= 1 << PSK_DHE_KE;


Building on macOS-clang, PSK_DHE_KE needs PSK enabled.

ejohnstown · 2017-01-09T19:13:25Z

src/tls.c

+                if (!ssl->options.noPskDheKe)
+                    modes |= 1 << PSK_DHE_KE;
+            #endif
+                ret = TLSX_PskKeModes_Use(ssl, modes);


Building on macOS-clang, TLSX_PskKeModes_Use() needs PSK enabled.

ejohnstown

Just a few small formatting tweaks.

ejohnstown · 2017-01-13T16:50:20Z

wolfssl/ssl.h

@@ -200,6 +200,9 @@ enum AlertDescription {
    certificate_expired             =  45,
    certificate_unknown             =  46,
    illegal_parameter               =  47,
+#ifdef WOLFSSL_TLS13


Doesn't need to be optional. decode_error is used in master now.

ejohnstown · 2017-01-13T16:51:48Z

src/keys.c

@@ -2427,14 +2531,16 @@ static int SetKeys(Ciphers* enc, Ciphers* dec, Keys* keys, CipherSpecs* specs,
                                      specs->key_size);
                if (gcmRet != 0) return gcmRet;
                XMEMCPY(keys->aead_enc_imp_IV, keys->client_write_IV,
-                        AESGCM_IMP_IV_SZ);
+                        AEAD_MAX_IMP_SZ
+                        );


Formatting: Please move the paren up a line.

ejohnstown · 2017-01-13T16:51:57Z

src/keys.c

            }
            if (dec) {
                gcmRet = wc_AesGcmSetKey(dec->aes, keys->server_write_key,
                                      specs->key_size);
                if (gcmRet != 0) return gcmRet;
                XMEMCPY(keys->aead_dec_imp_IV, keys->server_write_IV,
-                        AESGCM_IMP_IV_SZ);
+                        AEAD_MAX_IMP_SZ
+                        );


Formatting: Please move the paren up a line.

ejohnstown · 2017-01-13T16:52:04Z

src/keys.c

@@ -2443,14 +2549,16 @@ static int SetKeys(Ciphers* enc, Ciphers* dec, Keys* keys, CipherSpecs* specs,
                                      specs->key_size);
                if (gcmRet != 0) return gcmRet;
                XMEMCPY(keys->aead_enc_imp_IV, keys->server_write_IV,
-                        AESGCM_IMP_IV_SZ);
+                        AEAD_MAX_IMP_SZ
+                        );


Formatting: Please move the paren up a line.

ejohnstown · 2017-01-13T16:52:16Z

src/keys.c

            }
            if (dec) {
                gcmRet = wc_AesGcmSetKey(dec->aes, keys->client_write_key,
                                      specs->key_size);
                if (gcmRet != 0) return gcmRet;
                XMEMCPY(keys->aead_dec_imp_IV, keys->client_write_IV,
-                        AESGCM_IMP_IV_SZ);
+                        AEAD_MAX_IMP_SZ
+                        );


Formatting: Please move the paren up a line.

ejohnstown · 2017-01-13T16:54:20Z

src/internal.c

+        WOLFSSL_MSG("    decode error");
+    }
+    if (*type == illegal_parameter) {
+        WOLFSSL_MSG("    illiegal parameter");


"illegal parameter"

ejohnstown · 2017-01-13T17:38:30Z

We need tests/suites.c updated for handling TLSv1.3 andtest cases for tests/test.conf.

ejohnstown

valgrind leak checks, clang static analysis checks, infer static analysis checks

ejohnstown · 2017-01-13T19:00:46Z

src/tls.c

+        return ret;
+
+    /* Move private key to client entry. */
+    clientKSE->key = serverKSE->key;


I think this is leaking the client ecc_key. Valgrind is reporting

==12644== by 0x418E9D: wolfSSL_Malloc (memory.c:94) ==12644== by 0x42424A: TLSX_KeyShare_GenEccKey (tls.c:4425) ==12644== by 0x4243FF: TLSX_KeyShare_GenKey (tls.c:4481) ==12644== by 0x425086: TLSX_KeyShare_Use (tls.c:5005) ==12644== by 0x42520D: TLSX_KeyShare_SetSupported (tls.c:5104) ==12644== by 0x42530E: TLSX_KeyShare_Establish (tls.c:5148) ==12644== by 0x45C935: VerifyServerSuite (internal.c:18897) ==12644== by 0x45CA3C: MatchSuite (internal.c:18927) ==12644== by 0x429D9D: DoTls13ClientHello (tls13.c:2445) ==12644== by 0x42D143: DoTls13HandShakeMsgType (tls13.c:4688) ==12644== by 0x42D576: DoTls13HandShakeMsg (tls13.c:4817)

and this line looked like it might be losing something. (I added an XFREE of the clientKSE key before the assignment and valgrind accepted it.)

ejohnstown · 2017-01-13T19:28:26Z

src/tls13.c

+    if (outputLen == -1)
+        outputLen = hashSz;
+
+    return HKDF_Expand_Label(output, outputLen, secret, hashSz,


clang scan-build is reporting that protocol and protocolLen are being used without being initialized.

ejohnstown · 2017-01-13T19:38:16Z

src/tls13.c

+#ifndef NO_PSK
+    if (ssl->options.resuming) {
+        TLSX* ext = TLSX_Find(ssl->extensions, TLSX_PRE_SHARED_KEY);
+        PreSharedKey *psk = (PreSharedKey*)ext->data;


infer static analysis: if TLSX_Find returns a null, we would be dereferencing it

ejohnstown · 2017-01-13T19:42:58Z

src/tls.c

+    }
+
+    /* Try to find the key share entry with this group. */
+    keyShareEntry = (KeyShareEntry*)extension->data;


infer static analysis: extension may be NULL at this point.

ejohnstown · 2017-01-13T19:44:23Z

src/tls.c

+    }
+
+    /* Try to find the pre-shared key with this identity. */
+    psk = (PreSharedKey*)extension->data;


infer static analysis: extension may be NULL

ejohnstown · 2017-01-13T19:45:11Z

src/tls.c

+        extension = TLSX_Find(ssl->extensions, TLSX_PSK_KEY_EXCHANGE_MODES);
+    }
+
+    extension->val = modes;


infer static analysis: extension may be NULL

toddouska · 2017-02-19T18:03:33Z

examples/client/client.c

@@ -182,6 +188,17 @@ static int ClientBenchmarkConnections(WOLFSSL_CTX* ctx, char* host, word16 port,
            if (ssl == NULL)
                err_sys("unable to get SSL object");

+    #ifdef WOLFSSL_TLS13
+            if (!benchResume || !noPskDheKe) {
+                if (wolfSSL_UseKeyShare(ssl, WOLFSSL_ECC_SECP256R1)


Let's pick a default key share like p256, have that be the default, and then allow the user to override by calling this function. That eliminates the need for every user to call every time.

Changed to use a default unless user calls wolfSSL_NoKeyShare() in which case no KeyShare data is sent and the HelloRetryRequest message will be used to negotiate group.

toddouska · 2017-02-19T18:04:19Z

examples/client/client.c

+    }
+
+    if (noPskDheKe)
+        wolfSSL_no_dhe_psk(ssl);


Should there be a wolfSSL_CTX_ version of no_dhe_psk as well to make this easier to use?

Added wolfSSL_CTX_no_dhe_psk().

toddouska · 2017-02-19T18:23:17Z

examples/client/client.c

@@ -1543,6 +1654,11 @@ THREAD_RETURN WOLFSSL_THREAD client_test(void* args)
            sleep(1);
    #endif
 #endif /* WOLFSSL_SESSION_EXPORT_DEBUG */
+#ifdef WOLFSSL_TLS13
+    if (updateKeysIVs)
+        wolfSSL_update_keys(ssl);


update_keys should be made non-blocking.

Changes made to deal with WANT_WRITE.

toddouska · 2017-02-19T18:35:40Z

src/internal.c

+        /* TODO: [TLS13] Remove this.
+         * Translate the draft TLS v1.3 version to final version.
+         */
+        if (pv.major == 0x7f) {


should 0x7f as major temp version be a define for easy switching in the future? same with minor?

toddouska · 2017-02-19T18:40:56Z

scripts/tls13.test

+# getting unique port is modeled after resume.test script
+# need a unique port since may run the same time as testsuite
+# use server port zero hack to get one
+port=0


let's also add TLS 1.3 cipher suite tests into test/tls13.conf.

TLS v1.3 cipher suites added to tests/test-tls13.conf. Test conf added to tests/suites.c.

toddouska · 2017-02-19T18:48:36Z

src/internal.c

@@ -13390,6 +13675,13 @@ static void PickHashSigAlgo(WOLFSSL* ssl,
        int                ret;
        word16             extSz = 0;

+
+#ifdef WOLFSSL_TLS13
+        if (ssl->version.major == SSLv3_MAJOR &&


why not use the is TLS 1.3 function?

toddouska · 2017-02-19T18:52:57Z

wolfssl/ssl.h

 WOLFSSL_API int  wolfSSL_write(WOLFSSL*, const void*, int);
 WOLFSSL_API int  wolfSSL_read(WOLFSSL*, void*, int);
 WOLFSSL_API int  wolfSSL_peek(WOLFSSL*, void*, int);
 WOLFSSL_API int  wolfSSL_accept(WOLFSSL*);
+#ifdef WOLFSSL_TLS13
+WOLFSSL_API int  wolfSSL_create_ticket(WOLFSSL* ssl);


Should there be a wolfSSL_CTX version of create_ticket() as well for easier use?

Should create ticket be the default? And instead have no ticket option for those that want to disable tickets?

CTX version added.

When using TLS v1.2 or below then the wolfSSL_CTX_UseSessionTicket() function is called and the new Ticket is sent.
In TLS v1.3 the extension is sent when you can fall back to TLS v1.2 or below. But when you are doing TLS v1.3 you need to send the new ticket if you want to perform resumption in later handshakes. I can see that sending the ticket should be the default behaviour for TLS v1.3 - but not for earlier versions. User only knows whether you negotiated TLS v1.3 once the handshake is happening.

Changing the API to wolfSSL_no_ticket_TLSv13() makes sense to me and internally a new flag would be used called noTicketTls13.
Please advise.

Changed as described.

kaleb-himes · 2017-03-06T22:23:30Z

retest this please jenkins (Windows machine was offline)

dgarske · 2017-03-23T17:10:33Z

wolfssl/internal.h

@@ -2501,6 +2700,9 @@ typedef struct Arrays {
    byte            serverRandom[RAN_LEN];
    byte            sessionID[ID_LEN];
    byte            sessionIDSz;
+    byte            clientSecret[SECRET_LEN];
+    byte            serverSecret[SECRET_LEN];
+    byte            secret[SECRET_LEN];


Hey Sean, looks like these three new members are TLS 1.3 specific and should have the #ifdef WOLFSSL_TLS13 around them? Then the struct Arrays won't grow for non-TLS 1.3 builds.

That's the only issue I saw looking through internal.c and internal.h. Some of the cleanups you've done I also did in async_intelqa, so whoever gets into master second will have some serious rebase/merge conflicts to resolve. ;-)

Thanks David!
Change made.
Good to see we cleaned up the same things. :-) Hopefully my changes won't cause you to many problems when you merge! ;-)

SparkiDev · 2017-03-28T05:44:14Z

Rebase with fixup

These are fixes

dgarske

See comments.

Also had build failure with:

./configure --enable-tls13 --enable-psk CFLAGS="-DHAVE_NULL_CIPHER -DWOLFSSL_STATIC_PSK"

src/tls.c:6747:25: error: implicit declaration of function 'TimeNowInMilliseconds' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
                milli = TimeNowInMilliseconds() - sess->ticketSeen +
                        ^
src/tls.c:6747:57: error: no member named 'ticketSeen' in 'struct WOLFSSL_SESSION'
                milli = TimeNowInMilliseconds() - sess->ticketSeen +
                                                  ~~~~  ^
src/tls.c:6748:31: error: no member named 'ticketAdd' in 'struct WOLFSSL_SESSION'
                        sess->ticketAdd;
                        ~~~~  ^
src/tls.c:6750:56: error: no member named 'ticket' in 'struct WOLFSSL_SESSION'
                ret = TLSX_PreSharedKey_Use(ssl, sess->ticket, sess->ticketLen,
                                                 ~~~~  ^
src/tls.c:6750:70: error: no member named 'ticketLen' in 'struct WOLFSSL_SESSION'
                ret = TLSX_PreSharedKey_Use(ssl, sess->ticket, sess->ticketLen,
                                                               ~~~~  ^
5 errors generated.
make[1]: *** [src/src_libwolfssl_la-tls.lo] Error 1
make[1]: *** Waiting for unfinished jobs....
src/tls13.c:2222:20: error: use of undeclared identifier 'WOLFSSL_TICKET_RET_OK'
        if (ret != WOLFSSL_TICKET_RET_OK)
                   ^
src/tls13.c:2227:24: error: implicit declaration of function 'TimeNowInMilliseconds' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
            int diff = TimeNowInMilliseconds() - ssl->session.ticketSeen;
                       ^
src/tls13.c:2227:63: error: no member named 'ticketSeen' in 'struct WOLFSSL_SESSION'
            int diff = TimeNowInMilliseconds() - ssl->session.ticketSeen;
                                                 ~~~~~~~~~~~~ ^
src/tls13.c:2228:55: error: no member named 'ticketAdd' in 'struct WOLFSSL_SESSION'
            diff -= current->ticketAge - ssl->session.ticketAdd;
                                         ~~~~~~~~~~~~ ^
src/tls13.c:2254:52: error: no member named 'ticketAdd' in 'struct WOLFSSL_SESSION'
            if (current->ticketAge != ssl->session.ticketAdd)
                                      ~~~~~~~~~~~~ ^
src/tls13.c:2299:40: error: no member named 'namedGroup' in 'struct WOLFSSL_SESSION'
        ssl->namedGroup = ssl->session.namedGroup;

Thanks

dgarske · 2017-03-31T20:18:33Z

src/tls.c

+    }
+
+#endif /* WOLFSSL_HAVE_MIN */
+


This isn't required here because we have the misc.c logic at the top. This section with WOLFSSL_HAVE_MIN can be remove.

dgarske · 2017-03-31T20:24:34Z

src/tls.c

@@ -860,12 +896,16 @@ static INLINE word16 TLSX_ToSemaphore(word16 type)

 /** Checks if a specific light (tls extension) is not set in the semaphore. */
 #define IS_OFF(semaphore, light) \
-    ((semaphore)[(light) / 8] ^  (byte) (0x01 << ((light) % 8)))
+    (!(((semaphore)[(light) / 8] &  (byte) (0x01 << ((light) % 8)))))


Was the change from XOR to AND on purpose and if so can you add comment for why it was wrong? Thanks.

When XOR:
0 ^ 0 = 0
0 ^ 1 = 1
1 ^ 0 = 1
1 ^ 1 = 0
Therefore if any other bits (the ones not being checked) are 1 then the result is non-zero or TRUE.
The code with an AND will mask out the bit to be checked.

dgarske · 2017-03-31T20:26:56Z

src/tls.c

+            continue;
+
+        c16toa(current->group, &output[i]);
+        i += 2;


Can you avoid hard coded 2 here? Perhaps sizeof(current->group)?

Changed to KE_GROUP_LEN

dgarske · 2017-03-31T20:32:03Z

wolfcrypt/src/hmac.c

+        return a > b ? b : a;
+    }
+
+#endif /* WOLFSSL_HAVE_MIN */


This section for WOLFSSL_HAVE_MIN can be removed. Was part of a previous cleanup and is now in misc.c

dgarske · 2017-04-14T23:51:44Z

@toddouska and @SparkiDev : Didn't mean to "approve" it. Just dismissed my review comments since they have been addressed. The rebase has been pushed but there is an issue with the TLS 1.3 test script. Looks like something in SendTls13CertificateVerify causing a Decrypt error on the server side. Sent you both an email with details.

SparkiDev added the enhancement label Dec 15, 2016

ejohnstown self-requested a review January 9, 2017 18:15

ejohnstown requested changes Jan 9, 2017

View reviewed changes

ejohnstown reviewed Jan 13, 2017

View reviewed changes

ejohnstown previously requested changes Jan 13, 2017

View reviewed changes

toddouska reviewed Feb 19, 2017

View reviewed changes

SparkiDev force-pushed the tls13 branch from 618eceb to dc9c980 Compare March 8, 2017 01:35

dgarske reviewed Mar 23, 2017

View reviewed changes

SparkiDev force-pushed the tls13 branch from a82d998 to 06a5a8f Compare March 28, 2017 05:43

dgarske requested changes Mar 31, 2017

View reviewed changes

SparkiDev force-pushed the tls13 branch from 61d322d to ccf869c Compare April 4, 2017 02:16

dgarske force-pushed the tls13 branch from ccf869c to aa9e892 Compare April 14, 2017 23:39

dgarske approved these changes Apr 14, 2017

View reviewed changes

dgarske self-assigned this Apr 24, 2017

dgarske force-pushed the tls13 branch from 2cbb277 to 1f0ad35 Compare April 24, 2017 23:28

SparkiDev closed this Apr 26, 2017

SparkiDev force-pushed the tls13 branch from 7e9a53e to dd2915f Compare April 26, 2017 03:48

Add TLS v1.3 as an option #661

Add TLS v1.3 as an option #661

Conversation

SparkiDev commented Dec 12, 2016

JacobBarthelmeh commented Dec 28, 2016 • edited Loading

kaleb-himes commented Dec 28, 2016

ejohnstown 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

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

ejohnstown 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

ejohnstown commented Jan 13, 2017

ejohnstown 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkiDev Mar 2, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaleb-himes commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkiDev commented Mar 28, 2017

dgarske 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

Choose a reason for hiding this comment

dgarske commented Apr 14, 2017

JacobBarthelmeh commented Dec 28, 2016 •

edited

Loading

SparkiDev Mar 2, 2017 •

edited

Loading