TLS 1.3 draft 18 - Githubissues

alexmgr commented 7 years ago

This is a big change, I had to clean up a lot of stuff in order to get TLS 1.3 functional.

Support for both client and server side TLS 1.3 (NIST curves + AES ciphers). No FF yet
Refactoring of stacked TLSHandshakes. This breaks the previous interface.
Encapsulation of tls_to_raw inside the TLSSocket. Allows transparent encryption, whilst preserving the ability to modify padding and macs
TLSFinish have to be constructed explicitly. This breaks the previous interface
No support for 0-RTT yet
Updated a bunch of examples to stick to the new interface
Added TLS 1.3 examples
Still needs parsing of PSK extensions, status request and Session Ticket

alexmgr commented 7 years ago

OK, the UT breaks because of the import of from scapy.all import conf in ssl_tls. WTF???

tintinweb commented 7 years ago

sweet xmas present! :)

tintinweb commented 7 years ago

:bowtie: rebased off master on top of integration suite.

tintinweb commented 7 years ago

both integration tests fail against openssl because of an issue when assembling TLSRecords. It looks like the content_type field is not updated anymore when the packet is serialized.

on master (correct):

>>> TLSRecord()/TLSHandshake()
<TLSRecord  content_type=handshake |<TLSHandshake  |>>
>>> str(TLSRecord()/TLSHandshake())
'\x16\x03\x01\x00\x04\x01\x00\x00\x00'

on tls1.3 (wrong):

>>> TLSRecord()/TLSHandshake()
<TLSRecord  |<TLSHandshake  |>>
>>> str(TLSRecord()/TLSHandshake())
'\x17\x03\x01\x00\x04\x01\x00\x00\x00'
# check dissect for expected tlshandshake record
>>> SSL('\x16\x03\x01\x00\x04\x01\x00\x00\x00')
<SSL  records=[<TLSRecord  content_type=handshake version=TLS_1_0 length=0x4 |<TLSHandshakes  handshakes=[<TLSHandshake  type=client_hello length=0x0 |>] |>>] |>

the \x17 is content_type application_data (the default value of the TLSRecord layer) instead of expected type handshake (\x16). Dissection seems to work though. It must be TLSRecord.do_build -> StackedLenPacket.do_build then.

tintinweb commented 7 years ago

/+ unittest

======================================================================
FAIL: test_pkt_tls_serializes_as_expected (tests.test_ssl_tls.TestTLSRecord)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/tin/scapy-ssl_tls/scapy-ssl_tls/tests/test_ssl_tls.py", line 85, in test_pkt_tls_serializes_as_expected
    self.assertEqual(str(tls.TLSRecord()/tls.TLSHandshake()), self.empty_tls_handshake_serialized_expected)
AssertionError: '\x17\x03\x01\x00\x04\x01\x00\x00\x00' != '\x16\x03\x01\x00\x04\x01\x00\x00\x00'

alexmgr commented 7 years ago

Hi @tintinweb,

That's "normal" ;) I had to fully support stacked handshakes for a few TLS 1.3 related things, so I had to change the handling of TLSHandshake. TLSHandshake is now a PacketListField in TLSHandshakes, which is just a dummy container for a list of stacked handshakes. TLSHandshakes is the one bind_layer'd to the TLSRecord now.

All that to say that you need to create handshake packets using TLSRecord() / TLSHandshakes(handshakes=[TLSHandshake()]) now. When I get a sec, I'll fix the integration test to reflect that.

Thanks for checking this out! Alex

alexmgr commented 7 years ago

Hi @tintinweb,

I've fixed all external integration UTs, so we've got a nice tool for new features now. Thanks for that, definitely useful. I haven't fixed the various failures in examples though. It's probably going to take a while, since I'm busy on other stuff currently.

In the meantime, could we not merge anything into master? Rebase is kind of painful on this branch since it has changed so much. Thoughts?

Alex

tintinweb commented 7 years ago

Hey @alexmgr,

we should definitely merge this asap and build on top of it. Feel free to merge when you're ready and I'll try to alloc some time to fix the examples and document the interface changes for the next release.

Thanks for the PR, really great work! 👍

cheers tin.

alexmgr commented 7 years ago

Thanks @tintinweb,

I'd rather keep it in this separate branch until all UTs pass.

Whenever we merge this, we should cut a new major version number to mark for the interface change.

What do you think? Alex

tintinweb commented 7 years ago

make sense, especially making sure to keep master in an as stable as possible condition. Regarding the new major: There's already a draft v1.3 to documents interface changes to v1.2.3.1 that we could rename to v2.0 to clearly signal that there is a possibility that some scripts might require adjustments due to the interface changes. I'd also make sure to clearly communicate the changes in the release notes to ease with adapting the changes.

what is the current status of this branch? in progress or just waiting for the examples/UTs to be fixed?

alexmgr commented 7 years ago

Branch is good to go for me. "Only" needs example fixing.

It still needs a better interface to building KeyShares but that can be done later. 0-RTT needs a bit of tweaking to get working (I have some changes on a local branch), and finally the KeyUpdate logic is not there. All this can be cleanly done later. But for now, I think we should merge it into master when examples are fixed. TLS 1.3 1-RTT works in both client and server scenarios.

I don't have a ton of time right now to fix the examples, but I'll try and get around to fixing a few in the upcoming weeks.

Cheers, Alex

tintinweb commented 7 years ago

fixed the missing TLSHandshakes middlelayer
added tls_1_3-client.py to integration_examples (working fine 👍 )
Fixed unittest for cve-2014-3466.py which was actually running tls_server_automata.py. copy paste issue.
Fixed TLSCertificatelist for tls_server_automata

This leaves us with 5 of 20 cases failing with two different root causes:

1) I've digged into the KeyError: 'Unhandled encryption for TLS protocol: TLS Record Exception which causes test_client_hello_with_session_ticket_py, test_padding_and_mac_checks_py, test_security_scanner_client_mode and tls_server_automata to fail. The exception is caused by a missing handler for TLSFinished (and TLSRecord in another case) in to_raw. The handler for TLSFinished was removed with this branch so I guess that this layer is now handled differently? What would be the correct way to send a non tls1.3 TLSFinished? Both security_scanner and padding_checks are failing because they're manipulating the encrypted packet which causes a TLSRecord to be passed to to_raw and to_raw then raises an exception because there is no handler for this layer type. In the past to_raw never had to handle TLSRecords directly but TLSCiphertext or TLSPlaintext instad. Not quite sure what causes this atm but I'll dig some deeper in the next days.

2) tls_client_automata_py fails, because the server_ctx.crypto_ctx was not initialized. Investigation pending.

   Traceback (most recent call last):
    File ".scapy/automaton.py", line 548, in _do_control
      state = iterator.next()
    File ".scapy/automaton.py", line 595, in _do_iter
      self._run_condition(cond, *state_output)
    File ".scapy/automaton.py", line 495, in _run_condition
      cond(self,*args, **kargs)
    File ".scapy_ssl_tls/ssl_tls_automata.py", line 23, in wrapped_f
      return f(*args, **kwargs)
    File ".scapy_ssl_tls/ssl_tls_automata.py", line 178, in recv_server_hello
      p = self.tlssock.recvall(timeout=self.timeout)
    File ".scapy_ssl_tls/ssl_tls.py", line 1279, in recvall
      records = TLS("".join(resp), ctx=self.tls_ctx)
    File ".scapy/base_classes.py", line 196, in __call__
      i.__init__(*args, **kargs)
    File ".scapy_ssl_tls/ssl_tls.py", line 1310, in __init__
      Packet.__init__(self, *args, **fields)
    File ".scapy/packet.py", line 85, in __init__
      self.dissect(_pkt)
    File ".scapy/packet.py", line 618, in dissect
      s = self.do_dissect(s)
    File ".scapy_ssl_tls/ssl_tls.py", line 1342, in do_dissect
      payload = self.do_decrypt_payload(payload)
    File ".scapy_ssl_tls/ssl_tls.py", line 1360, in do_decrypt_payload
      cleartext = self.tls_ctx.server_ctx.crypto_ctx.decrypt(encrypted_payload,
    AttributeError: 'NoneType' object has no attribute 'decrypt'

I am confident that fixing the to_raw issue will most likely resolve all issue in 1) 👍 Looks like we're on track :relaxed:

cheers, tin

alexmgr commented 7 years ago

@tintinweb, nice stuff, thanks for checking this out.

For 1), it's expected for the TLSFinish handler to be removed from to_raw. The finished message is just treated as any TLSHandshake message now. In the caller of to_raw, replace old style:

tls_socket.do_round_trip(to_raw(TLSFinished(), tls_socket.tls_ctx))

with new style:

tls_socket.do_round_trip(TLSHandshakes(handshakes=[TLSHandshake() /
TLSFinished(data=tls_socket.tls_ctx.get_verify_data())]))

Transparent encryption couldn't work nicely otherwise. I generally tried to remove some of the special cases I'd added which didn't really makes sense.

Regarding 2), not sure ;)

I'll investigate when I get a sec. Alex

tintinweb commented 7 years ago

@alexmgr excellent, I'll check if I can fix this tomorrow! Regarding releases and versioning. Do you want me to rename the draft release v1.3 (our prev. next release) to v2.0 indicating that ther're major api changes? I'll also have a quick look if it is possible to design the TLSHandshake layer in a way it is possible to both use TLSHandshakes or TLSHandshake layers directly. Just for the sake of compatibility with release/1.2.3.

alexmgr commented 7 years ago

Regarding releases and versioning. Do you want me to rename the draft release v1.3 (our prev. next release) to v2.0 indicating that ther're major api changes?

Maybe we can release whatever is in master now as 1.3? And then cut another release for v2.0 with the TLS 1.3 stuff + interface breakage?

I'll also have a quick look if it is possible to design the TLSHandshake layer in a way it is possible to both use TLSHandshakes or TLSHandshake layers directly. Just for the sake of compatibility with release/1.2.3.

Yeah, could be good. It's probably a pain to implement though, some custom do_build, 'do_dissect' in the TLSRecord layer.

tintinweb commented 7 years ago

❤ new interface!

fixed all but the example for the session ticket which fails with a local s_server routines:SSL3_READ_BYTES:bad change cipher spec but passes when being tested with external webservers.
added a ticket member to the TLSSessionCtx which is being updated when a new Session Ticket is received by the server or a client attempts to resume a session witha ticket. However it does not null the ticket if the server rejected it.
created draft for release v2.0 and updated draft for release v1.3 - I wouldn't want to release without the examples (session_sniffer decryption) being fixed first.

tintinweb commented 7 years ago

Hi @alexmgr ,

:boom: - all green. It took another quick debugging session to fix session resumption. The problem was that for the abbreviated handshake the order of the CCS messages is different. For the full handshake, the client sends it first and for the abbreviated it is the server. Our TLSSessionCtx.__handle_ccs did not take this into account only counting the number of CCS seen and infering that the first CCS always means the client switched to encrypted mode. In order to fix that I've added an optional origin tag to TLSSessionCtx._process which can either be the packet being processed is a client message or the packet being processed is a server message to distinguish serverCCS from clientCCS. The minimal fix was to add a param to _process. Let me know what you think about this fix (1babef0)

cheers, tin

alexmgr commented 7 years ago

@tintinweb Beautiful! Thanks for cleaning up after my mess ;) Overall, looks good to merge.

Good catch around resumption, I'd definitely broken that use-case.

I've got some local 0-rtt stuff working locally. I think I'll put that in another PR once we merge this, this is becoming crazy.

tintinweb commented 7 years ago

👍 thanks for putting this together!

Btw. I've tagged prevs. master as v1.3-pre to make it easier to branch off v1.3 if we decide to release before v2.0. beware that v1.3 already introduces minor interface changes and requires some fixups (partially already present in current master/ release/v1.2.3) to be in a releasable condition. Not quite sure if it is worth the hassle. What do you think?

tintinweb / scapy-ssl_tls

TLS 1.3 draft 18 #85