diff --git a/pkg/catalog/config/nucleiconfig.go b/pkg/catalog/config/nucleiconfig.go index a0788fc0af..b06aad4657 100644 --- a/pkg/catalog/config/nucleiconfig.go +++ b/pkg/catalog/config/nucleiconfig.go @@ -11,6 +11,7 @@ import ( "sync" "github.com/projectdiscovery/gologger" + filepathutil "github.com/projectdiscovery/nuclei/v3/pkg/utils/filepath" "github.com/projectdiscovery/nuclei/v3/pkg/utils/json" "github.com/projectdiscovery/utils/env" "github.com/projectdiscovery/utils/errkit" @@ -70,11 +71,14 @@ func (c *Config) IsCustomTemplate(templatePath string) bool { } for _, dir := range customDirs { - if strings.HasPrefix(templatePath, dir) { + if dir != "" && filepathutil.IsPathWithinDirectory(templatePath, dir) { return true } } - return !strings.HasPrefix(templatePath, c.TemplatesDirectory) + if c.TemplatesDirectory == "" { + return false + } + return !filepathutil.IsPathWithinDirectory(templatePath, c.TemplatesDirectory) } // WriteVersionCheckData writes version check data to config file diff --git a/pkg/catalog/config/nucleiconfig_test.go b/pkg/catalog/config/nucleiconfig_test.go new file mode 100644 index 0000000000..5b185a838b --- /dev/null +++ b/pkg/catalog/config/nucleiconfig_test.go @@ -0,0 +1,48 @@ +package config + +import ( + "path/filepath" + "testing" +) + +func TestIsCustomTemplateUsesPathBoundaries(t *testing.T) { + templatesDir := filepath.Join(t.TempDir(), "nuclei-templates") + cfg := &Config{} + cfg.SetTemplatesDir(templatesDir) + + tests := []struct { + name string + templatePath string + want bool + }{ + { + name: "official template", + templatePath: filepath.Join(templatesDir, "http", "test.yaml"), + want: false, + }, + { + name: "official template sibling prefix", + templatePath: filepath.Join(templatesDir+"-evil", "test.yaml"), + want: true, + }, + { + name: "custom template", + templatePath: filepath.Join(cfg.CustomGitHubTemplatesDirectory, "owner", "repo", "test.yaml"), + want: true, + }, + { + name: "custom template sibling prefix", + templatePath: filepath.Join(cfg.CustomGitHubTemplatesDirectory+"-evil", "test.yaml"), + want: false, + }, + } + + for _, testCase := range tests { + t.Run(testCase.name, func(t *testing.T) { + got := cfg.IsCustomTemplate(testCase.templatePath) + if got != testCase.want { + t.Fatalf("expected %v, got %v", testCase.want, got) + } + }) + } +} diff --git a/pkg/catalog/config/template.go b/pkg/catalog/config/template.go index bbafefccf5..3c0a43f4dc 100644 --- a/pkg/catalog/config/template.go +++ b/pkg/catalog/config/template.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/projectdiscovery/nuclei/v3/pkg/templates/extensions" + filepathutil "github.com/projectdiscovery/nuclei/v3/pkg/utils/filepath" fileutil "github.com/projectdiscovery/utils/file" stringsutil "github.com/projectdiscovery/utils/strings" ) @@ -85,7 +86,7 @@ func IsTemplateWithRoot(fpath, rootDir string) bool { if rootDir != "" { if filepath.IsAbs(fpath) { rel, err := filepath.Rel(rootDir, fpath) - if err == nil && !strings.HasPrefix(rel, "..") { + if err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { pathToCheck = rel } else { pathToCheck = fpath @@ -184,7 +185,7 @@ func GetNucleiTemplatesIndex() (map[string]string, error) { DefaultConfig.Logger.Verbose().Msgf("failed to walk path=%v err=%v", path, err) return nil } - if d.IsDir() || !IsTemplateWithRoot(path, DefaultConfig.TemplatesDirectory) || stringsutil.ContainsAny(path, ignoreDirs...) { + if d.IsDir() || !IsTemplateWithRoot(path, DefaultConfig.TemplatesDirectory) || filepathutil.IsPathWithinAnyDirectory(path, ignoreDirs...) { return nil } // Normalize path for consistent comparison (handles Windows path issues) diff --git a/pkg/catalog/config/template_test.go b/pkg/catalog/config/template_test.go index f44242cf44..1d8c6f1894 100644 --- a/pkg/catalog/config/template_test.go +++ b/pkg/catalog/config/template_test.go @@ -98,3 +98,38 @@ func TestIsTemplate(t *testing.T) { }) } } + +func TestGetNucleiTemplatesIndexIncludesCustomDirSiblingPrefix(t *testing.T) { + tmpDir := t.TempDir() + cfgDir := t.TempDir() + + oldConfigDir := DefaultConfig.GetConfigDir() + oldTemplatesDir := DefaultConfig.TemplatesDirectory + DefaultConfig.SetConfigDir(cfgDir) + DefaultConfig.SetTemplatesDir(tmpDir) + t.Cleanup(func() { + DefaultConfig.SetConfigDir(oldConfigDir) + DefaultConfig.SetTemplatesDir(oldTemplatesDir) + }) + + customGitHubDir := filepath.Join(tmpDir, "github") + require.NoError(t, os.MkdirAll(customGitHubDir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(customGitHubDir, "custom-template.yaml"), []byte(`id: custom-template +info: + name: Custom Template + author: test + severity: info`), 0644)) + + siblingDir := filepath.Join(tmpDir, "github-evil") + require.NoError(t, os.MkdirAll(siblingDir, 0755)) + require.NoError(t, os.WriteFile(filepath.Join(siblingDir, "sibling-template.yaml"), []byte(`id: sibling-template +info: + name: Sibling Template + author: test + severity: info`), 0644)) + + index, err := GetNucleiTemplatesIndex() + require.NoError(t, err) + require.NotContains(t, index, "custom-template", "custom template directory should be excluded") + require.Contains(t, index, "sibling-template", "custom directory sibling prefix should be indexed") +} diff --git a/pkg/catalog/disk/find.go b/pkg/catalog/disk/find.go index 2aff62b2c3..00e11fc2ee 100644 --- a/pkg/catalog/disk/find.go +++ b/pkg/catalog/disk/find.go @@ -10,6 +10,7 @@ import ( "github.com/logrusorgru/aurora/v4" "github.com/pkg/errors" "github.com/projectdiscovery/nuclei/v3/pkg/catalog/config" + filepathutil "github.com/projectdiscovery/nuclei/v3/pkg/utils/filepath" stringsutil "github.com/projectdiscovery/utils/strings" updateutils "github.com/projectdiscovery/utils/update" urlutil "github.com/projectdiscovery/utils/url" @@ -131,20 +132,27 @@ func (c *DiskCatalog) convertPathToAbsolute(t string) (string, error) { // findGlobPathMatches returns the matched files from a glob path func (c *DiskCatalog) findGlobPathMatches(absPath string, processed map[string]struct{}) ([]string, error) { - // trim templateDir if any - relPath := strings.TrimPrefix(absPath, c.templatesDirectory) - // trim leading slash if any - if c.templatesFS != nil { - // fs.FS always uses forward slashes - relPath = strings.TrimPrefix(relPath, "/") - } else { - relPath = strings.TrimPrefix(relPath, string(os.PathSeparator)) - } - var err error var matches []string if c.templatesFS != nil { + // Compute the path relative to the templates directory using + // canonical containment instead of a lexical TrimPrefix. The previous + // strings.TrimPrefix(absPath, c.templatesDirectory) would, for a + // templatesDirectory like "/foo" and an absPath like + // "/foo-evil/x*.yaml", silently truncate to "-evil/x*.yaml" — a + // sibling-prefix bug equivalent to the one fixed across the rest of + // the codebase. With canonical containment the sibling-prefix case + // no longer aliases into the embedded FS root. + relPath := absPath + if c.templatesDirectory != "" && filepathutil.IsPathWithinDirectory(absPath, c.templatesDirectory) { + if rel, relErr := filepath.Rel(c.templatesDirectory, absPath); relErr == nil && rel != "." { + relPath = rel + } + } + // fs.FS always uses forward slashes. + relPath = filepath.ToSlash(relPath) + relPath = strings.TrimPrefix(relPath, "/") matches, err = fs.Glob(c.templatesFS, relPath) } else { matches, err = filepath.Glob(absPath) diff --git a/pkg/catalog/disk/find_test.go b/pkg/catalog/disk/find_test.go new file mode 100644 index 0000000000..408ac76a8e --- /dev/null +++ b/pkg/catalog/disk/find_test.go @@ -0,0 +1,58 @@ +package disk + +import ( + "testing/fstest" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestFindGlobPathMatchesUsesCanonicalContainment ensures that when an +// absPath shares a lexical prefix with templatesDirectory but is not actually +// inside it (a sibling-prefix path), the embedded-FS lookup does NOT silently +// strip part of the prefix and resolve into the templates root. +func TestFindGlobPathMatchesUsesCanonicalContainment(t *testing.T) { + // Build a small in-memory FS that simulates an embedded templates tree + // and a sibling tree. fs.Glob is rooted at the FS root, so the test + // asserts that with the buggy prefix-trim a sibling-prefix glob would + // match the in-bounds entry; with the fix it does not. + memFS := fstest.MapFS{ + "http/test.yaml": {Data: []byte("legit")}, + "http-evil/test.yaml": {Data: []byte("sibling")}, + } + + templatesDir := filepath.Join(t.TempDir(), "templates") + + cat := NewFSCatalog(memFS, templatesDir) + + // Glob for siblings of templatesDir (the previous TrimPrefix would have + // silently aliased "/.../templates-evil/*.yaml" into "-evil/*.yaml" and + // then collapsed to a glob with no leading separator that ran against + // memFS root — observable as confused matches in production code paths + // that rely on this method). + sibling := templatesDir + "-evil" + matches, err := cat.findGlobPathMatches(filepath.Join(sibling, "*.yaml"), map[string]struct{}{}) + require.NoError(t, err) + // Whatever the glob produces, it must NOT include the in-bounds + // http/test.yaml from inside the templates directory. + for _, m := range matches { + require.NotContains(t, m, "http/test.yaml", + "sibling-prefix glob must not alias into the templates root") + } +} + +func TestFindGlobPathMatchesResolvesContainedPath(t *testing.T) { + memFS := fstest.MapFS{ + "http/test.yaml": {Data: []byte("legit")}, + } + + templatesDir := filepath.Join(t.TempDir(), "templates") + cat := NewFSCatalog(memFS, templatesDir) + + // A glob that lives inside templatesDir resolves correctly via the + // canonical relative path (no surprises from the rewrite). + matches, err := cat.findGlobPathMatches(filepath.Join(templatesDir, "http", "*.yaml"), map[string]struct{}{}) + require.NoError(t, err) + require.Equal(t, []string{"http/test.yaml"}, matches) +} diff --git a/pkg/catalog/disk/path.go b/pkg/catalog/disk/path.go index c840824623..a5b0593235 100644 --- a/pkg/catalog/disk/path.go +++ b/pkg/catalog/disk/path.go @@ -5,12 +5,10 @@ import ( "io/fs" "os" "path/filepath" - "strings" "github.com/pkg/errors" "github.com/projectdiscovery/nuclei/v3/pkg/catalog/config" fileutil "github.com/projectdiscovery/utils/file" - urlutil "github.com/projectdiscovery/utils/url" ) // ResolvePath resolves the path to an absolute one in various ways. @@ -69,51 +67,3 @@ func (c *DiskCatalog) tryResolve(fullPath string) (string, error) { } return "", errNoValidCombination } - -// BackwardsCompatiblePaths returns new paths for all old/legacy template paths -// Note: this is a temporary function and will be removed in the future release -// -// Deprecated: No longer used since the official Nuclei Templates repository -// have restructured this a long time ago. -func BackwardsCompatiblePaths(templateDir string, oldPath string) string { - // TODO: remove this function in the future release - // 1. all http related paths are now moved at path /http - // 2. network related CVES are now moved at path /network/cves - newPathCallback := func(path string) string { - // trim prefix slash if any - path = strings.TrimPrefix(path, "/") - // try to resolve path at /http subdirectory - if fileutil.FileOrFolderExists(filepath.Join(templateDir, "http", path)) { - return filepath.Join(templateDir, "http", path) - // try to resolve path at /network/cves subdirectory - } else if strings.HasPrefix(path, "cves") && fileutil.FileOrFolderExists(filepath.Join(templateDir, "network", "cves", path)) { - return filepath.Join(templateDir, "network", "cves", path) - } - // most likely the path is not found - return filepath.Join(templateDir, path) - } - switch { - case fileutil.FileOrFolderExists(oldPath): - // new path specified skip processing - return oldPath - case filepath.IsAbs(oldPath): - tmp := strings.TrimPrefix(oldPath, templateDir) - if tmp == oldPath { - // user provided absolute path which is not in template directory - // skip processing - return oldPath - } - // trim the template directory from the path - return newPathCallback(tmp) - case strings.Contains(oldPath, urlutil.SchemeSeparator): - // scheme separator is used to identify the path as url - // TBD: add support for url directories ?? - return oldPath - case strings.Contains(oldPath, "*"): - // this is most likely a glob path skip processing - return oldPath - default: - // this is most likely a relative path - return newPathCallback(oldPath) - } -} diff --git a/pkg/external/customtemplates/azure_blob.go b/pkg/external/customtemplates/azure_blob.go index 4dc935a9cb..e33e1b25ac 100644 --- a/pkg/external/customtemplates/azure_blob.go +++ b/pkg/external/customtemplates/azure_blob.go @@ -89,8 +89,16 @@ func (bk *customTemplateAzureBlob) Download(ctx context.Context) { for _, blob := range resp.Segment.BlobItems { // If the blob is a .yaml download the file to the local filesystem if strings.HasSuffix(*blob.Name, ".yaml") { + // Resolve the destination path safely so a blob name carrying + // path-traversal segments cannot escape the configured download + // directory. + outputPath, err := safeJoinWithinDirectory(downloadPath, *blob.Name) + if err != nil { + gologger.Error().Msgf("Skipping unsafe Azure blob name %q: %v", *blob.Name, err) + continue + } // Download the template to the local filesystem at the downloadPath - err := downloadTemplate(bk.azureBlobClient, bk.containerName, *blob.Name, filepath.Join(downloadPath, *blob.Name), ctx) + err = downloadTemplate(bk.azureBlobClient, bk.containerName, *blob.Name, outputPath, ctx) if err != nil { gologger.Error().Msgf("Error downloading template: %v", err) } else { diff --git a/pkg/external/customtemplates/azure_blob_test.go b/pkg/external/customtemplates/azure_blob_test.go new file mode 100644 index 0000000000..fddf49c170 --- /dev/null +++ b/pkg/external/customtemplates/azure_blob_test.go @@ -0,0 +1,57 @@ +package customtemplates + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// The Azure Download path validates blob names through safeJoinWithinDirectory. +// These tests cover that validation directly, mirroring the per-blob iteration +// in customTemplateAzureBlob.Download so we lock in the same allow/deny shape +// the runtime applies to server-controlled blob names. + +func TestAzureSafeJoinRejectsTraversalBlobNames(t *testing.T) { + downloadPath := t.TempDir() + + cases := []struct { + name string + blobName string + }{ + {"parent traversal", "../evil.yaml"}, + {"deep traversal", "a/b/../../../etc/evil.yaml"}, + {"bare dotdot", ".."}, + {"sibling prefix via dotdot", "../" + filepath.Base(downloadPath) + "-evil/template.yaml"}, + {"absolute escape via dotdot prefix", "../../etc/evil.yaml"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := safeJoinWithinDirectory(downloadPath, tc.blobName) + require.Errorf(t, err, "blob name %q must be rejected", tc.blobName) + }) + } +} + +func TestAzureSafeJoinPreservesNestedBlobNames(t *testing.T) { + downloadPath := t.TempDir() + + cases := []struct { + name string + blobName string + want string + }{ + {"flat", "template.yaml", filepath.Join(downloadPath, "template.yaml")}, + {"nested", "dir/sub/template.yaml", filepath.Join(downloadPath, "dir", "sub", "template.yaml")}, + {"sibling-basename in different dirs A", "alpha/template.yaml", filepath.Join(downloadPath, "alpha", "template.yaml")}, + {"sibling-basename in different dirs B", "beta/template.yaml", filepath.Join(downloadPath, "beta", "template.yaml")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := safeJoinWithinDirectory(downloadPath, tc.blobName) + require.NoError(t, err) + require.Equal(t, tc.want, got) + }) + } +} diff --git a/pkg/external/customtemplates/gitlab.go b/pkg/external/customtemplates/gitlab.go index 9a0836e146..ffb44f431f 100644 --- a/pkg/external/customtemplates/gitlab.go +++ b/pkg/external/customtemplates/gitlab.go @@ -13,6 +13,21 @@ import ( gitlab "gitlab.com/gitlab-org/api/client-go" ) +// safeProjectOutputPath joins the GitLab project's path component to the +// custom-templates directory, ensuring the resulting path stays inside +// downloadDir even if the GitLab server returns a malicious project path. +func safeProjectOutputPath(downloadDir, projectPath string) (string, error) { + return safeJoinWithinDirectory(downloadDir, projectPath) +} + +// safeProjectFileOutputPath joins a per-file path to the per-project output +// directory, again ensuring containment. fileRelPath is preferred over the +// API-returned basename so that nested directory structure is preserved and a +// malicious server cannot collapse multiple files onto one another. +func safeProjectFileOutputPath(projectDir, fileRelPath string) (string, error) { + return safeJoinWithinDirectory(projectDir, fileRelPath) +} + var _ Provider = &customTemplateGitLabRepo{} type customTemplateGitLabRepo struct { @@ -73,8 +88,28 @@ func (bk *customTemplateGitLabRepo) Download(_ context.Context) { return } - // Add a subdirectory with the project ID as the subdirectory within the location - projectOutputPath := filepath.Join(location, project.Path) + // Add a subdirectory with the project path as the subdirectory within + // the location. The project path is attacker-controllable on a + // malicious or self-hosted GitLab server, so it must be validated for + // containment before we MkdirAll into it. + // + // Use PathWithNamespace (e.g. "group/sub/repo") rather than the bare + // repo slug Path so that two configured projects sharing a slug in + // different namespaces land in distinct directories and cannot + // silently overwrite each other's templates. PathWithNamespace is + // still server-controlled, so it goes through the same containment + // check as before. + projectKey := project.PathWithNamespace + if projectKey == "" { + // Defensive fallback for older API responses where + // PathWithNamespace might not be populated. + projectKey = project.Path + } + projectOutputPath, err := safeProjectOutputPath(location, projectKey) + if err != nil { + gologger.Error().Msgf("Skipping unsafe GitLab project path %q: %v", projectKey, err) + continue + } // Ensure the subdirectory exists or create it if it doesn't yet exist err = os.MkdirAll(projectOutputPath, 0755) @@ -96,6 +131,16 @@ func (bk *customTemplateGitLabRepo) Download(_ context.Context) { for _, file := range tree { // If the object is not a file or file extension is not .yaml, skip it if file.Type == "blob" && filepath.Ext(file.Path) == ".yaml" { + // Resolve the destination path before reaching out to the + // server so a malicious tree path cannot escape projectOutputPath + // even if the server later returns a basename that points + // elsewhere. + outputPath, err := safeProjectFileOutputPath(projectOutputPath, file.Path) + if err != nil { + gologger.Error().Msgf("Skipping unsafe GitLab tree path %q: %v", file.Path, err) + continue + } + gf := &gitlab.GetFileOptions{ Ref: gitlab.Ptr(project.DefaultBranch), } @@ -112,10 +157,22 @@ func (bk *customTemplateGitLabRepo) Download(_ context.Context) { return } - // Write the downloaded template to the local filesystem at the location with the filename of the blob name - err = os.WriteFile(filepath.Join(projectOutputPath, f.FileName), contents, 0644) + // Make sure the parent directory of the output file exists. + // This preserves nested directory structure inside the project + // (the previous implementation flattened everything by writing + // only the basename, silently clobbering files with identical + // names in different directories). + if err := os.MkdirAll(filepath.Dir(outputPath), 0755); err != nil { + gologger.Error().Msgf("error creating parent directory for GitLab project (%s) file: %s %s", project.Name, file.Path, err) + return + } + + // Write the downloaded template to the local filesystem at + // the precomputed safe output path (preserves directory + // structure and prevents traversal). + err = os.WriteFile(outputPath, contents, 0644) if err != nil { - gologger.Error().Msgf("error writing GitLab project (%s) file: %s %s", project.Name, f.FileName, err) + gologger.Error().Msgf("error writing GitLab project (%s) file: %s %s", project.Name, file.Path, err) return } diff --git a/pkg/external/customtemplates/gitlab_test.go b/pkg/external/customtemplates/gitlab_test.go new file mode 100644 index 0000000000..2c7e77cb94 --- /dev/null +++ b/pkg/external/customtemplates/gitlab_test.go @@ -0,0 +1,71 @@ +package customtemplates + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestSafeProjectOutputPathRejectsTraversal(t *testing.T) { + location := t.TempDir() + + cases := []struct { + name string + projectPath string + }{ + {"parent traversal", "../etc"}, + {"deep traversal", "a/b/../../../etc"}, + {"empty base loop via dotdot", ".."}, + {"absolute escape via dotdot", "../" + filepath.Base(location) + "-evil"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := safeProjectOutputPath(location, tc.projectPath) + require.Error(t, err, "expected %q to be rejected", tc.projectPath) + }) + } +} + +func TestSafeProjectOutputPathAllowsExpectedShape(t *testing.T) { + location := t.TempDir() + projectPath := "owner-foo/repo-bar" + got, err := safeProjectOutputPath(location, projectPath) + require.NoError(t, err) + require.Equal(t, filepath.Join(location, projectPath), got) +} + +func TestSafeProjectFileOutputPathPreservesNestedStructure(t *testing.T) { + projectDir := t.TempDir() + + // Two files in different subdirectories with the same basename must + // resolve to two different output paths so they don't clobber each other + // (regression test for the previous flatten-via-basename behaviour). + pathA := "alpha/template.yaml" + pathB := "beta/template.yaml" + + gotA, err := safeProjectFileOutputPath(projectDir, pathA) + require.NoError(t, err) + gotB, err := safeProjectFileOutputPath(projectDir, pathB) + require.NoError(t, err) + require.NotEqual(t, gotA, gotB, "nested files with identical basenames must not collide") + + require.Equal(t, filepath.Join(projectDir, "alpha", "template.yaml"), gotA) + require.Equal(t, filepath.Join(projectDir, "beta", "template.yaml"), gotB) +} + +func TestSafeProjectFileOutputPathRejectsTraversal(t *testing.T) { + projectDir := t.TempDir() + + cases := []string{ + "../escape.yaml", + "sub/../../escape.yaml", + "sub/sub2/../../../escape.yaml", + } + for _, c := range cases { + t.Run(c, func(t *testing.T) { + _, err := safeProjectFileOutputPath(projectDir, c) + require.Error(t, err) + }) + } +} diff --git a/pkg/external/customtemplates/s3.go b/pkg/external/customtemplates/s3.go index 8eb73c09ca..7dd71ac7cc 100644 --- a/pkg/external/customtemplates/s3.go +++ b/pkg/external/customtemplates/s3.go @@ -14,6 +14,7 @@ import ( "github.com/projectdiscovery/gologger" nucleiConfig "github.com/projectdiscovery/nuclei/v3/pkg/catalog/config" "github.com/projectdiscovery/nuclei/v3/pkg/types" + filepathutil "github.com/projectdiscovery/nuclei/v3/pkg/utils/filepath" "github.com/projectdiscovery/utils/errkit" stringsutil "github.com/projectdiscovery/utils/strings" ) @@ -83,8 +84,11 @@ func NewS3Providers(options *types.Options) ([]*customTemplateS3Bucket, error) { } func downloadToFile(downloader *manager.Downloader, targetDirectory, bucket, key string) error { - // Create the directories in the path - file := filepath.Join(targetDirectory, key) + file, err := safeJoinWithinDirectory(targetDirectory, key) + if err != nil { + return errkit.Wrapf(err, "skipping s3 object %q with unsafe key", key) + } + // If empty dir in s3 if stringsutil.HasSuffixI(key, "/") { return os.MkdirAll(file, 0775) @@ -108,6 +112,21 @@ func downloadToFile(downloader *manager.Downloader, targetDirectory, bucket, key return err } +// safeJoinWithinDirectory joins relPath to baseDir and ensures the resulting +// path stays inside baseDir after canonicalization. It is used to defend +// custom-template downloaders against path-traversal in attacker-controlled +// keys/blob names/file paths. +func safeJoinWithinDirectory(baseDir, relPath string) (string, error) { + if baseDir == "" { + return "", errkit.New("base directory must not be empty") + } + cleaned := filepath.Clean(filepath.Join(baseDir, relPath)) + if !filepathutil.IsPathWithinDirectory(cleaned, baseDir) { + return "", errkit.Newf("relative path %q escapes %q", relPath, baseDir) + } + return cleaned, nil +} + func getS3Client(ctx context.Context, accessKey string, secretKey string, region string, profile string) (*s3.Client, error) { var cfg aws.Config var err error diff --git a/pkg/external/customtemplates/s3_test.go b/pkg/external/customtemplates/s3_test.go new file mode 100644 index 0000000000..6a78da2e78 --- /dev/null +++ b/pkg/external/customtemplates/s3_test.go @@ -0,0 +1,225 @@ +package customtemplates + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestSafeJoinWithinDirectoryRejectsTraversal(t *testing.T) { + baseDir := t.TempDir() + + traversalKeys := []string{ + "../etc/passwd", + "../../etc/passwd", + "foo/../../etc/passwd", + "./../etc/passwd", + "a/b/../../../etc/passwd", + } + + for _, key := range traversalKeys { + t.Run(key, func(t *testing.T) { + _, err := safeJoinWithinDirectory(baseDir, key) + require.Error(t, err, "expected key %q to be rejected", key) + }) + } +} + +func TestSafeJoinWithinDirectoryAllowsContainedPaths(t *testing.T) { + baseDir := t.TempDir() + + tests := map[string]string{ + "file.yaml": filepath.Join(baseDir, "file.yaml"), + "sub/file.yaml": filepath.Join(baseDir, "sub", "file.yaml"), + "a/b/c/file.yaml": filepath.Join(baseDir, "a", "b", "c", "file.yaml"), + "sub/../sibling.yaml": filepath.Join(baseDir, "sibling.yaml"), + "sub/inner/../i.yaml": filepath.Join(baseDir, "sub", "i.yaml"), + "./relative-file.yaml": filepath.Join(baseDir, "relative-file.yaml"), + "trailing/dir/": filepath.Join(baseDir, "trailing", "dir"), + "name-with-dotdot..bar": filepath.Join(baseDir, "name-with-dotdot..bar"), + } + + for key, want := range tests { + t.Run(key, func(t *testing.T) { + got, err := safeJoinWithinDirectory(baseDir, key) + require.NoError(t, err) + require.Equal(t, want, got) + }) + } +} + +func TestSafeJoinWithinDirectoryRejectsSiblingPrefix(t *testing.T) { + parent := t.TempDir() + baseDir := filepath.Join(parent, "templates") + require.NoError(t, os.MkdirAll(baseDir, 0o755)) + + // Sibling directory that shares the templates prefix lexically but is + // not actually inside the templates directory. + sibling := baseDir + "-evil" + require.NoError(t, os.MkdirAll(sibling, 0o755)) + + // An S3 object key that lexically prefix-matches templates would have + // been treated as in-bounds by a HasPrefix check. The canonical + // containment check must reject it. + key := "../" + filepath.Base(sibling) + "/file.yaml" + _, err := safeJoinWithinDirectory(baseDir, key) + require.Error(t, err) +} + +func TestSafeJoinWithinDirectoryEmptyBase(t *testing.T) { + _, err := safeJoinWithinDirectory("", "anything") + require.Error(t, err) +} + +func TestDownloadToFileRejectsTraversalKey(t *testing.T) { + baseDir := t.TempDir() + // Use a unique sibling outside baseDir we want to make sure we never write to. + parent := filepath.Dir(baseDir) + maliciousKey := "../" + filepath.Base(baseDir) + "-evil/poc.yaml" + + // downloader is nil on purpose: with a safe-join failure we must return + // before any download attempt is made (otherwise we'd panic on the nil + // downloader, which itself is a regression assertion). + err := downloadToFile(nil, baseDir, "irrelevant-bucket", maliciousKey) + require.Error(t, err) + + // Belt-and-suspenders: nothing got written under the sibling. + _, statErr := os.Stat(filepath.Join(parent, filepath.Base(baseDir)+"-evil")) + require.True(t, os.IsNotExist(statErr), "no sibling directory should have been created") +} + +func TestSafeJoinWithinDirectoryHandlesLeadingSlash(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("absolute path semantics differ on Windows") + } + baseDir := t.TempDir() + // filepath.Join discards the leading "/" of the relative segment, so a + // "rooted" key like "/etc/passwd" lands at /etc/passwd. That is + // still inside baseDir, so we accept it (no escape happened). + got, err := safeJoinWithinDirectory(baseDir, "/etc/passwd") + require.NoError(t, err) + require.Equal(t, filepath.Join(baseDir, "etc", "passwd"), got) +} + +// TestSafeJoinWithinDirectoryRejectsSymlinkEscape covers the case where +// baseDir already contains a pre-existing symlink that points OUTSIDE of +// baseDir, and an attacker-controlled key tries to ride that symlink to write +// somewhere on the filesystem the operator doesn't control. The canonicalized +// IsPathWithinDirectory check inside safeJoinWithinDirectory must reject this +// even though the lexical Clean of (baseDir + relPath) looks fine. +func TestSafeJoinWithinDirectoryRejectsSymlinkEscape(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliable on all Windows runners") + } + parent := t.TempDir() + baseDir := filepath.Join(parent, "templates") + require.NoError(t, os.MkdirAll(baseDir, 0o755)) + + // A pre-existing escape symlink under baseDir. EvalSymlinks resolves it + // before we Rel against baseDir, so any key that would write through it + // must be rejected. + outside := filepath.Join(parent, "outside") + require.NoError(t, os.MkdirAll(outside, 0o755)) + require.NoError(t, os.Symlink(outside, filepath.Join(baseDir, "escape"))) + + _, err := safeJoinWithinDirectory(baseDir, "escape/poc.yaml") + require.Error(t, err, "must reject keys that ride a symlink out of baseDir") +} + +// TestSafeJoinWithinDirectoryAcceptsInBoundsSymlink is a positive companion +// to the symlink-escape test: a symlink that stays inside baseDir must be +// honoured, otherwise users would lose support for legitimate setups (e.g. +// macOS /tmp -> /private/tmp aliases). +func TestSafeJoinWithinDirectoryAcceptsInBoundsSymlink(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliable on all Windows runners") + } + baseDir := t.TempDir() + inner := filepath.Join(baseDir, "real") + require.NoError(t, os.MkdirAll(inner, 0o755)) + require.NoError(t, os.Symlink(inner, filepath.Join(baseDir, "alias"))) + + got, err := safeJoinWithinDirectory(baseDir, "alias/file.yaml") + require.NoError(t, err) + require.Equal(t, filepath.Join(baseDir, "alias", "file.yaml"), got) +} + +// TestSafeJoinWithinDirectoryAdversarialKeys exhaustively probes +// well-known traversal patterns that historically bypass naive joiners. +// All of these must be rejected on Linux/macOS; the Windows-only patterns +// are also asserted on Windows. +func TestSafeJoinWithinDirectoryAdversarialKeys(t *testing.T) { + parent := t.TempDir() + baseDir := filepath.Join(parent, "templates") + require.NoError(t, os.MkdirAll(baseDir, 0o755)) + siblingName := filepath.Base(baseDir) + "-evil" + require.NoError(t, os.MkdirAll(filepath.Join(parent, siblingName), 0o755)) + + keys := []string{ + "../" + siblingName + "/file.yaml", + "./../" + siblingName + "/file.yaml", + "sub/../../" + siblingName + "/file.yaml", + strings.Repeat("../", 10) + "etc/passwd", + "sub/" + strings.Repeat("../", 10) + "etc/passwd", + // Excessive depth that should still resolve outside. + "a/b/c/d/e/../../../../../../../../etc/passwd", + // Mixed dot-only segments that fold into "../". + "./.././../../" + siblingName, + } + for _, key := range keys { + t.Run(key, func(t *testing.T) { + _, err := safeJoinWithinDirectory(baseDir, key) + require.Error(t, err, "expected key %q to be rejected", key) + }) + } +} + +// FuzzSafeJoinWithinDirectory is a property test for safeJoinWithinDirectory: +// no matter what the attacker-controlled relPath looks like, the helper +// either errors (rejecting) or returns a path whose canonical resolution +// stays inside baseDir. We cannot rely solely on the lexical Clean of the +// returned string here — the canonical resolver follows symlinks, so the +// strongest invariant is "the caller's eventual writes go inside baseDir". +func FuzzSafeJoinWithinDirectory(f *testing.F) { + seeds := []string{ + "", + ".", + "..", + "foo", + "foo/bar", + "../etc", + "foo/../../etc", + "./../etc", + "\\..\\..\\foo", + "a/b/../../../../../../etc/passwd", + strings.Repeat("../", 1000), + strings.Repeat("..\\", 1000), + "\x00..", + "\u202e..\u202d", + } + for _, s := range seeds { + f.Add(s) + } + baseDir := f.TempDir() + + f.Fuzz(func(t *testing.T, relPath string) { + got, err := safeJoinWithinDirectory(baseDir, relPath) + if err != nil { + return + } + // The accepted path must be under baseDir according to filepath.Rel. + // We cleaned both sides to defeat trailing-separator differences. + rel, relErr := filepath.Rel(filepath.Clean(baseDir), filepath.Clean(got)) + if relErr != nil { + t.Fatalf("Rel error: relPath=%q got=%q err=%v", relPath, got, relErr) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + t.Fatalf("safeJoinWithinDirectory accepted escape: relPath=%q got=%q rel=%q", + relPath, got, rel) + } + }) +} diff --git a/pkg/installer/template.go b/pkg/installer/template.go index 8987b305f8..da142c9318 100644 --- a/pkg/installer/template.go +++ b/pkg/installer/template.go @@ -17,6 +17,7 @@ import ( "github.com/projectdiscovery/gologger" "github.com/projectdiscovery/nuclei/v3/pkg/catalog/config" "github.com/projectdiscovery/nuclei/v3/pkg/external/customtemplates" + filepathutil "github.com/projectdiscovery/nuclei/v3/pkg/utils/filepath" "github.com/projectdiscovery/utils/errkit" fileutil "github.com/projectdiscovery/utils/file" mapsutil "github.com/projectdiscovery/utils/maps" @@ -264,7 +265,16 @@ func (t *TemplateManager) getAbsoluteFilePath(templateDir, uri string, f fs.File if index == -1 { // zip files does not have directory at all , in this case log error but continue gologger.Warning().Msgf("failed to get directory name from uri: %s", uri) - return filepath.Join(templateDir, uri) + // Even in this fallback path the entry name comes from a downloaded + // archive, so we must still verify it cannot escape templateDir. + // On Windows in particular, an entry named "..\\foo" has no slash but + // is a parent reference that filepath.Join+Clean will happily resolve + // to outside the configured templates directory. + fallbackPath := filepath.Clean(filepath.Join(templateDir, uri)) + if !filepathutil.IsPathWithinDirectory(fallbackPath, templateDir) { + return "" + } + return fallbackPath } // separator is also included in rootDir rootDirectory := uri[:index+1] @@ -277,12 +287,12 @@ func (t *TemplateManager) getAbsoluteFilePath(templateDir, uri string, f fs.File newPath := filepath.Clean(filepath.Join(templateDir, relPath)) - if !strings.HasPrefix(newPath, templateDir) { + if !filepathutil.IsPathWithinDirectory(newPath, templateDir) || !filepathutil.IsPathWithinDirectory(filepath.Dir(newPath), templateDir) { // we don't allow LFI return "" } - if newPath == templateDir || newPath == templateDir+string(os.PathSeparator) { + if filepath.Clean(newPath) == filepath.Clean(templateDir) { // skip writing the folder itself since it already exists return "" } @@ -468,10 +478,8 @@ func (t *TemplateManager) cleanupOrphanedTemplates(dir string, writtenPaths *map absPath = filepath.Clean(absPath) // Skip custom template directories - for _, customDir := range customDirAbs { - if strings.HasPrefix(absPath, customDir) { - return nil - } + if filepathutil.IsPathWithinAnyDirectory(absPath, customDirAbs...) { + return nil } // Only process template files @@ -617,7 +625,7 @@ func (t *TemplateManager) calculateChecksumMap(dir string) (map[string]string, e return err } // skip checksums of custom templates i.e github and s3 - if stringsutil.HasPrefixAny(path, config.DefaultConfig.GetAllCustomTemplateDirs()...) { + if filepathutil.IsPathWithinAnyDirectory(path, config.DefaultConfig.GetAllCustomTemplateDirs()...) { return nil } diff --git a/pkg/installer/template_test.go b/pkg/installer/template_test.go index 57ca67901a..f5d31763ae 100644 --- a/pkg/installer/template_test.go +++ b/pkg/installer/template_test.go @@ -216,6 +216,49 @@ info: require.FileExists(t, customTemplate, "custom template should be preserved") }) + t.Run("removes orphaned templates from custom dir sibling prefixes", func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "nuclei-cleanup-custom-prefix-test-*") + require.NoError(t, err) + defer func() { + _ = os.RemoveAll(tmpDir) + }() + + cfgdir, err := os.MkdirTemp("", "nuclei-config-*") + require.NoError(t, err) + defer func() { + _ = os.RemoveAll(cfgdir) + }() + + config.DefaultConfig.SetConfigDir(cfgdir) + config.DefaultConfig.SetTemplatesDir(tmpDir) + + customGitHubDir := filepath.Join(tmpDir, "github", "owner", "repo") + require.NoError(t, os.MkdirAll(customGitHubDir, 0755)) + customTemplate := filepath.Join(customGitHubDir, "custom-template.yaml") + require.NoError(t, os.WriteFile(customTemplate, []byte(`id: custom-template +info: + name: Custom Template + author: test + severity: info`), 0644)) + + siblingDir := filepath.Join(tmpDir, "github-evil") + require.NoError(t, os.MkdirAll(siblingDir, 0755)) + siblingTemplate := filepath.Join(siblingDir, "orphaned-template.yaml") + require.NoError(t, os.WriteFile(siblingTemplate, []byte(`id: orphaned-template +info: + name: Orphaned Template + author: test + severity: info`), 0644)) + + writtenPaths := mapsutil.NewSyncLockMap[string, struct{}]() + + err = tm.cleanupOrphanedTemplates(tmpDir, writtenPaths) + require.NoError(t, err) + + require.FileExists(t, customTemplate, "custom template should be preserved") + require.NoFileExists(t, siblingTemplate, "custom directory sibling prefix should not be preserved") + }) + t.Run("skips non-template files", func(t *testing.T) { // Create temporary directories tmpDir, err := os.MkdirTemp("", "nuclei-cleanup-nontemplate-test-*") @@ -330,6 +373,46 @@ info: require.FileExists(t, template1, "template should be preserved when in written paths") }) + t.Run("checksums custom dir sibling prefixes", func(t *testing.T) { + tmpDir, err := os.MkdirTemp("", "nuclei-checksum-custom-prefix-test-*") + require.NoError(t, err) + defer func() { + _ = os.RemoveAll(tmpDir) + }() + + cfgdir, err := os.MkdirTemp("", "nuclei-config-*") + require.NoError(t, err) + defer func() { + _ = os.RemoveAll(cfgdir) + }() + + config.DefaultConfig.SetConfigDir(cfgdir) + config.DefaultConfig.SetTemplatesDir(tmpDir) + + customGitHubDir := filepath.Join(tmpDir, "github") + require.NoError(t, os.MkdirAll(customGitHubDir, 0755)) + customTemplate := filepath.Join(customGitHubDir, "custom-template.yaml") + require.NoError(t, os.WriteFile(customTemplate, []byte(`id: custom-template +info: + name: Custom Template + author: test + severity: info`), 0644)) + + siblingDir := filepath.Join(tmpDir, "github-evil") + require.NoError(t, os.MkdirAll(siblingDir, 0755)) + siblingTemplate := filepath.Join(siblingDir, "sibling-template.yaml") + require.NoError(t, os.WriteFile(siblingTemplate, []byte(`id: sibling-template +info: + name: Sibling Template + author: test + severity: info`), 0644)) + + checksums, err := tm.calculateChecksumMap(tmpDir) + require.NoError(t, err) + require.NotContains(t, checksums, customTemplate, "custom template should be excluded") + require.Contains(t, checksums, siblingTemplate, "custom directory sibling prefix should be checksummed") + }) + t.Run("handles empty templates directory", func(t *testing.T) { // Create temporary directories tmpDir, err := os.MkdirTemp("", "nuclei-cleanup-empty-dir-test-*") diff --git a/pkg/installer/zipslip_unix_test.go b/pkg/installer/zipslip_unix_test.go index f910c5cab8..d234b4b6c4 100644 --- a/pkg/installer/zipslip_unix_test.go +++ b/pkg/installer/zipslip_unix_test.go @@ -58,6 +58,7 @@ func TestZipSlip(t *testing.T) { "nuclei-templates/././../cve/test.yaml", "nuclei-templates/.././../cve/test.yaml", "nuclei-templates/.././../cve/../test.yaml", + "nuclei-templates/../templates-evil/test.yaml", } tm := TemplateManager{} @@ -67,4 +68,68 @@ func TestZipSlip(t *testing.T) { require.Equal(t, "", writePath, filePathFromZip) } }) + + t.Run("positive no-slash fallback", func(t *testing.T) { + // Entry names with no slash exercise the fallback branch in + // getAbsoluteFilePath. Legitimate single-name entries must still be + // written into templateDir (regression test for the containment check + // added to that branch). + tm := TemplateManager{} + var tmp fs.FileInfo = &tempFileInfo{name: "single-file.yaml"} + writePath := tm.getAbsoluteFilePath(configuredTemplateDirectory, "single-file.yaml", tmp) + require.Equal(t, filepath.Join(configuredTemplateDirectory, "single-file.yaml"), writePath) + }) + + t.Run("positive scenarios", func(t *testing.T) { + filePathsFromZip := map[string]string{ + "nuclei-templates/cves/test.yaml": filepath.Join(configuredTemplateDirectory, "cves", "test.yaml"), + "nuclei-templates/test.yaml": filepath.Join(configuredTemplateDirectory, "test.yaml"), + } + tm := TemplateManager{} + + for filePathFromZip, expectedWritePath := range filePathsFromZip { + var tmp fs.FileInfo = &tempFileInfo{name: filePathFromZip} + writePath := tm.getAbsoluteFilePath(configuredTemplateDirectory, filePathFromZip, tmp) + require.Equal(t, expectedWritePath, writePath, filePathFromZip) + } + }) + + t.Run("positive symlinked template directory", func(t *testing.T) { + realDir := t.TempDir() + aliasDir := filepath.Join(t.TempDir(), "templates-link") + require.NoError(t, os.Symlink(realDir, aliasDir)) + + tm := TemplateManager{} + var tmp fs.FileInfo = &tempFileInfo{name: "nuclei-templates/cves/test.yaml"} + writePath := tm.getAbsoluteFilePath(aliasDir, "nuclei-templates/cves/test.yaml", tmp) + require.Equal(t, filepath.Join(aliasDir, "cves", "test.yaml"), writePath) + }) + + // Regression: a malicious archive can target an entry whose intermediate + // path component is itself a symlink that points outside the configured + // templates directory. Because both the entry's leaf and its parent on + // disk may be missing, a naive lexical containment check could miss the + // escape. canonicalizePath inside IsPathWithinDirectory walks up to the + // nearest existing ancestor (the symlink) and resolves it, which is what + // makes this rejection sound. This test pins that behavior. + t.Run("negative symlinked ancestor escapes templateDir", func(t *testing.T) { + templateDir := t.TempDir() + outsideDir := t.TempDir() + // Plant a symlink "evil" inside templateDir that points outside. + // A malicious zip entry that traverses through it must be rejected + // before it ever reaches WriteFile / CreateFolder. + require.NoError(t, os.Symlink(outsideDir, filepath.Join(templateDir, "evil"))) + + tm := TemplateManager{} + entries := []string{ + "nuclei-templates/evil/file.yaml", + "nuclei-templates/evil/nested/file.yaml", + "nuclei-templates/evil", + } + for _, entry := range entries { + var tmp fs.FileInfo = &tempFileInfo{name: entry} + writePath := tm.getAbsoluteFilePath(templateDir, entry, tmp) + require.Equal(t, "", writePath, entry) + } + }) } diff --git a/pkg/protocols/common/protocolstate/state.go b/pkg/protocols/common/protocolstate/state.go index 61232df1a5..6cebdbc41b 100644 --- a/pkg/protocols/common/protocolstate/state.go +++ b/pkg/protocols/common/protocolstate/state.go @@ -55,7 +55,19 @@ func ShouldInit(id string) bool { // Init creates the Dialers instance based on user configuration func Init(options *types.Options) error { - if GetDialersWithId(options.ExecutionId) != nil { + if existingDialers := GetDialersWithId(options.ExecutionId); existingDialers != nil { + // Dialers already exist for this ExecutionId. Refresh the LFA / + // network-policy state derived from options so that a second + // Init call with different options (e.g. flipping + // AllowLocalFileAccess) is reflected in IsLfaAllowed and the + // per-execution dialer state. Without this refresh the second + // caller silently keeps the first caller's settings, which is a + // footgun for tests and SDK callers that share an execution id. + existingDialers.Lock() + existingDialers.LocalFileAccessAllowed = options.AllowLocalFileAccess + existingDialers.RestrictLocalNetworkAccess = options.RestrictLocalNetworkAccess + existingDialers.Unlock() + SetLfaAllowed(options) return nil } diff --git a/pkg/protocols/headless/engine/page_actions.go b/pkg/protocols/headless/engine/page_actions.go index a3126569e7..d9fcc7c451 100644 --- a/pkg/protocols/headless/engine/page_actions.go +++ b/pkg/protocols/headless/engine/page_actions.go @@ -21,6 +21,7 @@ import ( "github.com/projectdiscovery/nuclei/v3/pkg/protocols/common/contextargs" "github.com/projectdiscovery/nuclei/v3/pkg/protocols/common/expressions" "github.com/projectdiscovery/nuclei/v3/pkg/protocols/common/protocolstate" + filepathutil "github.com/projectdiscovery/nuclei/v3/pkg/utils/filepath" contextutil "github.com/projectdiscovery/utils/context" "github.com/projectdiscovery/utils/errkit" fileutil "github.com/projectdiscovery/utils/file" @@ -128,7 +129,12 @@ func (p *Page) ExecuteActions(input *contextargs.Context, actions []*Action) (ou case ActionWaitDialog: err = p.HandleDialog(act, outData) case ActionFilesInput: - if p.options.Options.AllowLocalFileAccess { + // Use the same canonical predicate used by the screenshot action + // rather than reading Options.AllowLocalFileAccess directly so the + // two file-touching actions cannot disagree about whether LFA is + // enabled (e.g. when callers use protocolstate.SetLfaAllowed + // without also flipping the field on Options). + if protocolstate.IsLfaAllowed(p.options.Options) { err = p.FilesInput(act, outData) } else { err = ErrLFAccessDenied @@ -527,17 +533,17 @@ func (p *Page) Screenshot(act *Action, out ActionData) error { return errkit.Newf("could not clean output screenshot path %s", to) } - // allow if targetPath is child of current working directory - if !protocolstate.IsLfaAllowed(p.options.Options) { - cwd, err := os.Getwd() - if err != nil { - return errkit.Wrap(err, "could not get current working directory") - } + // Build the final write path (with .png) BEFORE running any containment + // gate. Otherwise inputs like "." or a bare directory name pass the gate + // against `to` and then the .png suffix moves the actual write outside + // cwd (e.g. .png is a sibling of cwd and not contained by it). + filePath := to + if !strings.HasSuffix(filePath, ".png") { + filePath += ".png" + } - if !strings.HasPrefix(to, cwd) { - // writing outside of cwd requires -lfa flag - return ErrLFAccessDenied - } + if err := p.isScreenshotPathAllowed(filePath); err != nil { + return err } mkdir, err := p.getActionArg(act, "mkdir") @@ -546,20 +552,14 @@ func (p *Page) Screenshot(act *Action, out ActionData) error { } // edgecase create directory if mkdir=true and path contains directory - if mkdir == "true" && stringsutil.ContainsAny(to, folderutil.UnixPathSeparator, folderutil.WindowsPathSeparator) { - // creates new directory if needed based on path `to` + if mkdir == "true" && stringsutil.ContainsAny(filePath, folderutil.UnixPathSeparator, folderutil.WindowsPathSeparator) { + // creates new directory if needed based on the final filePath // TODO: replace all permission bits with fileutil constants (https://github.com/projectdiscovery/utils/issues/113) - if err := os.MkdirAll(filepath.Dir(to), 0700); err != nil { + if err := os.MkdirAll(filepath.Dir(filePath), 0700); err != nil { return errkit.Wrap(err, "failed to create directory while writing screenshot") } } - // actual file path to write - filePath := to - if !strings.HasSuffix(filePath, ".png") { - filePath += ".png" - } - if fileutil.FileExists(filePath) { // return custom error as overwriting files is not supported return errkit.Newf("failed to write screenshot, file %v already exists", filePath) @@ -572,6 +572,45 @@ func (p *Page) Screenshot(act *Action, out ActionData) error { return nil } +func (p *Page) isScreenshotPathAllowed(to string) error { + if protocolstate.IsLfaAllowed(p.options.Options) { + return nil + } + + cwd, err := os.Getwd() + if err != nil { + return errkit.Wrap(err, "could not get current working directory") + } + + if !isScreenshotPathWithinDirectory(to, cwd) { + // writing outside of cwd requires -lfa flag + return ErrLFAccessDenied + } + + return nil +} + +func isScreenshotPathWithinDirectory(to, cwd string) bool { + if !filepathutil.IsPathWithinDirectory(to, cwd) { + return false + } + + existingParent := filepath.Dir(to) + for { + if _, err := os.Stat(existingParent); err == nil { + return filepathutil.IsPathWithinDirectory(existingParent, cwd) + } else if !os.IsNotExist(err) { + return false + } + + parent := filepath.Dir(existingParent) + if parent == existingParent { + return false + } + existingParent = parent + } +} + // InputElement executes input element actions for an element. func (p *Page) InputElement(act *Action, out ActionData) error { value, err := p.getActionArg(act, "value") diff --git a/pkg/protocols/headless/engine/page_actions_test.go b/pkg/protocols/headless/engine/page_actions_test.go index c31226c3f4..1b3287cc9c 100644 --- a/pkg/protocols/headless/engine/page_actions_test.go +++ b/pkg/protocols/headless/engine/page_actions_test.go @@ -11,6 +11,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strconv" "strings" "testing" @@ -248,6 +249,149 @@ func TestActionScreenshotToDir(t *testing.T) { }) } +func TestActionScreenshotDeniesSiblingPrefixPathWithoutLFA(t *testing.T) { + tmpDir := t.TempDir() + cwd := filepath.Join(tmpDir, "work") + sibling := cwd + "-evil" + require.NoError(t, os.MkdirAll(cwd, 0700)) + require.NoError(t, os.MkdirAll(sibling, 0700)) + + originalWd, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(cwd)) + t.Cleanup(func() { + require.NoError(t, os.Chdir(originalWd)) + }) + + filePath := filepath.Join(sibling, "test.png") + opts := &types.Options{ExecutionId: t.Name(), AllowLocalFileAccess: false} + page := &Page{options: &Options{Options: opts}} + err = page.isScreenshotPathAllowed(filePath) + require.ErrorIs(t, err, ErrLFAccessDenied) + + err = page.isScreenshotPathAllowed(filepath.Join(cwd, "test.png")) + require.NoError(t, err) +} + +// TestFilesInputAndScreenshotShareLfaGate verifies that the LFA gate used by +// ActionFilesInput is the same predicate used by Screenshot — i.e. +// protocolstate.IsLfaAllowed — so a runtime LfaAllowed override (no Options +// field flip) is honoured in both code paths. +// +// Regression: ActionFilesInput previously read p.options.Options.AllowLocalFileAccess +// directly, which silently disagreed with Screenshot whenever a caller +// configured LFA via protocolstate.SetLfaAllowed without also editing the +// Options struct. To prove the call sites actually consult the same +// predicate (and not just the predicate in isolation), this test exercises +// page.isScreenshotPathAllowed against a path outside cwd while flipping +// only the runtime override. Before the fix, screenshot dispatch would deny +// the path (correct) but FilesInput would still consult Options.AllowLocalFileAccess +// (incorrect). With the fix, both share IsLfaAllowed and both observe the +// override. +func TestFilesInputAndScreenshotShareLfaGate(t *testing.T) { + executionId := t.Name() + t.Cleanup(func() { + protocolstate.LfaAllowed.Delete(executionId) + }) + + opts := &types.Options{ExecutionId: executionId, AllowLocalFileAccess: false} + page := &Page{options: &Options{Options: opts}} + + // Sanity: with no override, both gates must report deny. + require.False(t, protocolstate.IsLfaAllowed(opts), + "baseline IsLfaAllowed should be false when nothing is configured") + + tmpDir := t.TempDir() + cwd := filepath.Join(tmpDir, "work") + require.NoError(t, os.MkdirAll(cwd, 0700)) + outsideTarget := filepath.Join(tmpDir, "outside", "test.png") + require.NoError(t, os.MkdirAll(filepath.Dir(outsideTarget), 0700)) + originalWd, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(cwd)) + t.Cleanup(func() { require.NoError(t, os.Chdir(originalWd)) }) + + // Without override: Screenshot's gate denies a path outside cwd. + require.ErrorIs(t, page.isScreenshotPathAllowed(outsideTarget), ErrLFAccessDenied, + "baseline: writing outside cwd must be denied without LFA") + + // Configure a runtime override via the LfaAllowed map without touching + // opts.AllowLocalFileAccess. + require.NoError(t, protocolstate.LfaAllowed.Set(executionId, true)) + require.True(t, protocolstate.IsLfaAllowed(opts), + "IsLfaAllowed must honour the LfaAllowed runtime override") + + // With override: Screenshot's gate now allows the same path. The + // FilesInput dispatch in page_actions.go reads from this same predicate, + // so a runtime override unblocks both call sites. + require.NoError(t, page.isScreenshotPathAllowed(outsideTarget), + "runtime override must unblock Screenshot's path gate") +} + +// TestActionScreenshotDeniesPostExtensionEscape locks in the fix for the +// pre-extension containment bypass. Inputs like "." or a bare directory path +// would lexically pass the containment gate against the unmodified `to` +// argument and only ESCAPE cwd after the screenshot writer appends ".png" to +// produce a sibling file (e.g. .png). The gate must run on the final +// filePath, not on the pre-extension input. +func TestActionScreenshotDeniesPostExtensionEscape(t *testing.T) { + tmpDir := t.TempDir() + cwd := filepath.Join(tmpDir, "work") + require.NoError(t, os.MkdirAll(cwd, 0700)) + + originalWd, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(cwd)) + t.Cleanup(func() { require.NoError(t, os.Chdir(originalWd)) }) + + opts := &types.Options{ExecutionId: t.Name(), AllowLocalFileAccess: false} + page := &Page{options: &Options{Options: opts}} + + // "." would resolve to cwd; cwd + ".png" is the SIBLING .png in + // tmpDir, which is outside the cwd sandbox. The gate must reject the + // final write target, not the pre-extension input. + postExtension := cwd + ".png" + require.ErrorIs(t, page.isScreenshotPathAllowed(postExtension), ErrLFAccessDenied, + ".png is a sibling of cwd and must be rejected") + + // Same idea, expressed via a child-of-parent path. + require.ErrorIs(t, + page.isScreenshotPathAllowed(filepath.Join(filepath.Dir(cwd), "evil.png")), + ErrLFAccessDenied, + "a sibling .png in cwd's parent must be rejected") + + // Sanity: a path inside cwd remains allowed. + require.NoError(t, page.isScreenshotPathAllowed(filepath.Join(cwd, "ok.png")), + "a path inside cwd must still be allowed") +} + +func TestActionScreenshotDeniesSymlinkedParentOutsideCWDWithoutLFA(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliable on all Windows runners") + } + + tmpDir := t.TempDir() + cwd := filepath.Join(tmpDir, "work") + outside := filepath.Join(tmpDir, "outside") + require.NoError(t, os.MkdirAll(cwd, 0700)) + require.NoError(t, os.MkdirAll(outside, 0700)) + + linkPath := filepath.Join(cwd, "link") + require.NoError(t, os.Symlink(outside, linkPath)) + + originalWd, err := os.Getwd() + require.NoError(t, err) + require.NoError(t, os.Chdir(cwd)) + t.Cleanup(func() { + require.NoError(t, os.Chdir(originalWd)) + }) + + opts := &types.Options{ExecutionId: t.Name(), AllowLocalFileAccess: false} + page := &Page{options: &Options{Options: opts}} + err = page.isScreenshotPathAllowed(filepath.Join(linkPath, "test.png")) + require.ErrorIs(t, err, ErrLFAccessDenied) +} + func TestActionTimeInput(t *testing.T) { response := ` diff --git a/pkg/reporting/exporters/markdown/markdown.go b/pkg/reporting/exporters/markdown/markdown.go index fb1d4df0f0..e7b08e08de 100644 --- a/pkg/reporting/exporters/markdown/markdown.go +++ b/pkg/reporting/exporters/markdown/markdown.go @@ -145,5 +145,12 @@ func sanitizeFilename(filename string) string { if len(filename) > 256 { filename = filename[0:255] } - return stringsutil.ReplaceAll(filename, "_", "?", "/", ">", "|", ":", ";", "*", "<", "\"", "'", " ") + // Note: "\\" must be replaced together with "/" so an attacker-controlled + // host or template id with Windows-style path separators cannot traverse + // out of the configured directory when this value is later used as a + // subdirectory or filename. ".." is replaced for the same reason — even + // without a separator, a name of "..foo" is harmless but a name of ".." + // alone (or any sequence containing "..") is treated by filepath.Clean as + // a parent reference once joined with the report directory. + return stringsutil.ReplaceAll(filename, "_", "?", "/", "\\", "..", ">", "|", ":", ";", "*", "<", "\"", "'", " ") } diff --git a/pkg/reporting/exporters/markdown/markdown_test.go b/pkg/reporting/exporters/markdown/markdown_test.go new file mode 100644 index 0000000000..d7e7b69458 --- /dev/null +++ b/pkg/reporting/exporters/markdown/markdown_test.go @@ -0,0 +1,183 @@ +package markdown + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestSanitizeFilenameStripsPathSeparatorsAndDotDot ensures user-supplied +// values (event.Host, event.TemplateID) used to build subdirectory and file +// names cannot escape the reporting directory. +// +// The previous sanitizer only replaced "/" but not "\\" and not "..", which +// allowed Windows-style traversal like "..\\..\\foo" to flow through to +// filepath.Join + filepath.Clean and end up outside the configured directory. +func TestSanitizeFilenameStripsPathSeparatorsAndDotDot(t *testing.T) { + cases := []struct { + name string + input string + mustNot []string + mustHave []string + }{ + { + name: "forward slash traversal", + input: "../../etc/passwd", + mustNot: []string{"/", ".."}, + }, + { + name: "backslash traversal", + input: "..\\..\\etc\\passwd", + mustNot: []string{"\\", ".."}, + }, + { + name: "mixed separators", + input: "..\\../etc", + mustNot: []string{"/", "\\", ".."}, + }, + { + name: "bare dotdot", + input: "..", + mustNot: []string{".."}, + }, + { + name: "embedded dotdot stays substringless", + input: "evil..foo", + mustNot: []string{".."}, + mustHave: []string{"evil"}, + }, + { + name: "legitimate hostname is preserved enough", + input: "example.com", + mustHave: []string{"example", "com"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + out := sanitizeFilename(tc.input) + for _, s := range tc.mustNot { + require.False(t, strings.Contains(out, s), + "sanitized %q (in=%q) must not contain %q", out, tc.input, s) + } + for _, s := range tc.mustHave { + require.True(t, strings.Contains(out, s), + "sanitized %q (in=%q) should still contain %q", out, tc.input, s) + } + }) + } +} + +// TestSanitizedSubdirectoryStaysWithinReportDirectory is a stronger assertion +// that the sanitizer's output, when joined into the report directory and +// cleaned by filepath.Clean (the same code path the markdown exporter walks), +// never escapes that directory. This guards against future regressions in the +// sanitizer that might let a separator slip through. +func TestSanitizedSubdirectoryStaysWithinReportDirectory(t *testing.T) { + reportDir := t.TempDir() + hostileInputs := []string{ + "../../etc/passwd", + "..\\..\\etc\\passwd", + "..", + "..\\..\\..\\Windows\\System32", + "some/host/../../escape", + // Many-dot variants that earlier ReplaceAll iterations might leave a + // stray ".." behind (strings.ReplaceAll guarantees no remaining match + // of the pattern, so even adversarial dot runs collapse safely). + "...", + "....", + ".....", + "......", + "./.../...", + "/../../foo", + "\\..\\..\\foo", + // Mixed separators on Linux/Windows. + "a/b\\..\\..\\..\\c", + "a\\b/../../../c", + // Embedded NUL — written here as escape; the sanitizer doesn't + // special-case it, but filepath.Clean handles it as a literal byte. + "foo\x00..\\bar", + // Long traversal sequences that cross MAX_PATH-ish boundaries. + strings.Repeat("../", 200) + "etc/passwd", + strings.Repeat("..\\", 200) + "Windows", + } + + for _, in := range hostileInputs { + t.Run(in, func(t *testing.T) { + subdir := sanitizeFilename(in) + // After sanitize the result must not contain a path separator or + // a parent-reference token at all; otherwise filepath.Clean + + // filepath.Join could collapse it into a traversal. + require.False(t, strings.ContainsAny(subdir, "/\\"), + "sanitizer leaked a path separator: in=%q out=%q", in, subdir) + require.False(t, strings.Contains(subdir, ".."), + "sanitizer leaked '..' substring: in=%q out=%q", in, subdir) + + joined := filepath.Clean(filepath.Join(reportDir, subdir)) + + // Cleaned path must remain a child of reportDir (or equal to it + // when the sanitizer reduced everything to underscores). + rel, err := filepath.Rel(reportDir, joined) + require.NoError(t, err) + require.NotEqual(t, "..", rel) + require.False(t, strings.HasPrefix(rel, ".."+string(filepath.Separator)), + "input %q produced rel %q which escapes reportDir", in, rel) + }) + } +} + +// FuzzSanitizeFilenameStaysContained is a property test: for any input the +// sanitizer can be fed (event.Host, event.TemplateID, etc.), the output joined +// to a fixed report directory must always resolve back inside that directory. +// This is the reporter's invariant, and the fuzz test makes future regressions +// in the sanitizer impossible to land silently. +func FuzzSanitizeFilenameStaysContained(f *testing.F) { + seeds := []string{ + "", + "foo", + "..", + "../..", + "/../etc/passwd", + "\\..\\..\\Windows", + "..\\../mixed", + "foo..bar..baz", + strings.Repeat("..", 100), + strings.Repeat("..\\", 100), + strings.Repeat("../", 100), + strings.Repeat(".", 1000), + strings.Repeat("\\", 1000), + strings.Repeat("/", 1000), + "\x00\x00\x00..", + "foo\x00..\\bar", + "\u202e..\u202d", // RTL/LTR marks around dotdot + } + for _, s := range seeds { + f.Add(s) + } + + reportDir := f.TempDir() + + f.Fuzz(func(t *testing.T, in string) { + out := sanitizeFilename(in) + // Sanitize must never emit a separator or "..". This is the invariant + // the markdown exporter relies on at filepath.Join time. + if strings.ContainsAny(out, "/\\") { + t.Fatalf("sanitizer leaked a separator: in=%q out=%q", in, out) + } + if strings.Contains(out, "..") { + t.Fatalf("sanitizer leaked '..': in=%q out=%q", in, out) + } + + // The joined+cleaned path must stay inside the report dir. + joined := filepath.Clean(filepath.Join(reportDir, out)) + rel, err := filepath.Rel(reportDir, joined) + if err != nil { + t.Fatalf("Rel error for in=%q out=%q: %v", in, out, err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + t.Fatalf("escape from reportDir: in=%q out=%q rel=%q", in, out, rel) + } + }) +} diff --git a/pkg/types/types.go b/pkg/types/types.go index 5ddf542ecc..96afae15ba 100644 --- a/pkg/types/types.go +++ b/pkg/types/types.go @@ -883,22 +883,33 @@ func (o *Options) GetValidAbsPath(helperFilePath, templatePath string) (string, } } - // CleanPath resolves using CWD and cleans the path - helperFilePath, err = fileutil.CleanPath(helperFilePath) + // templatePath must be absolute for the rule-2 sandbox checks below. + cleanedTemplatePath, err := fileutil.CleanPath(templatePath) if err != nil { - return "", errkit.Wrapf(err, "could not clean helper file path %v", helperFilePath) + return "", errkit.Wrapf(err, "could not clean template path %v", templatePath) } - templatePath, err = fileutil.CleanPath(templatePath) + // Resolve relative helper paths against the template's own directory + // rather than the process CWD. fileutil.CleanPath on a relative path + // uses os.Getwd(), which silently turns a helper reference like + // "payloads.txt" into "/payloads.txt"; that disagrees with how + // templates expect helpers to be looked up (relative to the template + // file itself) and makes rule 2 effectively unreachable unless the + // process happens to be running from the template's directory. + cleanedHelperPath := helperFilePath + if !filepath.IsAbs(cleanedHelperPath) { + cleanedHelperPath = filepath.Join(filepath.Dir(cleanedTemplatePath), cleanedHelperPath) + } + cleanedHelperPath, err = fileutil.CleanPath(cleanedHelperPath) if err != nil { - return "", errkit.Wrapf(err, "could not clean template path %v", templatePath) + return "", errkit.Wrapf(err, "could not clean helper file path %v", helperFilePath) } // As per rule 2, if template and helper file exist in same directory or helper file existed in any child dir of template dir // and both of them are present in user home directory, allow it // Review: should we keep this rule ? add extra option to disable this ? - if isHomeDir(helperFilePath) && isHomeDir(templatePath) && strings.HasPrefix(filepath.Dir(helperFilePath), filepath.Dir(templatePath)) { - return helperFilePath, nil + if isHomeDir(cleanedHelperPath) && isHomeDir(cleanedTemplatePath) && filepathutil.IsPathWithinDirectory(cleanedHelperPath, filepath.Dir(cleanedTemplatePath)) { + return cleanedHelperPath, nil } // all other cases are denied @@ -922,5 +933,8 @@ func (options *Options) GetExecutionID() string { // isHomeDir checks if given is home directory func isHomeDir(path string) bool { homeDir := folderutil.HomeDirOrDefault("") - return strings.HasPrefix(path, homeDir) + if homeDir == "" { + return false + } + return filepathutil.IsPathWithinDirectory(path, homeDir) } diff --git a/pkg/types/types_test.go b/pkg/types/types_test.go new file mode 100644 index 0000000000..be60ed575f --- /dev/null +++ b/pkg/types/types_test.go @@ -0,0 +1,232 @@ +package types + +import ( + "os" + "path/filepath" + "runtime" + "testing" + + "github.com/projectdiscovery/nuclei/v3/pkg/catalog/config" +) + +func TestGetValidAbsPathAllowsExpectedHelperPaths(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + templatesDir := filepath.Join(home, "nuclei-templates") + templateDir := filepath.Join(home, "custom-templates") + outsideHomeDir := t.TempDir() + + for _, dir := range []string{templatesDir, templateDir, filepath.Join(templateDir, "payloads")} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + + restoreTemplatesDir(t, templatesDir) + + templatePath := filepath.Join(templateDir, "template.yaml") + if err := os.WriteFile(templatePath, []byte("id: allowed\n"), 0o600); err != nil { + t.Fatal(err) + } + outsideHomeTemplatePath := filepath.Join(outsideHomeDir, "template.yaml") + if err := os.WriteFile(outsideHomeTemplatePath, []byte("id: allowed\n"), 0o600); err != nil { + t.Fatal(err) + } + + testCases := []struct { + name string + helperPath string + template string + }{ + { + name: "configured templates directory", + helperPath: filepath.Join(templatesDir, "payloads.txt"), + template: outsideHomeTemplatePath, + }, + { + name: "same template directory under home", + helperPath: filepath.Join(templateDir, "payloads.txt"), + template: templatePath, + }, + { + name: "child directory under home", + helperPath: filepath.Join(templateDir, "payloads", "payloads.txt"), + template: templatePath, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + if err := os.WriteFile(testCase.helperPath, []byte("dummy\n"), 0o600); err != nil { + t.Fatal(err) + } + + got, err := (&Options{}).GetValidAbsPath(testCase.helperPath, testCase.template) + if err != nil { + t.Fatalf("expected helper path %q to be allowed: %v", testCase.helperPath, err) + } + if got != testCase.helperPath { + t.Fatalf("expected %q, got %q", testCase.helperPath, got) + } + }) + } +} + +func TestGetValidAbsPathRejectsSiblingPrefixDirectory(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + templatesDir := filepath.Join(home, "nuclei-templates") + siblingDir := filepath.Join(home, "nuclei-templates-evil") + if err := os.MkdirAll(templatesDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(siblingDir, 0o755); err != nil { + t.Fatal(err) + } + + restoreTemplatesDir(t, templatesDir) + + helperPath := filepath.Join(siblingDir, "payloads.txt") + if err := os.WriteFile(helperPath, []byte("dummy\n"), 0o600); err != nil { + t.Fatal(err) + } + + _, err := (&Options{}).GetValidAbsPath(helperPath, filepath.Join(templatesDir, "template.yaml")) + if err == nil { + t.Fatalf("expected sibling prefix helper path %q to be denied", helperPath) + } +} + +func TestGetValidAbsPathRejectsTemplateDirSymlinkToOutside(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink creation is not reliable on all Windows runners") + } + + home := t.TempDir() + t.Setenv("HOME", home) + + templatesDir := filepath.Join(home, "nuclei-templates") + outsideDir := filepath.Join(home, "outside") + if err := os.MkdirAll(templatesDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(outsideDir, 0o755); err != nil { + t.Fatal(err) + } + + restoreTemplatesDir(t, templatesDir) + + outsideFile := filepath.Join(outsideDir, "secret.txt") + if err := os.WriteFile(outsideFile, []byte("dummy\n"), 0o600); err != nil { + t.Fatal(err) + } + helperPath := filepath.Join(templatesDir, "linked-secret.txt") + if err := os.Symlink(outsideFile, helperPath); err != nil { + t.Fatal(err) + } + + _, err := (&Options{}).GetValidAbsPath(helperPath, filepath.Join(templatesDir, "template.yaml")) + if err == nil { + t.Fatalf("expected helper symlink %q to outside file %q to be denied", helperPath, outsideFile) + } +} + +// TestGetValidAbsPathResolvesRelativeHelperAgainstTemplateDir locks in the +// rule-2 fix: a relative helper reference (e.g. "payloads.txt") must resolve +// against the template's own directory, not the process working directory. +// Before the fix, fileutil.CleanPath turned a bare "payloads.txt" into +// "/payloads.txt", which made rule 2 effectively unreachable unless +// the process happened to be running from the template's directory. +func TestGetValidAbsPathResolvesRelativeHelperAgainstTemplateDir(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + templatesDir := filepath.Join(home, "nuclei-templates") + templateDir := filepath.Join(home, "custom-templates") + for _, dir := range []string{templatesDir, templateDir} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + + restoreTemplatesDir(t, templatesDir) + + templatePath := filepath.Join(templateDir, "template.yaml") + if err := os.WriteFile(templatePath, []byte("id: rel\n"), 0o600); err != nil { + t.Fatal(err) + } + helperPath := filepath.Join(templateDir, "payloads.txt") + if err := os.WriteFile(helperPath, []byte("dummy\n"), 0o600); err != nil { + t.Fatal(err) + } + + // Drive the test from a CWD that is unrelated to the template directory + // so a CWD-based resolution would not hit the right file. With the fix, + // the relative helper must still resolve under templateDir and pass + // the sandbox checks. + originalWd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + unrelatedCwd := t.TempDir() + if err := os.Chdir(unrelatedCwd); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = os.Chdir(originalWd) + }) + + got, err := (&Options{}).GetValidAbsPath("payloads.txt", templatePath) + if err != nil { + t.Fatalf("expected relative helper to be allowed under templateDir: %v", err) + } + if got != helperPath { + t.Fatalf("expected resolution under templateDir %q, got %q", helperPath, got) + } +} + +// TestGetValidAbsPathRejectsRelativeHelperEscapingTemplateDir ensures the +// new template-relative resolution does not become a traversal vector: +// "../outside.txt" still has to land inside the template's directory under +// home for rule 2 to apply. +func TestGetValidAbsPathRejectsRelativeHelperEscapingTemplateDir(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + templatesDir := filepath.Join(home, "nuclei-templates") + templateDir := filepath.Join(home, "custom-templates") + for _, dir := range []string{templatesDir, templateDir} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + + restoreTemplatesDir(t, templatesDir) + + templatePath := filepath.Join(templateDir, "template.yaml") + if err := os.WriteFile(templatePath, []byte("id: esc\n"), 0o600); err != nil { + t.Fatal(err) + } + // Plant a file ABOVE templateDir (still under home) so a successful + // traversal would otherwise satisfy isHomeDir and rule 2. + outsideHelper := filepath.Join(home, "outside.txt") + if err := os.WriteFile(outsideHelper, []byte("dummy\n"), 0o600); err != nil { + t.Fatal(err) + } + + if _, err := (&Options{}).GetValidAbsPath("../outside.txt", templatePath); err == nil { + t.Fatalf("expected ../outside.txt to escape templateDir to be denied") + } +} + +func restoreTemplatesDir(t *testing.T, templatesDir string) { + t.Helper() + + oldTemplatesDir := config.DefaultConfig.TemplatesDirectory + config.DefaultConfig.SetTemplatesDir(templatesDir) + t.Cleanup(func() { + config.DefaultConfig.SetTemplatesDir(oldTemplatesDir) + }) +} diff --git a/pkg/utils/filepath/filepath.go b/pkg/utils/filepath/filepath.go index 5d3f5ea32d..91622fa5b9 100644 --- a/pkg/utils/filepath/filepath.go +++ b/pkg/utils/filepath/filepath.go @@ -8,7 +8,17 @@ import ( // IsPathWithinDirectory returns true when path resolves inside directory. // Both values are canonicalized to handle symlinks and platform-specific case rules. +// +// As a fail-closed safety net, an empty path or empty directory ALWAYS returns +// false. filepath.Abs("") returns the process working directory, which would +// otherwise turn an unset/missing argument into a silent CWD-relative sandbox +// — a footgun that callers must not rely on. Callers that need to anchor on +// the working directory must pass it explicitly via os.Getwd(). func IsPathWithinDirectory(path string, directory string) bool { + if path == "" || directory == "" { + return false + } + canonicalPath := canonicalizePath(path) canonicalDirectory := canonicalizePath(directory) @@ -19,6 +29,19 @@ func IsPathWithinDirectory(path string, directory string) bool { return relativePath == "." || (relativePath != ".." && !strings.HasPrefix(relativePath, ".."+string(filepath.Separator))) } +// IsPathWithinAnyDirectory returns true when path resolves inside any directory. +func IsPathWithinAnyDirectory(path string, directories ...string) bool { + for _, directory := range directories { + if directory == "" { + continue + } + if IsPathWithinDirectory(path, directory) { + return true + } + } + return false +} + func canonicalizePath(path string) string { canonicalPath, err := filepath.Abs(path) if err != nil { @@ -26,6 +49,8 @@ func canonicalizePath(path string) string { } if resolvedPath, err := filepath.EvalSymlinks(canonicalPath); err == nil { canonicalPath = resolvedPath + } else { + canonicalPath = resolveExistingPathPrefix(canonicalPath) } canonicalPath = filepath.Clean(canonicalPath) if runtime.GOOS == "windows" { @@ -33,3 +58,26 @@ func canonicalizePath(path string) string { } return canonicalPath } + +func resolveExistingPathPrefix(path string) string { + cleaned := filepath.Clean(path) + current := cleaned + var missing []string + + for { + resolved, err := filepath.EvalSymlinks(current) + if err == nil { + for i := len(missing) - 1; i >= 0; i-- { + resolved = filepath.Join(resolved, missing[i]) + } + return resolved + } + + parent := filepath.Dir(current) + if parent == current { + return cleaned + } + missing = append(missing, filepath.Base(current)) + current = parent + } +} diff --git a/pkg/utils/filepath/filepath_test.go b/pkg/utils/filepath/filepath_test.go index e5a17d1c6a..97c435db96 100644 --- a/pkg/utils/filepath/filepath_test.go +++ b/pkg/utils/filepath/filepath_test.go @@ -4,6 +4,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" ) @@ -30,6 +31,154 @@ func TestIsPathWithinDirectory(t *testing.T) { } } +func TestIsPathWithinAnyDirectory(t *testing.T) { + baseDir := t.TempDir() + otherDir := t.TempDir() + childFile := filepath.Join(baseDir, "child.txt") + if err := os.WriteFile(childFile, []byte("ok"), 0o600); err != nil { + t.Fatal(err) + } + + if !IsPathWithinAnyDirectory(childFile, "", otherDir, baseDir) { + t.Fatalf("expected %q to be inside one of the allowed directories", childFile) + } + if IsPathWithinAnyDirectory(filepath.Join(t.TempDir(), "outside.txt"), "", otherDir, baseDir) { + t.Fatal("expected outside path not to be inside allowed directories") + } +} + +// TestIsPathWithinDirectoryRejectsEmptyInputs documents the hard +// fail-closed behaviour of the helper for empty arguments. +// +// filepath.Abs("") returns the process working directory, so a naive +// canonicalization-then-Rel chain would silently treat empty arguments as a +// CWD-relative sandbox: e.g. IsPathWithinDirectory("/etc/passwd", "") +// resolved as "is /etc/passwd inside CWD?" and IsPathWithinDirectory("", +// cwd) resolved as "is CWD inside CWD?" (true). That is a footgun for +// callers that omit an explicit empty check and would let unset config +// values silently widen the sandbox to the working directory. The helper +// must always return false on empty inputs, regardless of CWD. +func TestIsPathWithinDirectoryRejectsEmptyInputs(t *testing.T) { + cwd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + + // Empty directory must be rejected even when path equals or is below CWD. + if IsPathWithinDirectory(cwd, "") { + t.Fatal("empty directory must never be treated as CWD") + } + if IsPathWithinDirectory(filepath.Join(cwd, "anywhere"), "") { + t.Fatal("empty directory must reject CWD-relative paths") + } + + // Empty path must be rejected even when directory is CWD. + if IsPathWithinDirectory("", cwd) { + t.Fatal("empty path must never be treated as CWD") + } + if IsPathWithinDirectory("", "/some/dir") { + t.Fatal("empty path must reject any directory") + } + + // Both empty: trivially false. + if IsPathWithinDirectory("", "") { + t.Fatal("empty path and directory must be rejected") + } +} + +// TestIsPathWithinDirectoryRejectsRelativeInputsViaCWD covers the related +// footgun where a "." path or a bare-relative path resolves to CWD via +// filepath.Abs. The helper still canonicalizes them, so the assertion is +// "Rel from a real anchor directory rejects them" — matching the intent +// that callers always pass an explicit, non-empty anchor. +func TestIsPathWithinDirectoryRejectsRelativeInputsViaCWD(t *testing.T) { + otherDir := t.TempDir() + + // A bare "." or "./foo" canonicalizes to CWD or CWD/foo. Unless CWD is + // otherDir (it isn't — t.TempDir produces a fresh path), Rel resolves + // to a parent traversal and the helper rejects. + if IsPathWithinDirectory(".", otherDir) { + t.Fatal("CWD-relative \".\" must not satisfy a different anchor dir") + } + if IsPathWithinDirectory("./inner", otherDir) { + t.Fatal("CWD-relative \"./inner\" must not satisfy a different anchor dir") + } +} + +// FuzzIsPathWithinDirectory is a property test asserting the contract: +// when IsPathWithinDirectory returns true, lexically computing +// filepath.Rel between the cleaned-and-canonicalized arguments must not +// produce a parent traversal. We canonicalize via the same helper used +// internally; if a fuzz seed ever finds a discrepancy that's a real +// containment bypass. +func FuzzIsPathWithinDirectory(f *testing.F) { + f.Add("foo", "/tmp") + f.Add("../foo", "/tmp") + f.Add("/etc/passwd", "/tmp") + f.Add("foo/../bar", "/tmp") + f.Add("..", "/tmp") + f.Add("\x00..", "/tmp") + f.Add("a/b/../../c", "/tmp/d") + f.Add(strings.Repeat("../", 50), "/tmp") + + f.Fuzz(func(t *testing.T, path, directory string) { + if !IsPathWithinDirectory(path, directory) { + return + } + // Recompute the canonical relation independently and assert no + // escape. We use the same canonicalizePath path to defeat + // platform-dependent symlink resolution. + canonPath := canonicalizePath(path) + canonDir := canonicalizePath(directory) + rel, err := filepath.Rel(canonDir, canonPath) + if err != nil { + t.Fatalf("Rel error: path=%q dir=%q canonPath=%q canonDir=%q", + path, directory, canonPath, canonDir) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + t.Fatalf("IsPathWithinDirectory accepted escape: path=%q dir=%q rel=%q", + path, directory, rel) + } + }) +} + +// TestIsPathWithinDirectoryRejectsSiblingPrefix locks in the invariant +// this whole helper exists to enforce: a sibling directory whose path +// happens to share a textual prefix with the trusted directory (e.g. +// "/trusted-dir" vs "/trusted-dir-evil") must never satisfy containment. +// The original lexical strings.HasPrefix checks scattered across the +// codebase failed exactly this case; documenting it at the source — not +// just at every call site — is what callers should rely on. +func TestIsPathWithinDirectoryRejectsSiblingPrefix(t *testing.T) { + baseDir := t.TempDir() + siblingDir := baseDir + "-evil" + if err := os.MkdirAll(siblingDir, 0o755); err != nil { + t.Fatal(err) + } + siblingFile := filepath.Join(siblingDir, "payload.txt") + if err := os.WriteFile(siblingFile, []byte("not yours"), 0o600); err != nil { + t.Fatal(err) + } + + if IsPathWithinDirectory(siblingFile, baseDir) { + t.Fatalf("sibling-prefix path %q must NOT be reported inside %q", + siblingFile, baseDir) + } + if IsPathWithinDirectory(siblingDir, baseDir) { + t.Fatalf("sibling-prefix dir %q must NOT be reported inside %q", + siblingDir, baseDir) + } + + // Non-existent sibling-prefix path: same answer. Canonicalization must + // still reject because the existing prefix it walks up to is the + // sibling directory itself, not a child of baseDir. + missingInSibling := filepath.Join(siblingDir, "does", "not", "exist.txt") + if IsPathWithinDirectory(missingInSibling, baseDir) { + t.Fatalf("non-existent sibling-prefix %q must NOT be reported inside %q", + missingInSibling, baseDir) + } +} + func TestIsPathWithinDirectoryWithSymlinkedDirectory(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("symlink creation is not reliable on all Windows runners") @@ -52,4 +201,9 @@ func TestIsPathWithinDirectoryWithSymlinkedDirectory(t *testing.T) { if !IsPathWithinDirectory(childFile, aliasDir) { t.Fatalf("expected %q to be inside symlinked dir %q", childFile, aliasDir) } + + missingChildFile := filepath.Join(aliasDir, "helpers", "missing.js") + if !IsPathWithinDirectory(missingChildFile, realDir) { + t.Fatalf("expected non-existent child %q to be inside real dir %q", missingChildFile, realDir) + } } diff --git a/pkg/utils/template_path.go b/pkg/utils/template_path.go index 6570d90f20..174d67f14f 100644 --- a/pkg/utils/template_path.go +++ b/pkg/utils/template_path.go @@ -1,10 +1,11 @@ package utils import ( - "strings" + "path/filepath" "github.com/projectdiscovery/nuclei/v3/pkg/catalog/config" "github.com/projectdiscovery/nuclei/v3/pkg/keys" + filepathutil "github.com/projectdiscovery/nuclei/v3/pkg/utils/filepath" ) const ( @@ -15,8 +16,11 @@ const ( // TemplatePathURL returns the Path and URL for the provided template func TemplatePathURL(fullPath, templateId, templateVerifier string) (path string, url string) { configData := config.DefaultConfig - if configData.TemplatesDirectory != "" && strings.HasPrefix(fullPath, configData.TemplatesDirectory) { - path = strings.TrimPrefix(strings.TrimPrefix(fullPath, configData.TemplatesDirectory), "/") + if configData.TemplatesDirectory != "" && filepathutil.IsPathWithinDirectory(fullPath, configData.GetTemplateDir()) { + relPath, err := filepath.Rel(configData.GetTemplateDir(), fullPath) + if err == nil && relPath != "." { + path = relPath + } } if templateVerifier == keys.PDVerifier { url = TemplatesRepoURL + templateId diff --git a/pkg/utils/template_path_test.go b/pkg/utils/template_path_test.go new file mode 100644 index 0000000000..942741853c --- /dev/null +++ b/pkg/utils/template_path_test.go @@ -0,0 +1,28 @@ +package utils + +import ( + "path/filepath" + "testing" + + "github.com/projectdiscovery/nuclei/v3/pkg/catalog/config" +) + +func TestTemplatePathURLUsesTemplateDirBoundaries(t *testing.T) { + templatesDir := filepath.Join(t.TempDir(), "nuclei-templates") + + oldTemplatesDir := config.DefaultConfig.TemplatesDirectory + config.DefaultConfig.SetTemplatesDir(templatesDir) + t.Cleanup(func() { + config.DefaultConfig.SetTemplatesDir(oldTemplatesDir) + }) + + path, _ := TemplatePathURL(filepath.Join(templatesDir, "http", "test.yaml"), "test", "") + if path != filepath.Join("http", "test.yaml") { + t.Fatalf("expected relative template path, got %q", path) + } + + path, _ = TemplatePathURL(filepath.Join(templatesDir+"-evil", "test.yaml"), "test", "") + if path != "" { + t.Fatalf("expected sibling prefix path not to be relativized, got %q", path) + } +}