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

Add parsing for client_channels in RDP analyzer #384

Merged
merged 2 commits into from May 28, 2019

Conversation

grigorescu
Copy link
Contributor

Parses the Client Network Data (TS_UD_CS_NET) packet to extract channels the client requested. This is timely due to CVE-2019-0708 abusing client channels for an unauthenticated remote code execution vulnerability.

@jsiwek jsiwek self-assigned this May 28, 2019
jsiwek added a commit to zeek/zeek-docs that referenced this pull request May 28, 2019
@jsiwek jsiwek merged commit 85fc553 into master May 28, 2019
@jsiwek
Copy link
Contributor

jsiwek commented May 28, 2019

Merged, thanks.

One thing I noticed from the spec was that the maximum allowed channel count is 31. Not sure if that warrants doing an &enforce or "weird" by default. Maybe not a big deal to just leave it up to user to detect that at script-layer if they want.

@grigorescu
Copy link
Contributor Author

The recent vulnerability is when you have MS_T120 on a channel != 31. I think that this code path will be increasingly attacked given the recent CVE, so I think it makes sense to just parse it and leave it to script-land to deal with. Maybe have script-land generate a weird?

A hypothetical is that maybe there's another buffer overflow in RDP if you have more than 31 channels, but it requires a specific channel name in a specific location? I wouldn't want the analyzer bailing out early in that case.

@jsiwek
Copy link
Contributor

jsiwek commented May 28, 2019

Yeah, makes sense and seems fine to always try parsing out and providing the full list.

Generating a weird by default seems convenient enough in this case to give a straightforward path for users to turn it into a notice and seems like it's something I would have done/suggested not even knowing it was related to the recent CVE so seems even more justified then.

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

2 participants