diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index b8c0d924eb98..b645a558be27 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -16,6 +16,8 @@ import ( "strings" "time" + "github.com/argoproj/argo-cd/v2/util/io/files" + "github.com/Masterminds/semver" "github.com/TomOnTime/utfutil" "github.com/argoproj/gitops-engine/pkg/utils/kube" @@ -726,7 +728,8 @@ func GenerateManifests(appPath, repoRoot, revision string, q *apiclient.Manifest if directory = q.ApplicationSource.Directory; directory == nil { directory = &v1alpha1.ApplicationSourceDirectory{} } - targetObjs, err = findManifests(appPath, repoRoot, env, *directory) + logCtx := log.WithField("application", q.AppName) + targetObjs, err = findManifests(logCtx, appPath, repoRoot, env, *directory) } if err != nil { return nil, err @@ -924,12 +927,32 @@ func ksShow(appLabelKey, appPath string, ksonnetOpts *v1alpha1.ApplicationSource var manifestFile = regexp.MustCompile(`^.*\.(yaml|yml|json|jsonnet)$`) // findManifests looks at all yaml files in a directory and unmarshals them into a list of unstructured objects -func findManifests(appPath string, repoRoot string, env *v1alpha1.Env, directory v1alpha1.ApplicationSourceDirectory) ([]*unstructured.Unstructured, error) { +func findManifests(logCtx *log.Entry, appPath string, repoRoot string, env *v1alpha1.Env, directory v1alpha1.ApplicationSourceDirectory) ([]*unstructured.Unstructured, error) { var objs []*unstructured.Unstructured err := filepath.Walk(appPath, func(path string, f os.FileInfo, err error) error { if err != nil { return err } + relPath, err := filepath.Rel(appPath, path) + if err != nil { + return fmt.Errorf("failed to get relative path of symlink: %w", err) + } + if files.IsSymlink(f) { + realPath, err := filepath.EvalSymlinks(path) + if err != nil { + logCtx.Debugf("error checking symlink realpath: %s", err) + if os.IsNotExist(err) { + log.Warnf("ignoring out-of-bounds symlink at %q: %s", relPath, err) + return nil + } else { + return fmt.Errorf("failed to evaluate symlink at %q: %w", relPath, err) + } + } + if !files.Inbound(realPath, appPath) { + logCtx.Warnf("illegal filepath in symlink: %s", realPath) + return fmt.Errorf("illegal filepath in symlink at %q", relPath) + } + } if f.IsDir() { if path != appPath && !directory.Recurse { return filepath.SkipDir @@ -942,10 +965,6 @@ func findManifests(appPath string, repoRoot string, env *v1alpha1.Env, directory return nil } - relPath, err := filepath.Rel(appPath, path) - if err != nil { - return err - } if directory.Exclude != "" && glob.Match(directory.Exclude, relPath) { return nil } diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index a577bfb93869..95d5d3a164aa 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -8,12 +8,15 @@ import ( "io/ioutil" "os" "os/exec" + "path" "path/filepath" "regexp" "strings" "testing" "time" + log "github.com/sirupsen/logrus" + "github.com/ghodss/yaml" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -27,6 +30,7 @@ import ( "github.com/argoproj/argo-cd/v2/reposerver/cache" "github.com/argoproj/argo-cd/v2/reposerver/metrics" fileutil "github.com/argoproj/argo-cd/v2/test/fixture/path" + "github.com/argoproj/argo-cd/v2/util/argo" cacheutil "github.com/argoproj/argo-cd/v2/util/cache" "github.com/argoproj/argo-cd/v2/util/git" gitmocks "github.com/argoproj/argo-cd/v2/util/git/mocks" @@ -50,7 +54,7 @@ func newServiceWithMocks(root string, signed bool) (*Service, *gitmocks.Client) return newServiceWithOpt(func(gitClient *gitmocks.Client) { gitClient.On("Init").Return(nil) gitClient.On("Fetch", mock.Anything).Return(nil) - gitClient.On("Checkout", mock.Anything).Return(nil) + gitClient.On("Checkout", mock.Anything, mock.Anything).Return(nil) gitClient.On("LsRemote", mock.Anything).Return(mock.Anything, nil) gitClient.On("CommitSHA").Return(mock.Anything, nil) gitClient.On("Root").Return(root) @@ -79,7 +83,6 @@ func newServiceWithOpt(cf clientFunc) (*Service, *gitmocks.Client) { }}, nil) helmClient.On("ExtractChart", chart, version).Return("./testdata/my-chart", io.NopCloser, nil) helmClient.On("CleanChartCache", chart, version).Return(nil) - service.newGitClient = func(rawRepoURL string, creds git.Creds, insecure bool, enableLfs bool, prosy string, opts ...git.ClientOpts) (client git.Client, e error) { return gitClient, nil } @@ -110,7 +113,7 @@ func newServiceWithCommitSHA(root, revision string) *Service { service, gitClient := newServiceWithOpt(func(gitClient *gitmocks.Client) { gitClient.On("Init").Return(nil) gitClient.On("Fetch", mock.Anything).Return(nil) - gitClient.On("Checkout", mock.Anything).Return(nil) + gitClient.On("Checkout", mock.Anything, mock.Anything).Return(nil) gitClient.On("LsRemote", revision).Return(revision, revisionErr) gitClient.On("CommitSHA").Return("632039659e542ed7de0c170a4fcc1c571b288fc0", nil) gitClient.On("Root").Return(root) @@ -130,7 +133,7 @@ func TestGenerateYamlManifestInDir(t *testing.T) { q := apiclient.ManifestRequest{Repo: &argoappv1.Repository{}, ApplicationSource: &src} // update this value if we add/remove manifests - const countOfManifests = 34 + const countOfManifests = 41 res1, err := service.GenerateManifest(context.Background(), &q) @@ -143,6 +146,76 @@ func TestGenerateYamlManifestInDir(t *testing.T) { assert.Equal(t, 3, len(res2.Manifests)) } +func Test_GenerateManifests_NoOutOfBoundsAccess(t *testing.T) { + testCases := []struct { + name string + outOfBoundsFilename string + outOfBoundsFileContents string + mustNotContain string // Optional string that must not appear in error or manifest output. If empty, use outOfBoundsFileContents. + }{ + { + name: "out of bounds JSON file should not appear in error output", + outOfBoundsFilename: "test.json", + outOfBoundsFileContents: `{"some": "json"}`, + }, + { + name: "malformed JSON file contents should not appear in error output", + outOfBoundsFilename: "test.json", + outOfBoundsFileContents: "$", + }, + { + name: "out of bounds JSON manifest should not appear in manifest output", + outOfBoundsFilename: "test.json", + // JSON marshalling is deterministic. So if there's a leak, exactly this should appear in the manifests. + outOfBoundsFileContents: `{"apiVersion":"v1","kind":"Secret","metadata":{"name":"test","namespace":"default"},"type":"Opaque"}`, + }, + { + name: "out of bounds YAML manifest should not appear in manifest output", + outOfBoundsFilename: "test.yaml", + outOfBoundsFileContents: "apiVersion: v1\nkind: Secret\nmetadata:\n name: test\n namespace: default\ntype: Opaque", + mustNotContain: `{"apiVersion":"v1","kind":"Secret","metadata":{"name":"test","namespace":"default"},"type":"Opaque"}`, + }, + } + + for _, testCase := range testCases { + testCaseCopy := testCase + t.Run(testCaseCopy.name, func(t *testing.T) { + t.Parallel() + + outOfBoundsDir := t.TempDir() + outOfBoundsFile := path.Join(outOfBoundsDir, testCaseCopy.outOfBoundsFilename) + err := os.WriteFile(outOfBoundsFile, []byte(testCaseCopy.outOfBoundsFileContents), os.FileMode(0444)) + require.NoError(t, err) + + repoDir := t.TempDir() + err = os.Symlink(outOfBoundsFile, path.Join(repoDir, testCaseCopy.outOfBoundsFilename)) + require.NoError(t, err) + + var mustNotContain = testCaseCopy.outOfBoundsFileContents + if testCaseCopy.mustNotContain != "" { + mustNotContain = testCaseCopy.mustNotContain + } + + q := apiclient.ManifestRequest{Repo: &argoappv1.Repository{}, ApplicationSource: &argoappv1.ApplicationSource{}} + res, err := GenerateManifests(repoDir, "", "", &q, false) + require.Error(t, err) + assert.NotContains(t, err.Error(), mustNotContain) + assert.Contains(t, err.Error(), "illegal filepath") + assert.Nil(t, res) + }) + } +} + +func TestGenerateManifests_MissingSymlinkDestination(t *testing.T) { + repoDir := t.TempDir() + err := os.Symlink("/obviously/does/not/exist", path.Join(repoDir, "test.yaml")) + require.NoError(t, err) + + q := apiclient.ManifestRequest{Repo: &argoappv1.Repository{}, ApplicationSource: &argoappv1.ApplicationSource{}} + _, err = GenerateManifests(repoDir, "", "", &q, false) + require.NoError(t, err) +} + func TestGenerateManifests_K8SAPIResetCache(t *testing.T) { service := newService("../..") @@ -1610,7 +1683,7 @@ func TestFindResources(t *testing.T) { for i := range testCases { tc := testCases[i] t.Run(tc.name, func(t *testing.T) { - objs, err := findManifests("testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ + objs, err := findManifests(&log.Entry{}, "testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ Recurse: true, Include: tc.include, Exclude: tc.exclude, @@ -1628,7 +1701,7 @@ func TestFindResources(t *testing.T) { } func TestFindManifests_Exclude(t *testing.T) { - objs, err := findManifests("testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ + objs, err := findManifests(&log.Entry{}, "testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ Recurse: true, Exclude: "subdir/deploymentSub.yaml", }) @@ -1641,7 +1714,7 @@ func TestFindManifests_Exclude(t *testing.T) { } func TestFindManifests_Exclude_NothingMatches(t *testing.T) { - objs, err := findManifests("testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ + objs, err := findManifests(&log.Entry{}, "testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ Recurse: true, Exclude: "nothing.yaml", }) diff --git a/util/io/files/util.go b/util/io/files/util.go new file mode 100644 index 000000000000..7bb2cd13d960 --- /dev/null +++ b/util/io/files/util.go @@ -0,0 +1,35 @@ +package files + +import ( + "io/fs" + "os" + "path/filepath" + "strings" +) + +// Inbound will validate if the given candidate path is inside the +// baseDir. This is useful to make sure that malicious candidates +// are not targeting a file outside of baseDir boundaries. +// Considerations: +// - baseDir must be absolute path. Will return false otherwise +// - candidate can be absolute or relative path +// - candidate should not be symlink as only syntatic validation is +// applied by this function +func Inbound(candidate, baseDir string) bool { + if !filepath.IsAbs(baseDir) { + return false + } + var target string + if filepath.IsAbs(candidate) { + target = filepath.Clean(candidate) + } else { + target = filepath.Join(baseDir, candidate) + } + return strings.HasPrefix(target, filepath.Clean(baseDir)+string(os.PathSeparator)) +} + +// IsSymlink return true if the given FileInfo relates to a +// symlink file. Returns false otherwise. +func IsSymlink(fi os.FileInfo) bool { + return fi.Mode()&fs.ModeSymlink == fs.ModeSymlink +} diff --git a/util/io/files/util_test.go b/util/io/files/util_test.go new file mode 100644 index 000000000000..f7a34f9ec234 --- /dev/null +++ b/util/io/files/util_test.go @@ -0,0 +1,63 @@ +package files_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/argoproj/argo-cd/v2/util/io/files" +) + +func TestInbound(t *testing.T) { + type testcase struct { + name string + candidate string + basedir string + expected bool + } + cases := []testcase{ + { + name: "will return true if candidate is inbound", + candidate: "/home/test/app/readme.md", + basedir: "/home/test", + expected: true, + }, + { + name: "will return false if candidate is not inbound", + candidate: "/home/test/../readme.md", + basedir: "/home/test", + expected: false, + }, + { + name: "will return true if candidate is relative inbound", + candidate: "./readme.md", + basedir: "/home/test", + expected: true, + }, + { + name: "will return false if candidate is relative outbound", + candidate: "../readme.md", + basedir: "/home/test", + expected: false, + }, + { + name: "will return false if basedir is relative", + candidate: "/home/test/app/readme.md", + basedir: "./test", + expected: false, + }, + } + for _, c := range cases { + c := c + t.Run(c.name, func(t *testing.T) { + // given + t.Parallel() + + // when + inbound := files.Inbound(c.candidate, c.basedir) + + // then + assert.Equal(t, c.expected, inbound) + }) + } +} \ No newline at end of file