Skip to content

Commit

Permalink
fix: prevent file traversal using helm file values param and applicat…
Browse files Browse the repository at this point in the history 
…ion details api (argoproj#8606)

* fix: prevent file traversal using helm file values param and application details api

Signed-off-by: Alexander Matyushentsev <[email protected]>

* apply reviewer notes: move resolve.go into separate package; use uuid to generate random file

Signed-off-by: Alexander Matyushentsev <[email protected]>
  • Loading branch information
@alexmt
alexmt authored Feb 24, 2022
1 parent c7da148 commit 76eddaf
Show file tree
Hide file tree
Showing 19 changed files with 600 additions and 464 deletions.
15 changes: 15 additions & 0 deletions reposerver/apiclient/repository.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package apiclient

func (q *ManifestRequest) GetValuesFileSchemes() []string {
if q.HelmOptions == nil {
return nil
}
return q.HelmOptions.ValuesFileSchemes
}

func (q *RepoServerAppDetailsQuery) GetValuesFileSchemes() []string {
if q.HelmOptions == nil {
return nil
}
return q.HelmOptions.ValuesFileSchemes
}
262 changes: 161 additions & 101 deletions reposerver/apiclient/repository.pb.go
View file

Large diffs are not rendered by default.

198 changes: 29 additions & 169 deletions reposerver/repository/repository.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"github.com/ghodss/yaml"
gogit "github.com/go-git/go-git/v5"
"github.com/google/go-jsonnet"
"github.com/google/uuid"
log "github.com/sirupsen/logrus"
"golang.org/x/sync/semaphore"
"google.golang.org/grpc/codes"
Expand All @@ -52,6 +53,7 @@ import (
"github.com/argoproj/argo-cd/v2/util/grpc"
"github.com/argoproj/argo-cd/v2/util/helm"
"github.com/argoproj/argo-cd/v2/util/io"
pathutil "github.com/argoproj/argo-cd/v2/util/io/path"
"github.com/argoproj/argo-cd/v2/util/ksonnet"
"github.com/argoproj/argo-cd/v2/util/kustomize"
"github.com/argoproj/argo-cd/v2/util/text"
Expand Down Expand Up @@ -606,153 +608,6 @@ func runHelmBuild(appPath string, h helm.Helm) error {
return ioutil.WriteFile(markerFile, []byte("marker"), 0644)
}

// resolveSymbolicLinkRecursive resolves the symlink path recursively to its
// canonical path on the file system, with a maximum nesting level of maxDepth.
// If path is not a symlink, returns the verbatim copy of path and err of nil.
func resolveSymbolicLinkRecursive(path string, maxDepth int) (string, error) {
resolved, err := os.Readlink(path)
if err != nil {
// path is not a symbolic link
_, ok := err.(*os.PathError)
if ok {
return path, nil
}
// Other error has occured
return "", err
}

if maxDepth == 0 {
return "", fmt.Errorf("maximum nesting level reached")
}

// If we resolved to a relative symlink, make sure we use the absolute
// path for further resolving
if !strings.HasPrefix(resolved, "/") {
basePath := filepath.Dir(path)
resolved = filepath.Join(basePath, resolved)
}

return resolveSymbolicLinkRecursive(resolved, maxDepth-1)
}

// isURLSchemeAllowed returns true if the protocol scheme is in the list of
// allowed URL schemes.
func isURLSchemeAllowed(scheme string, allowed []string) bool {
isAllowed := false
if len(allowed) > 0 {
for _, s := range allowed {
if strings.EqualFold(scheme, s) {
isAllowed = true
break
}
}
}

// Empty scheme means local file
return isAllowed && scheme != ""
}

// resolveHelmValueFilePath will inspect and resolve a path to a Helm value
// file, and make sure that its final path is within the boundaries of the
// path specified in repoRoot.
//
// appPath is the path we're operating in, e.g. where a Helm chart was unpacked
// to. repoRoot is the path to the root of the repository.
//
// If either appPath or repoRoot is relative, it will be treated as relative
// to the current working directory.
//
// valueFile is the path to a value file, relative to appPath. If valueFile is
// specified as an absolute path (i.e. leading slash), it will be treated as
// relative to the repoRoot. In case valueFile is a symlink in the extracted
// chart, it will be resolved recursively and the decision of whether it is in
// the boundary of repoRoot will be made using the final resolved path.
// valueFile can also be a remote URL with a protocol scheme as prefix,
// in which case the scheme must be included in the list of allowed schemes
// specified by allowedURLSchemes.
//
// Will return an error if either valueFile is outside the boundaries of the
// repoRoot, valueFile is an URL with a forbidden protocol scheme or if
// valueFile is a recursive symlink nested too deep. May return errors for
// other reasons as well.
//
// resolvedPath will hold the absolute, resolved path for valueFile on success
// or set to the empty string on failure.
//
// isRemote will be set to true if valueFile is an URL using an allowed
// protocol scheme, or to false if it resolved to a local file.
func resolveHelmValueFilePath(appPath, repoRoot, valueFile string, allowedURLSchemes []string) (resolvedPath string, isRemote bool, err error) {

// We do not provide the path in the error message, because it will be
// returned to the user and could be used for information gathering.
// Instead, we log the concrete error details.
resolveFailure := func(path string, err error) error {
log.Errorf("failed to resolve path '%s': %v", path, err)
return fmt.Errorf("internal error: failed to resolve path. Check logs for more details")
}

// A value file can be specified as an URL to a remote resource.
// We only allow certain URL schemes for remote value files.
url, err := url.Parse(valueFile)
if err == nil {
// If scheme is empty, it means we parsed a path only
if url.Scheme != "" {
if isURLSchemeAllowed(url.Scheme, allowedURLSchemes) {
return valueFile, true, nil
} else {
return "", false, fmt.Errorf("the URL scheme '%s' is not allowed", url.Scheme)
}
}
}

// Ensure that our repository root is absolute
absRepoPath, err := filepath.Abs(repoRoot)
if err != nil {
return "", false, resolveFailure(repoRoot, err)
}

// If the path to the file is relative, join it with the current working directory (appPath)
// Otherwise, join it with the repository's root
path := valueFile
if !filepath.IsAbs(path) {
absWorkDir, err := filepath.Abs(appPath)
if err != nil {
return "", false, resolveFailure(repoRoot, err)
}
path = filepath.Join(absWorkDir, path)
} else {
path = filepath.Join(absRepoPath, path)
}

// Ensure any symbolic link is resolved before we
delinkedPath, err := resolveSymbolicLinkRecursive(path, 10)
if err != nil {
return "", false, resolveFailure(path, err)
}
path = delinkedPath

// Resolve the joined path to an absolute path
path, err = filepath.Abs(path)
if err != nil {
return "", false, resolveFailure(path, err)
}

// Ensure our root path has a trailing slash, otherwise the following check
// would return true if root is /foo and path would be /foo2
requiredRootPath := absRepoPath
if !strings.HasSuffix(requiredRootPath, "/") {
requiredRootPath += "/"
}

// Make sure that the resolved path to values file is within the repository's root path
if !strings.HasPrefix(path, requiredRootPath) {
return "", false, fmt.Errorf("value file '%s' resolved to outside repository root", valueFile)
}

return path, false, nil

}

func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclient.ManifestRequest, isLocal bool) ([]*unstructured.Unstructured, error) {
concurrencyAllowed := isConcurrencyAllowed(appPath)
if !concurrencyAllowed {
Expand All @@ -767,7 +622,7 @@ func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclie
APIVersions: q.ApiVersions,
Set: map[string]string{},
SetString: map[string]string{},
SetFile: map[string]string{},
SetFile: map[string]pathutil.ResolvedFilePath{},
}

appHelm := q.ApplicationSource.Helm
Expand All @@ -784,17 +639,13 @@ func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclie
for _, val := range appHelm.ValueFiles {

// This will resolve val to an absolute path (or an URL)
var protocols []string
if q.HelmOptions != nil {
protocols = q.HelmOptions.ValuesFileSchemes
}
path, isRemote, err := resolveHelmValueFilePath(appPath, repoRoot, val, protocols)
path, isRemote, err := pathutil.ResolveFilePath(appPath, repoRoot, val, q.GetValuesFileSchemes())
if err != nil {
return nil, err
}

if !isRemote {
_, err = os.Stat(path)
_, err = os.Stat(string(path))
if os.IsNotExist(err) {
if appHelm.IgnoreMissingValueFiles {
log.Debugf(" %s values file does not exist", path)
Expand All @@ -807,18 +658,17 @@ func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclie
}

if appHelm.Values != "" {
file, err := ioutil.TempFile("", "values-*.yaml")
rand, err := uuid.NewRandom()
if err != nil {
return nil, err
}
p := file.Name()
p := path.Join(os.TempDir(), rand.String())
defer func() { _ = os.RemoveAll(p) }()
err = ioutil.WriteFile(p, []byte(appHelm.Values), 0644)
if err != nil {
return nil, err
}
defer file.Close()
templateOpts.Values = append(templateOpts.Values, p)
templateOpts.Values = append(templateOpts.Values, pathutil.ResolvedFilePath(p))
}

for _, p := range appHelm.Parameters {
Expand All @@ -829,7 +679,11 @@ func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclie
}
}
for _, p := range appHelm.FileParameters {
templateOpts.SetFile[p.Name] = p.Path
resolvedPath, _, err := pathutil.ResolveFilePath(appPath, repoRoot, env.Envsubst(p.Path), q.GetValuesFileSchemes())
if err != nil {
return nil, err
}
templateOpts.SetFile[p.Name] = resolvedPath
}
passCredentials = appHelm.PassCredentials
templateOpts.SkipCrds = appHelm.SkipCrds
Expand All @@ -843,9 +697,6 @@ func helmTemplate(appPath string, repoRoot string, env *v1alpha1.Env, q *apiclie
for i, j := range templateOpts.SetString {
templateOpts.SetString[i] = env.Envsubst(j)
}
for i, j := range templateOpts.SetFile {
templateOpts.SetFile[i] = env.Envsubst(j)
}

repos, err := getHelmDependencyRepos(appPath)
if err != nil {
Expand Down Expand Up @@ -1285,11 +1136,11 @@ func makeJsonnetVm(appPath string, repoRoot string, sourceJsonnet v1alpha1.Appli
// Jsonnet Imports relative to the repository path
jpaths := []string{appPath}
for _, p := range sourceJsonnet.Libs {
jpath := path.Join(repoRoot, p)
if !strings.HasPrefix(jpath, repoRoot) {
return nil, status.Errorf(codes.FailedPrecondition, "%s: referenced library points outside the repository", p)
jpath, _, err := pathutil.ResolveFilePath(appPath, repoRoot, p, nil)
if err != nil {
return nil, err
}
jpaths = append(jpaths, jpath)
jpaths = append(jpaths, string(jpath))
}

vm.Importer(&jsonnet.FileImporter{
Expand Down Expand Up @@ -1467,7 +1318,7 @@ func (s *Service) GetAppDetails(ctx context.Context, q *apiclient.RepoServerAppD
return err
}
case v1alpha1.ApplicationSourceTypeHelm:
if err := populateHelmAppDetails(res, opContext.appPath, q); err != nil {
if err := populateHelmAppDetails(res, opContext.appPath, repoRoot, q); err != nil {
return err
}
case v1alpha1.ApplicationSourceTypeKustomize:
Expand Down Expand Up @@ -1529,7 +1380,7 @@ func populateKsonnetAppDetails(res *apiclient.RepoAppDetailsResponse, appPath st
return nil
}

func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath string, q *apiclient.RepoServerAppDetailsQuery) error {
func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath string, repoRoot string, q *apiclient.RepoServerAppDetailsQuery) error {
var selectedValueFiles []string

if q.Source.Helm != nil {
Expand Down Expand Up @@ -1563,7 +1414,16 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin
if err := loadFileIntoIfExists(filepath.Join(appPath, "values.yaml"), &res.Helm.Values); err != nil {
return err
}
params, err := h.GetParameters(selectedValueFiles)
var resolvedSelectedValueFiles []pathutil.ResolvedFilePath
// drop not allowed values files
for _, file := range selectedValueFiles {
if resolvedFile, _, err := pathutil.ResolveFilePath(appPath, repoRoot, file, q.GetValuesFileSchemes()); err == nil {
resolvedSelectedValueFiles = append(resolvedSelectedValueFiles, resolvedFile)
} else {
log.Debugf("Values file %s is not allowed: %v", file, err)
}
}
params, err := h.GetParameters(resolvedSelectedValueFiles)
if err != nil {
return err
}
Expand Down
1 change: 1 addition & 0 deletions reposerver/repository/repository.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ message RepoServerAppDetailsQuery {
bool noRevisionCache = 7;
string trackingMethod = 8;
map<string, bool> enabledSourceTypes = 9;
github.com.argoproj.argo_cd.v2.pkg.apis.application.v1alpha1.HelmOptions helmOptions = 10;
}

// RepoAppDetailsResponse application details
Expand Down
Loading

0 comments on commit 76eddaf

Please sign in to comment.