Fix dissection of TLS communication with NULL Cipher.

[wasilukm]

Due to invalid handling of TLS record length, fields (MSG and MAC) were
incorrectly interpreted in packets encrypted with NULL Cipher.

fixes #1976

[codecov]

Codecov Report

Merging #1986 into master will decrease coverage by 1.49%.
The diff coverage is 85.71%.

@@            Coverage Diff            @@
##           master    #1986     +/-   ##
=========================================
- Coverage   85.87%   84.37%   -1.5%     
=========================================
  Files         187      187             
  Lines       43007    43007             
=========================================
- Hits        36933    36288    -645     
- Misses       6074     6719    +645

Impacted Files	Coverage Δ
scapy/layers/tls/record.py	91.22% <100%> (-0.88%)	⬇️
scapy/layers/tls/cert.py	84.27% <83.33%> (ø)	⬆️
scapy/contrib/cansocket_python_can.py	37.7% <0%> (-54.1%)	⬇️
scapy/contrib/cansocket_native.py	29.57% <0%> (-50.71%)	⬇️
scapy/arch/linux.py	52.14% <0%> (-22.42%)	⬇️
scapy/contrib/isotp.py	65.97% <0%> (-22.34%)	⬇️
scapy/supersocket.py	53.87% <0%> (-18.22%)	⬇️
scapy/arch/common.py	78.26% <0%> (-13.05%)	⬇️
scapy/arch/pcapdnet.py	47.81% <0%> (-9.23%)	⬇️
scapy/layers/l2.py	75.43% <0%> (-8.94%)	⬇️
... and 23 more

[gpotter2]

It would be great if you could add a unit test for that. For instance a packet to dissect. See the .uts files in test/.

The format is custom (historic reasons), but quite self explicit 😄

[wasilukm]

Yeah I'm woring on UT.
BTW. I wonder why coverage report is so bad.

[gpotter2]

The Travis tests didn't because it's a draft.
We can't do much about this, therefore only Windows tests (AppVeyor) were launched

[wasilukm]

@gpotter2 is anything else which needs to be addressed? Could you please consider approving the change?

[gpotter2]

Looks okay to me, though I'm not familiar enough with TLS to confirm it won't break anything.
Other maintainers will merge it :-)

[gpotter2]

I had another look at this. The fix you've done seems wrong.

The code clearly says the deciphered_len should only be defined when something was actually deciphered (not the case with a null encoding), therefore the issue is more likely in the place that uses deciphered_len

[wasilukm]

@gpotter2 thanks for the feedback.
I would assume that null cipher automatically becomes deciphered but I may be wrong as I'm not TLS expert.
Do you have any clue where to look?

[guedou]

I am suite unsure that this is a correst fix.