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

Fixed licensing issue and added licensing boilerplate #118

Merged
merged 2 commits into from
Jul 2, 2019
Merged

Conversation

Pourliver
Copy link
Contributor

I have fixed the issue discussed in #104. The actual fix in in SecurityMITM.py, but I have left some boilerplate code inside the (unused) licensing.py if we ever feel like parsing the data.

I had to make a pre-parse validation in order to avoir sending licensing data to the slowpath. I am not quite happy with that method, and I'm open to change it if you have any propositions.

@Pourliver Pourliver requested review from Res260 and xshill June 12, 2019 19:11
@Res260
Copy link
Collaborator

Res260 commented Jun 12, 2019

From what you saw of the licensing packets, are there nice information that could be used to fingerprint clients or might be relevant to log in general?

@@ -135,7 +135,9 @@ def __init__(self, parser = BasicSecurityParser()):
self.securityHeaderExpected = False

def recv(self, data: bytes):
if not self.securityHeaderExpected:
# Licensing happens in the security layer
licensingBytes = b"\x80\x00"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does that correspond to a flag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, but since this isn't parsed data, it is not in the correct format. Would it be better if I refer to this flag, and parse the 0x80\x00 and compare to the flag?

In pyrdp/enum/rdp.py - line 43
SEC_LICENSE_PKT = 0x0080

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah for that @xshill would have a better idea than me. I have an idea but i’m really not sure if its good

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

The licensingBytes you're looking for IS part of a security header. You shouldn't need to check for that if securityHeaderExpected is set properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Licensing is an optional part of the protocol, and the boolean "securityHeaderExpected" is false when receiving a licensing PDU. securityHeaderExpected is only used to redirect traffic to the securityLayer when we know we are expecting it in the handshake

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh right, since this is TLS it starts at false.

@Pourliver
Copy link
Contributor Author

The only two interesting things I saw were the "clientUserName" and "clientMachineName", so current username and hostname. With Remmina, the client sent "username" instead of a username to bind the certificate to Remmina instead of a specific user.

I don't think these two have any value since they can be tampered.

Reference : https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-rdpele/c57e4890-9049-421e-9fe8-9a6f9519675a

@Res260
Copy link
Collaborator

Res260 commented Jun 12, 2019

The value might come for fingerprinting purposes, even if they can be tampered. Ex: debbie didnt tamper anything, so he was pretty easy to identify.

Of course its not urgent, simply a nice to have and good to know

@Pourliver Pourliver merged commit 9e68a45 into master Jul 2, 2019
@obilodeau obilodeau deleted the rds_license 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.

3 participants