Prevent activated environments from hijacking Hermit tools

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())

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))

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)
 		}
 	}

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 <<EOF
+env = {}
+sources = ["$PWD/source.git"]
+EOF
+				rm -f HOST_GIT_USED.txt
+
+				./bin/hermit search safehelper
+				assert test -e HOST_GIT_USED.txt
+			`,
+			expectations: exp{outputContains("safehelper")},
+		},
+		{
+			// Regression test for DX-29: after activation, internal tools must
+			// ignore the repository bin directory and use the pre-activation PATH.
+			name:         "InternalToolsRestorePathAfterActivation",
+			preparations: prep{gitSourceWithHostGit()},
+			script: `
+				hermit init --no-git .
+				cat > bin/hermit.hcl <<EOF
+env = {}
+sources = ["$PWD/source.git"]
+EOF
+				cat > 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 {

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)
 	// <oid> TAB <ref> 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)
 	}

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,21 +25,99 @@ 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.
 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...))
 	cmd := exec.Command(args[0], args[1:]...) //nolint:gosec,noctx
 	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