Repository navigation
fix(interactsh): serialize InternalEvent access - #7322
Conversation
`interactsh.(*Client).processInteractionForRequest` was updating the wrapped event under lock but releasing it before calling `Operators.Execute()`, allowing async interactsh updates to race with matcher expr evaluation on the same `InternalEvent` map, causing fatal concurrent map iteration/write crashes. Hold the event lock while populating interactsh fields and running operators, and use a locked `eventHash()` helper for matched-template cache lookups that access the same event data. Fixes #7319 Signed-off-by: Dwi Siswanto <git@dw1.io>
WalkthroughUpdated interactsh event processing to use a hash wrapper function instead of directly hashing event data, preventing concurrent map iteration issues during template matching. Refactored operator execution error handling to ensure proper unlock sequencing, and added a concurrent event mutation test to validate thread-safety. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Neo - PR Security ReviewNo security issues found Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/protocols/common/interactsh/interactsh_test.go (1)
55-60: Consider adding a brief comment explaining the synchronization pattern.The
MatchFuncclosure orchestrates the race condition scenario, but the purpose isn't immediately obvious.📝 Suggested documentation improvement
MatchFunc: func(data map[string]interface{}, matcher *matchers.Matcher) (bool, []string) { + // Signal writer goroutine to start mutating while we're in the middle of matching. + // With the fix, the writer will block waiting for the lock that processInteractionForRequest holds. startWriter.Do(func() { close(writerStarted) }) + // Yield to allow writer goroutine to be scheduled (it will block on the lock). runtime.Gosched() return matcher.MatchWords("not-present-in-corpus", data) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/protocols/common/interactsh/interactsh_test.go` around lines 55 - 60, Add a short comment inside the MatchFunc closure (near startWriter.Do, close(writerStarted), and runtime.Gosched()) that explains the synchronization pattern: that startWriter.Do signals the writer goroutine to begin by closing writerStarted, runtime.Gosched is used to yield the scheduler to increase the likelihood of the race for the test, and matcher.MatchWords("not-present-in-corpus", data) is then evaluated to exercise the race condition; reference the symbols MatchFunc, startWriter, writerStarted, runtime.Gosched, and matcher.MatchWords to make the intent and safety of the ordering explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/protocols/common/interactsh/interactsh_test.go`:
- Around line 55-60: Add a short comment inside the MatchFunc closure (near
startWriter.Do, close(writerStarted), and runtime.Gosched()) that explains the
synchronization pattern: that startWriter.Do signals the writer goroutine to
begin by closing writerStarted, runtime.Gosched is used to yield the scheduler
to increase the likelihood of the race for the test, and
matcher.MatchWords("not-present-in-corpus", data) is then evaluated to exercise
the race condition; reference the symbols MatchFunc, startWriter, writerStarted,
runtime.Gosched, and matcher.MatchWords to make the intent and safety of the
ordering explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa46cbb1-277c-46ad-8ac1-18d8f8a3677c
📒 Files selected for processing (2)
pkg/protocols/common/interactsh/interactsh.gopkg/protocols/common/interactsh/interactsh_test.go
nuclei 3.9.0 Created-by: HarmonybrewBot Commit-by: HarmonybrewBot Merged-by: HarmonybrewBot Description: Created by `brew bump` --- Created with `brew bump-formula-pr`.<details> <summary>release notes</summary> <pre>## What's Changed ### 🎉 New Features * Added support for protocol redirects by @Mzack9999 in projectdiscovery/nuclei#7296 * Added impacket integration by @Mzack9999 in projectdiscovery/nuclei#7356 * Added wmi, tsch, scmr, and dcom helper modules for JS by @dwisiswant0 in projectdiscovery/nuclei#7388 * Added mirroring of -config, -report-config, and -dashboard flags for SDK callers by @ShubhamRasal in projectdiscovery/nuclei#7393 ### 🐞 Bug Fixes * Fixed DNS variables resolving issue by @Mzack9999 in projectdiscovery/nuclei#7379 * Fixed expressions to prefer exact placeholders over expressions by @dwisiswant0 in projectdiscovery/nuclei#7397 * Fixed an issue in JS where runtimes that outlive the interrupt grace period are not abandoned, by @Mzack9999 in projectdiscovery/nuclei#7378 * Fixed an issue in reporting/jira where status-not was not matched against the transition target status name by @knakul853 in projectdiscovery/nuclei#7361 * Fixed runner to write unsigned-templates warning (WRN) to stderr by @ChrisJr404 in projectdiscovery/nuclei#7371 * Fixed `InternalEvent` access serialization in interactsh by @dwisiswant0 in projectdiscovery/nuclei#7322 * Fixed workflow to ensure context is properly propagated to child and race steps by @dwisiswant0 in projectdiscovery/nuclei#7383 * Fixed reporting/gitlab by bumping client-go to v1.9.1 and widening IDs to int64 by @ShubhamRasal in projectdiscovery/nuclei#7398 * Fixed memogen to skip context correctly and auto-push scope to upstream by @Mzack9999 in projectdiscovery/nuclei#7419 * Fixed a slice-bounds panic in telnetmini's ParseNTLMResponse when handling truncated NTLM challenges by @tejgokani in projectdiscovery/nuclei#7425 * Fixed SMBv1 probing in JS to occur after SMB2 negotiation by @dwisiswant0 in projectdiscovery/nuclei#7430 * Fixed templates to reject unknown fields when loading JSON templates by @tejgokani in projectdiscovery/nuclei#7453 * Fixed persistence of QueryAuthStrategy parameters in ApplyAuthStrategy by @akhilesharora in projectdiscovery/nuclei#7410 * Fixed loader to warn when templates are excluded by `.nuclei-ignore` tags by @dogancanbakir in projectdiscovery/nuclei#7452 ### Other Changes * Splitted exact matches and pattern handling in catalog for improved performance by @mikhail5555 in projectdiscovery/nuclei#7340 * Updated README_PT-BR.md to reflect current project description and features by @gugacyber in projectdiscovery/nuclei#7405 * Fixed broken cloud scanning link by @dogancanbakir in projectdiscovery/nuclei#7413 * Bumped pdsec modules + govaluate/aurora migration by @dogancanbakir in projectdiscovery/nuclei#7414 * Dropped vulnerable docker/docker dependency by @dogancanbakir in projectdiscovery/nuclei#7447 * Used path-aware filesystem containment checks by @dwisiswant0 in projectdiscovery/nuclei#7420 * Centralized runtime session handling by @dwisiswant0 in projectdiscovery/nuclei#7449 ## New Contributors * @ChrisJr404 made their first contribution in projectdiscovery/nuclei#7371 * @gugacyber made their first contribution in projectdiscovery/nuclei#7405 * @akhilesharora made their first contribution in projectdiscovery/nuclei#7410 * @tejgokani made their first contribution in projectdiscovery/nuclei#7425 **Full Changelog**: https://github.com/projectdiscovery/nuclei/compare/v3.8.0...v3.9.0</pre> <p>View the full release notes at <a href="https://github.com/projectdiscovery/nuclei/releases/tag/v3.9.0">https://github.com/projectdiscovery/nuclei/releases/tag/v3.9.0</a>.</p> </details> <hr> See merge request: Harmonybrew/homebrew-core!11229
Proposed changes
fix(interactsh): serialize
InternalEventaccessinteractsh.(*Client).processInteractionForRequestwas updating the wrapped event under lock but
releasing it before calling
Operators.Execute(),allowing async interactsh updates to race with
matcher expr evaluation on the same
InternalEventmap, causing fatal concurrent map iteration/write
crashes.
Hold the event lock while populating interactsh
fields and running operators, and use a locked
eventHash()helper for matched-template cachelookups that access the same event data.
Fixes #7319
Proof
patch:
dev:
Checklist
Summary by CodeRabbit
Bug Fixes
Tests