fix: prevent path traversal in markdown exporter#7217
fix: prevent path traversal in markdown exporter#7217sandiyochristan wants to merge 2 commits intoprojectdiscovery:devfrom
Conversation
…sal sequences The sanitizeFilename function did not handle ".." sequences, allowing path traversal when event.Host or event.TemplateID contains dot-dot patterns. Added ".." replacement in sanitization and path prefix validation to ensure exported files stay within the intended directory.
Neo - PR Security ReviewNo security issues found Highlights
Comment |
WalkthroughValidate resolved subdirectory and output file paths against the configured export directory to prevent path traversal in Markdown export flow; sanitize filenames by replacing suspicious ".." sequences before further processing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
…ectory bypass The strings.HasPrefix check without a trailing separator allows bypass via sibling directories with matching prefixes (e.g., /reports-evil matching /reports). Appending filepath.Separator ensures only true subdirectories pass validation.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/reporting/exporters/markdown/markdown.go`:
- Around line 90-94: Replace the current prefix-based containment checks with a
filepath.Rel-based check: compute rel, err :=
filepath.Rel(filepath.Clean(exporter.directory),
filepath.Clean(filepath.Join(exporter.directory, subdirectory))) and return an
error if err != nil or strings.HasPrefix(rel, "..") to robustly detect path
traversal; apply the same change wherever the code currently validates
resolvedDir against exporter.directory (references: resolvedDir,
exporter.directory, subdirectory) including the other occurrence around the
123-127 region so both validations use filepath.Rel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a460c7ba-9ee7-4fa3-82fb-4a89a5f4ed54
📒 Files selected for processing (1)
pkg/reporting/exporters/markdown/markdown.go
| // Validate the resolved path stays within the export directory | ||
| resolvedDir := filepath.Clean(filepath.Join(exporter.directory, subdirectory)) | ||
| if !strings.HasPrefix(resolvedDir, filepath.Clean(exporter.directory)) { | ||
| return fmt.Errorf("path traversal detected in subdirectory: %s", subdirectory) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: find filesystem containment checks using strings.HasPrefix in Go code.
# Expected: no path-containment logic should rely on strings.HasPrefix after remediation.
rg -nP --type=go -C3 'filepath\.Clean\(filepath\.Join\([^\n]*\)\)[\s\S]{0,200}?strings\.HasPrefix\('Repository: projectdiscovery/nuclei
Length of output: 49
🏁 Script executed:
cat -n pkg/reporting/exporters/markdown/markdown.go | sed -n '80,130p'Repository: projectdiscovery/nuclei
Length of output: 2518
Path containment check should use filepath.Rel for more robust containment validation.
The code now appends a separator to prevent sibling directory bypass (e.g., /reports-evil matching /reports), but this heuristic-based approach does not prevent all path traversal vectors. At lines 90-96 and 123-127, use filepath.Rel-based containment checks instead:
Suggested fix
- cleanBase := filepath.Clean(exporter.directory) + string(filepath.Separator)
- resolvedDir := filepath.Clean(filepath.Join(exporter.directory, subdirectory))
- if !strings.HasPrefix(resolvedDir, cleanBase) {
+ baseDir := filepath.Clean(exporter.directory)
+ resolvedDir := filepath.Clean(filepath.Join(baseDir, subdirectory))
+ relDir, relErr := filepath.Rel(baseDir, resolvedDir)
+ if relErr != nil || relDir == ".." || strings.HasPrefix(relDir, ".."+string(filepath.Separator)) {
return fmt.Errorf("path traversal detected in subdirectory: %s", subdirectory)
}- cleanBase := filepath.Clean(exporter.directory) + string(filepath.Separator)
- finalPath := filepath.Clean(filepath.Join(exporter.directory, subdirectory, filename))
- if !strings.HasPrefix(finalPath, cleanBase) {
+ baseDir := filepath.Clean(exporter.directory)
+ finalPath := filepath.Clean(filepath.Join(baseDir, subdirectory, filename))
+ relFile, relErr := filepath.Rel(baseDir, finalPath)
+ if relErr != nil || relFile == ".." || strings.HasPrefix(relFile, ".."+string(filepath.Separator)) {
return fmt.Errorf("path traversal detected in output path: %s", finalPath)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/reporting/exporters/markdown/markdown.go` around lines 90 - 94, Replace
the current prefix-based containment checks with a filepath.Rel-based check:
compute rel, err := filepath.Rel(filepath.Clean(exporter.directory),
filepath.Clean(filepath.Join(exporter.directory, subdirectory))) and return an
error if err != nil or strings.HasPrefix(rel, "..") to robustly detect path
traversal; apply the same change wherever the code currently validates
resolvedDir against exporter.directory (references: resolvedDir,
exporter.directory, subdirectory) including the other occurrence around the
123-127 region so both validations use filepath.Rel.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/reporting/exporters/markdown/markdown.go (1)
123-128: Consider extractingcleanBasecomputation to reduce duplication.
cleanBaseis computed identically at line 92 and line 123. Since the check at lines 123-127 always runs (to validate the full path including filename), you could computecleanBaseonce at the start of theExportfunction.♻️ Suggested refactor
Compute
cleanBaseonce near the beginning ofExport():func (exporter *Exporter) Export(event *output.ResultEvent) error { cleanBase := filepath.Clean(exporter.directory) + string(filepath.Separator) // ... rest of function uses cleanBaseThen remove the duplicate computation at line 92, reusing the same variable in both validation blocks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reporting/exporters/markdown/markdown.go` around lines 123 - 128, Compute cleanBase once at the top of Export(): set cleanBase := filepath.Clean(exporter.directory) + string(filepath.Separator) near the start of the Export method and replace the duplicate computations (the one used before validating subdirectory and the one at the filename validation) to reuse this single variable; remove the redundant cleanBase declaration around line 92/123 so both validation blocks reference the same cleanBase and keep the existing filepath.Join/finalPath and path traversal checks intact.
🤖 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/reporting/exporters/markdown/markdown.go`:
- Around line 123-128: Compute cleanBase once at the top of Export(): set
cleanBase := filepath.Clean(exporter.directory) + string(filepath.Separator)
near the start of the Export method and replace the duplicate computations (the
one used before validating subdirectory and the one at the filename validation)
to reuse this single variable; remove the redundant cleanBase declaration around
line 92/123 so both validation blocks reference the same cleanBase and keep the
existing filepath.Join/finalPath and path traversal checks intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bcebf48-0af6-49b2-a1da-03e745aa8f9f
📒 Files selected for processing (1)
pkg/reporting/exporters/markdown/markdown.go
dwisiswant0
left a comment
dwisiswant0
left a comment
There was a problem hiding this comment.
Please file an issue before submitting fix patches.
Include reproduction steps as per the template and the exact nuclei command (or custom runner PoC for Nuclei SDK) that triggers unexpected behavior. The reproduction must be based on a realistic execution path that is fully concrete and reproducible.
Thanks!
2 participants
Proposed Changes
The markdown report exporter is vulnerable to path traversal (CWE-22) when the sort mode is set to
hostortemplate. An attacker-controlledHostheader or craftedTemplateIDcontaining..sequences can cause report files to be written outside the intended export directory.Root Cause
The
sanitizeFilename()function inpkg/reporting/exporters/markdown/markdown.goreplaces dangerous characters (/,?,*, etc.) with_, but does not handle..(dot-dot) directory traversal sequences.When sort mode is
host, theevent.Hostvalue is used as a subdirectory:After sanitization,
..passes through unchanged. Thenfilepath.Join(exporter.directory, "..")resolves to the parent of the export directory, enabling arbitrary file writes.Exploitation Scenario
Hostheader value:../../../tmp/malicious/characters are replaced with_by sanitizeFilename →_.._.._.._tmp_malicious..— it passes through completely unchangedfilepath.Join("/reports", "..", "evil.md")→/evil.md— file written outside reports directoryThe Fix (Defense in Depth — Two Layers)
Layer 1: Sanitization —
sanitizeFilename()now replaces..with_before other character replacements:Layer 2: Path Validation — Added
filepath.Clean()+strings.HasPrefix()checks before both directory creation and file writing, ensuring the resolved path always stays withinexporter.directory. This is the same validated pattern used inpkg/installer/template.go(the zip extraction path traversal guard).Files Changed
pkg/reporting/exporters/markdown/markdown.go— added..replacement in sanitizeFilename, added path prefix validation before directory creation and file writeSecurity Impact
Proof
The fix follows the exact same defensive pattern already established in the codebase:
Checklist
devbranchSummary by CodeRabbit