Add TLS v1.3 as an option by SparkiDev · Pull Request #661 · wolfSSL/wolfssl

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

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

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

src/tls13.c

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

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

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

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

src/tls.c

Building on macOS-clang, PSK_DHE_KE needs PSK enabled.

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

src/tls.c

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

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

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

src/keys.c

Formatting: Please move the paren up a line.

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

src/keys.c

Formatting: Please move the paren up a line.

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

src/keys.c

Formatting: Please move the paren up a line.

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

src/keys.c

Formatting: Please move the paren up a line.

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

src/internal.c

"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

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

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

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

src/tls13.c

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

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

src/tls.c

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

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

src/tls.c

infer static analysis: extension may be NULL

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

src/tls.c

infer static analysis: extension may be NULL

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

examples/client/client.c

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

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

update_keys should be made non-blocking.

Changes made to deal with WANT_WRITE.

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

src/internal.c

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

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

why not use the is TLS 1.3 function?

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

wolfssl/ssl.h

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

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

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

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

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

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

Conversation

SparkiDev commented Dec 12, 2016

Uh oh!

JacobBarthelmeh commented Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaleb-himes commented Dec 28, 2016

Uh oh!

ejohnstown 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!

ejohnstown 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!

ejohnstown commented Jan 13, 2017

Uh oh!

ejohnstown 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!

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

JacobBarthelmeh commented Dec 28, 2016 •

edited

Loading

SparkiDev Mar 2, 2017 •

edited

Loading