Fixed dissection of stacked TLS records

[alexmgr]

• Added helper functions to TLSRecord to help navigate records
• Simplified confusing do_dissect() function
• Added unittest coverage

This allows handling stacked TLS Records, and proper dissection of them.

Still need fixing TLSRecord length, so that generated stacked TLS records have proper lengths. Should happen in a later commit.

[tintinweb]

atm only SSL() handles stacked TLSRecords(). I am not sure if it makes sense to add logic for handling stacked TLSRecords to the actual TLSRecord implementation as

SSL(raw)

should yield something similar to

SSL.records=[TLSRecord/TLSHandshake/..., TLSRecord/TLSHandshake/TLSCertificateList/..., TLSRecord/TLSHandsake/TLSServerHelloDone/..., TLSRecord/TLSAlert ...]

for a raw stream with stacked records. So whenever you want to stack records, just use the SSL compound class and add the individual records to the .records attribute.

this should pull away complexity from TLSRecord layer and leave it as a "per packet" dissector since for dissection SSL() can better decide when to chop the raw stream into individual TLSRecords and for packet building it is up to the developer to feed individual records into the SSL().records pktlist.

what do you think?

[alexmgr]

Put my comments in the bug notes. I'm ok either way, but I think people will expect the following to work:

TLSRecord()/TLSHandshake()/TLSClientKeyExchange(data=tls_ctx.get_encrypted_pms())/TLSRecord()/TLSChangeCipherSpec()/TLSRecord()/to_raw(TLSFinished(), tls_ctx)

This makes me realize that this probably doesn't work today ;), since TLSClientKeyExchange length is going to be miscalculated.

I have no hard feelings, either way works for me, but I find the SSL.records solution less intuitive.

[tintinweb]

I still have concerns allowing the stacking of TLSRecords with custom lengh-fields as you propose but I'm good with it as long as we have arguments in favor.

Here's some thoughts, feel free to add notes:

• By adding magic len-fields that calculate length up to a specific underlayer we're adding side-effects as it is not immediately obvious that stacked TLSRecords() are chopped on certain underlayers. Afaik this is usually not happening when working with other scapy layers. E.g. someone might not be aware of the fact that the proposed

  TLSRecord()/TLSHandshake()/TLSClientKeyExchange(data=tls_ctx.get_encrypted_pms())/TLSRecord()/TLSChangeCipherSpec()/TLSRecord()/to_raw(TLSFinished(), tls_ctx)

  is equivalent to concatenating two individual TLSRecords() like

  TLSRecord()/TLSHandshake()/TLSClientKeyExchange(data=tls_ctx.get_encrypted_pms()) + TLSRecord()/TLSChangeCipherSpec()/TLSRecord()/to_raw(TLSFinished(), tls_ctx)

  which can also be achieved by the much more strict and telling interface that the SSL compound layer currently provides

  SSL(records=[ TLSRecord()/TLSHandshake()/TLSClientKeyExchange(data=tls_ctx.get_encrypted_pms()), TLSRecord()/TLSChangeCipherSpec()/TLSRecord()/to_raw(TLSFinished(), tls_ctx)])

  In this example SSL() leaves no space for side-effects or misinterpretations and the fact that two individual TLSRecords() are involved.

• when working with scapy I usually expect that all sublayers will add up to the top layer and be part of the top-layers calculation. If I want to create two individual layers I usually tend to have a list of individual packets opposed to concating two packets by sublayering them with the layering operator. e.g. the top (left) TLSRecord.length should include all the sublayers in its length calculations.

  TLSRecord()/TLSHandshake()/TLSClientHello()/TLSRecord()

• adding side-effects adds complexity, that may lead to more maintenance work and unexpected behavior/future bugs.
• it currently adds non-default behavior to TLSRecords and TLSHandshake and may require us to also alter other fields.
• may not allow to cascade TLSRecords in TLSRecords anymore.
• will require adaptions for DTLS
• A scapy packet-layer like TLSRecord(packet) operates on raw-packet data. TLSRecord(packet) is a template for a single TLSRecord() only. It is supposed to consume as many packet-bytes as TLSRecord requires and leave everything else as padding. Padding might be consumed by any other sublayer. This implies that it treats any sublayer as data to the current layer.
• If we get do_dissect() right we should not even have to worry about dissection as scapy should already dissect TLSRecords from the packet-bytes and store them in the PacketListField. This is actually built-in to PacketListField but I was missing the packet-class guessing thing to distingiush DTLS from TLS. We could as well just copy the do_dissect from PacketListField and make it use the self.guessed_class for filling the SSL.records attribute as it is already to late to re-define the PacketListField class prototype.
• the SSL() compound layers purpose is to deserialize (dissect) raw-bytes into according TLSRecords, therefore it should also be its purpose to serialize (build) one or multiple records to raw-bytes.

Does this make sense? :)

[alexmgr]

I agree on all the added complexity part for sure. This has been a pain to code, and does require extension in a bunch of spots. I'm not very familiar with Scapy, so I indeed might be trying to solve this in a non efficient or incorrect way.

The parsing withing the SSL() dissector is pretty broken today (extensions are miscalculated or added when non-existent, raw payload are added to the end of packets, ...). E.g: So your point is to fix this stuff at the SSL() layer and use that parsing to generate the stacked packets.

I think I lean towards your suggested approach, even though I'm not 100% sure how it translates into code.

[tintinweb]

Ill have a look at the SSL dissector. At least in theory it should be possible to have scapy do most of the work :) and I remember that it already used to work with stacked records during handshake, otherwise the sniffer and decryption wouldn't have worked. Let me know if you have a broken testcase you require and want me to test against.

[alexmgr]

All test cases I tried so far have this issue (remaining layers seen as payload, and incorrectly parsed extensions). Just running openssl s_server -accept 8443 -www -debug -msg -key keys/server-key.pem -cert keys/server-cert.pem -tls1 -no_ssl3 and issuing a ClientHello should trigger it:

###[ TLS Handshake ]###
  type      = server_hello
  length    = 0x46
###[ TLS Server Hello ]###
     version   = TLS_1_0
     gmt_unix_time= 1432081811
     random_bytes= '\xf7\xb8\x1di\xd4\xca\n\xe7_\t\xc2\xcf\xe7\x1cG\xe6\xf3G \x04xKn*"\xc5\x1e_'
     session_id_length= 0x20
     session_id= '\x97\xea\x15\xc3$\x1d\xe9\xdb\x02T\xa4\x8a15\xa0iv\x9e\xc1\x03[X\x86\x04e\xaf\x8fF\x1b\xd5\x07\x9d'
     cipher_suite= RSA_WITH_AES_128_CBC_SHA
     compression_method= NULL
     extensions_length= 0x1603
     \extensions\
      |###[ TLS Extension ]###
      |  type      = 0x104
      |  length    = 0x650b
      |###[ Raw ]###
      |     load      = '\x00\x04a\x00\x04^\x00\x04[0\x82\x04W0\x82\x03?\xa0\x03\x02\x01\x02\x02\t\x00\x91Yu\x85m;\x8e&0\r\x06\t*\x86H\x86\xf7\r\x01\x01\x05\x05\x000z1\x0b0\t\x06\x03U\x04\x06\x13\x02US1\x130\x11\x06\x03U\x04\x08\x13\nSome-State1\x120\x10\x06\x03U\x04\x07\x13\tSome-City1\x160\x14\x06\x03U\x04\n\x14\rscapy-ssl_tls1\x0c0\n\x06\x03U\x04\x0b\x13\x03dev1\x1c0\x1a\x06\x03U\x04\x03\x14\x13scapy-ssl_tls.local0\x1e\x17\r150519174525Z\x17\r250516174525Z0z1\x0b0\t\x06\x03U\x04\x06\x13\x02US1\x130\x11\x06\x03U\x04\x08\x13\nSome-State1\x120\x10\x06\x03U\x04\x07\x13\tSome-City1\x160\x14\x06\x03U\x04\n\x14\rscapy-ssl_tls1\x0c0\n\x06\x03U\x04\x0b\x13\x03dev1\x1c0\x1a\x06\x03U\x04\x03\x14\x13scapy-ssl_tls.local0\x82\x01"0\r\x06\t*\x86H\x86\xf7\r\x01\x01\x01\x05\x00\x03\x82\x01\x0f\x000\x82\x01\n\x02\x82\x01\x01\x00\xc3.\xb9\xad\xe2R\x91\xa6n\x8f\xd9\xb9i\xb7\x02lkQ\x01\xbb\x1b\xae\x01\xac\x8b\x8f\x03j(k\xe6\xe5\xcdB\x01A\x1a\xfc\xdbD\xa5C\x0c\xf1f\nD<1\xcd\xa9\x14 b?\x05\xd3\xf74\xe1\x0c\xa6\x17,\xe7\xe3\x9e\x92\x02K\x87:\x12v\xd0\x08\xads\xf2\xf0\x92"tCY\x13\xa1s\x08\x9d\x9b\xe9\xf1\xd4\xe4\xee!\xa0\xdb\xef\xd4\x1cy\xca\x86k\x1dV\x96$\x86LE\x1a\x1c$\x9d\r\x7f\xda\x0ciO\x08\x13\xa9\xba9\xd0s@q0rr\xf6\xfd\xaf\x10\xdd\\l\xb4{\x9f\xce2\x0b\x99\xa6C\xf5b\xfe\xe8\x01\x12\x95\x03\nd\xf3P\xf9w{w\x89\xef\x9dY\x18\xe1\x95x\xea\xd0>*\xf0\x03\x1a\xea-\x82\x8b\xd0\xc0o\xae\x8b$(\x19\x1a\xd0Rk6\xd6\xf1\xc5\xcc\x8b\xdd\xdcoN\n,\x82L!\xd2\xc9\xfb\x04\xde\x0e\xed\x92\xcd\xf75\x9f\x0e\xf2\x01|\xc5\xafN\x99OS@\xfc0\x03%+\xe8\x94\x17\xcf@\x96\xef\xba;\n\xbfK\xa4{\x8d\x02\x03\x01\x00\x01\xa3\x81\xdf0\x81\xdc0\x1d\x06\x03U\x1d\x0e\x04\x16\x04\x14&\x93NbB"\xe9~EN\x99w\x8d\xd8\x9b\xc8\x00\xa0\x8d\xde0\x81\xac\x06\x03U\x1d#\x04\x81\xa40\x81\xa1\x80\x14&\x93NbB"\xe9~EN\x99w\x8d\xd8\x9b\xc8\x00\xa0\x8d\xde\xa1~\xa4|0z1\x0b0\t\x06\x03U\x04\x06\x13\x02US1\x130\x11\x06\x03U\x04\x08\x13\nSome-State1\x120\x10\x06\x03U\x04\x07\x13\tSome-City1\x160\x14\x06\x03U\x04\n\x14\rscapy-ssl_tls1\x0c0\n\x06\x03U\x04\x0b\x13\x03dev1\x1c0\x1a\x06\x03U\x04\x03\x14\x13scapy-ssl_tls.local\x82\t\x00\x91Yu\x85m;\x8e&0\x0c\x06\x03U\x1d\x13\x04\x050\x03\x01\x01\xff0\r\x06\t*\x86H\x86\xf7\r\x01\x01\x05\x05\x00\x03\x82\x01\x01\x00\x16\xbf\xb9L_M\xab\xa0\x1dd\xde\xfdl\xf2:\xd4\xd2\xae\xe6\x04\xc5z359r{\x94\xac-\xe2\x9ak\xee\x98\x0cl\x1f\x12\x85\xb0/\x06w\x9b\xc0\x9a\'F\x9d\x8d\x0b\xab\xa0F.\x0c\xf8\xf4\xf3\x06(\x15\x13\xaa&\xb5Ky\x8f\x02\x8b(I\xae\xe8g9\xf0\xb4\n{\xa4\x9e 7QzNe\x07\xe4\xf8\x9b&\x14(\x138\x1bP\xdbn\xc1\xc5\x9c\xdb*\x00\x0c\x81\xaf\xd7\xee}\xf2\xb9\xaeAQ\xd1\xfb\x1b\x1e\x10Z")\x1c\xf8\xc5\xa9\x82\xa5\x0c\x11\xb7\xfd\xee\xffa6@\xfc$\xfb\xd5\xb0\xab\xc4m6m\t?\x80\x83\x9d\x1c\x99q\x16\xc6kHu\x9fm=\xf8`\xee.\xe5\x02T\xdc\xe3c4\xbdE\xf3\xa2\xc3K\xf4V\xc5\x17Yq\xb6\xf2\xde]\xc3Gj\x0bwY\xea\xac\xd0\xab\x93D`\x12\xd34\xb3\xb8\xc8\xaa\x1c\xb9\x1fS\xfe\xe8\\L\xc8\xd5`\xa3\xe4\x90\xff=_R\xb9Nl\x9c\x9b0W\xbey\x96J\x16\xa7\xdb\x83\\\x9b\x04\xe6yL\xed\x16\x03\x01\x00\x04\x0e\x00\x00\x00'
###[ TLS Handshake ]###
  type      = certificate
  length    = 0x461
###[ TLS Certificate List ]###
     length    = 0x45e
     \certificates\
      |###[ TLS Certificate ]###
      |  length    = 0x45b
      |  data      = '0\x82\x04W0\x82\x03?\xa0\x03\x02\x01\x02\x02\t\x00\x91Yu\x85m;\x8e&0\r\x06\t*\x86H\x86\xf7\r\x01\x01\x05\x05\x000z1\x0b0\t\x06\x03U\x04\x06\x13\x02US1\x130\x11\x06\x03U\x04\x08\x13\nSome-State1\x120\x10\x06\x03U\x04\x07\x13\tSome-City1\x160\x14\x06\x03U\x04\n\x14\rscapy-ssl_tls1\x0c0\n\x06\x03U\x04\x0b\x13\x03dev1\x1c0\x1a\x06\x03U\x04\x03\x14\x13scapy-ssl_tls.local0\x1e\x17\r150519174525Z\x17\r250516174525Z0z1\x0b0\t\x06\x03U\x04\x06\x13\x02US1\x130\x11\x06\x03U\x04\x08\x13\nSome-State1\x120\x10\x06\x03U\x04\x07\x13\tSome-City1\x160\x14\x06\x03U\x04\n\x14\rscapy-ssl_tls1\x0c0\n\x06\x03U\x04\x0b\x13\x03dev1\x1c0\x1a\x06\x03U\x04\x03\x14\x13scapy-ssl_tls.local0\x82\x01"0\r\x06\t*\x86H\x86\xf7\r\x01\x01\x01\x05\x00\x03\x82\x01\x0f\x000\x82\x01\n\x02\x82\x01\x01\x00\xc3.\xb9\xad\xe2R\x91\xa6n\x8f\xd9\xb9i\xb7\x02lkQ\x01\xbb\x1b\xae\x01\xac\x8b\x8f\x03j(k\xe6\xe5\xcdB\x01A\x1a\xfc\xdbD\xa5C\x0c\xf1f\nD<1\xcd\xa9\x14 b?\x05\xd3\xf74\xe1\x0c\xa6\x17,\xe7\xe3\x9e\x92\x02K\x87:\x12v\xd0\x08\xads\xf2\xf0\x92"tCY\x13\xa1s\x08\x9d\x9b\xe9\xf1\xd4\xe4\xee!\xa0\xdb\xef\xd4\x1cy\xca\x86k\x1dV\x96$\x86LE\x1a\x1c$\x9d\r\x7f\xda\x0ciO\x08\x13\xa9\xba9\xd0s@q0rr\xf6\xfd\xaf\x10\xdd\\l\xb4{\x9f\xce2\x0b\x99\xa6C\xf5b\xfe\xe8\x01\x12\x95\x03\nd\xf3P\xf9w{w\x89\xef\x9dY\x18\xe1\x95x\xea\xd0>*\xf0\x03\x1a\xea-\x82\x8b\xd0\xc0o\xae\x8b$(\x19\x1a\xd0Rk6\xd6\xf1\xc5\xcc\x8b\xdd\xdcoN\n,\x82L!\xd2\xc9\xfb\x04\xde\x0e\xed\x92\xcd\xf75\x9f\x0e\xf2\x01|\xc5\xafN\x99OS@\xfc0\x03%+\xe8\x94\x17\xcf@\x96\xef\xba;\n\xbfK\xa4{\x8d\x02\x03\x01\x00\x01\xa3\x81\xdf0\x81\xdc0\x1d\x06\x03U\x1d\x0e\x04\x16\x04\x14&\x93NbB"\xe9~EN\x99w\x8d\xd8\x9b\xc8\x00\xa0\x8d\xde0\x81\xac\x06\x03U\x1d#\x04\x81\xa40\x81\xa1\x80\x14&\x93NbB"\xe9~EN\x99w\x8d\xd8\x9b\xc8\x00\xa0\x8d\xde\xa1~\xa4|0z1\x0b0\t\x06\x03U\x04\x06\x13\x02US1\x130\x11\x06\x03U\x04\x08\x13\nSome-State1\x120\x10\x06\x03U\x04\x07\x13\tSome-City1\x160\x14\x06\x03U\x04\n\x14\rscapy-ssl_tls1\x0c0\n\x06\x03U\x04\x0b\x13\x03dev1\x1c0\x1a\x06\x03U\x04\x03\x14\x13scapy-ssl_tls.local\x82\t\x00\x91Yu\x85m;\x8e&0\x0c\x06\x03U\x1d\x13\x04\x050\x03\x01\x01\xff0\r\x06\t*\x86H\x86\xf7\r\x01\x01\x05\x05\x00\x03\x82\x01\x01\x00\x16\xbf\xb9L_M\xab\xa0\x1dd\xde\xfdl\xf2:\xd4\xd2\xae\xe6\x04\xc5z359r{\x94\xac-\xe2\x9ak\xee\x98\x0cl\x1f\x12\x85\xb0/\x06w\x9b\xc0\x9a\'F\x9d\x8d\x0b\xab\xa0F.\x0c\xf8\xf4\xf3\x06(\x15\x13\xaa&\xb5Ky\x8f\x02\x8b(I\xae\xe8g9\xf0\xb4\n{\xa4\x9e 7QzNe\x07\xe4\xf8\x9b&\x14(\x138\x1bP\xdbn\xc1\xc5\x9c\xdb*\x00\x0c\x81\xaf\xd7\xee}\xf2\xb9\xaeAQ\xd1\xfb\x1b\x1e\x10Z")\x1c\xf8\xc5\xa9\x82\xa5\x0c\x11\xb7\xfd\xee\xffa6@\xfc$\xfb\xd5\xb0\xab\xc4m6m\t?\x80\x83\x9d\x1c\x99q\x16\xc6kHu\x9fm=\xf8`\xee.\xe5\x02T\xdc\xe3c4\xbdE\xf3\xa2\xc3K\xf4V\xc5\x17Yq\xb6\xf2\xde]\xc3Gj\x0bwY\xea\xac\xd0\xab\x93D`\x12\xd34\xb3\xb8\xc8\xaa\x1c\xb9\x1fS\xfe\xe8\\L\xc8\xd5`\xa3\xe4\x90\xff=_R\xb9Nl\x9c\x9b0W\xbey\x96J\x16\xa7\xdb\x83\\\x9b\x04\xe6yL\xed'
###[ Raw ]###
        load      = '\x16\x03\x01\x00\x04\x0e\x00\x00\x00'
###[ TLS Handshake ]###
  type      = server_hello_done
  length    = 0x0

Everything works more or less, but you end up with a different packet then what you received ;), because it's str() representation is broken (due to trailing payload). This means that verify_data is incorrectly calculated for example.

The trailing payload is easy to fix, but the extensions is tricky. I'll push a fix tomorrow.

I think we can drop this pull request

[alexmgr]

Fixed trailing data, but not extensions yet in branch dissect-improvements. I'll see if there is a better way to do this through Scapy.

Overall, my plan tomorrow is to merge the certificate parsing and setuptools branch while trying to fix the extension problems. I'll also drop this PR. Let me know if that works for you.

[tintinweb]

I guess if we properly implement Packet.extract_padding(s) for our layers to make it return own_size, padding scapy should be able to auto-dissect them when used in PacketListFields and we should not have to worry about this anymore.