diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a750c4c..00692bb 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -29,3 +29,7 @@ jobs: # Includes the interop tests against the runner's git. - run: go test ./... + + # The trust tests give files to another user, which needs root. + - if: runner.os != 'Windows' + run: sudo -E env "PATH=$PATH" go test -count=1 -run Trust ./... diff --git a/discover.go b/discover.go index c97fb54..b2d0b08 100644 --- a/discover.go +++ b/discover.go @@ -3,6 +3,7 @@ package gitconfig import ( "errors" "fmt" + "io/fs" "os" "path/filepath" "runtime" @@ -24,7 +25,7 @@ func get(env environ, key string) string { return v } -func discover(gitDir string, env environ) (Env, error) { +func discover(dir string, env environ) (Env, error) { var e Env e.Home = get(env, "HOME") @@ -77,23 +78,74 @@ func discover(gitDir string, env environ) (Env, error) { } e.Command = append(cmd, params...) - if gitDir != "" { - e.GitDir, err = resolveGitDir(gitDir) - if err != nil { + if dir != "" { + var owned []string + if e.GitDir, owned, err = findRepo(dir); err != nil { + return Env{}, err + } + if err := e.checkTrust(owned, get(env, "SUDO_UID")); err != nil { return Env{}, err } } return e, nil } -// resolveGitDir makes gitDir absolute and follows a "gitdir: " file. -func resolveGitDir(gitDir string) (string, error) { - gitDir, err := filepath.Abs(gitDir) - if err != nil { - return "", err +// findRepo returns the git directory of the repository at dir, as git finds +// it when started there, and the paths whose owner git checks. In a working +// tree (dir holds .git, or is a "gitdir:" file), they are the working tree, +// its .git and the git directory; elsewhere, dir alone. The first is the +// path safe.directory lists. +func findRepo(dir string) (gitDir string, owned []string, err error) { + if dir, err = filepath.Abs(dir); err != nil { + return "", nil, err + } + dot := filepath.Join(dir, ".git") + if fi, err := os.Stat(dir); err == nil && fi.Mode().IsRegular() { + dir, dot = filepath.Dir(dir), dir + } else if _, err := os.Stat(dot); errors.Is(err, fs.ErrNotExist) { + // Also a directory that does not exist (yet): loading finds no + // files in it. + return dir, []string{dir}, nil + } else if err != nil { + return "", nil, &FileError{Path: dot, Err: err} } - // A directory is the git directory itself. One that does not exist (yet) - // is not an error here: loading finds no files in it. + gitDir, err = resolveGitDir(dot) + return gitDir, []string{dir, dot, gitDir}, err +} + +// checkTrust fails with ErrUntrusted unless the current user owns every +// path of owned that exists, or safe.directory lists the first. +func (e Env) checkTrust(owned []string, sudoUID string) error { + for _, p := range owned { + ok, err := ownedByCurrentUser(p, sudoUID) + if errors.Is(err, fs.ErrNotExist) { + continue + } + if err != nil { + return &FileError{Path: p, Err: err} + } + if ok { + continue + } + // Without the repository, as git: its HEAD and path must not + // match an includeIf that lists it. + p := e + p.GitDir = "" + c, err := p.LoadProtected() + if err != nil { + return err + } + if !safeDirectory(c, owned[0], e.Home) { + return &FileError{Path: owned[0], Err: ErrUntrusted} + } + return nil + } + return nil +} + +// resolveGitDir follows a "gitdir: " file; any other path is the git +// directory itself. gitDir is absolute. +func resolveGitDir(gitDir string) (string, error) { fi, err := os.Stat(gitDir) if err != nil || !fi.Mode().IsRegular() { return gitDir, nil diff --git a/doc.go b/doc.go index d69dfaa..bec0bf8 100644 --- a/doc.go +++ b/doc.go @@ -13,8 +13,8 @@ // // Reading the effective configuration of a repository and changing a value: // -// env, err := gitconfig.Discover(".git") -// if err != nil { ... } +// env, err := gitconfig.Discover(".") // the working tree +// if err != nil { ... } // ErrUntrusted, as git refuses it // // cfg, err := env.Load() // if err != nil { ... } diff --git a/docs/COMPATIBILITY.md b/docs/COMPATIBILITY.md index 89314e6..ae35b0f 100644 --- a/docs/COMPATIBILITY.md +++ b/docs/COMPATIBILITY.md @@ -73,7 +73,12 @@ in [GIT-BEHAVIOUR.md](GIT-BEHAVIOUR.md). | System file | set at build time (`$(prefix)/etc/gitconfig`) | `/etc/gitconfig` on Unix, none elsewhere | a git built with another prefix reads another file; callers can set `SystemConfig` | | `safe.directory` entry that does not resolve | skipped | the same | 1 | | `safe.directory = .` | trusted at the top of a repository | skipped | needs the working directory — *safe default* | -| Repository ownership check | done by git | left to the caller | platform specific (X5) | +| Repository ownership check | done when git finds the repository, not for `GIT_DIR` | done by `Discover`, not for an `Env` filled by hand | 1 (X5) | +| What is checked, and listed in `safe.directory` | depends on where git starts: a working tree, or a git directory | the same, from the path given to `Discover` | 1 — the caller should pass the working tree, which users list | +| Untrusted repository | `git config` silently skips the local scope; other commands fail | `Discover` fails with `ErrUntrusted` | a silently missing scope surprises; reading less is not safer than refusing — *safe default* | +| `SUDO_UID` = `' 1000'`, `+1000`, or beyond 32 bits | accepted; a value beyond 32 bits wraps around (2³² + 1000 is 1000) | ignored: digits only, at most 32 bits | the wrap-around trusts a user nobody named — *safe default* | +| Administrators group on Windows | trusted for an administrator (Git for Windows commit messages, 2022 and 2025) | the same | 1 — not observed: no Windows git to run but CI's | +| UNC paths on Windows | must be listed in `safe.directory` since Git for Windows 2.54 (release notes) | owner checked like any path | the release note is not precise enough to follow; not observed | | Paths on Windows | some written with `/` (`HOME`), some with `\` | the caller's separators, kept | matching git's spelling serves no one | ## Editing diff --git a/docs/GIT-BEHAVIOUR.md b/docs/GIT-BEHAVIOUR.md index c76db62..f4c31a8 100644 --- a/docs/GIT-BEHAVIOUR.md +++ b/docs/GIT-BEHAVIOUR.md @@ -239,3 +239,64 @@ the same results. | `hasconfig:remote.*.url:https://example.com/**` | no until a matching remote is configured, then yes | | `git -c 'includeIf.gitdir:./.path=…'` | `error: relative config include conditionals must come from files`, then git goes on without the include, exit 0 *(2.47.3)* — this library fails with `ErrNotAbsolute`, having no one to warn | | `gitdir:C:/x/link/`, `GIT_DIR=C:\x\link\proj\.git` with `link` a symbolic link to `work` (Windows) | **no**, while `gitdir:C:/x/work/` (the real path) matches *(2.55.0.windows.5)* — this library compares both paths with `/` and matches | + +## Ownership and `safe.directory` + +Observed with **git 2.56.0** (`alpine:edge`, Linux), as user `alice` (uid +1000) unless stated, `bob` being uid 1001. "Refused" means `fatal: detected +dubious ownership in repository at ''`, exit 128. The repository is +`/r/w` (working tree) with `.git` inside, all owned by `alice` except the +path named. + +| Owned by `bob` | Result | +|---|---| +| nothing | accepted | +| `/r/w` (top of the working tree) | refused, at `/r/w` — also when run from a subdirectory owned by `alice` | +| `/r/w/.git` (directory) | refused, at `/r/w` | +| `/r/w/.git` as a `gitdir:` file | refused, at `/r/w` | +| the directory a `gitdir:` file points to | refused, at `/r/w` | +| `/r/w/.git/config` | accepted: files inside the git directory are not checked | +| `/r` (parent of the working tree) | accepted | +| a symbolic link to `/r/w` (`chown -h`), used as `-C` | accepted: only the target counts | +| a bare repository `/r/b.git` | refused, at `/r/b.git` | +| `/r/b.git/config` in a bare repository | accepted | +| linked worktree `/r/lw`: `/r/w/.git/worktrees/lw` | refused, at `/r/lw` | +| linked worktree `/r/lw`: `/r/w/.git` or `/r/w` | accepted: the main repository is not checked | +| `/r/w`, mode 777, or group `alice` | refused: only the owner uid counts | + +| Setup | Result | +|---|---| +| `/r/w/.git` owned by `bob`, `-c safe.directory=/r/w` | accepted | +| `/r/w/.git` owned by `bob`, `-c safe.directory=/r/w/.git` | refused: the entry must name the working tree, whichever path failed | +| `gitdir:` target owned by `bob`, `safe.directory` = the working tree / the target | accepted / refused | +| bare `/r/b.git` owned by `bob`, `safe.directory=/r/b.git` | accepted | +| `/r/w` owned by `bob`, `GIT_DIR=/r/w/.git` (with or without `GIT_WORK_TREE`) | accepted: nothing is checked when the repository is given rather than discovered | +| `/r/w` owned by `bob`, `git config --show-scope --get-regexp foo` (`foo.bar` in `.git/config`) | exit 1, no output: the repository is treated as absent, its configuration silently skipped | +| the same, `git config --local --list` | `fatal: --local can only be used inside a git repository`, exit 128 | +| the same, `git rev-parse --show-toplevel` | refused | +| as root, repository owned by `alice`, no `SUDO_UID` | refused | +| as root, owned by `alice`, `SUDO_UID=1000` | accepted | +| as root, owned by `alice`, `SUDO_UID=1001` or `SUDO_UID=abc` | refused | +| as root, owned by root, `SUDO_UID=1000` | accepted: root is still trusted | +| as `alice`, owned by `bob`, `SUDO_UID=1001` | refused: `SUDO_UID` counts only for root | +| as root, owned by `alice`, `SUDO_UID` = `' 1000'`, `+1000`, `01000` | accepted | +| as root, owned by `alice`, `SUDO_UID` = `'1000 '`, `1000x` | refused | +| as root, owned by `alice`, `SUDO_UID=4294968296` (2³² + 1000) | accepted: the value wraps around to 1000 | + +What is checked depends on where git starts, not on the layout of the +repository: started in a working tree, git checks the working tree, its +`.git` and the git directory, and `safe.directory` must name the working +tree; started in a git directory, it checks that directory alone, and +`safe.directory` must name it. + +| Setup (`bob` owns the path named) | Result | +|---|---| +| normal repository, `git -C /r/w/.git`, `/r/w` owned by `bob` | accepted: the working tree is not checked | +| normal repository, `git -C /r/w/.git`, `.git` owned by `bob`, `safe.directory=/r/w` / `=/r/w/.git` | refused, at `/r/w/.git` / accepted | +| bare repository named `.git` at `/r/p/.git`, `/r/p` owned by `bob`, `git -C /r/p/.git` | accepted | +| the same, `git -C /r/p` | refused, at `/r/p` (git looks for `/r/p/.git` from there) | +| bare `/r/p/.git` owned by `bob`, `safe.directory=/r/p/.git` / `=/r/p` | accepted / refused, at `/r/p/.git` | +| bare `/r/dots` with `core.worktree=/r/home` (and `core.bare=false`), `/r/home` owned by `bob` | accepted: `core.worktree` is not checked | +| the same, `/r/dots` owned by `bob`, `safe.directory=/r/dots` / `=/r/home` | accepted / refused, at `/r/dots` | +| `--separate-git-dir=/r/sep/x` for `/r/w`, `/r/w` owned by `bob`, `git -C /r/sep/x` | accepted | +| linked worktree `/r/lw` owned by `bob`, `git -C /r/w/.git/worktrees/lw` / `git -C /r/lw` | accepted / refused, at `/r/lw` | diff --git a/docs/INTEGRATION.md b/docs/INTEGRATION.md index 213fa72..53ae7eb 100644 --- a/docs/INTEGRATION.md +++ b/docs/INTEGRATION.md @@ -25,9 +25,11 @@ Usage outside `repository/`: `ReadString` ×5, `RemoveAll` ×4, `ReadAll` ×3, ### Mapping -`env, err := gitconfig.Discover(gitDir)` once per repository (a `.git` -file of a linked worktree is followed); the `Env` is a plain value and can be -kept. +`env, err := gitconfig.Discover(workTree)` once per repository (the +working tree, or the git directory of a bare repository; a `.git` file of a +linked worktree is followed); the `Env` is a plain value and can be kept. +`Discover` refuses a repository git would refuse (`ErrUntrusted`): git-bug +gets git's `safe.directory` protection without code of its own. | git-bug | gitconfig | |---|---| @@ -119,5 +121,6 @@ Adoption does not need the full RFC first: go-git-specific needs to keep in mind: `url.insteadOf`/`pushInsteadOf` (go-git#865, #844), `http..*` matching (go-git#474), `safe.directory` -(`Env.SafeDirectory`, plus an ownership check go-git must do itself), `core.sharedRepository` permissions on written files +and the ownership check (done by `Discover`; go-git must pass the working +tree, and not build the `Env` by hand for a repository it found), `core.sharedRepository` permissions on written files (go-git#843, #1572) — set by go-git's own write path. diff --git a/docs/REQUIREMENTS.md b/docs/REQUIREMENTS.md index 12e6838..2f49d83 100644 --- a/docs/REQUIREMENTS.md +++ b/docs/REQUIREMENTS.md @@ -92,7 +92,7 @@ and callers must be able to tell whether to trust it. | X2 | Parsing untrusted input never panics and is linear in time and memory. Parser and edits are fuzzed continuously. | MUST | | X3 | Bounded loads: git's include depth limit, plus at most 1000 files and 64 MiB read by one load, counting every read (an include fan-out of 10 per level makes git read 10ⁿ files from a few KB, *verified*); configuration files and includes must be regular files (git hangs on a FIFO and fails on a directory, *verified*). A deliberate improvement over git, reached only by files built to reach it. | MUST | | X4 | Errors do not leak or spoof: a syntax error never quotes the file's content (an include may point at a secret file), values in messages are quoted with `%q` (no terminal escape sequences). | MUST | -| X5 | Trust is decidable before the repository is read: `Env.LoadProtected` loads only the protected scopes (system, global, command; never `Parse` content or an explicit file) and `Env.SafeDirectory` applies git's `safe.directory` matching to them, without reading the repository's configuration. Each entry's scope is known. The ownership check and the policy belong to the caller. | MUST | +| X5 | Trust is checked before the repository is read, as git does: `Discover` refuses (`ErrUntrusted`) a repository that is neither owned by the current user (on Unix, also the `SUDO_UID` user for root; on Windows, the Administrators group for an administrator) nor listed in `safe.directory`, with git's rules for which paths are checked and listed. `safe.directory` is read from the protected scopes only (`Env.LoadProtected`: system, global, command; never `Parse` content or an explicit file), never from the repository's configuration. An `Env` filled by hand is not checked, as git does not check `GIT_DIR`. Each entry's scope is known. *verified* | MUST | | X6 | Writes cannot be redirected through the lock file: it is created with `O_CREATE\|O_EXCL` and never through a symbolic link. Following a link at the config path itself is git's behaviour (W7) and is kept; the documentation says that only X5 protects against a hostile `.git` directory. | MUST | ## 7. Won't diff --git a/env.go b/env.go index c7ec6d4..1791579 100644 --- a/env.go +++ b/env.go @@ -55,9 +55,10 @@ func (s Scope) String() string { // // Configuration can come from someone else: a repository on a shared disk, // an unpacked archive, a file reached through include.path. Loading such a -// file is safe (bounded, see Load), but its values are not to be trusted: -// check SafeDirectory before loading or editing a repository's -// configuration, as git does. +// file is safe (bounded, see Load), but its values are not to be trusted. +// Discover refuses a repository git would refuse (ErrUntrusted); an Env +// filled by hand is not checked, as git does not check a repository named +// by GIT_DIR. type Env struct { // Home is the user's home directory. Load uses it to expand "~/" in // include paths; pass it to Config.Path and Entry.Path for values. @@ -104,13 +105,20 @@ type CommandEntry struct { // XDG_CONFIG_HOME, …), the user's home directory, the working directory and // the platform's system configuration file. // -// gitDir is the repository's git directory, such as ".git", or the -// repository itself for a bare one; it is made absolute. If gitDir is a file -// holding "gitdir: ", as .git is in a linked worktree or a submodule, -// the path it names is used. Finding the repository is left to the caller: -// Discover does not search parent directories and does not read GIT_DIR. -// Pass "" when not in a repository: only the system, global and command-line -// scopes exist then. +// dir is where the repository is found, as git finds it when started there +// (git -C dir): a working tree (a directory holding .git), its .git, or a +// git directory, such as a bare repository; it is made absolute. A .git +// file holding "gitdir: ", as in a linked worktree or a submodule, is +// followed. Finding the repository is left to the caller: Discover does not +// search parent directories and does not read GIT_DIR. Pass "" when not in +// a repository: only the system, global and command-line scopes exist then. +// +// The repository must be trusted, as git requires: owned by the current +// user (on Unix, also the SUDO_UID user for root; on Windows, the +// Administrators group for an administrator), or listed in safe.directory. +// Otherwise Discover fails with ErrUntrusted, in a *FileError naming the +// path to list. As in git, that path is the working tree when dir is one, +// dir itself otherwise: pass the working tree when there is one. // // Without a home directory, Home is "" and the global files are those git // reads then: the XDG file if XDG_CONFIG_HOME is set, no ~/.gitconfig. @@ -125,8 +133,8 @@ type CommandEntry struct { // // It is the only function of this package that reads the process // environment. -func Discover(gitDir string) (Env, error) { - return discover(gitDir, osEnviron{}) +func Discover(dir string) (Env, error) { + return discover(dir, osEnviron{}) } // Load returns the effective configuration: system, global, local, worktree @@ -201,7 +209,7 @@ func (l *loader) global() error { // includes followed: system, global and command line, in that order. This is // what git uses for keys a repository must not be able to set, such as // safe.directory or safe.bareRepository. The repository's configuration is -// not read, so this is safe to call before SafeDirectory. +// not read. func (e Env) LoadProtected() (*Config, error) { l, err := e.newLoader() if err != nil { @@ -219,21 +227,11 @@ func (e Env) LoadProtected() (*Config, error) { return l.config(), nil } -// SafeDirectory reports whether dir is allowed by the safe.directory values -// of LoadProtected, with git's matching rules ("*", a trailing "/*", an empty -// value resetting the list). dir is the repository's working tree, or its -// git directory for a bare repository, as git checks. It may be relative to -// the working directory; symbolic links are resolved on both sides. -// -// The repository's configuration is not read. It does not check who owns -// dir: git trusts a repository owned by the current user without consulting -// safe.directory, and that check is platform specific and left to the -// caller. A false result therefore means "not listed", not "unsafe". -func (e Env) SafeDirectory(dir string) (bool, error) { - c, err := e.LoadProtected() - if err != nil { - return false, err - } +// safeDirectory reports whether dir is allowed by the safe.directory values +// of c, with git's matching rules ("*", a trailing "/*", an empty value +// resetting the list). dir may be relative to the working directory; +// symbolic links are resolved on both sides. +func safeDirectory(c *Config, dir, home string) bool { dir = realPath(dir) if !filepath.IsAbs(dir) { // The entries that count are absolute. Joining the real working @@ -250,7 +248,7 @@ func (e Env) SafeDirectory(dir string) (bool, error) { case v == "*": safe = true case strings.HasSuffix(v, "/*"): - prefix, err := expandPath(strings.TrimSuffix(v, "*"), e.Home) + prefix, err := expandPath(strings.TrimSuffix(v, "*"), home) if err != nil || !filepath.IsAbs(prefix) { // As for exact entries, relative ones are skipped. continue @@ -261,7 +259,7 @@ func (e Env) SafeDirectory(dir string) (bool, error) { } } default: - p, err := expandPath(v, e.Home) + p, err := expandPath(v, home) if err != nil || !filepath.IsAbs(p) { // "." and unexpandable values are skipped. continue @@ -271,7 +269,7 @@ func (e Env) SafeDirectory(dir string) (bool, error) { } } } - return safe, nil + return safe } // realPath returns path with symbolic links resolved, or cleaned if it @@ -446,7 +444,7 @@ func (e Env) UpdateScope(s Scope, fn func(*File) error) error { // Following the symbolic link in step 1 is what git does, and what dotfile // managers rely on. It also means that whoever controls the directory of // path controls which file is written: do not edit the configuration of a -// repository you have not checked with SafeDirectory and an ownership check. +// repository that Discover has not accepted. func (e Env) UpdateFile(path string, fn func(*File) error) error { return updateFile(path, ScopeFile, fn) } diff --git a/env_test.go b/env_test.go index c8a5e7f..ea37316 100644 --- a/env_test.go +++ b/env_test.go @@ -827,85 +827,129 @@ func TestEnvLoadProtected(t *testing.T) { }) } -func TestEnvSafeDirectory(t *testing.T) { - // The directories exist: SafeDirectory skips a listed path it cannot - // resolve, and /srv only exists on some systems. Values are written - // with '/', as a backslash is an escape in a configuration file. - root := tempDir(t) - for _, d := range []string{"a/b", "ab"} { - require.NoError(t, os.MkdirAll(filepath.Join(root, d), 0o755)) +// otherUID owns the paths a trustCase gives to someone else. +const otherUID = 1001 + +// requireRoot skips the test unless it can give files to another user. +func requireRoot(t *testing.T) { + t.Helper() + if runtime.GOOS == "windows" || os.Geteuid() != 0 { + t.Skip("needs root, to give files to another user") } - s := filepath.ToSlash(root) - a := filepath.Join(root, "a") +} - for _, tc := range []struct { - name string - global string - local string - cmd []gitconfig.CommandEntry - dir string - want bool - }{ - {"not listed", "", "", nil, a, false}, - {"listed", "[safe]\n\tdirectory = " + s + "/a\n", "", nil, a, true}, - {"other directory listed", "[safe]\n\tdirectory = " + s + "/a\n", "", nil, filepath.Join(root, "a", "b"), false}, - {"no prefix match", "[safe]\n\tdirectory = " + s + "/a\n", "", nil, filepath.Join(root, "ab"), false}, - {"star", "[safe]\n\tdirectory = *\n", "", nil, a, true}, - {"trailing star", "[safe]\n\tdirectory = " + s + "/*\n", "", nil, filepath.Join(root, "a", "b"), true}, - {"trailing star, the directory itself", "[safe]\n\tdirectory = " + s + "/*\n", "", nil, root, false}, - {"empty value resets", "[safe]\n\tdirectory = *\n\tdirectory =\n", "", nil, a, false}, - {"listed after a reset", "[safe]\n\tdirectory =\n\tdirectory = " + s + "/a\n", "", nil, a, true}, - {"repository config ignored", "", "[safe]\n\tdirectory = *\n", nil, a, false}, - {"command line", "", "", []gitconfig.CommandEntry{{Key: "safe.directory", Value: s + "/a"}}, a, true}, - {"command line resets the global list", "[safe]\n\tdirectory = *\n", "", []gitconfig.CommandEntry{{Key: "safe.directory", Value: ""}}, a, false}, - } { - t.Run(tc.name, func(t *testing.T) { - dir := tempDir(t) - gitDir := filepath.Join(dir, ".git") - initRepo(t, gitDir, tc.local) - global := filepath.Join(dir, "global") - writeFile(t, global, tc.global) +// trustCase is a case of the ownership check, as recorded in +// docs/GIT-BEHAVIOUR.md, on the repositories trustLayout builds. Paths are +// relative to its root. +type trustCase struct { + name string + start string // the directory given to Discover, or to git -C + other []string // paths owned by otherUID + safe string // safe.directory in the global file, "" for none + cond string // the includeIf condition safe is under, "" for none + sudoUID string // SUDO_UID, "" for unset + refused string // the path to list, "" if trusted +} - got, err := gitconfig.Env{GitDir: gitDir, GlobalConfig: []string{global}, Command: tc.cmd}.SafeDirectory(tc.dir) - require.NoError(t, err) - assert.Equal(t, tc.want, got) - }) +// trustLayout builds a working tree w with a linked worktree lw, a working +// tree g whose .git file points to gd/x.git, and bare repositories b.git +// and p/.git. w lists every directory in its own safe.directory, which git +// ignores. +func trustLayout(t *testing.T, root string) { + for _, d := range []string{"w/.git", "gd/x.git", "b.git", "p/.git"} { + initRepo(t, filepath.Join(root, d), "[safe]\n\tdirectory = *\n") + for _, sub := range []string{"objects", "refs"} { + require.NoError(t, os.MkdirAll(filepath.Join(root, d, sub), 0o755)) + } } + writeFile(t, filepath.Join(root, "g", ".git"), "gitdir: "+filepath.Join(root, "gd", "x.git")+"\n") + wt := addWorktree(t, filepath.Join(root, "w", ".git"), "lw", "other") + writeFile(t, filepath.Join(root, "lw", ".git"), "gitdir: "+wt+"\n") +} - t.Run("relative directory", func(t *testing.T) { - // Checked against git 2.56: "../*" trusts nothing, as "../repo" - // does not. A relative dir is taken from the working directory. - t.Chdir(a) - for _, tc := range []struct { - entry string - dir string - want bool - }{ - {s + "/a", ".", true}, - {s + "/a/b", "b", true}, - {s + "/ab", "../ab", true}, - {s + "/*", "../ab", true}, - {"../*", "../ab", false}, - {"../ab", "../ab", false}, - } { - t.Run(tc.entry+" "+tc.dir, func(t *testing.T) { - global := filepath.Join(tempDir(t), "global") - writeFile(t, global, "[safe]\n\tdirectory = "+tc.entry+"\n") - got, err := gitconfig.Env{GlobalConfig: []string{global}}.SafeDirectory(tc.dir) - require.NoError(t, err) - assert.Equal(t, tc.want, got) - }) +var trustCases = []trustCase{ + {name: "all owned", start: "w"}, + {name: "all owned, started in .git", start: "w/.git"}, + {name: "gitdir file, all owned", start: "g"}, + {name: "bare, all owned", start: "b.git"}, + {name: "bare named .git, all owned", start: "p/.git"}, + {name: "linked worktree, all owned", start: "lw"}, + {name: "working tree owned by another", start: "w", other: []string{"w"}, refused: "w"}, + {name: ".git owned by another", start: "w", other: []string{"w/.git"}, refused: "w"}, + {name: "config owned by another", start: "w", other: []string{"w/.git/config"}}, + {name: "parent owned by another", start: "w", other: []string{"."}}, + {name: "working tree listed", start: "w", other: []string{"w/.git"}, safe: "w"}, + {name: ".git listed instead of the working tree", start: "w", other: []string{"w/.git"}, safe: "w/.git", refused: "w"}, + {name: "every directory listed", start: "w", other: []string{"w"}, safe: "*"}, + {name: "started in .git, working tree owned by another", start: "w/.git", other: []string{"w"}}, + {name: "started in .git, .git owned by another", start: "w/.git", other: []string{"w/.git"}, refused: "w/.git"}, + {name: "started in .git, working tree listed", start: "w/.git", other: []string{"w/.git"}, safe: "w", refused: "w/.git"}, + {name: "started in .git, .git listed", start: "w/.git", other: []string{"w/.git"}, safe: "w/.git"}, + {name: "gitdir file, git directory owned by another", start: "g", other: []string{"gd/x.git"}, refused: "g"}, + {name: "gitdir file, git directory listed", start: "g", other: []string{"gd/x.git"}, safe: "gd/x.git", refused: "g"}, + {name: "gitdir file, working tree listed", start: "g", other: []string{"gd/x.git"}, safe: "g"}, + {name: "gitdir file owned by another", start: "g", other: []string{"g/.git"}, refused: "g"}, + {name: "bare, owned by another", start: "b.git", other: []string{"b.git"}, refused: "b.git"}, + {name: "bare, listed", start: "b.git", other: []string{"b.git"}, safe: "b.git"}, + {name: "bare, config owned by another", start: "b.git", other: []string{"b.git/config"}}, + {name: "bare named .git, parent owned by another", start: "p/.git", other: []string{"p"}}, + {name: "bare named .git, owned by another, parent listed", start: "p/.git", other: []string{"p/.git"}, safe: "p", refused: "p/.git"}, + {name: "linked worktree, its git directory owned by another", start: "lw", other: []string{"w/.git/worktrees/lw"}, refused: "lw"}, + {name: "linked worktree, main .git owned by another", start: "lw", other: []string{"w/.git"}}, + {name: "linked worktree, main working tree owned by another", start: "lw", other: []string{"w"}}, + {name: "SUDO_UID owns it", start: "w", other: []string{"w", "w/.git"}, sudoUID: strconv.Itoa(otherUID)}, + {name: "SUDO_UID, root owns it", start: "w", sudoUID: strconv.Itoa(otherUID)}, + {name: "SUDO_UID not a number", start: "w", other: []string{"w"}, sudoUID: "abc", refused: "w"}, + {name: "listed under onbranch:", start: "w", other: []string{"w"}, safe: "*", cond: "onbranch:main", refused: "w"}, + {name: "listed under gitdir:", start: "w", other: []string{"w"}, safe: "*", cond: "gitdir:w/", refused: "w"}, +} + +// prepare builds trustLayout in a clean environment, gives the paths of +// tc.other to otherUID and writes the global file, and returns the root. +// Under tc.cond, safe is in a file the global file includes. +func (tc trustCase) prepare(t *testing.T) string { + t.Helper() + home := cleanGitEnv(t) + t.Setenv("GIT_CONFIG_NOSYSTEM", "1") + root := tempDir(t) + trustLayout(t, root) + for _, p := range tc.other { + require.NoError(t, os.Chown(filepath.Join(root, p), otherUID, otherUID)) + } + if tc.safe != "" { + v := tc.safe + if v != "*" { + v = filepath.ToSlash(filepath.Join(root, v)) } - }) + safe := "[safe]\n\tdirectory = " + v + "\n" + if tc.cond != "" { + writeFile(t, filepath.Join(home, "safe.inc"), safe) + safe = "[includeIf \"" + tc.cond + "\"]\n\tpath = safe.inc\n" + } + writeFile(t, filepath.Join(home, ".gitconfig"), safe) + } + if tc.sudoUID != "" { + t.Setenv("SUDO_UID", tc.sudoUID) + } + return root +} - t.Run("listed through an include", func(t *testing.T) { - dir := tempDir(t) - writeFile(t, filepath.Join(dir, "global"), "[include]\n\tpath = inc\n") - writeFile(t, filepath.Join(dir, "inc"), "[safe]\n\tdirectory = "+s+"/a\n") - got, err := gitconfig.Env{GlobalConfig: []string{filepath.Join(dir, "global")}}.SafeDirectory(a) - require.NoError(t, err) - assert.True(t, got) - }) +func TestDiscoverTrust(t *testing.T) { + requireRoot(t) + for _, tc := range trustCases { + t.Run(tc.name, func(t *testing.T) { + root := tc.prepare(t) + _, err := gitconfig.Discover(filepath.Join(root, tc.start)) + if tc.refused == "" { + require.NoError(t, err) + return + } + require.ErrorIs(t, err, gitconfig.ErrUntrusted) + fe, ok := errors.AsType[*gitconfig.FileError](err) + require.True(t, ok, "got %T", err) + assert.Equal(t, filepath.Join(root, tc.refused), fe.Path) + }) + } } func TestEnvWritePath(t *testing.T) { @@ -975,6 +1019,7 @@ func cleanGitEnv(t *testing.T) string { } } unsetenv(t, "XDG_CONFIG_HOME") + unsetenv(t, "SUDO_UID") home := tempDir(t) t.Setenv("HOME", home) t.Setenv("USERPROFILE", home) @@ -1159,6 +1204,23 @@ func TestDiscover(t *testing.T) { assert.Equal(t, gitDir, env.GitDir) }) + t.Run("working tree", func(t *testing.T) { + home := cleanGitEnv(t) + gitDir := filepath.Join(home, "repo", ".git") + initRepo(t, gitDir, "") + env, err := gitconfig.Discover(filepath.Join(home, "repo")) + require.NoError(t, err) + assert.Equal(t, gitDir, env.GitDir) + }) + + t.Run("missing directory", func(t *testing.T) { + // Nothing to check, nothing to load. + home := cleanGitEnv(t) + env, err := gitconfig.Discover(filepath.Join(home, "missing")) + require.NoError(t, err) + assert.Equal(t, filepath.Join(home, "missing"), env.GitDir) + }) + t.Run("malformed gitdir file", func(t *testing.T) { home := cleanGitEnv(t) writeFile(t, filepath.Join(home, "wt", ".git"), "garbage\n") diff --git a/errors.go b/errors.go index 7161373..aeb2533 100644 --- a/errors.go +++ b/errors.go @@ -18,7 +18,8 @@ import ( // unwrapped, as no file is concerned. The error returned by the // callback of Env.UpdateFile or Env.UpdateScope is returned as is. // - Discover also returns an *EnvError for an environment variable it -// cannot interpret. +// cannot interpret, and ErrUntrusted in a *FileError for a repository +// git would refuse. var ( // ErrNotFound is returned when the section or subsection to rename is // absent. Removing something absent is not an error. @@ -61,6 +62,11 @@ var ( // in an Env. ErrNoScope = errors.New("gitconfig: scope not available") + // ErrUntrusted is returned by Discover for a repository git would + // refuse: not owned by the current user, and not listed in + // safe.directory. The *FileError's Path is the path to list. + ErrUntrusted = errors.New("gitconfig: repository not owned by the current user nor listed in safe.directory") + // ErrClosed is returned when a File is edited after the callback of // Env.UpdateFile or Env.UpdateScope has returned. ErrClosed = errors.New("gitconfig: file is closed") diff --git a/example_test.go b/example_test.go index 12a5710..c13f3d3 100644 --- a/example_test.go +++ b/example_test.go @@ -9,7 +9,7 @@ import ( // Read the effective configuration of a repository, as git would use it. func ExampleEnv_Load() { - env, err := gitconfig.Discover(".git") + env, err := gitconfig.Discover(".") if err != nil { log.Fatal(err) } @@ -22,7 +22,6 @@ func ExampleEnv_Load() { fmt.Println("user:", e.Value) } - // Missing key: the default. Malformed value ("maybe"): an error, as in git. bare, err := cfg.Bool("core.bare", false) if err != nil { log.Fatal(err) @@ -33,7 +32,7 @@ func ExampleEnv_Load() { // Change a value in the repository's config. Comments and layout are kept, // and the file is not touched if the value is already set. func ExampleEnv_UpdateScope() { - env, err := gitconfig.Discover(".git") + env, err := gitconfig.Discover(".") if err != nil { log.Fatal(err) } @@ -44,28 +43,3 @@ func ExampleEnv_UpdateScope() { log.Fatal(err) } } - -// Check that a repository may be trusted before reading its configuration, -// as git does. SafeDirectory reads only the system, global and command-line -// scopes; the ownership check is the caller's. -func ExampleEnv_SafeDirectory() { - env, err := gitconfig.Discover("/srv/shared/proj/.git") - if err != nil { - log.Fatal(err) - } - ok, err := env.SafeDirectory("/srv/shared/proj") - if err != nil { - log.Fatal(err) - } - if !ok && !ownedByCurrentUser("/srv/shared/proj") { - log.Fatal("unsafe repository") - } - - cfg, err := env.Load() - if err != nil { - log.Fatal(err) - } - fmt.Println("editor:", cfg.Value("core.editor", "vi")) -} - -func ownedByCurrentUser(dir string) bool { return false } // platform specific diff --git a/go.mod b/go.mod index cb51b06..8f32ac0 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,9 @@ module github.com/git-bug/gitconfig go 1.26.0 -require github.com/stretchr/testify v1.12.1 +require ( + github.com/stretchr/testify v1.12.1 + golang.org/x/sys v0.49.0 +) require go.yaml.in/yaml/v3 v3.0.5 // indirect diff --git a/go.sum b/go.sum index c233683..b1e5629 100644 --- a/go.sum +++ b/go.sum @@ -2,3 +2,5 @@ github.com/stretchr/testify v1.12.1 h1:EuwCh5fleGS7H32xRwO3wRGT7DxrDhLAT6FF8MpWD github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUmoNlxoYthg= go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= +golang.org/x/sys v0.49.0 h1:XbzkgYJHdqh/8m2Uu0W/dQv8nktxx4BFHp1M0gROTXA= +golang.org/x/sys v0.49.0/go.mod h1:hNLxWAXmnKAxqDtdwIYC4bM9oQPEecfsnNMuSxOs3og= diff --git a/interop_test.go b/interop_test.go index fb45641..47a426b 100644 --- a/interop_test.go +++ b/interop_test.go @@ -6,12 +6,14 @@ package gitconfig_test import ( "bytes" + "errors" "fmt" "math/rand/v2" "os" "os/exec" "path/filepath" "regexp" + "runtime" "slices" "strconv" "strings" @@ -1030,3 +1032,37 @@ func TestGitUpdate(t *testing.T) { }) } + +var dubious = regexp.MustCompile(`dubious ownership in repository at '([^']*)'`) + +// TestGitTrust checks that Discover trusts the repositories git trusts, and +// names the path git names when it refuses one. Without root, only the +// layouts owned by the current user are compared: on Windows, that covers a +// test run as an administrator, whose files belong to the Administrators +// group. +func TestGitTrust(t *testing.T) { + requireGit(t) + root := runtime.GOOS != "windows" && os.Geteuid() == 0 + for _, tc := range trustCases { + t.Run(tc.name, func(t *testing.T) { + if (len(tc.other) > 0 || tc.sudoUID != "") && !root { + t.Skip("needs root, to give files to another user") + } + dir := tc.prepare(t) + start := filepath.Join(dir, tc.start) + + res := runGit(t, dir, processEnv(), "-C", start, "rev-parse", "--git-dir") + _, err := gitconfig.Discover(start) + if res.code == 0 { + require.NoError(t, err, "git trusts it") + return + } + m := dubious.FindStringSubmatch(res.stderr) + require.NotNil(t, m, "git fails for another reason: %s", res.stderr) + require.ErrorIs(t, err, gitconfig.ErrUntrusted, "git refuses it: %s", res.stderr) + fe, ok := errors.AsType[*gitconfig.FileError](err) + require.True(t, ok, "got %T", err) + assertSamePath(t, m[1], fe.Path) + }) + } +} diff --git a/platform.go b/platform.go index 2a26e15..48a5648 100644 --- a/platform.go +++ b/platform.go @@ -1,54 +1,37 @@ +//go:build !windows + package gitconfig import ( - "errors" "os" - "runtime" + "strconv" "syscall" - "time" ) // openRegular opens a file for reading without blocking on a FIFO; the -// caller checks that it is a regular file. Windows has no FIFO to open by -// name. +// caller checks that it is a regular file. func openRegular(path string) (*os.File, error) { - flag := os.O_RDONLY - if runtime.GOOS != "windows" { - flag |= syscall.O_NONBLOCK - } - return os.OpenFile(path, flag, 0) + return os.OpenFile(path, os.O_RDONLY|syscall.O_NONBLOCK, 0) } -// Another process holding a file for a moment, as a concurrent writer or a -// reader (an editor, an antivirus) does, makes opening or replacing it fail -// on Windows. Such failures are retried for up to a second. -const retryFor = time.Second - -// Windows error codes for a file another process holds. -const ( - errorAccessDenied syscall.Errno = 5 - errorSharingViolation syscall.Errno = 32 - errorLockViolation syscall.Errno = 33 -) - -// transient reports whether err is a Windows failure to retry. -func transient(err error) bool { - return runtime.GOOS == "windows" && - (errors.Is(err, errorAccessDenied) || errors.Is(err, errorSharingViolation) || errors.Is(err, errorLockViolation)) +// retryTransient calls fn once: only Windows refuses to open or replace a +// file another process holds. +func retryTransient(fn func() error) error { + return fn() } -// retryTransient calls fn until it succeeds, fails for another reason, or -// a second has passed. Only Windows refuses to open or replace a file -// another process holds: elsewhere fn is called once. -func retryTransient(fn func() error) error { - deadline := time.Now().Add(retryFor) - delay := time.Millisecond - for { - err := fn() - if err == nil || !transient(err) || time.Now().After(deadline) { - return err - } - time.Sleep(delay) - delay = min(2*delay, 50*time.Millisecond) +// ownedByCurrentUser reports whether path belongs to the effective user or, +// running as root, to the user sudo recorded in SUDO_UID (sudoUID). +func ownedByCurrentUser(path, sudoUID string) (bool, error) { + fi, err := os.Stat(path) + if err != nil { + return false, err + } + owner := fi.Sys().(*syscall.Stat_t).Uid + euid := os.Geteuid() + if uint64(owner) == uint64(euid) { + return true, nil } + uid, err := strconv.ParseUint(sudoUID, 10, 32) + return euid == 0 && err == nil && uint64(owner) == uid, nil } diff --git a/platform_windows.go b/platform_windows.go new file mode 100644 index 0000000..46381c7 --- /dev/null +++ b/platform_windows.go @@ -0,0 +1,102 @@ +package gitconfig + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "time" + + "golang.org/x/sys/windows" +) + +// openRegular opens a file for reading; Windows has no FIFO to open by name. +func openRegular(path string) (*os.File, error) { + return os.Open(path) +} + +// Another process holding a file for a moment, as a concurrent writer or a +// reader (an editor, an antivirus) does, makes opening or replacing it fail. +// Such failures are retried for up to a second. +const retryFor = time.Second + +// Windows error codes for a file another process holds. +const ( + errorAccessDenied syscall.Errno = 5 + errorSharingViolation syscall.Errno = 32 + errorLockViolation syscall.Errno = 33 +) + +// retryTransient calls fn until it succeeds, fails for another reason, or +// a second has passed. +func retryTransient(fn func() error) error { + deadline := time.Now().Add(retryFor) + delay := time.Millisecond + for { + err := fn() + transient := errors.Is(err, errorAccessDenied) || errors.Is(err, errorSharingViolation) || errors.Is(err, errorLockViolation) + if err == nil || !transient || time.Now().After(deadline) { + return err + } + time.Sleep(delay) + delay = min(2*delay, 50*time.Millisecond) + } +} + +// ownedByCurrentUser reports whether path belongs to the current user. The +// Administrators group counts as the user if the user is an administrator, +// elevated or not: files an elevated process creates belong to that group. +// sudoUID is not used. +func ownedByCurrentUser(path, sudoUID string) (bool, error) { + // The owner checked is the link target's, as with os.Stat elsewhere. + // GetNamedSecurityInfo follows links itself; resolving them first + // keeps that from resting on its behaviour. + path, err := filepath.EvalSymlinks(path) + if err != nil { + return false, err + } + sd, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.OWNER_SECURITY_INFORMATION) + if err != nil { + return false, err + } + owner, _, err := sd.Owner() + if err != nil || owner == nil { + return false, nil // no owner recorded, as on FAT32 + } + token := windows.GetCurrentProcessToken() + user, err := token.GetTokenUser() + if err != nil { + return false, err + } + if owner.Equals(user.User.Sid) { + return true, nil + } + if !owner.IsWellKnown(windows.WinBuiltinAdministratorsSid) { + return false, nil + } + admins, err := windows.CreateWellKnownSid(windows.WinBuiltinAdministratorsSid) + if err != nil { + return false, err + } + // Token 0 is the calling thread's: elevated. Not elevated, an + // administrator's token is linked to an elevated one. + if ok, err := windows.Token(0).IsMember(admins); err == nil && ok { + return true, nil + } + linked, err := token.GetLinkedToken() + if err != nil { + return false, nil + } + defer linked.Close() + // IsMember needs an impersonation token. Without SeTcbPrivilege the + // linked token already is one, but a caller holding it gets a primary + // token: duplicating covers both. Should duplicating fail, the linked + // token is tried as is. + var imp windows.Token + if err := windows.DuplicateTokenEx(linked, windows.TOKEN_QUERY, nil, windows.SecurityIdentification, windows.TokenImpersonation, &imp); err == nil { + defer imp.Close() + linked = imp + } + ok, err := linked.IsMember(admins) + return err == nil && ok, nil +} diff --git a/safedir_test.go b/safedir_test.go new file mode 100644 index 0000000..6075a69 --- /dev/null +++ b/safedir_test.go @@ -0,0 +1,70 @@ +package gitconfig + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestSafeDirectory tests git's safe.directory matching, which Discover +// applies to a repository the user does not own. The directories exist: a +// listed path that cannot be resolved is skipped. Values are written with +// '/', as a backslash is an escape in a configuration file. +func TestSafeDirectory(t *testing.T) { + root, err := filepath.EvalSymlinks(t.TempDir()) + require.NoError(t, err) + for _, d := range []string{"a/b", "ab"} { + require.NoError(t, os.MkdirAll(filepath.Join(root, d), 0o755)) + } + s := filepath.ToSlash(root) + a := filepath.Join(root, "a") + + check := func(t *testing.T, config, dir string, want bool) { + t.Helper() + f, err := Parse("global", []byte("[safe]\n"+config)) + require.NoError(t, err) + assert.Equal(t, want, safeDirectory(f.Config(), dir, "")) + } + + for _, tc := range []struct { + name string + config string + dir string + want bool + }{ + {"not listed", "", a, false}, + {"listed", "directory = " + s + "/a\n", a, true}, + {"other directory listed", "directory = " + s + "/a\n", filepath.Join(a, "b"), false}, + {"no prefix match", "directory = " + s + "/a\n", filepath.Join(root, "ab"), false}, + {"star", "directory = *\n", a, true}, + {"trailing star", "directory = " + s + "/*\n", filepath.Join(a, "b"), true}, + {"trailing star, the directory itself", "directory = " + s + "/*\n", root, false}, + {"empty value resets", "directory = *\ndirectory =\n", a, false}, + {"listed after a reset", "directory =\ndirectory = " + s + "/a\n", a, true}, + } { + t.Run(tc.name, func(t *testing.T) { check(t, tc.config, tc.dir, tc.want) }) + } + + t.Run("relative directory", func(t *testing.T) { + // Checked against git 2.56: "../*" trusts nothing, as "../repo" + // does not. A relative dir is taken from the working directory. + t.Chdir(a) + for _, tc := range []struct { + entry string + dir string + want bool + }{ + {s + "/a", ".", true}, + {s + "/a/b", "b", true}, + {s + "/ab", "../ab", true}, + {s + "/*", "../ab", true}, + {"../*", "../ab", false}, + {"../ab", "../ab", false}, + } { + t.Run(tc.entry+" "+tc.dir, func(t *testing.T) { check(t, "directory = "+tc.entry+"\n", tc.dir, tc.want) }) + } + }) +}