Conversation
WalkthroughAdds RDP probing: a new RDPEncryptionResponse type and CheckRDPEncryption(ctx, host, port) that actively probes RDP security layers and cipher strengths, plus a memoized wrapper memoizedcheckRDPEncryption(executionId, host, port) to cache probe results. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RDPpkg as RDP package
participant Memoizer
participant Target as RDP Server
Caller->>RDPpkg: CheckRDPEncryption(ctx, host, port)
RDPpkg->>Memoizer: Do("checkRDPEncryption", executionId, host, port)
alt cache hit
Memoizer-->>RDPpkg: cached RDPEncryptionResponse
else cache miss
Memoizer-->>RDPpkg: callback -> checkRDPEncryption()
RDPpkg->>Target: TCP connect + probe (protocol/cipher)
Target-->>RDPpkg: probe responses
RDPpkg->>RDPpkg: analyze -> RDPEncryptionResponse
RDPpkg-->>Memoizer: store RDPEncryptionResponse
Memoizer-->>RDPpkg: stored
end
RDPpkg-->>Caller: RDPEncryptionResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. Comment |
|
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)
|
@dogancanbakir any blockers on this? We can merge it with any required changes to get it ready. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
pkg/js/libs/rdp/rdp.go (3)
132-156: Stabilize marshaling surface (optional)Consider adding explicit json tags and/or named nested types to future‑proof the JS/JSON shape and avoid accidental field renames.
243-281: Heuristic protocol negotiation is fragileThese magic‑byte writes/reads risk false positives and version variance. Prefer leveraging fingerprintx’s RDP negotiation primitives (as used in DetectRDP/DetectRDPAuth) to probe security layers, or extend that lib to expose needed signals.
282-316: Cipher probing via raw bytes is brittleSame concern as above; consider a library‑backed negotiation to derive supported encryption levels reliably.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/js/generated/go/librdp/rdp.gois 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)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/js/libs/rdp/memo.rdp.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/js/libs/rdp/rdp.go
🧬 Code graph analysis (1)
pkg/js/libs/rdp/rdp.go (1)
pkg/js/generated/ts/rdp.ts (2)
CheckRDPEncryption(26-28)RDPEncryptionResponse(73-87)
🪛 GitHub Check: Lint
pkg/js/libs/rdp/rdp.go
[failure] 218-218:
undefined: protocolstate.Dialer (typecheck)
[failure] 185-185:
undefined: protocolstate.Dialer
🪛 GitHub Actions: 🔨 Tests
pkg/js/libs/rdp/rdp.go
[error] 185-185: undefined: protocolstate.Dialer (golangci-lint: undefined symbol in rdp.go line 185)
🔇 Additional comments (3)
pkg/js/libs/rdp/rdp.go (3)
6-6: Import looks correct"net" is needed for net.Conn in helpers; no issues.
184-206: Fix compile error and fd leak in protocol loop
- protocolstate.Dialer is undefined (pipeline failure).
- defer conn.Close() inside loop leaks fds until function exit.
Apply this diff:
- for name, value := range protocols { - conn, err := protocolstate.Dialer.Dial(context.TODO(), "tcp", fmt.Sprintf("%s:%d", host, port)) + for name, value := range protocols { + conn, err := dialer.Fastdialer.Dial(context.TODO(), "tcp", target) if err != nil { continue } - defer conn.Close() - - // Test protocol - isRDP, err := testRDPProtocol(conn, timeout, value) + // Test protocol + isRDP, err := testRDPProtocol(conn, timeout, value) + _ = conn.Close() if err == nil && isRDP { switch name { case "NativeRDP": resp.SecurityLayer.NativeRDP = true case "SSL": resp.SecurityLayer.SSL = true case "CredSSP": resp.SecurityLayer.CredSSP = true case "RDSTLS": resp.SecurityLayer.RDSTLS = true case "CredSSPWithEarlyUserAuth": resp.SecurityLayer.CredSSPWithEarlyUserAuth = true } } }As per coding guidelines
217-238: Same issues in cipher loop: undefined dialer and fd leakUse Fastdialer and close immediately after testing.
Apply this diff:
- for name, value := range ciphers { - conn, err := protocolstate.Dialer.Dial(context.TODO(), "tcp", fmt.Sprintf("%s:%d", host, port)) + for name, value := range ciphers { + conn, err := dialer.Fastdialer.Dial(context.TODO(), "tcp", target) if err != nil { continue } - defer conn.Close() - - // Test cipher - isRDP, err := testRDPCipher(conn, timeout, value) + // Test cipher + isRDP, err := testRDPCipher(conn, timeout, value) + _ = conn.Close() if err == nil && isRDP { switch name { case "RC4_40bit": resp.EncryptionLevel.RC4_40bit = true case "RC4_56bit": resp.EncryptionLevel.RC4_56bit = true case "RC4_128bit": resp.EncryptionLevel.RC4_128bit = true case "FIPS140_1": resp.EncryptionLevel.FIPS140_1 = true } } }As per coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
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)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/js/libs/rdp/rdp.gopkg/js/libs/rdp/memo.rdp.go
🧬 Code graph analysis (2)
pkg/js/libs/rdp/rdp.go (2)
pkg/js/generated/ts/rdp.ts (2)
CheckRDPEncryption(26-28)RDPEncryptionResponse(73-86)pkg/protocols/common/protocolstate/state.go (1)
GetDialersWithId(39-45)
pkg/js/libs/rdp/memo.rdp.go (3)
pkg/js/libs/rdp/rdp.go (1)
RDPEncryptionResponse(165-178)pkg/js/generated/ts/rdp.ts (1)
RDPEncryptionResponse(73-86)pkg/protocols/common/protocolstate/memoizer.go (1)
Memoizer(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (1)
pkg/js/libs/rdp/memo.rdp.go (1)
43-57: Memoization wiring matches existing patternThe new
memoizedcheckRDPEncryptionsticks to the existingMemoizer.Doconvention and cleanly wrapscheckRDPEncryption; looks good.
|
@CodeRabbit resolve |
✅ Actions performedComments resolved. |
3 participants
Proposed changes
Detect RDP encryption using CheckRDPEncryption()
Checklist
Summary by CodeRabbit