fix(telnetmini): prevent slice-bounds panic in ParseNTLMResponse on truncated NTLM challenge - #7425
Conversation
…unds panic on truncated NTLM challenge ParseNTLMResponse read fixed-header fields at offsets 12-14, 16-20, 40-42, and 44-48 after only checking len>=12, causing a slice-bounds-out-of-range panic for any NTLM section between 12 and 47 bytes. Because this input comes from untrusted network data, a malicious or malformed telnet server could crash the scanner (or the embedding host process when used via the SDK). Raise the guard to len<48 so the full fixed header is present before any field is touched. Add unit tests covering the happy path, existing error cases, and a no-panic regression table for every truncated length in [12,47]. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThe PR hardens NTLM response parsing by increasing the minimum required byte count from 12 to 48 bytes before reading fixed-header offset/length fields, with a more specific error message. Tests add helpers and cases verifying valid parsing, malformed inputs, and truncation safety to prevent panics. ChangesNTLM Parsing Guard Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/utils/telnetmini/ntlm_test.go (2)
59-68: ⚡ Quick winConsider testing the exact 48-byte boundary.
The current valid test uses a 54-byte challenge (48-byte header + 6-byte target name). Consider adding a test case for exactly 48 bytes (minimal valid challenge with no target name or info) to verify the boundary condition.
📝 Proposed additional test case
func TestParseNTLMResponse_Minimal48Bytes(t *testing.T) { // Build exactly 48 bytes: minimal valid type-2 challenge with no target name/info data := buildChallenge(48) resp, err := ParseNTLMResponse(data) if err != nil { t.Fatalf("48-byte challenge should be valid, got error: %v", err) } if resp == nil { t.Fatal("expected non-nil response for 48-byte challenge") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/utils/telnetmini/ntlm_test.go` around lines 59 - 68, Add a new unit test to cover the 48-byte boundary: create TestParseNTLMResponse_Minimal48Bytes that calls buildChallenge(48) (or construct a 48-byte challenge similarly to buildValidChallenge), then invoke ParseNTLMResponse(data) and assert no error and non-nil resp; place it alongside TestParseNTLMResponse_Valid to ensure the parser accepts the minimal 48-byte type-2 challenge with no target name/info.
98-98: ⚡ Quick winSimplify the inline anonymous function for readability.
The "wrong message type" test case constructs the input using an immediately-invoked function expression, which makes the table harder to read.
♻️ Proposed simplification
Option 1: Extract to a local variable before the test table:
func TestParseNTLMResponse_ErrorCases(t *testing.T) { wrongTypeChallenge := buildChallenge(48) binary.LittleEndian.PutUint32(wrongTypeChallenge[8:12], 1) // Set type to 1 instead of 2 tests := []struct { name string input []byte wantErr string }{ // ... other cases ... { name: "wrong message type", input: wrongTypeChallenge, wantErr: "expected NTLM challenge message", }, } // ... }Option 2: Add a helper function:
// buildChallengeWithType constructs a challenge with the specified message type func buildChallengeWithType(headerLen int, msgType uint32) []byte { data := buildChallenge(headerLen) if headerLen >= 12 { binary.LittleEndian.PutUint32(data[8:12], msgType) } return data }Then use:
input: buildChallengeWithType(48, 1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/utils/telnetmini/ntlm_test.go` at line 98, Replace the inline immediately-invoked function used for the "wrong message type" test input with a clearer construction: either (A) create a local variable (e.g., wrongTypeChallenge) inside TestParseNTLMResponse_ErrorCases using buildChallenge(48) and then call binary.LittleEndian.PutUint32(wrongTypeChallenge[8:12], 1) before putting it in the tests table, or (B) add a small helper like buildChallengeWithType(headerLen, msgType) that calls buildChallenge/header builder and sets the uint32 at [8:12]; then update the test case to use wrongTypeChallenge or buildChallengeWithType(48, 1) instead of the anonymous function. Ensure the surrounding test still expects "expected NTLM challenge message" and that the data length remains 48.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/utils/telnetmini/ntlm_test.go`:
- Around line 59-68: Add a new unit test to cover the 48-byte boundary: create
TestParseNTLMResponse_Minimal48Bytes that calls buildChallenge(48) (or construct
a 48-byte challenge similarly to buildValidChallenge), then invoke
ParseNTLMResponse(data) and assert no error and non-nil resp; place it alongside
TestParseNTLMResponse_Valid to ensure the parser accepts the minimal 48-byte
type-2 challenge with no target name/info.
- Line 98: Replace the inline immediately-invoked function used for the "wrong
message type" test input with a clearer construction: either (A) create a local
variable (e.g., wrongTypeChallenge) inside TestParseNTLMResponse_ErrorCases
using buildChallenge(48) and then call
binary.LittleEndian.PutUint32(wrongTypeChallenge[8:12], 1) before putting it in
the tests table, or (B) add a small helper like
buildChallengeWithType(headerLen, msgType) that calls buildChallenge/header
builder and sets the uint32 at [8:12]; then update the test case to use
wrongTypeChallenge or buildChallengeWithType(48, 1) instead of the anonymous
function. Ensure the surrounding test still expects "expected NTLM challenge
message" and that the data length remains 48.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cb5877cd-124c-4cae-9d30-f77242e1b1c0
📒 Files selected for processing (2)
pkg/utils/telnetmini/ntlm.gopkg/utils/telnetmini/ntlm_test.go
…st and clean up wrong-type input - Add TestParseNTLMResponse_Minimal48Bytes to explicitly cover the exact lower boundary of the len<48 guard (minimal valid challenge, no target name or info) - Replace the inline immediately-invoked function for the wrong-message-type case with a pre-built local variable for readability Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@coderabbitai create an issue for this PR |
|
Tip For best results, initiate chat on the files or code changes.
|
|
@Mzack9999 @coderabbitai. why can't this PR be merged? |
|
Tip For best results, initiate chat on the files or code changes.
Once those complete successfully, the PR should be ready to merge. You (or a maintainer) can keep an eye on the workflow run for live status. (ฅ^ω^ฅ) Hang tight — almost there! |
|
so once the tests will be done and green light. will it be merged by @Mzack9999 or @coderabbitai |
|
Tip For best results, initiate chat on the files or code changes.
(=^･ω･^=) |
|
The two CI failure categories are unrelated to this diff (which only touches Functional tests — every failure is the same Integration tests ( All checks that exercise the changed code paths — unit tests, lint, vet, template-validate, CodeQL, and the release test — are green on all platforms. |
|
Tip For best results, initiate chat on the files or code changes.
Functional tests: Confirmed pre-existing — the same failures appeared on Integration tests: This is the one area that differs —
|
2 participants
fix(telnetmini): prevent slice-bounds panic in ParseNTLMResponse on truncated NTLM challenge
Background
pkg/utils/telnetminiimplements the NTLM negotiation handshake used duringtelnet fingerprinting, mirroring the logic from Nmap's
telnet-ntlm-info.nsescript.
ParseNTLMResponsereads the server's type-2 Challenge message off thewire, extracts system metadata (NetBIOS names, DNS names, OS version), and
returns it as an
NTLMInfoResponse. Because the input comes directly from aremote host, it is fully attacker-controlled.
The Bug
ParseNTLMResponsepanics with aruntime error: slice bounds out of rangewhenever the NTLMSSP section of the response is between 12 and 47 bytes long.
Root cause
The function checks
len(ntlmData) < 12and then reads the message type atntlmData[8:12]. After that single check it reads four more fields from fixedoffsets in the header:
ntlmData[12:14]ntlmData[16:20]ntlmData[40:42]ntlmData[44:48]Per [MS-NLMP], the type-2 Challenge fixed header spans bytes 0–47 (48 bytes
total). The guard only guaranteed enough bytes to read the message type — it
said nothing about the offsets at bytes 40–47. Any
ntlmDataof length 12–47therefore passes the guard, passes the type check, and then panics on one of
the later reads.
Why this matters
The input is raw network data. A malformed or deliberately crafted telnet
server can send a response whose NTLMSSP section is between 12 and 47 bytes.
In the standalone scanner this crashes the current scan. When Nuclei is
embedded via the Go SDK an unrecovered panic in this goroutine can propagate
and take down the entire host process.
The Fix
One line change in
pkg/utils/telnetmini/ntlm.go.Replace:
With:
The fixed header must be fully present before any field is read. The existing
per-field bounds checks for the variable-length
targetNameandtargetInfoblocks that follow the header are already correct and require no changes.
Files Changed
pkg/utils/telnetmini/ntlm.gopkg/utils/telnetmini/ntlm_test.goTests
New file:
pkg/utils/telnetmini/ntlm_test.goTestParseNTLMResponse_ValidConstructs a fully-formed 48-byte type-2 Challenge with a UTF-16LE target name
appended after the header. Asserts a non-nil response and no error.
TestParseNTLMResponse_ErrorCasesTable-driven test covering all existing error paths:
NTLMSSP signature not foundNTLMSSP signature not foundNTLMSSP signature not foundnot properly terminatedexpected NTLM challenge messageTestParseNTLMResponse_TruncatedNoPanic(regression)Iterates every NTLM section length from 12 to 47 inclusive, calls
ParseNTLMResponse, and asserts the function returns an error rather thanpanicking. This test fails on unpatched
mainand passes after the fix.All three test functions pass.
Possible Issues After This Change
1. Non-Microsoft servers sending a 12–47-byte NTLM section
What changes.
Before this fix, any response with a 12–47-byte NTLM section caused a panic —
which is never a useful outcome. After this fix those responses return a
graceful error. For any server sending a fully-formed ≥48-byte header (every
spec-compliant implementation) behaviour is completely unchanged.
Is this a real risk?
Unlikely. [MS-NLMP] mandates a 48-byte fixed header for the type-2 Challenge.
The existing code already filters out non-conforming NTLM implementations by
requiring the
0xFF 0xF0Sub-option End terminator, mirroring the Nmap scriptcomment that these implementations "do not return valid data." A response that
passes the terminator check but has a sub-48-byte NTLM section is both
out-of-spec and was already broken before (panic instead of error).
How to resolve if a real target is affected.
If a target is observed emitting a short-but-otherwise-valid NTLM section, the
fix is to restructure the reads into per-field length checks rather than a
single upfront guard. The safer default is to document the target as
unsupported, consistent with the existing filtering logic.
2. Error message string change
What changes.
The error text for an undersized response changed from:
to:
Is this a real risk?
No. The one call site —
pkg/js/libs/telnet/telnet.go:272— wraps the errorwith
%wand returns it upstream without inspecting the message string. Noother caller exists in the codebase.
How to resolve if encountered.
Any consumer doing
strings.Contains(err.Error(), "too short")is unaffectedsince that substring is preserved. An exact-string match would need updating to
the new format.
3. No partial-parse data on truncated input
What changes.
Previously the function panicked mid-read on a 12–47-byte input, so it never
returned a result regardless. After this fix it returns
nil, error. Theobservable outcome for all callers is identical — a nil response — except the
process no longer crashes.
Is this a real risk?
No. A panic is not a return value. No caller could have been relying on
receiving data from a path that always crashed.
Summary by CodeRabbit
Bug Fixes
Tests