Skip to content
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

Added credential logger #106

Merged
merged 7 commits into from
Jul 2, 2019
Merged

Added credential logger #106

merged 7 commits into from
Jul 2, 2019

Conversation

Pourliver
Copy link
Contributor

Here is the password harvester showcased in this issue #103.

I implemented a new logger based on the FastPathLogger. The new CredentialLogger is a singleton, since it needs to be referenced from somewhere it won't be defined in order to print credentials. I tried an implementation with a static method and a few static variable at first, but this once made more sense since there will never be two instances of a logger.

We keylog everything the users sends, and we save them as a candidate whenever the user presses the "ENTER" key. When we receive a confirmation that the user logged in, we print the candidate (or the buffer if the user submitted the form with a click)

When the user logs in, the logger deactivate itself. Whenever a new client connects, it gets reactivated.

This should give us good credentials most of the time, but It may give false-positives if the user press keys while not in an input field, or if he spams his keyboard right after being logged in but before receiving the "RDPDR" signal.

@Res260
Copy link
Collaborator

Res260 commented May 15, 2019

@xshill Been a while since I dove into the code, but wasnt there a way to avoid having a singleton for this?

@Res260 Res260 requested a review from xshill May 15, 2019 00:41
Copy link
Collaborator

@xshill xshill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow me to suggest a rework. If I understand this correctly, there's 3 parts to this:

  • Collect fast-path input while the user is on the login screen
  • Collect slow-path input while the user is on the login screen
  • Expect an RDPDR logged in message to know when the user logs in

Instead of forcing this whole feature into a single class, it should be split between the MITM handlers of the protocols that are relevant to it. Every protocol should have its own simple handling logic, and the collective action of all these handling functions is what makes the actual feature work.

Put fast-path input collection in FastPathMITM, slow-path input collection in SlowPathMITM, and login PDU handler in DeviceRedirectionMITM. FastPathMITM and SlowPathMITM will build up a shared input buffer if the user isn't logged in. DeviceRedirectionMITM will handle PAKID_CORE_USER_LOGGEDON messages: set a shared loggedIn boolean to True and log the current input buffer.

There's already an RDPMITMState class that contains data shared between all the handlers for a single connection. You can just add an input buffer, a loggedIn boolean, and all the information that should be shared between 2 or more protocol handlers in there. If the protocol handlers don't already have that state object passed in their constructors, you can just add that parameter.

If you want to extract shared logic between the fast-path and slow-path handlers, you can create a base class for both FastPathMITM and SlowPathMITM and put that logic in there. You can also just use a function that will take a buffer and a scan code and return the resulting buffer.

With the current implementation, you're:

  • Using a singleton, which will most likely break when you get simultaneous connections
  • Putting MITM logic in the logging module (the logging classes should be primarily simple and self-contained classes that can be tacked on to layers without many side-effects)

@Pourliver
Copy link
Contributor Author

Thanks a lot for the feedback! I lacked the vision to clearly see how to correctly integrate this feature in the project, but your proposal makes a lot of sense.

@Pourliver Pourliver requested a review from xshill June 3, 2019 17:45
@Pourliver
Copy link
Contributor Author

I refactored the feature according to your feedback. It's much cleaner, and we even have a good base to time payloads when user logs in (as requested in #98).

Copy link
Collaborator

@Res260 Res260 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beside my minor comment, lgtm.
@xshill

pyrdp/mitm/FastPathMITM.py Outdated Show resolved Hide resolved
Copy link
Member

@obilodeau obilodeau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective only minor changes required. Log formats and store some special keys in creds candidate buffer.

pyrdp/mitm/DeviceRedirectionMITM.py Outdated Show resolved Hide resolved
self.client.sendPDU(pdu)

def onScanCode(self, scanCode: int, isReleased: bool, isExtended: bool):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably store some of the special characters and output them in the logs/json. For example: backspace and tab.

A user has more chances of understanding P<\b>Passw0rd! rather than PPassw0rd!. I'm not too opinionated about what format it should be but since we are already seeing \x00 in some fields, we could just pass it like that, so backspace would be \x08 and tab \x09. That's just a suggestion, something fancier would be ok too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example candidate currently. I tokenized each action the same way to simplify logging / parsing accuracy.
MonPass<ctrl-a><\b>Passw0rd!

pyrdp/mitm/FastPathMITM.py Outdated Show resolved Hide resolved
pyrdp/mitm/FastPathMITM.py Outdated Show resolved Hide resolved
pyrdp/mitm/FastPathMITM.py Outdated Show resolved Hide resolved
pyrdp/mitm/state.py Outdated Show resolved Hide resolved
@Pourliver Pourliver requested a review from obilodeau June 5, 2019 18:15
Copy link
Collaborator

@xshill xshill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good, but for it to be fully functional you'd need to catch slow-path input as well.

@Res260
Copy link
Collaborator

Res260 commented Jun 9, 2019

For reference: slow-path inputs are less used because most clients use fast-path by default for user input, but it happens that they use slow-path only.

@Pourliver
Copy link
Contributor Author

Eh, sorry, I overlooked that part of your first review, I wasn't aware inputs could go through the slow-path. I added that part, and made a base class for both FastPathMITM and SlowPathMITM.

@Pourliver Pourliver requested a review from xshill June 12, 2019 14:12
Copy link
Collaborator

@xshill xshill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good!

@Pourliver Pourliver merged commit de67e7e into master Jul 2, 2019
@obilodeau obilodeau deleted the harvest_creds branch November 27, 2019 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants