fix: prevent path traversal in S3 custom template downloads

[sandiyochristan]

Proposed Changes

The S3 custom template download function is vulnerable to path traversal (CWE-22), similar to the well-known "Zip Slip" vulnerability class. S3 object keys are attacker-controlled and were used directly in filepath.Join() without validation, allowing template files to be written outside the intended download directory.

Additionally, directories were created with overly permissive 0775 permissions, allowing other local users to modify downloaded templates.

Root Cause

In pkg/external/customtemplates/s3.go, the downloadToFile function constructs the output path by directly joining the target directory with the S3 object key:

func downloadToFile(downloader *manager.Downloader, targetDirectory, bucket, key string) error {
    file := filepath.Join(targetDirectory, key)  // key is attacker-controlled
    // ...
    fd, err := os.Create(file)  // arbitrary file write
}

S3 object keys can contain path separators and traversal sequences. An attacker who controls the contents of an S3 bucket (or compromises one) can create objects with keys like:

• ../../../etc/cron.d/malicious-job
• ../../.ssh/authorized_keys
• ../../../home/user/.bashrc

Exploitation Scenario

1. User configures nuclei to download custom templates from an S3 bucket: -aws-bucket-name my-templates
2. Attacker has write access to the bucket (compromised credentials, misconfigured bucket policy, or social engineering)
3. Attacker uploads an S3 object with key ../../../home/user/.bashrc
4. When nuclei downloads templates, filepath.Join(downloadPath, "../../../home/user/.bashrc") resolves outside downloadPath
5. The malicious file overwrites the user's .bashrc → arbitrary code execution on next shell login

The Fix

Path Validation — The output path is now cleaned with filepath.Clean() and validated with strings.HasPrefix() to ensure it stays within targetDirectory:

file := filepath.Clean(filepath.Join(targetDirectory, key))
if !strings.HasPrefix(file, filepath.Clean(targetDirectory)) {
    return fmt.Errorf("path traversal detected in S3 object key: %s", key)
}

Tightened Permissions — Directory permissions changed from 0775 to 0750, preventing other local users from modifying downloaded template files.

Files Changed

• pkg/external/customtemplates/s3.go — added path traversal validation in downloadToFile, tightened directory permissions from 0775 to 0750

Security Impact

• CWE-22: Path Traversal via S3 object keys (Zip Slip variant)
• CWE-732: Incorrect Permission Assignment for Critical Resource
• Severity: High
• Attack Vector: Malicious S3 object keys → arbitrary file write on the machine running nuclei
• Impact: Arbitrary file overwrite, potential code execution via overwritten config/script files

Proof

This follows the same validated pattern already used in pkg/installer/template.go for zip extraction path traversal prevention. The fix is a standard defense against Zip Slip-style attacks as recommended by Snyk's research.

Checklist

• PR created against dev branch
• All checks passed (lint, unit/integration/regression tests)
• Minimal, focused change — only S3 download function modified
• Follows existing codebase patterns for path traversal prevention
• Permissions tightened to principle of least privilege

Summary by CodeRabbit

• Bug Fixes
  • Added path traversal protection for downloads to prevent unauthorized file access.
  • Tightened directory creation permissions to improve access control.
  • Improved error handling and control flow around path validation and directory operations.

[neo-by-projectdiscovery-dev]

Neo - PR Security Review

No security issues found

Highlights

• Adds path traversal protection to S3 custom template downloads using filepath.Clean() + strings.HasPrefix() validation
• Hardens directory permissions from 0775 to 0750, preventing other local users from modifying downloaded templates
• Implements sibling directory bypass prevention by appending separator to target path before prefix check

Hardening Notes

• Consider using O_NOFOLLOW flag (via syscall) or lstat before os.Create in downloadToFile (s3.go:104) to prevent symlink-following attacks in multi-user environments where nuclei runs with elevated privileges
• Consider validating bucket name against path traversal in NewS3Providers (s3.go:73) before using it in filepath.Join on line 33, even though bucket name is user-controlled, to prevent accidental misconfigurations

_{Comment @pdneo help for available commands. · Open in Neo}

[coderabbitai]

Walkthrough

Added path traversal protection and stricter directory permissions to S3 download logic in pkg/external/customtemplates/s3.go. File paths are cleaned and validated to stay inside the target directory; directory creation permissions changed from 0775 to 0750.

Changes

Cohort / File(s)	Summary
S3 Download Security pkg/external/customtemplates/s3.go	Imported fmt. In downloadToFile: use filepath.Clean(filepath.Join(...)), enforce path traversal protection via a cleanTarget + prefix check, adjust MkdirAll permissions from 0775 to 0750 for directory keys and parent directories, and refine related error handling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 I tidy paths where wild keys rove,
I clean each join and guard the grove.
Permissions snug, set neat and tight,
I keep your files safe through the night. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix: prevent path traversal in S3 custom template downloads' directly and specifically summarizes the main security fix in the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/external/customtemplates/s3.go`:
- Around line 88-92: The current path traversal check using strings.HasPrefix on
the cleaned paths (file := filepath.Clean(filepath.Join(targetDirectory, key)))
can be bypassed by sibling directories with matching prefixes; update the check
to compare against the cleaned target directory with a trailing path separator
(e.g., base := filepath.Clean(targetDirectory) + string(os.PathSeparator)) and
then verify that strings.HasPrefix(file, base); reference the variables file,
targetDirectory, key and the calls to
filepath.Clean/filepath.Join/strings.HasPrefix when making this change.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a5e9ac65-3ee3-4897-bc03-dac543969719

📥 Commits

Reviewing files that changed from the base of the PR and between 979c867 and 0859d58.

📒 Files selected for processing (1)

• pkg/external/customtemplates/s3.go

[coderabbitai]

🧹 Nitpick comments (1)

pkg/external/customtemplates/s3.go (1)

88-94: Prefer filepath.Rel for containment checks instead of strings.HasPrefix.

Current logic fixes the sibling-prefix bypass, but filepath.Rel is a more robust path-containment check and avoids edge-case path-form handling via string operations.

♻️ Optional refactor

-	file := filepath.Clean(filepath.Join(targetDirectory, key))
-	// Prevent path traversal - ensure the file stays within targetDirectory
-	// Append separator to prevent sibling directory bypass (e.g., /templates-evil matching /templates)
-	cleanTarget := filepath.Clean(targetDirectory) + string(filepath.Separator)
-	if !strings.HasPrefix(file, cleanTarget) {
+	cleanTarget := filepath.Clean(targetDirectory)
+	file := filepath.Clean(filepath.Join(cleanTarget, key))
+	relPath, err := filepath.Rel(cleanTarget, file)
+	if err != nil || relPath == ".." || strings.HasPrefix(relPath, ".."+string(filepath.Separator)) {
 		return fmt.Errorf("path traversal detected in S3 object key: %s", key)
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/external/customtemplates/s3.go` around lines 88 - 94, Replace the current
string-prefix containment check with a filesystem-aware containment check using
filepath.Rel: compute file := filepath.Clean(filepath.Join(targetDirectory,
key)), then call rel, err := filepath.Rel(filepath.Clean(targetDirectory), file)
and ensure err == nil and that rel does not start with ".." (or equal "." is
allowed) to detect path traversal; update the error to return fmt.Errorf("path
traversal detected in S3 object key: %s", key) when rel indicates escaping the
targetDirectory. Use the existing variables file, targetDirectory and key and
remove the strings.HasPrefix / cleanTarget logic.

🤖 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/external/customtemplates/s3.go`:
- Around line 88-94: Replace the current string-prefix containment check with a
filesystem-aware containment check using filepath.Rel: compute file :=
filepath.Clean(filepath.Join(targetDirectory, key)), then call rel, err :=
filepath.Rel(filepath.Clean(targetDirectory), file) and ensure err == nil and
that rel does not start with ".." (or equal "." is allowed) to detect path
traversal; update the error to return fmt.Errorf("path traversal detected in S3
object key: %s", key) when rel indicates escaping the targetDirectory. Use the
existing variables file, targetDirectory and key and remove the
strings.HasPrefix / cleanTarget logic.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 6ba46475-73fa-4604-bb33-0356525842c8

📥 Commits

Reviewing files that changed from the base of the PR and between 0859d58 and 93d8319.

📒 Files selected for processing (1)

• pkg/external/customtemplates/s3.go