CheckRDPEncryption function #6204
Conversation
WalkthroughA new feature was introduced to the RDP package to actively probe and report the supported security layers and encryption levels of a given RDP server. This includes the addition of a new response struct, probing logic for various protocols and ciphers, and memoization to cache results for repeated checks. The probing is performed by establishing TCP connections, sending crafted RDP requests, and interpreting the server's responses. The results are aggregated and returned in a structured format, with memoization ensuring efficiency for repeated queries. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant rdp
participant Memoizer
participant RDPServer
Caller->>rdp: CheckRDPEncryption(host, port)
rdp->>Memoizer: Check cache for (host, port)
alt Cache hit
Memoizer-->>rdp: Cached RDPEncryptionResponse
else Cache miss
rdp->>RDPServer: Probe security protocols/ciphers
RDPServer-->>rdp: Responses to probes
rdp->>Memoizer: Store RDPEncryptionResponse
Memoizer-->>rdp: Confirmation
end
rdp-->>Caller: RDPEncryptionResponse
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Debug Data |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pkg/js/libs/rdp/memo.rdp.go (1)
43-57: Memoization logic looks solid – tiny opportunity for naming consistencyBehaviour mirrors the existing
memoizedcheckRDPAuthimplementation and should work as-is.
If you ever touch this file again, consider renaming all three helpers tomemoizedCheckXYZ(capital C) for camel-case consistency, but this is definitely non-blocking.pkg/js/libs/rdp/rdp.go (3)
127-140: Add JSON tags to keep the public JS API stable
toJSON()in the usage examples will currently serialise fields asNativeRDP,RC4_40bit, … which is fine, but any future refactor (e.g. renaming fields) could silently break templates.
Explicit JSON tags make the contract crystal-clear and avoid surprises.- SecurityLayer struct { - NativeRDP bool - SSL bool - CredSSP bool - RDSTLS bool - CredSSPWithEarlyUserAuth bool - } - EncryptionLevel struct { - RC4_40bit bool - RC4_56bit bool - RC4_128bit bool - FIPS140_1 bool - } + SecurityLayer struct { + NativeRDP bool `json:"native_rdp"` + SSL bool `json:"ssl"` + CredSSP bool `json:"credssp"` + RDSTLS bool `json:"rdstls"` + CredSSPWithEarlyUserAuth bool `json:"credssp_early_user_auth"` + } `json:"security_layer"` + EncryptionLevel struct { + RC4_40bit bool `json:"rc4_40bit"` + RC4_56bit bool `json:"rc4_56bit"` + RC4_128bit bool `json:"rc4_128bit"` + FIPS140_1 bool `json:"fips140_1"` + } `json:"encryption_level"`
194-201: Hard-coded magic numbers deserve named constantsUsing integers
0,1,3,4,8for protocols and1,8,2,16for ciphers makes the code error-prone.
Defining constants (or even slices of structs) improves readability and prevents accidental duplication.
229-243: Missing ingress‐egress validation & packet crafting disclaimerThe simplified TPDU you craft (
[]byte{0x03,0x00,…}) may not trigger deterministic behaviour across all RDP implementations and could create false negatives.
Consider adding comments that this is heuristic-only and, if possible, move the magic slice into aconst baseConnectionRequest = [...]byte{…}for maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
cmd/nuclei/rdp.yamlis excluded by!**/*.yamlpkg/js/generated/go/librdp/rdp.gois excluded by!**/generated/**pkg/js/generated/ts/rdp.tsis excluded by!**/generated/**
📒 Files selected for processing (2)
pkg/js/libs/rdp/memo.rdp.go(1 hunks)pkg/js/libs/rdp/rdp.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
| // Test different security protocols | ||
| protocols := map[string]int{ | ||
| "NativeRDP": 0, | ||
| "SSL": 1, | ||
| "CredSSP": 3, | ||
| "RDSTLS": 4, | ||
| "CredSSPWithEarlyUserAuth": 8, | ||
| } | ||
|
|
||
| for name, value := range protocols { | ||
| conn, err := protocolstate.Dialer.Dial(context.TODO(), "tcp", fmt.Sprintf("%s:%d", host, port)) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| defer conn.Close() | ||
|
|
||
| // Test protocol |
There was a problem hiding this comment.
🛠️ Refactor suggestion
defer conn.Close() inside a tight loop leaks sockets until function exit
Every iteration defers a close that is only executed when checkRDPEncryption returns.
For a single call that’s ~9 open TCP fds; across many goroutines this quickly exhausts file descriptors and slows scanning.
- conn, err := protocolstate.Dialer.Dial(...)
- ...
- defer conn.Close()
+ conn, err := protocolstate.Dialer.Dial(...)
+ ...
+ // close immediately after the test to free the fd
+ isRDP, err := testRDPProtocol(conn, timeout, value)
+ _ = conn.Close()Do the same in the cipher loop below (lines 202-208).
Committable suggestion skipped: line range outside the PR's diff.
Proposed changes
Detect RDP encryption using CheckRDPEncryption()
Checklist
Summary by CodeRabbit