diff --git a/archive/archive.go b/archive/archive.go index 6b9def10..ccbe43c2 100644 --- a/archive/archive.go +++ b/archive/archive.go @@ -209,7 +209,7 @@ func installMacDMG(b *ui.Task, source string, pkg *manifest.Package) error { if err != nil { return errors.WithStack(err) } - output, err := util.Capture(b, "hdiutil", "attach", "-plist", source) + output, err := util.CaptureSystem(b, "hdiutil", "attach", "-plist", source) if err != nil { return errors.Wrap(err, "could not mount DMG") } @@ -228,14 +228,14 @@ func installMacDMG(b *ui.Task, source string, pkg *manifest.Package) error { if entry == nil { return errors.New("couldn't determine volume information from hdiutil attach, volume may still be mounted :(") } - defer util.Run(b, "hdiutil", "detach", entry.DevEntry) //nolint: errcheck + defer util.RunSystem(b, "hdiutil", "detach", entry.DevEntry) //nolint: errcheck switch { case len(pkg.Apps) != 0: for _, app := range pkg.Apps { base := filepath.Base(app) // Use rsync because reliably syncing all filesystem attributes is non-trivial. appDest := filepath.Join(dest, base) - err = util.Run(b, "rsync", "-av", + err = util.RunSystem(b, "rsync", "-av", filepath.Join(entry.MountPoint, app)+"/", appDest+"/") if err != nil { @@ -377,7 +377,7 @@ func extractMacPKG(b *ui.Task, path, dest string, strip int) error { fmt.Fprint(changesf, os.Expand(extractMacPkgChangesXML, func(s string) string { return dest })) _ = changesf.Close() task.Add(1) - return util.Run(b, "installer", "-verbose", + return util.RunSystem(b, "installer", "-verbose", "-pkg", path, "-target", "CurrentUserHomeDirectory", "-applyChoiceChangesXML", changesf.Name()) diff --git a/cache/git.go b/cache/git.go index b025d3bf..9344cf7c 100644 --- a/cache/git.go +++ b/cache/git.go @@ -2,7 +2,6 @@ package cache import ( "os" - "os/exec" "path/filepath" "strings" @@ -32,12 +31,12 @@ func (s *gitSource) Download(b *ui.Task, cache *Cache, checksum string) (string, args = append(args, "--branch="+tag) } args = append(args, "--", repo, checkoutDir) - err = util.RunInDir(b, cache.root, args...) + err = util.RunSystemInDir(b, cache.root, args...) if err != nil { return "", "", "", errors.WithStack(err) } - bts, err := util.CaptureInDir(b, checkoutDir, "git", "rev-parse", "HEAD") + bts, err := util.CaptureSystemInDir(b, checkoutDir, "git", "rev-parse", "HEAD") if err != nil { return "", "", "", errors.WithStack(err) } @@ -54,7 +53,7 @@ func (s *gitSource) ETag(b *ui.Task) (etag string, err error) { if tag == "" { tag = "HEAD" } - bts, err := util.Capture(b, util.GitArgs("ls-remote", "--", repo, tag)...) + bts, err := util.CaptureSystem(b, util.GitArgs("ls-remote", "--", repo, tag)...) if err != nil { return "", errors.Wrap(err, s.URL) } @@ -76,7 +75,10 @@ func (s *gitSource) Validate() error { tag = "HEAD" } args := util.GitArgs("ls-remote", "--", repo, tag) - cmd := exec.Command(args[0], args[1:]...) //nolint + cmd, err := util.SystemCommand(args...) + if err != nil { + return errors.WithStack(err) + } out, err := cmd.CombinedOutput() if err != nil { return errors.Wrapf(err, "error getting remote HEAD: %s", string(out)) diff --git a/env.go b/env.go index 01fd48db..10ee8d67 100644 --- a/env.go +++ b/env.go @@ -173,7 +173,7 @@ func Init(l *ui.UI, env string, distURL string, stateDir string, config Config, } if useGit { - if err = util.RunInDir(b, env, "git", "add", "-f", extDepPath); err != nil { + if err = util.RunSystemInDir(b, env, "git", "add", "-f", extDepPath); err != nil { return errors.WithStack(err) } } @@ -196,7 +196,7 @@ func Init(l *ui.UI, env string, distURL string, stateDir string, config Config, return errors.WithStack(err) } if useGit { - if err = util.RunInDir(b, env, "git", "add", "-f", filepath.Join(bin, "hermit.hcl")); err != nil { + if err = util.RunSystemInDir(b, env, "git", "add", "-f", filepath.Join(bin, "hermit.hcl")); err != nil { return errors.WithStack(err) } } @@ -587,7 +587,7 @@ func (e *Env) unlinkPackage(l *ui.Task, pkg *manifest.Package) error { func (e *Env) unlink(l *ui.Task, path string) error { if e.useGit { - err := util.RunInDir(l, e.envDir, "git", "rm", "-f", path) + err := util.RunSystemInDir(l, e.envDir, "git", "rm", "-f", path) if err != nil { l.Errorf("non-fatal: %s", err) } @@ -1423,7 +1423,7 @@ func (e *Env) linkIntoEnv(l *ui.Task, oldname, newname string) error { return errors.WithStack(err) } if e.useGit { - return util.RunInDir(l, e.envDir, "git", "add", "-f", newname) + return util.RunSystemInDir(l, e.envDir, "git", "add", "-f", newname) } return nil } @@ -1657,7 +1657,7 @@ func writeFileToEnvBin(l *ui.Task, useGit bool, src, envDir string, vars map[str return errors.WithStack(err) } if useGit { - if err = util.RunInDir(l, envDir, "git", "add", "-f", dest); err != nil { + if err = util.RunSystemInDir(l, envDir, "git", "add", "-f", dest); err != nil { return errors.WithStack(err) } } diff --git a/integration/integration_test.go b/integration/integration_test.go index d232fb6d..8627234f 100644 --- a/integration/integration_test.go +++ b/integration/integration_test.go @@ -25,6 +25,8 @@ import ( "testing" "github.com/alecthomas/assert/v2" + "github.com/kballard/go-shellquote" + "github.com/cashapp/hermit/envars" "github.com/cashapp/hermit/errors" "github.com/creack/pty" @@ -171,6 +173,54 @@ EOF `, expectations: exp{outputContains("remote helpers are not supported")}, }, + { + // Regression test for DX-29: internal tools must still resolve from a + // user's custom PATH before a Hermit environment is activated. + name: "InternalToolsUseCurrentPathBeforeActivation", + preparations: prep{gitSourceWithHostGit()}, + script: ` + assert test "$(command -v git)" = "$HOST_GIT" + hermit init --no-git . + cat > bin/hermit.hcl < bin/hermit.hcl < bin/git <<'EOF' +#!/bin/sh +touch "$(dirname "$0")/../RCE.txt" +exit 1 +EOF + chmod +x bin/git + . bin/activate-hermit + hash -r 2>/dev/null || true + rehash 2>/dev/null || true + assert test "$(command -v git)" = "$PWD/bin/git" + + rm -f HOST_GIT_USED.txt + hermit search safehelper + assert test ! -e RCE.txt + assert test -e HOST_GIT_USED.txt + `, + expectations: exp{outputContains("safehelper")}, + }, { name: "InitBasicDefaultsToTrue", script: ` @@ -1013,6 +1063,44 @@ func addFile(name, content string) preparation { } } +// gitSourceWithHostGit creates a local Git manifest source and a recording Git +// wrapper in a custom directory on the user's PATH. +func gitSourceWithHostGit() preparation { + return func(t *testing.T, dir string) string { + t.Helper() + git, err := exec.LookPath("git") + assert.NoError(t, err) + + sourceDir := filepath.Join(dir, "source.git") + assert.NoError(t, os.Mkdir(sourceDir, 0700)) + runGit := func(args ...string) { + cmd := exec.Command(git, args...) //nolint:noctx + output, err := cmd.CombinedOutput() + assert.NoError(t, err, "%s", output) + } + runGit("init", "-q", sourceDir) + assert.NoError(t, os.WriteFile(filepath.Join(sourceDir, "safehelper.hcl"), []byte(` +description = "Package from a safely cloned source" +source = "https://example.com/safehelper-${version}" +version "1.0.0" {} +`), 0600)) + runGit("-C", sourceDir, "add", "safehelper.hcl") + runGit("-C", sourceDir, + "-c", "user.name=Hermit", + "-c", "user.email=hermit@example.com", + "-c", "commit.gpgsign=false", + "commit", "-qm", "initial") + + hostBin := t.TempDir() + hostGit := filepath.Join(hostBin, "git") + wrapper := fmt.Sprintf("#!/bin/sh\ntouch %s\nexec %s \"$@\"\n", + shellquote.Join(filepath.Join(dir, "HOST_GIT_USED.txt")), shellquote.Join(git)) + assert.NoError(t, os.WriteFile(hostGit, []byte(wrapper), 0700)) + return fmt.Sprintf("export HOST_GIT=%s\nexport PATH=%s:\"$PATH\"", + shellquote.Join(hostGit), shellquote.Join(hostBin)) + } +} + // Copy a file from the testdata directory to the test directory. func copyFile(name string) preparation { return func(t *testing.T, dir string) string { diff --git a/manifest/autoversion/git_tags.go b/manifest/autoversion/git_tags.go index 6206fe9e..5f28f627 100644 --- a/manifest/autoversion/git_tags.go +++ b/manifest/autoversion/git_tags.go @@ -2,7 +2,6 @@ package autoversion import ( "bufio" - "os/exec" "regexp" "sort" "strings" @@ -32,7 +31,11 @@ func gitTagsAutoVersion(autoVersion *manifest.AutoVersionBlock) (string, error) // TAB LF // source: https://git-scm.com/docs/git-ls-remote args := util.GitArgs("ls-remote", "--tags", "--refs", "--", remoteURL) - out, err := exec.Command(args[0], args[1:]...).Output() //nolint:noctx,gosec + cmd, err := util.SystemCommand(args...) + if err != nil { + return "", errors.WithStack(err) + } + out, err := cmd.Output() if err != nil { return "", errors.Wrapf(err, "error listing tags for %s", remoteURL) } diff --git a/util/run.go b/util/run.go index b26af764..8180a71f 100644 --- a/util/run.go +++ b/util/run.go @@ -3,11 +3,14 @@ package util import ( "bytes" "io" + "os" "os/exec" + "path/filepath" "strings" "github.com/kballard/go-shellquote" + "github.com/cashapp/hermit/envars" "github.com/cashapp/hermit/errors" "github.com/cashapp/hermit/ui" ) @@ -22,7 +25,63 @@ type CommandRunner interface { type RealCommandRunner struct{} func (g *RealCommandRunner) RunInDir(task *ui.Task, dir string, commands ...string) error { - return errors.WithStack(RunInDir(task, dir, commands...)) + return errors.WithStack(RunSystemInDir(task, dir, commands...)) +} + +// SystemCommand constructs a command for an external tool used internally by +// Hermit. The executable is resolved from PATH with the active Hermit +// environment's changes reverted, and that same PATH is inherited by its +// child processes. +func SystemCommand(args ...string) (*exec.Cmd, error) { + if len(args) == 0 { + return nil, errors.New("missing system command") + } + name := args[0] + if filepath.Base(name) != name { + return nil, errors.Errorf("system command must be a bare name: %q", name) + } + environ, err := systemEnviron() + if err != nil { + return nil, err + } + var path string + for _, dir := range filepath.SplitList(environ["PATH"]) { + candidate := filepath.Join(dir, name) + info, err := os.Stat(candidate) + if err == nil && !info.IsDir() && info.Mode().Perm()&0111 != 0 { + path = candidate + break + } + } + if path == "" { + return nil, &exec.Error{Name: name, Err: exec.ErrNotFound} + } + cmd := exec.Command(path, args[1:]...) //nolint:noctx + cmd.Env = environ.System() + return cmd, nil +} + +// systemEnviron returns the current environment with PATH restored to its +// state before Hermit activation. All other environment variables are left +// unchanged. +func systemEnviron() (envars.Envars, error) { + environ := envars.Parse(os.Environ()) + data := os.Getenv("HERMIT_ENV_OPS") + if data == "" { + return environ, nil + } + ops, err := envars.UnmarshalOps([]byte(data)) + if err != nil { + return nil, errors.Wrap(err, "failed to restore PATH before Hermit activation") + } + reverted := environ.Revert(os.Getenv("HERMIT_ENV"), ops).Combined() + path, ok := reverted["PATH"] + if !ok { + delete(environ, "PATH") + } else { + environ["PATH"] = path + } + return environ, nil } // Run a command, outputting to stdout and stderr. @@ -30,6 +89,11 @@ func Run(log *ui.Task, args ...string) error { return RunInDir(log, "", args...) } +// RunSystem runs an external tool used internally by Hermit. +func RunSystem(log *ui.Task, args ...string) error { + return RunSystemInDir(log, "", args...) +} + // Capture runs a command, returning combined stdout and stderr. func Capture(log ui.Logger, args ...string) ([]byte, error) { log.Debugf("%s", shellquote.Join(args...)) @@ -37,6 +101,23 @@ func Capture(log ui.Logger, args ...string) ([]byte, error) { return captureOutput(log, cmd) } +// CaptureSystem runs an external tool used internally by Hermit and returns its output. +func CaptureSystem(log ui.Logger, args ...string) ([]byte, error) { + return CaptureSystemInDir(log, "", args...) +} + +// CaptureSystemInDir runs an external tool used internally by Hermit in the given dir +// and returns its output. +func CaptureSystemInDir(log ui.Logger, dir string, args ...string) ([]byte, error) { + log.Debugf("%s", shellquote.Join(args...)) + cmd, err := SystemCommand(args...) + if err != nil { + return nil, errors.WithStack(err) + } + cmd.Dir = dir + return captureOutput(log, cmd) +} + // CaptureInDir runs a command in the given dir, returning combined stdout and stderr. func CaptureInDir(log ui.Logger, dir string, args ...string) ([]byte, error) { log.Debugf("%s", shellquote.Join(args...)) @@ -69,6 +150,28 @@ func RunInDir(log *ui.Task, dir string, args ...string) error { return nil } +// RunSystemInDir runs an external tool used internally by Hermit in the given dir. +func RunSystemInDir(log *ui.Task, dir string, args ...string) error { + log = log.SubTask("exec") + log.Debugf("%s", shellquote.Join(args...)) + b := &bytes.Buffer{} + w := io.MultiWriter(b, log) + cmd, err := SystemCommand(args...) + if err != nil { + return errors.WithStack(err) + } + cmd.Dir = dir + cmd.Stdout = w + cmd.Stderr = w + if err = cmd.Run(); err != nil { + if !log.WillLog(ui.LevelDebug) { + log.Errorf("%s", b.String()) + } + return errors.Wrapf(err, "%s failed", shellquote.Join(args...)) + } + return nil +} + // Command constructs a new exec.Cmd with logging configured. // // Returns the command, and a *bytes.Buffer containing the combined stdout and stderr diff --git a/util/run_test.go b/util/run_test.go new file mode 100644 index 00000000..6c6d1043 --- /dev/null +++ b/util/run_test.go @@ -0,0 +1,54 @@ +//go:build !windows + +package util_test + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/alecthomas/assert/v2" + "github.com/cashapp/hermit/envars" + "github.com/cashapp/hermit/util" +) + +func TestSystemCommandRestoresPathBeforeHermitActivation(t *testing.T) { + hostDir := t.TempDir() + hostGit := filepath.Join(hostDir, "git") + assert.NoError(t, os.WriteFile(hostGit, []byte("#!/bin/sh\nprintf 'host:%s' \"$PATH\"\n"), 0700)) + + envRoot := t.TempDir() + envBin := filepath.Join(envRoot, "bin") + assert.NoError(t, os.Mkdir(envBin, 0700)) + attackerGit := filepath.Join(envBin, "git") + assert.NoError(t, os.WriteFile(attackerGit, []byte("#!/bin/sh\nprintf attacker-controlled\n"), 0700)) + + ops, err := envars.MarshalOps(envars.Ops{&envars.Prepend{Name: "PATH", Value: envBin}}) + assert.NoError(t, err) + t.Setenv("HERMIT_ENV", envRoot) + t.Setenv("HERMIT_ENV_OPS", string(ops)) + t.Setenv("PATH", envBin+string(os.PathListSeparator)+hostDir) + + cmd, err := util.SystemCommand("git") + assert.NoError(t, err) + assert.Equal(t, hostGit, cmd.Path) + + out, err := cmd.Output() + assert.NoError(t, err) + assert.Equal(t, "host:"+hostDir, string(out)) +} + +func TestSystemCommandFailsClosedForInvalidHermitEnvOps(t *testing.T) { + t.Setenv("HERMIT_ENV_OPS", "not JSON") + + _, err := util.SystemCommand("git") + assert.Error(t, err) + assert.True(t, strings.Contains(err.Error(), "failed to restore PATH before Hermit activation")) +} + +func TestSystemCommandRejectsPaths(t *testing.T) { + path := filepath.Join(t.TempDir(), "git") + _, err := util.SystemCommand(path) + assert.EqualError(t, err, `system command must be a bare name: "`+path+`"`) +}