Fix bugs for TLS PCAPs and refactor pyrdp-convert code #311
Conversation
|
I ran the convert tool on a couple of pcaps and I find odd file structures being generated: There shouldn't be a |
|
Another issue, with one pcap I have, Python segfaults with a stack overflow error: Running in ipdb it took 100% CPU for several minutes. Slowly increased CPU usage until my out of memory manager (OOM) killed it. Investigating an upstream issue. |
|
With scapy 2.4.4 (latest is 2.4.5) on that same pcap, memory consumed by Python increases up to 8GB RSS memory. At this point, something bubbles up and Python is terminated. This is for a relatively straightforward 4MB pcap. Something is wrong. Wait a second. I wasn't specifying a TLS master secret log... |
|
In the code, the TLS secret verification happens after that stacktrace or OOM reap so it didn't matter if I passed the file or not. The issue is still present. |
This is all scapy stuff, we would need the actual PyRDP line that provokes this. Also, how big is the PCAP? |
The stacktrace is too long, it finishes before it leads to pyrdp. With the debugger it makes me think it is at the
Except for the file output problem, all the tests I have done today and documented here were on a 4 megabyte pcap with 5 TCP streams. 2 or 3 of which gathers certificates and 2 real connections (same source and dst IP). Manually splitting the pcap to layer 7 PDUs works fine. |
|
More tests tonight:
The problem is in scapy. Using rdpcap finishes so there is no problem there but the session reconstruction code is not in there... |
|
After a long investigation, we can say that the pcap issue is from scapy and is a corner-case that can be worked around by exporting PDUs from wireshark instead. The problem will be tracked here: #318. In order to move forward with 1.1.0 we will proceed with the review ignoring the specific scapy problems. |
|
After some initial attempts at reproducing the issue I thought it was no longer there but it only affects replay output type. I'll be back with a fix hopefully |
However there's a regression in the pcap -> mp4 conversion right now...
|
I fixed the path issues I was worried about. However, there's a regression with the |
|
I might have an initial fix. Some test cases the fix needs to go through:
|
|
I fixed the master branch's problem with this: --- a/bin/pyrdp-convert.py
+++ b/bin/pyrdp-convert.py
@@ -66,7 +67,7 @@ def getSink(format: str, outfile: str, progress=None) -> (str, str):
sys.exit(1)
sink, ext = HANDLERS[format]
- outfile += f".{ext}"
+ outfile = str(outfile / f".{ext}")
return sink(outfile, progress=progress) if sink else None, outfile
and I can confirm that files were extracted in the current master so I won't consider this a regression of that PR but something to fix for later. |
|
Confirmed that conversion speed on master was slow as well. See #329. Confirmed that file artifacts were generated on master as well, will fix later (if ever). So I only need to make sure that a mp4 file is actually being generated. |
|
Anything converted from a pcap with the output option ( |
|
Latest commits fixed Mp4 output but JSON output doesn't work. If that's a regression from master I'll try to fix it, if it never worked, I'll merge this and open a ticket. I updated task list with a couple of comments above to reflect current situation. |
|
In master, previous to this merge, pcap to json wasn't supported. I don't feel bad if this branch doesn't fix that. I opened #331 to track that and offered a workaround in it. This was the last thing to look at so this branch can be merged now. |
Big refactor of the pyrdp-convert code and some bug fixes. Most notably: