Skip to content

Commit

Permalink
fix: Path traversal (argoproj#69)
Browse files Browse the repository at this point in the history
Updated the repository to include patches for the
path traversal vulnerabilities.

Contributes to: automation-saas/native-AWS#2081
Contributes to: automation-saas/native-AWS#2101

Signed-off-by: Sujeily Fonseca <sujeily.fonseca@ibm.com>
  • Loading branch information
sujeilyfonseca authored and GitHub Enterprise committed Jul 11, 2022
1 parent c80bd09 commit 8562adf
Show file tree
Hide file tree
Showing 23 changed files with 51 additions and 49 deletions.
2 changes: 1 addition & 1 deletion applicationset/services/repo_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (a *argoCDService) GetFiles(ctx context.Context, repoURL string, revision s

res := map[string][]byte{}
for _, filePath := range paths {
bytes, err := os.ReadFile(filepath.Join(gitRepoClient.Root(), filePath))
bytes, err := os.ReadFile(filepath.Clean(filepath.Join(gitRepoClient.Root(), filePath)))
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions cmpserver/plugin/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ func (s *Service) matchRepository(ctx context.Context, workdir string, envEntrie
config := s.initConstants.PluginConfig
if config.Spec.Discover.FileName != "" {
log.Debugf("config.Spec.Discover.FileName is provided")
pattern := filepath.Join(workdir, config.Spec.Discover.FileName)
pattern := filepath.Clean(filepath.Join(workdir, config.Spec.Discover.FileName))
matches, err := filepath.Glob(pattern)
if err != nil {
e := fmt.Errorf("error finding filename match for pattern %q: %s", pattern, err)
Expand All @@ -267,7 +267,7 @@ func (s *Service) matchRepository(ctx context.Context, workdir string, envEntrie

if config.Spec.Discover.Find.Glob != "" {
log.Debugf("config.Spec.Discover.Find.Glob is provided")
pattern := filepath.Join(workdir, config.Spec.Discover.Find.Glob)
pattern := filepath.Clean(filepath.Join(workdir, config.Spec.Discover.Find.Glob))
// filepath.Glob doesn't have '**' support hence selecting third-party lib
// https://github.com/golang/go/issues/11862
matches, err := zglob.Glob(pattern)
Expand Down
4 changes: 2 additions & 2 deletions common/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -291,9 +291,9 @@ func GetCMPChunkSize() int {
// This directory and all it's contents will be deleted durring CMP bootstrap.
func GetCMPWorkDir() string {
if workDir := os.Getenv(EnvCMPWorkDir); workDir != "" {
return filepath.Join(workDir, DefaultCMPWorkDirName)
return filepath.Clean(filepath.Join(workDir, DefaultCMPWorkDirName))
}
return filepath.Join(os.TempDir(), DefaultCMPWorkDirName)
return filepath.Clean(filepath.Join(os.TempDir(), DefaultCMPWorkDirName))
}

const (
Expand Down
2 changes: 1 addition & 1 deletion hack/dev-mounter/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func newCommand() *cobra.Command {
}
// Create or update files that are specified in ConfigMap
for name, data := range cm.Data {
p := path.Join(destPath, name)
p := filepath.Clean(path.Join(destPath, name))
err := ioutil.WriteFile(p, []byte(data), 0600)
if err != nil {
log.Warnf("Failed to create file %s: %v", p, err)
Expand Down
10 changes: 5 additions & 5 deletions reposerver/repository/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ func repoExists(repo string, repos []*v1alpha1.Repository) bool {
}

func isConcurrencyAllowed(appPath string) bool {
if _, err := os.Stat(path.Join(appPath, allowConcurrencyFile)); err == nil {
if _, err := os.Stat(filepath.Clean(path.Join(appPath, allowConcurrencyFile))); err == nil {
return true
}
return false
Expand All @@ -682,7 +682,7 @@ func runHelmBuild(appPath string, h helm.Helm) error {
// the `helm dependency build` is potentially time consuming 1~2 seconds
// marker file is used to check if command already run to avoid running it again unnecessary
// file is removed when repository re-initialized (e.g. when another commit is processed)
markerFile := path.Join(appPath, helmDepUpMarkerFile)
markerFile := filepath.Clean(path.Join(appPath, helmDepUpMarkerFile))
_, err := os.Stat(markerFile)
if err == nil {
return nil
Expand Down Expand Up @@ -1002,10 +1002,10 @@ func newEnv(q *apiclient.ManifestRequest, revision string) *v1alpha1.Env {
// be read and merged. If appName is not the empty string, and a file named
// .argocd-source-<appName>.yaml exists, it will also be read and merged.
func mergeSourceParameters(source *v1alpha1.ApplicationSource, path, appName string) error {
repoFilePath := filepath.Join(path, repoSourceFile)
repoFilePath := filepath.Clean(filepath.Join(path, repoSourceFile))
overrides := []string{repoFilePath}
if appName != "" {
overrides = append(overrides, filepath.Join(path, fmt.Sprintf(appSourceFile, appName)))
overrides = append(overrides, filepath.Clean(filepath.Join(path, fmt.Sprintf(appSourceFile, appName))))
}

var merged = *source.DeepCopy()
Expand Down Expand Up @@ -1614,7 +1614,7 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin
return err
}
} else {
log.Warnf("Values file %s is not allowed: %v", filepath.Join(appPath, "values.yaml"), err)
log.Warnf("Values file %s is not allowed: %v", filepath.Clean(filepath.Join(appPath, "values.yaml")), err)
}
var resolvedSelectedValueFiles []pathutil.ResolvedFilePath
// drop not allowed values files
Expand Down
12 changes: 6 additions & 6 deletions test/e2e/fixture/fixture.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,15 @@ func Name() string {
}

func repoDirectory() string {
return path.Join(TmpDir, repoDir)
return filepath.Clean(path.Join(TmpDir, repoDir))
}

func submoduleDirectory() string {
return path.Join(TmpDir, submoduleDir)
return filepath.Clean(path.Join(TmpDir, submoduleDir))
}

func submoduleParentDirectory() string {
return path.Join(TmpDir, submoduleParentDir)
return filepath.Clean(path.Join(TmpDir, submoduleParentDir))
}

const (
Expand Down Expand Up @@ -636,7 +636,7 @@ func Patch(path string, jsonPatch string) {

log.WithFields(log.Fields{"path": path, "jsonPatch": jsonPatch}).Info("patching")

filename := filepath.Join(repoDirectory(), path)
filename := filepath.Clean(filepath.Join(repoDirectory(), path))
bytes, err := ioutil.ReadFile(filepath.Clean(filename))
CheckError(err)

Expand Down Expand Up @@ -673,7 +673,7 @@ func Delete(path string) {

log.WithFields(log.Fields{"path": path}).Info("deleting")

CheckError(os.Remove(filepath.Join(repoDirectory(), path)))
CheckError(os.Remove(filepath.Clean(filepath.Join(repoDirectory(), path))))

FailOnErr(Run(repoDirectory(), "git", "diff"))
FailOnErr(Run(repoDirectory(), "git", "commit", "-am", "delete"))
Expand All @@ -685,7 +685,7 @@ func Delete(path string) {
func WriteFile(path, contents string) {
log.WithFields(log.Fields{"path": path}).Info("adding")

CheckError(ioutil.WriteFile(filepath.Join(repoDirectory(), path), []byte(contents), 0600))
CheckError(ioutil.WriteFile(filepath.Clean(filepath.Join(repoDirectory(), path)), []byte(contents), 0600))
}

func AddFile(path, contents string) {
Expand Down
2 changes: 1 addition & 1 deletion util/app/path/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ func Path(root, path string) (string, error) {
if filepath.IsAbs(path) {
return "", fmt.Errorf("%s: app path is absolute", path)
}
appPath := filepath.Join(root, path)
appPath := filepath.Clean(filepath.Join(root, path))
if !strings.HasPrefix(appPath, filepath.Clean(root)) {
return "", fmt.Errorf("%s: app path outside root", path)
}
Expand Down
2 changes: 1 addition & 1 deletion util/cert/cert.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ func GetCertificateForConnect(serverName string) ([]string, error) {
if !strings.HasSuffix(dataPath, "/") {
dataPath += "/"
}
certPath, err := filepath.Abs(filepath.Join(dataPath, ServerNameWithoutPort(serverName)))
certPath, err := filepath.Abs(filepath.Clean(filepath.Join(dataPath, ServerNameWithoutPort(serverName))))
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions util/cli/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ func PrintDiff(name string, live *unstructured.Unstructured, target *unstructure
if err != nil {
return err
}
targetFile := path.Join(tempDir, name)
targetFile := filepath.Clean(path.Join(tempDir, name))
targetData := []byte("")
if target != nil {
targetData, err = yaml.Marshal(target)
Expand All @@ -289,7 +289,7 @@ func PrintDiff(name string, live *unstructured.Unstructured, target *unstructure
if err != nil {
return err
}
liveFile := path.Join(tempDir, fmt.Sprintf("%s-live.yaml", name))
liveFile := filepath.Clean(path.Join(tempDir, fmt.Sprintf("%s-live.yaml", name)))
liveData := []byte("")
if live != nil {
liveData, err = yaml.Marshal(live)
Expand Down
2 changes: 1 addition & 1 deletion util/git/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func WithEventHandlers(handlers EventHandlers) ClientOpts {

func NewClient(rawRepoURL string, creds Creds, insecure bool, enableLfs bool, proxy string, opts ...ClientOpts) (Client, error) {
r := regexp.MustCompile("(/|:)")
root := filepath.Join(os.TempDir(), r.ReplaceAllString(NormalizeGitURL(rawRepoURL), "_"))
root := filepath.Clean(filepath.Join(os.TempDir(), r.ReplaceAllString(NormalizeGitURL(rawRepoURL), "_")))
if root == os.TempDir() {
return nil, fmt.Errorf("Repository '%s' cannot be initialized, because its root would be system temp at %s", rawRepoURL, root)
}
Expand Down
10 changes: 5 additions & 5 deletions util/gpg/gpg.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func writeKeyToFile(keyData string) (string, error) {
// This must only be called on container startup, when no gpg-agent is running
// yet, otherwise key generation will fail.
func removeKeyRing(path string) error {
_, err := os.Stat(filepath.Join(path, canaryMarkerFilename))
_, err := os.Stat(filepath.Clean(filepath.Join(path, canaryMarkerFilename)))
if err != nil {
if os.IsNotExist(err) {
return fmt.Errorf("refusing to remove directory %s: it's not initialized by Argo CD", path)
Expand All @@ -201,7 +201,7 @@ func removeKeyRing(path string) error {
if p == "." || p == ".." {
continue
}
err := os.RemoveAll(filepath.Join(path, p))
err := os.RemoveAll(filepath.Clean(filepath.Join(path, p)))
if err != nil {
return err
}
Expand Down Expand Up @@ -252,7 +252,7 @@ func InitializeGnuPG() error {
}
}

err = ioutil.WriteFile(filepath.Join(gnuPgHome, canaryMarkerFilename), []byte("canary"), 0600)
err = ioutil.WriteFile(filepath.Clean(filepath.Join(gnuPgHome, canaryMarkerFilename)), []byte("canary"), 0600)
if err != nil {
return fmt.Errorf("could not create canary: %v", err)
}
Expand Down Expand Up @@ -724,12 +724,12 @@ func SyncKeyRingFromDirectory(basePath string) ([]string, []string, error) {
// First, add all keys that are found in the configuration but are not yet in the keyring
for key := range configured {
if _, ok := installed[key]; !ok {
addedKey, err := ImportPGPKeys(path.Join(basePath, key))
addedKey, err := ImportPGPKeys(filepath.Clean(path.Join(basePath, key)))
if err != nil {
return nil, nil, err
}
if len(addedKey) != 1 {
return nil, nil, fmt.Errorf("Invalid key found in %s", path.Join(basePath, key))
return nil, nil, fmt.Errorf("Invalid key found in %s", filepath.Clean(path.Join(basePath, key)))
}
importedKey, err := GetInstalledPGPKeys([]string{addedKey[0].KeyID})
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions util/helm/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ func (c *nativeHelmChart) ExtractChart(chart string, version string, passCredent
if len(infos) != 1 {
return "", nil, fmt.Errorf("expected 1 file, found %v", len(infos))
}
err = os.Rename(filepath.Join(tempDest, infos[0].Name()), cachedChartPath)
err = os.Rename(filepath.Clean(filepath.Join(tempDest, infos[0].Name())), cachedChartPath)
if err != nil {
return "", nil, err
}
Expand All @@ -205,7 +205,7 @@ func (c *nativeHelmChart) ExtractChart(chart string, version string, passCredent
_ = os.RemoveAll(tempDir)
return "", nil, err
}
return path.Join(tempDir, normalizeChartName(chart)), io.NewCloser(func() error {
return filepath.Clean(path.Join(tempDir, normalizeChartName(chart))), io.NewCloser(func() error {
return os.RemoveAll(tempDir)
}), nil
}
Expand Down Expand Up @@ -379,7 +379,7 @@ func getIndexURL(rawURL string) (string, error) {
if err != nil {
return "", err
}
repoURL.Path = path.Join(repoURL.Path, indexFile)
repoURL.RawPath = path.Join(repoURL.RawPath, indexFile)
repoURL.Path = filepath.Clean(path.Join(repoURL.Path, indexFile))
repoURL.RawPath = filepath.Clean(path.Join(repoURL.RawPath, indexFile))
return repoURL.String(), nil
}
2 changes: 1 addition & 1 deletion util/helm/helm.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath, appPath, r
}
values = append(values, out)
} else {
log.Warnf("Values file %s is not allowed: %v", filepath.Join(appPath, "values.yaml"), err)
log.Warnf("Values file %s is not allowed: %v", filepath.Clean(filepath.Join(appPath, "values.yaml")), err)
}
for i := range valuesFiles {
file := string(valuesFiles[i])
Expand Down
4 changes: 2 additions & 2 deletions util/io/files/tar.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func Untgz(dstPath string, r io.Reader) error {
continue
}

target := filepath.Join(dstPath, header.Name)
target := filepath.Clean(filepath.Join(dstPath, header.Name))
// Sanity check to protect against zip-slip
if !Inbound(target, dstPath) {
return fmt.Errorf("illegal filepath in archive: %s", target)
Expand All @@ -85,7 +85,7 @@ func Untgz(dstPath string, r io.Reader) error {
}
case tar.TypeSymlink:
// Sanity check to protect against symlink exploit
linkTarget := filepath.Join(filepath.Dir(target), header.Linkname)
linkTarget := filepath.Clean(filepath.Join(filepath.Dir(target), header.Linkname))
realPath, err := filepath.EvalSymlinks(linkTarget)
if os.IsNotExist(err) {
realPath = linkTarget
Expand Down
2 changes: 1 addition & 1 deletion util/io/files/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func Inbound(candidate, baseDir string) bool {
if filepath.IsAbs(candidate) {
target = filepath.Clean(candidate)
} else {
target = filepath.Join(baseDir, candidate)
target = filepath.Clean(filepath.Join(baseDir, candidate))
}
return strings.HasPrefix(target, filepath.Clean(baseDir)+string(os.PathSeparator))
}
6 changes: 3 additions & 3 deletions util/io/path/resolved.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func resolveSymbolicLinkRecursive(path string, maxDepth int) (string, error) {
// path for further resolving
if !strings.HasPrefix(resolved, "/") {
basePath := filepath.Dir(path)
resolved = filepath.Join(basePath, resolved)
resolved = filepath.Clean(filepath.Join(basePath, resolved))
}

return resolveSymbolicLinkRecursive(resolved, maxDepth-1)
Expand Down Expand Up @@ -125,9 +125,9 @@ func ResolveFilePath(appPath, repoRoot, valueFile string, allowedURLSchemes []st
if err != nil {
return "", false, resolveFailure(repoRoot, err)
}
path = filepath.Join(absWorkDir, path)
path = filepath.Clean(filepath.Join(absWorkDir, path))
} else {
path = filepath.Join(absRepoPath, path)
path = filepath.Clean(filepath.Join(absRepoPath, path))
}

// Ensure any symbolic link is resolved before we
Expand Down
2 changes: 1 addition & 1 deletion util/io/subdirfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ type subDirFs struct {
}

func (s subDirFs) Open(name string) (fs.File, error) {
return s.fs.Open(filepath.Join(s.dir, name))
return s.fs.Open(filepath.Clean(filepath.Join(s.dir, name)))
}

// NewSubDirFS returns file system that represents sub-directory in a wrapped file system
Expand Down
2 changes: 1 addition & 1 deletion util/ksonnet/ksonnet.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func NewKsonnetApp(path string) (KsonnetApp, error) {

func (k *ksonnetApp) appYamlPath() (string, error) {
const appYamlName = "app.yaml"
p := filepath.Join(k.Root(), appYamlName)
p := filepath.Clean(filepath.Join(k.Root(), appYamlName))
if _, err := os.Stat(p); err != nil {
return "", err
}
Expand Down
2 changes: 1 addition & 1 deletion util/kustomize/kustomize.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ var KustomizationNames = []string{"kustomization.yaml", "kustomization.yml", "Ku
// kustomization is a file that describes a configuration consumable by kustomize.
func (k *kustomize) findKustomization() (string, error) {
for _, file := range KustomizationNames {
kustomization := filepath.Join(k.path, file)
kustomization := filepath.Clean(filepath.Join(k.path, file))
if _, err := os.Stat(kustomization); err == nil {
return kustomization, nil
}
Expand Down
7 changes: 4 additions & 3 deletions util/localconfig/localconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"os/user"
"path"
"path/filepath"
"strings"

configUtil "github.com/argoproj/argo-cd/v2/util/config"
Expand Down Expand Up @@ -273,11 +274,11 @@ func DefaultConfigDir() (string, error) {

// Manually configured XDG config home
if xdgConfigHome := os.Getenv("XDG_CONFIG_HOME"); xdgConfigHome != "" {
return path.Join(xdgConfigHome, "argocd"), nil
return filepath.Clean(path.Join(xdgConfigHome, "argocd")), nil
}

// XDG config home fallback
return path.Join(homeDir, ".config", "argocd"), nil
return filepath.Clean(path.Join(homeDir, ".config", "argocd")), nil
}

func getHomeDir() (string, error) {
Expand All @@ -300,7 +301,7 @@ func DefaultLocalConfigPath() (string, error) {
if err != nil {
return "", err
}
return path.Join(dir, "config"), nil
return filepath.Clean(path.Join(dir, "config")), nil
}

// Get username from subject in a claim
Expand Down
2 changes: 1 addition & 1 deletion util/lua/lua.go
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ func GetConfigMapKey(gvk schema.GroupVersionKind) string {
}

func (vm VM) getPredefinedLuaScripts(objKey string, scriptFile string) (string, error) {
data, err := resource_customizations.Embedded.ReadFile(filepath.Join(objKey, scriptFile))
data, err := resource_customizations.Embedded.ReadFile(filepath.Clean(filepath.Join(objKey, scriptFile)))
if err != nil {
if os.IsNotExist(err) {
return "", nil
Expand Down
3 changes: 2 additions & 1 deletion util/settings/settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"math/big"
"net/url"
"path"
"path/filepath"
"reflect"
"strconv"
"strings"
Expand Down Expand Up @@ -1731,7 +1732,7 @@ func appendURLPath(inputURL string, inputPath string) (string, error) {
if err != nil {
return "", err
}
u.Path = path.Join(u.Path, inputPath)
u.Path = filepath.Clean(path.Join(u.Path, inputPath))
return u.String(), nil
}

Expand Down
Loading

0 comments on commit 8562adf

Please sign in to comment.