TLSRecord length calculation is incorrect

[alexmgr]

Currently TLSRecord length is a XLenField, which inherits from LenField. LenField calculates the length of the packet as being x = len(pkt.payload), which is incorrect in the cases where TLSRecords are stacked. E.g:

stacked_pkt = tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHello()/tls.TLSRecord()/tls.TLSHandshake()/tls.TLSCertificateList()/tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHelloDone()
pkt = tls.TLS(str(stacked_pkt))

The length of the TLSRecord should be length until the next record is reached.

This simple unittest should pass on fix:

import unittest
import ssl_tls as tls

class TestTLSRecord(unittest.TestCase):

    def test_stacked_tls_records_length_are_correct(self):
        # Highlights issue https://github.com/tintinweb/scapy-ssl_tls/issues/12
        # Before I try and fix it
        server_hello = tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHello()
        cert_list = tls.TLSRecord()/tls.TLSHandshake()/tls.TLSCertificateList()
        server_hello_done = tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHelloDone()
        stacked_pkt = server_hello/cert_list/server_hello_done
        pkt = tls.TLS(str(stacked_pkt))
        self.assertEqual(len(str(server_hello)) - len(tls.TLSRecord()), pkt[tls.TLSRecord].length)

[alexmgr]

Fixed in commit 27b3d1ea7774e3942ac8437ce258df5e259c323d of pull request #13

[tintinweb]

see comment in #13

the basic idea is to change

stacked_pkt = tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHello()/tls.TLSRecord()/tls.TLSHandshake()/tls.TLSCertificateList()/tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHelloDone()
pkt = tls.TLS(str(stacked_pkt))

to

stacked_pkt = SSL()
stacked_pkt.records = [(tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHello()), tls.TLSRecord()/tls.TLSHandshake()/tls.TLSCertificateList(), tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHelloDone() ]

[alexmgr]

I think people using this would expect this version to work

stacked_pkt = tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHello()/tls.TLSRecord()/tls.TLSHandshake()/tls.TLSCertificateList()/tls.TLSRecord()/tls.TLSHandshake()/tls.TLSServerHelloDone()
pkt = tls.TLS(str(stacked_pkt))

because it follows the same flow as other packets. I agree the solution you propose is simpler, but it forces people to read the documentation which is pretty optimistic ;)

[tintinweb]

The way to go is #16. #17 is DUP to #16 therefore we'll just close it. Sample output in #17 should also apply to #16. #16 therefore should fix dissecting of stacked records. As discussed in #13 support for stacked records will be implemented as SSL.from_records([TLSRecord/..., TLSRecord/...]) otherwise we'd add non obvious side-effects.

any objections about closing this issue?

[alexmgr]

No objection ;), All set.