Mandatory parts of TLS 1.3 #174

tomato42 · 2017-07-26T18:22:25Z

Work in progress on TLS 1.3 support.

Includes:

Will fix #39 once ready

more clever ciphersuite selection (for external PSKs) will be handled in #213

This change is

t8m

Reviewed 36edd15..23280e3

t8m · 2018-03-15T16:14:33Z

tlslite/handshakehelpers.py

        finished_key = HKDF_expand_label(binder_key, b"finished", b"", key_len,
                                         prf)
        binder = secureHMAC(finished_key, handshake_hash.digest(prf), prf)
        return binder

    @staticmethod
-    def update_binders(client_hello, handshake_hashes, psk_configs):
+    def calc_res_binder_psk(iden, resum_master_secret, tickets, ticket_hash):


The resum abbreviation in resum_master_secret looks to me a little bit awkward - I'd prefer res or rsmpt or resumpt. But certainly not a blocker.

t8m · 2018-03-15T16:24:04Z

tlslite/handshakehelpers.py

-                while config[0] != iden.identity:
+            # identities that are tickets don't carry PSK directly
+            if iden.identity in ticket_idens:
+                binder_hash = 'sha256' if len(resum_master_secret) == 32 else \


If the hash used in resumption psk calculation really depends only on length of the master secret it might make sense to move this calculation to calc_res_binder_psk() and spare the ticket_hash argument.

it essentially does (the relation is other way round, but with just two hashes to choose from, there's no difference)

I was thinking about SHA-3, but we can cross that bridge when we get there

it still needs to stay here for _calc_binder below...

t8m · 2018-03-15T16:41:31Z

tlslite/handshakesettings.py

+                             "{0}".format(other.ticketCipher))
+
+        if any(len(i) not in set([16, 32]) for i in other.ticketKeys):
+            raise ValueError("session ticket encryption keys must be 16 or 32"


Nit: Should the "session ticket..." start with upper case? "Session ticket ...."

t8m · 2018-03-15T16:41:52Z

tlslite/handshakesettings.py

+                             "bytes long")
+
+        if not 0 < other.ticketLifetime <= 7 * 24 * 60 * 60:
+            raise ValueError("ticket lifetime must be a positive integer "


Similarly as above "Ticket lifetime...."

t8m · 2018-03-15T17:08:21Z

tlslite/handshakesettings.py

+            raise ValueError("Unknown MAC name: {0}".format(unknown))
+
+        unknown = not_matching(other.keyExchangeNames, KEY_EXCHANGE_NAMES)
+        if unknown:


Nit: You inconsistently renamed the unknownXyz variables here but the other ones kept the original name. Not sure if you want to change that.

yeah, it's probably better to be consistent

add to the session object keying material necessary for TLS Exporter and session resumption to work

when sending the ticket we need to obfuscate the time, so we need to know when we received it

since the client saves session, not connection object keep the tickets inside session too since we're copying reference, the later updates to it will be reflected in the session object

fix the nits mentioned by codeclimate: - too long methods - similar code - whitespace issues - method complexity - redundant backslashes - bad line continuation

if client_hello is (3, 4) or greater, clamp it to (3, 3) i.e. TLS 1.2

compare the true highest advertised protocol version with the supported version for FALLBACK_SCSV if the client advertises TLS 1.2 (by sending some very high client_version, but no extension with protocol versions) and a FALLBACK_SCSV, that's an inappropriate fallback

draft-ietf-tls-tls13-25 calls for inclusion of Record Layer header as the associated data for the AEAD ciphers, update the code to do that

t8m

I reviewed the last two commits. No real bugs spotted.

t8m · 2018-03-16T12:34:02Z

tlslite/recordlayer.py

-            authData = bytearray(0)
+            # this is essentially a Record Layer header
+            authData = bytearray([recordType, 3, 3,
+                                  len(buf) // 256, len(buf) % 256])


I understand that this is probably easier to recreate the record layer header, but wouldn't it be more future-proof to carry it over from the actual data received?

this has the side-effect of enforcing them to those values, and I want to be strict on receiving

(on sending side I'm setting them to configured values as I want to be able to test this in other implementations)

But on the receiving side you then do not authenticate what was sent over the wire. I do not know whether there is any theoretical attack possible this way though. The enforcing to the expected values should be done explicitly I think.

fixed: 58a705e

tomato42 · 2018-03-16T13:38:44Z

@t8m Thanks!

explicitly verify the record layer values for encrypted messages match the ones mandated by TLS 1.3 specification

t8m

Reviewed

dweekly · 2018-11-23T17:38:31Z

scripts/tls.py

+    session.resumable = True
+
+    print("Received {0} ticket[s]".format(len(connection.tickets)))
+    assert connection.tickets is session.tickets


Note that this assertion triggers when connecting to nearly any HTTPS server on the internet.

$ tls.py client www.yahoo.com:443
Handshake success
Handshake time: 0.181 seconds
Version: TLS 1.2
Cipher: aes128gcm python
Ciphersuite: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
No client certificate provided by peer
Server X.509 SHA1 fingerprint: 3914af80223c833f26df001cbf342eff8a31aba1
Key exchange signature: rsa_pkcs1_sha512
Group used for key exchange: secp256r1
SNI: www.yahoo.com
Next-Protocol Negotiated: None
Encrypt-then-MAC: False
Extended Master Secret: False
Received 0 ticket[s]
Traceback (most recent call last):
File "/usr/local/bin/tls.py", line 541, in
clientCmd(sys.argv[2:])
File "/usr/local/bin/tls.py", line 377, in clientCmd
assert connection.tickets is session.tickets
AssertionError

$ tls.py client www.amazon.com:443
Handshake success
Handshake time: 0.186 seconds
Version: TLS 1.2
Cipher: aes128gcm python
Ciphersuite: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
No client certificate provided by peer
Server X.509 SHA1 fingerprint: 0e6bdcae568e3613c6d173c9b112c357ff5967b2
Key exchange signature: rsa_pkcs1_sha512
Group used for key exchange: secp256r1
SNI: www.amazon.com
Next-Protocol Negotiated: None
Encrypt-then-MAC: False
Extended Master Secret: False
Received 0 ticket[s]
Traceback (most recent call last):
File "/usr/local/bin/tls.py", line 541, in
clientCmd(sys.argv[2:])
File "/usr/local/bin/tls.py", line 377, in clientCmd
assert connection.tickets is session.tickets
AssertionError

$ tls.py client www.google.com:443
Handshake success
Handshake time: 0.068 seconds
Version: TLS 1.2
Cipher: chacha20-poly1305 python
Ciphersuite: TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
No client certificate provided by peer
Server X.509 SHA1 fingerprint: 62231769ed0bf01ecce073c24a668578c5ffb66e
Key exchange signature: rsa_pss_rsae_sha256
Group used for key exchange: x25519
SNI: www.google.com
Next-Protocol Negotiated: None
Encrypt-then-MAC: False
Extended Master Secret: True
Received 0 ticket[s]
Traceback (most recent call last):
File "/usr/local/bin/tls.py", line 541, in
clientCmd(sys.argv[2:])
File "/usr/local/bin/tls.py", line 377, in clientCmd
assert connection.tickets is session.tickets
AssertionError

please file an issue

tomato42 force-pushed the tls-1.3 branch 2 times, most recently from fafca3c to ef2cb66 Compare August 3, 2017 17:52

tomato42 added this to the v0.8.0 milestone Aug 3, 2017

tomato42 self-assigned this Aug 3, 2017

tomato42 added the enhancement new feature to be implemented label Aug 3, 2017

tomato42 force-pushed the tls-1.3 branch 9 times, most recently from 8483965 to 1ae1280 Compare August 11, 2017 18:08

tomato42 force-pushed the tls-1.3 branch 4 times, most recently from 6781e69 to 51f4e4e Compare August 18, 2017 18:13

tomato42 force-pushed the tls-1.3 branch 3 times, most recently from 982b4a7 to 608d7ba Compare August 29, 2017 17:52

tomato42 force-pushed the tls-1.3 branch from 608d7ba to 28e05f4 Compare August 31, 2017 18:20

tomato42 changed the title ~~[WIP] TLS 1.3~~ [WIP] Mandatory parts of TLS 1.3 Aug 31, 2017

tomato42 force-pushed the tls-1.3 branch 6 times, most recently from 876a1f7 to c7f4251 Compare September 5, 2017 17:50

tomato42 force-pushed the tls-1.3 branch from c7f4251 to a2d8b14 Compare November 10, 2017 18:29

tomato42 force-pushed the tls-1.3 branch from 5be9a4a to bbed2bb Compare March 15, 2018 15:58

t8m reviewed Mar 15, 2018

View reviewed changes

tomato42 mentioned this pull request Mar 15, 2018

TLS 1.3 1-RTT noHRR tlsfuzzer/tlsfuzzer#189

Merged

tomato42 added 18 commits March 16, 2018 13:09

psk support in tls.py script on server side

351c62a

simple PSK integration test

7ce663c

session obj TLS 1.3 exporter and resumption data

d9e855b

add to the session object keying material necessary for TLS Exporter and session resumption to work

save the time when ticket was received

6c28b6b

when sending the ticket we need to obfuscate the time, so we need to know when we received it

keep reference to tickets in session

7b44f57

since the client saves session, not connection object keep the tickets inside session too since we're copying reference, the later updates to it will be reflected in the session object

handshake helpers for client resumption

31400ea

TLS1.3 resumption support in client

1ee4c1b

resumption test in tls.py script

2e9f66b

handshake settings for server side support of session tickets

e059118

session ticket payload

daf8f52

add support for sending TLS 1.3 tickets in server

fe471d6

restoring session from session ticket

2507236

ticket support in tls.py server

6296421

clean up handshakesettings

578f38f

fix the nits mentioned by codeclimate: - too long methods - similar code - whitespace issues - method complexity - redundant backslashes - bad line continuation

don't try to negotiate TLS 1.3 with client_version

9a53913

if client_hello is (3, 4) or greater, clamp it to (3, 3) i.e. TLS 1.2

draft-25: include record layer into AEAD additional data

12aaaa1

draft-ietf-tls-tls13-25 calls for inclusion of Record Layer header as the associated data for the AEAD ciphers, update the code to do that

draft-26: update version number

dc07b02

tomato42 force-pushed the tls-1.3 branch from bbed2bb to dc07b02 Compare March 16, 2018 12:27

t8m reviewed Mar 16, 2018

View reviewed changes

verify TLS 1.3 record header

58a705e

explicitly verify the record layer values for encrypted messages match the ones mandated by TLS 1.3 specification

t8m approved these changes Mar 16, 2018

View reviewed changes

tomato42 merged commit 7b42d3d into master Mar 16, 2018

tomato42 deleted the tls-1.3 branch March 16, 2018 18:07

dweekly reviewed Nov 23, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mandatory parts of TLS 1.3 #174

Mandatory parts of TLS 1.3 #174

tomato42 commented Jul 26, 2017 •

edited

Loading

t8m left a comment

t8m Mar 15, 2018

t8m Mar 15, 2018

tomato42 Mar 16, 2018

tomato42 Mar 16, 2018

t8m Mar 16, 2018

t8m Mar 15, 2018

t8m Mar 15, 2018

t8m Mar 15, 2018

tomato42 Mar 16, 2018

t8m left a comment

t8m Mar 16, 2018

tomato42 Mar 16, 2018

t8m Mar 16, 2018

tomato42 Mar 16, 2018

tomato42 commented Mar 16, 2018

t8m left a comment

dweekly Nov 23, 2018

tomato42 Nov 23, 2018

dweekly Nov 23, 2018

Mandatory parts of TLS 1.3 #174

Mandatory parts of TLS 1.3 #174

Conversation

tomato42 commented Jul 26, 2017 • edited Loading

t8m 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

t8m 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

tomato42 commented Mar 16, 2018

t8m 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

tomato42 commented Jul 26, 2017 •

edited

Loading