tintinweb / scapy-ssl_tls

SSL/TLS layers for scapy the interactive packet manipulation tool
GNU General Public License v2.0
419 stars 156 forks source link

Why does the Server Hello Done have three extra bytes? #91

Closed RajeshGottlieb closed 7 years ago

RajeshGottlieb commented 7 years ago

I would expect the Length of the Server Hello Done to be zero, but it is three. Is there any good reason why? Do any well accepted TLS implementations have padding like this?

The problem could be with how I'm doing things. I'm trying to directly write a pcap and control the MACs, IPs explicitly. Maybe I just need a bit of advice on how best to accomplish this.

TLSv1 Record Layer: Handshake Protocol: Server Hello Done
    Content Type: Handshake (22)
    Version: TLS 1.0 (0x0301)
    Length: 7
    Handshake Protocol: Server Hello Done
        Handshake Type: Server Hello Done (14)
        Length: 3

tls_example.tar.gz

tintinweb commented 7 years ago

Hi @RajeshGottlieb,

Thank you for taking the time to report this issue.

This actually looks like like a copy-paste bug in scapy-ssl_tls. Our model of TLSServerHello is defined having a 3-byte length and optional data which does not match the rfc2246 TLS1.0/rfc5246 TLS1.2 specification of zero length payload.

I've proposed a fix with PR #92. Note that the TLSServerHelloDone() layer wont show up in dissected streams as the layer is actually zero-byte long and scapy only dissects as long as there's bytes to pass to the next layer. However, the TLSHandshake layer will have its type set to TLSHandshakeType.SERVER_HELLO_DONE. Also see the updated unit test.

Let me know if that works for you and we'll have it merged.

cheers, tin

RajeshGottlieb commented 7 years ago

Looks good to me.

TLSv1 Record Layer: Handshake Protocol: Server Hello Done
    Content Type: Handshake (22)
    Version: TLS 1.0 (0x0301)
    Length: 4
    Handshake Protocol: Server Hello Done
        Handshake Type: Server Hello Done (14)
        Length: 0
tintinweb commented 7 years ago

:information_source: merged to release/1.2.3 and will be included int bugfix release v1.2.3.2 (not yet scheduled).