Closed tintinweb closed 9 years ago
Looks good to me. +1 on removing PacketNoPadding, since this should be an in place replacement for it. Nice clean up!
tintin » scapy-ssl_tls #154 SUCCESS This pull request looks good (what's this?)
tintin » scapy-ssl_tls #163 SUCCESS This pull request looks good (what's this?)
Ok, it is not that easy :/ We kind of need both Packet types, here's why:
PacketNoPayload for leaf-packets which are not expected to have a sub-layer/payload. The Packet will terminate when all fields are set. Further bytes won't be dissected as payload to that packet but will be dissected in the overall bytestream dissection i.e. eventually dissecting another sublayer of another subtree. For example variable lists of TLSCertificates followed by a variable list of TLSProtoALPN.
class TLSCertificate(PacketNoPayload):
name = "TLS Certificate"
fields_desc = [ XBLenField("length", None, length_of="data", fmt="!I", numbytes=3),
PacketLenField("data", None, x509.X509Cert, length_from=lambda x:x.length),]
class TLSCertificateList(PacketNoPayload):
name = "TLS Certificate List"
fields_desc = [
XBLenField("length", None, length_of="certificates", fmt="!I", numbytes=3),
PacketListField("certificates", None, TLSCertificate, length_from=lambda x:x.length),
]
Note that both packets are PacketNoPayled as both the TLSCertificateList and the TLSCertificate are not expected to have a payload. TLSCertificate's are fully contained within TLSCertificateList in the TLSCertificateList.certificates PacketListField.
PacketLengthPayload for packets like TLSExtension where the last field is the length (in bytes) of the next payload/sub-layer. This packet type will only provide up to .length bytes to the dissector of the next layer. If no .length is available it'll act like an ordinary Packet()
class TLSExtension(PacketLengthFieldPayload):
name = "TLS Extension"
fields_desc = [XShortEnumField("type", TLSExtensionType.SERVER_NAME, TLS_EXTENSION_TYPES),
XLenField("length", None, fmt="!H"),
]
Note that TLSDecryptablePacket is of type PacketLengthPayload in order to work for TLSHandshake.
Also had a look at generic stacking for PacketLengthPayload type packets but it only works for packets in PacketListFields (auto.dissects Padding layers as PacketListField prototype layer) atm. Will draft a layer that allows generic stacking and only returns Padding when the bytestream cannot be dissected.
tintin » scapy-ssl_tls #165 SUCCESS This pull request looks good (what's this?)
Thanks for the detailed explanation. Both use cases make sense.
tintin » scapy-ssl_tls #172 SUCCESS This pull request looks good (what's this?)
Added LengthFieldPacket() type Packet which only provides up to .length bytes to the dissector of the next layer (its payload). This fixes dissection of unknown tlsextensions.
The LengthFieldPacket() packet type can be used whenever we want to only provide up to .length bytes to the next layer. I guess we could also use this to dissect the initial TLSRecords and get rid of the PacketNoPadding type layer.
Current dissector:
Note the Raw() layer is eating up all the bytes at we do not have a layer for the (deprecated) NPN extension. Also see the "status_request" extension.
Fixed dissector:
Note that the NPN extension is now dissected as TLSExtension()/Raw() with the sublayer having the size of TLSExtension.length.