Fix branch 1668

[vincae-dalean]

The problem was an extension of the handshake protocole "Server Hello" inside the TLS packet, named "supported_versions".

The length of the versions was supposed to be 3, but the real size was 1, so scapy was trying to parse more than it could, and because of that it was returning the raw data.

The fix is adding a test to know if the size is less than what it should be, and it also does like wireshark, which writes that there is an error/malformation, give the supposed length and the real one. But scapy will also show the raw data.

The result for the code in the issue will be :

###[ TLS ]###
type= handshake
version= TLS 1.2
len= 122
iv= b''
\msg\
|###[ TLS Handshake - Server Hello ]###
| msgtype= server_hello
| msglen= 118
| version= TLS 1.2
| gmt_unix_time= Mon, 09 Feb 2015 08:41:13 +0000 (1423471273)
| random_bytes= 60290781a55827539ab607abe2ba8db77a212053ad7e98c92598ba46
| sidlen= 32
| sid= '3\xd5b\xb3\x93T\x0cu @X3\xf0n\x8c\x84\xd0\xdf\xb0\xe8\xe0\x9f.\xa9\xd4\xe7\xb8T\x81\xb9\x89\xac'
| cipher= TLS_AES_128_GCM_SHA256
| comp= null
| extlen= 46
| \ext\
| |###[ TLS Extension - Scapy Unknown ]###
| | type= 51
| | len= 36
| | val= '\x00\x1d\x00 \x10\x0bro\xd7z\x0f\xd0\xe6\xa0\x02(W\x9d^d\x9fA\x05\x1a\x9b+Y\xf5\xaf\xb0\x9f\xe7/y4\x10'
| |###[ TLS Extension - Supported Versions ]###
| | type= supported_versions
| | len= 2
| | versionslen= 3
| | versions= [b'Error/Malformed: Vector length 3 is too large, truncating it to 1 original is : "\x04"']
mac= b''
pad= b''
padlen= None

fixes #1668

[gpotter2]

Hi, thanks for the PR

However I do not think that this is a proper fix to the issue. As wireshark gets the packets right, it sound like we are wrongly dissecting it.

[codecov-io]

Codecov Report

Merging #1684 into master will increase coverage by 0.04%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #1684      +/-   ##
==========================================
+ Coverage   85.34%   85.38%   +0.04%     
==========================================
  Files         180      180              
  Lines       42013    42009       -4     
==========================================
+ Hits        35855    35871      +16     
+ Misses       6158     6138      -20

Impacted Files	Coverage Δ
scapy/layers/tls/extensions.py	79.86% <83.33%> (+0.07%)	⬆️
scapy/layers/tls/record_sslv2.py	87% <0%> (-1.7%)	⬇️
scapy/layers/tls/handshake_sslv2.py	92.01% <0%> (-0.77%)	⬇️
scapy/layers/tls/basefields.py	79.86% <0%> (-0.68%)	⬇️
scapy/contrib/cansocket_native.py	87.14% <0%> (-0.36%)	⬇️
scapy/utils.py	75.83% <0%> (-0.01%)	⬇️
scapy/sendrecv.py	82.95% <0%> (-0.01%)	⬇️
scapy/supersocket.py	72.54% <0%> (ø)	⬆️
scapy/contrib/http2.py	96.8% <0%> (+0.01%)	⬆️
scapy/arch/windows/__init__.py	78.32% <0%> (+0.08%)	⬆️
... and 9 more

[vincae-dalean]

Hi, you are right, wireshark gets the packets right, i had an older version (2.4.2) and in this version it was showing an error message.

Now, I saw the problem in details, and the problem is with the extension of the handshake protocol helloclient / helloserver : "supported_versions".
Before the version TLS1.3, we could use this extension only in the helloclient, it was used to indicate what version of TLS the client was able to support and it was never used in the helloserver.
But since the TLS1.3, we can also use it in the helloserver, BUT the problem is that the structure is not the same.

In the helloclient, there is a byte for the "Supported Versions Length", but not in the helloserver (in the helloclient we can give several versions, but if it is in the helloserver, we are just giving the one we want, so we don't need to give a length)

helloclient:

helloserver:

So the problem is that we try to fill the structure for the helloclient which requires 1 more byte, and then we try to read 2 bytes when we have only 1 left.
My solution is to give another structure to fill in the case where it is a helloserver. In the code, we are taking the structure from a dictionary "_tls_ext_cls" in the file scapy/layers/tls/extensions.py.
And to get it, we are using the number of the extension (43 in our case), which is the same in helloclient and helloserver. So I just add a condition to verify if it is this extension and a helloserver, and if it's the case, we will use another structure named "TLS_Ext_SupportedVersions_Server".

The result for the sample in the issue will be :

The result in wireshark 2.6.3 :

[gpotter2]

That’s more like it :) thanks.

The implementation you provided is all right, however I’d rather have a unique single extension layer.

It might be better to use a ConditionalField on the versionslen. E.g:

class TLS_Ext_SupportedVersions(TLS_Ext_Unknown):
    name = "TLS Extension - Supported Versions"
    fields_desc = [ShortEnumField("type", 0x2b, _tls_ext),
                   ShortField("len", None),
                   ConditionalField(
                        FieldLenField("versionslen", None, fmt='B',
                                     length_of="versions"),
                        lambda x: x.len % 2),
                   FieldListField("versions", [],
                                  ShortEnumField("version", None,
                                                 _tls_version),
                                  length_from=lambda pkt: pkt.len - (pkt.len % 2))]

You could also try to use self.underlayer in the lambda to get the parent packet

And last but not lease, we need to have tests on his feature. They are already plenty in scapy/tests/tls13.uts, you could add a dissection test. The format is self explicit

[vincae-dalean]

I saw that you added the changes in the pull request #1705 , so I'm closing my pull request.