-
Notifications
You must be signed in to change notification settings - Fork 246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(331): fix json conversion #366
Conversation
7005f06
to
15b727a
Compare
I had to force push this because I accidentally included commits for the notify2 changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this another good look this morning and I think it's good to go. Now I'm going to test it on some pcaps and replays and advise.
I see what you mean now by the fact that you felt this is broken. I removed the global exception handler because it would hide errors but in the pcap conversion codepath the fact that the generator raises a StopIterator does make conversions always fail with a stack trace. I'm glad you caught this before the next release. |
I think this PR fixes the StopIteration throw by making ExportedPDUStream behave like a proper iterator. We should be able to keep the global exception handler. Technically we could get rid of the I may have a few extra modifications to this in my other PRs. I kinda worked on all of them in parallel so they got a bit mixed :O |
Right now this patch doesn't work for me. Replay to JSON:
Pcap to JSON:
L7 PDU Pcap to JSON:
It might be that the session enumeration code relies only on IP addresses instead of IP:port tuples. I've seen this bug elsewhere in the redirection code. |
I worked on this today. I'm not there yet but I'm making progress. |
> TypeError: 'EDecimal' object cannot be interpreted as an integer I wasn't getting those error before, I think it might be python 3.10 related. Flooring gets rid of the error. That information was lost anyway.
Introduced an InetSocketAddress abstraction and adapted all code paths to it.
I think I finally fixed the issue(s). Tested many cases: TLS, PDUs, etc. Some failures left but I think it is in cases where we couldn't convert those streams anyway: pcap contains I'll let the tests run and sleep on it before merging. |
Replay to JSON:
Pcap to JSON:
The TLS error at the end is expected. It is happening on the TLS session between the MITM and the server, on which we don't do a protocol downgrade attack so we don't support all of it. There are timestamp differences (looking at a diff) and there is 201 more events when converting from the pcap:
From L7 PDU to JSON (one stream):
Using on a multiple streams PDU is unsupported. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last little adjustments, I'll merge soon
This PR addresses several issues with the pyrdp-convert refactor:
__iter__
implementation forExportedPDUStream
resulting in improper cleanupPCAPConverter
Among other things, it closes #331.
Tested on both encrypted pcaps and ExportedPDU pcaps.