Cleanup IP fragmentation, TCP session and TLS sessions

[gpotter2]

• all Sessions: replace on_received_packet by two functions: recv that is an iterator and process. This makes a cleaner separation, and re-puts the storing/prn/etc; back into sniff (where it should have been left)
• cleanup TCPSession:
  • now do not dissect the packet just to re-build it afterwards, thanks to a new stop_payload_dissection attribute, which allows the caller to say "Don't parse above TCP". This required changing the layout of recv() in SuperSocket to allow for extra arguments.
  • better handling of out-of-order packets: handle the case where the first packet received wasn't the first (fixes sniff seems to crash with "ValueError: memoryview assignment: lvalue and rvalue have different structures" occasionally #4030)
• cleanup IP fragmentation utils:
  • deduplicate the two fragment() functions
  • fix layers.inet.fragment: use fragsize=8 if argument is lower than that #3970
• cleanup TLS sessions
  • fix TLS sessions being badly recognized when over TCP defragmented sessions (with TCPSession)
  • fix repr() not showing the correct direction in tls sessions
  • deprecate TLSSession (literally brings nothing. Just use TCPSession with conf.tls_session_enable=True). This is now also fixed thanks to the above fix.
  • fix the handling of NSS files: now supports multiple streams (fixes Use a NSS Key Log file to decrypt a TLS session #3374 (comment))
  • there was a bug in our handling of extended master secret key that made it so that we always called compute_ms_and_derive_keys even if no pre_master_secret_key was computed (i.e. we don't have any private key). This is now fixed, the function is only called whenever the pre_master_secret_key has been computed.
  • fix TLS Application Data not decrypted #3722 (the NSS keys were only computed when the bug above occured, leading to sessions not using extended master key to fail to compute them, and making the bug unclear). This is now properly fixed: NSS keys are checked separately as last resort.

fixes #4030
fixes #3722
closes #3966
closes #3970

[codecov]

Codecov Report

Merging #4082 (b5b47b7) into master (f311149) will decrease coverage by 1.00%.
The diff coverage is 88.79%.

@@            Coverage Diff             @@
##           master    #4082      +/-   ##
==========================================
- Coverage   81.93%   80.94%   -1.00%     
==========================================
  Files         330      330              
  Lines       76212    76234      +22     
==========================================
- Hits        62448    61710     -738     
- Misses      13764    14524     +760     

Files Changed	Coverage Δ
scapy/layers/tls/record_sslv2.py	86.03% <0.00%> (ø)
scapy/contrib/isotp/isotp_native_socket.py	44.23% <50.00%> (ø)
scapy/layers/dcerpc.py	78.41% <50.00%> (+0.03%)	⬆️
scapy/contrib/automotive/bmw/hsfz.py	59.78% <75.00%> (ø)
scapy/contrib/automotive/doip.py	71.15% <75.00%> (ø)
scapy/contrib/automotive/ecu.py	93.26% <75.00%> (ø)
scapy/layers/tls/record_tls13.py	87.28% <75.00%> (+3.95%)	⬆️
scapy/utils.py	77.90% <76.92%> (ø)
scapy/layers/tls/session.py	86.18% <82.35%> (-2.01%)	⬇️
scapy/layers/netflow.py	90.15% <85.71%> (-0.32%)	⬇️
... and 17 more

... and 17 files with indirect coverage changes

[stulle123]

Hi,

Just ran another test with this pcap and tcp_reassemble() (in scapy/layers/tls/session.py) takes forever to process.

from scapy.config import conf
from scapy.main import load_layer
from scapy.sendrecv import sniff
from scapy.sessions import TCPSession

conf.tls_session_enable=True
load_layer("tls")


capture = sniff(offline="test_2.pcap", session=TCPSession)

print("Done")

Pyinstrument output:

$ pyinstrument test.py -r text --show '*/scapy/*'    

  _     ._   __/__   _ _  _  _ _/_   Recorded: 11:34:08  Samples:  354062
 /_//_/// /_\ / //_// / //_'/ //     Duration: 361.988   CPU time: 360.251
/   _/                      v4.5.1

Program: test.py

361.987 <module>  test.py:1
└─ 359.257 sniff  scapy/sendrecv.py:1307
   └─ 359.257 AsyncSniffer._run  scapy/sendrecv.py:1064
      └─ 358.970 TCPSession.on_packet_received  scapy/sessions.py:385
         └─ 358.947 TCPSession._process_packet  scapy/sessions.py:288
            └─ 358.330 TLS.tcp_reassemble  scapy/layers/tls/session.py:1124
               └─ 353.972 [self]  scapy/layers/tls/session.py

[gpotter2]

Thanks for the report, I'll have a look.

[stulle123]

Hey @gpotter2,

Sorry for bothering :-D found another issue when processing this pcap (using the same code from above):

WARNING: No IPv4 address found on anpi2 !
WARNING: No IPv4 address found on anpi1 !
WARNING: more No IPv4 address found on anpi0 !
WARNING: Socket <scapy.utils.PcapNgReader object at 0x110de25d0> failed with 'PcapNgReader.recv() got an unexpected keyword argument 'stop_payload_dissection''. It was closed.

Cheers

[gpotter2]

Thanks. I'm currently rewriting the PR quite a bit.

[gpotter2]

@stulle123 this is fixed thanks.

[polybassa]

regarding the failing test, somehow the behavior of the count parameter changed.

diff --git a/test/contrib/automotive/ecu.uts b/test/contrib/automotive/ecu.uts
index 680a2b33..4ced5203 100644
--- a/test/contrib/automotive/ecu.uts
+++ b/test/contrib/automotive/ecu.uts
@@ -673,7 +673,7 @@ with PcapReader(scapy_path("test/pcaps/gmlan_trace.pcap.gz")) as sock:
     print("Check 1 after change to diagnostic mode")
     assert len(ecu.supported_responses) == 1
     assert ecu.state == EcuState(session=3)
-    gmlanmsgs = sniff(session=ISOTPSession(supersession=session, rx_id=[0x241, 0x641, 0x101], basecls=GMLAN), count=8, opened_socket=sock)
+    gmlanmsgs = sniff(session=ISOTPSession(supersession=session, rx_id=[0x241, 0x641, 0x101], basecls=GMLAN), count=6, opened_socket=sock)
     ecu = session.ecu
     print("Check 2 after some more messages were read1")
     assert len(ecu.supported_responses) == 3
@@ -681,13 +681,13 @@ with PcapReader(scapy_path("test/pcaps/gmlan_trace.pcap.gz")) as sock:
     assert ecu.state.session == 3
     print("assert 1")
     assert ecu.state.communication_control == 1
-    gmlanmsgs = sniff(session=ISOTPSession(supersession=session, rx_id=[0x241, 0x641, 0x101], basecls=GMLAN), count=10, opened_socket=sock)
+    gmlanmsgs = sniff(session=ISOTPSession(supersession=session, rx_id=[0x241, 0x641, 0x101], basecls=GMLAN), count=2, opened_socket=sock)
     ecu = session.ecu
     print("Check 3 after change to programming mode (bootloader)")
     assert len(ecu.supported_responses) == 4
     assert ecu.state.session == 2
     assert ecu.state.communication_control == 1
-    gmlanmsgs = sniff(session=ISOTPSession(supersession=session, rx_id=[0x241, 0x641, 0x101], basecls=GMLAN), count=16, opened_socket=sock)
+    gmlanmsgs = sniff(session=ISOTPSession(supersession=session, rx_id=[0x241, 0x641, 0x101], basecls=GMLAN), count=6, opened_socket=sock)
     ecu = session.ecu
     print("Check 4 after gaining security access")
     assert len(ecu.supported_responses) == 6

This changes make the test work again

[gpotter2]

Thanks a lot !!!

[evverx]

With this PR applied sniff seems to fail with

WARNING: Socket <scapy.arch.bpf.supersocket.L2bpfListenSocket object at 0x10e4e5910> failed with ''Conf' object has no attribute 'debug_dissect''. It was closed.

from time to time. I'll try to collect packets triggering it and attach a pcap.

[evverx]

Looks like it has nothing to do with this PR. It can be fixed with:

diff --git a/scapy/layers/tls/record_sslv2.py b/scapy/layers/tls/record_sslv2.py
index 8d311faa..8e99a352 100644
--- a/scapy/layers/tls/record_sslv2.py
+++ b/scapy/layers/tls/record_sslv2.py
@@ -174,7 +174,7 @@ class SSLv2(TLS):
             except KeyboardInterrupt:
                 raise
             except Exception:
-                if conf.debug_dissect:
+                if conf.debug_dissector:
                     raise
                 p = conf.raw_layer(s, _internal=1, _underlayer=self)
             self.add_payload(p)

[gpotter2]

Still waiting for more reviews !

Note: this PR should be squashed on merge.

[p-l-]

I don't have enough time for now to do a comprehensive review but overall LGTM, great work!

[guedou]

LGTM I will try to test the code further.