-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 TLS support for rdp-enum-encryption #1614
Conversation
I looked into CredSSP. I can a see the basic auth flow and I've created a script that doesn't fully authenticate but does allow determining host info pre-auth. It's very similar to other scripts. I'll clean up tonight and submit via a separate PR. Rough output from the unfinished script. PORT STATE SERVICE REASON
3389/tcp open ms-wbt-server syn-ack ttl 128
| rdp-credssp-info:
| Target_Name: W2016
| NetBIOS_Domain_Name: W2016
| NetBIOS_Computer_Name: W16GA-SRV01
| DNS_Domain_Name: W2016.lab
| DNS_Computer_Name: W16GA-SRV01.W2016.lab
| DNS_Tree_Name: W2016.lab
| Product_Version: 10.0.14393
|_ System_Time: 2019-05-30T13:02:39 |
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'm really excited about this. Only very minor issues in the comments. Thanks!
@@ -183,7 +239,7 @@ Request = { | |||
|
|||
local data = stdnse.fromhex( | |||
"7f 65" .. -- BER: Application-Defined Type = APPLICATION 101, | |||
"82 01 90" .. -- BER: Type Length = 404 bytes | |||
"82 01 94" .. -- BER: Type Length = 404 bytes |
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.
Can you update the comment, too? or just leave the decimal value out. Same on line 273.
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.
The value is correct as are most of the others. I'll fixed the one error I found.
scripts/rdp-enum-encryption.nse
Outdated
-- version to negotiate TLS or NLA. This section does that for TLS. There | ||
-- is no NLA currently. | ||
if status and (v == 1) then | ||
status, err = comm.socket:reconnect_ssl() |
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.
Just want to put a bug in your ear: if this part can be extracted into a "StartTLS" function in sslcert.lua
it opens up access to all the ssl-*
scripts to work on RDP, too. It sounded on Twitter like you were thinking of extracting SSL cert, and if so, this would be a great way to do that.
Don't hold up merging this PR to do that, though: it's better to commit a working intermediate (especially when it has such an impact as this) and then work on expanding it later.
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.
It thought about that.. and then I ran ssl-cert
and saw that it pulled the certificates without any changes. I'll revisit that.
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.
oh, IIRC, it just connects TLS directly without doing preliminary handshaking. That seems to work on some implementations, but I'm not sure how robust it is for all the configurations you're testing.
nselib/rdp.lua
Outdated
decoder:registerTagDecoders( tag_decoder ) | ||
|
||
local response_result, userdata | ||
_, pos = decoder.decodeLength(data, pos) |
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.
_
needs to be local here. You could do local _, pos = decoder.decodeLength(data, 3)
and eliminate the extra declaration on line 146.
scripts/rdp-enum-encryption.nse
Outdated
-- version to negotiate TLS or NLA. This section does that for TLS. There | ||
-- is no NLA currently. | ||
if status and (v == 1) then | ||
status, err = comm.socket:reconnect_ssl() |
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.
err
needs to be declared local, too. I find things like this with my Lua check script from the Code Standards page on SecWiki.
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 use luacheck
but forgot to do so after refactoring.
@dmiller-nmap |
This PR adds TLS support to
rdp-enum-encryption
. The value that it adds is that it enables determining the RDP protocol version against servers that require TLS and potentially lays the ground work for CredSSP. It also corrects a few values in the RDP payload that were incorrect.Windows Server 2016 with TLS required, NLA optional
Windows Server 2016 with NLA required
NLA is required so we don't see RDP Protocol Version
Windows Server 2019 with TLS required, NLA optional
Windows XP
No change in behavior