diff --git a/docs/REQUIREMENTS.md b/docs/REQUIREMENTS.md index fffcb58..516771e 100644 --- a/docs/REQUIREMENTS.md +++ b/docs/REQUIREMENTS.md @@ -91,7 +91,7 @@ and callers must be able to tell whether to trust it. | X1 | No injection through edits: whatever key, subsection and value an edit is given, the edited file parses to the previous entries with exactly that change. A subsection containing a newline or NUL is rejected (as by git). Sections to remove or rename are found by parsing, where git scans lines: git takes a line starting with `[` inside a continued value for a header (removing the entries after it, or rewriting the value), and misses a header after a BOM or after another header on the same line. A file holding only a BOM keeps it first, where git writes the new section before it and the file no longer parses. *verified* Fuzzed: parse ∘ edit ∘ parse, for key and section edits. | MUST | | 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, 16 MiB and 2²⁰ entries and distinct section headers 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*; an entry costs about a hundred times its size in memory, so the entry count, not the bytes, bounds memory); `Parse` and edits apply the same limits to their one file; 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 | +| 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`, and paths, which a configuration file can choose, have their control characters (newline and tab included), bidirectional controls and non-UTF-8 bytes replaced with `?`: stricter than git 2.47.3 and 2.56.0, which replace only C0 controls but tab and newline, and DEL (*verified*); no terminal escape sequences, forged lines or disguised paths. | 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 | diff --git a/errors.go b/errors.go index 523502b..4965ba2 100644 --- a/errors.go +++ b/errors.go @@ -3,6 +3,9 @@ package gitconfig import ( "errors" "fmt" + "strings" + "unicode" + "unicode/utf8" ) // The sentinel errors below are returned wrapped in a typed error that says @@ -20,6 +23,14 @@ import ( // - Discover also returns an *EnvError for an environment variable it // cannot interpret, and ErrUntrusted in a *FileError for a repository // git would refuse. +// +// Error messages name files, whose paths can come from a configuration +// file someone else wrote. As in git, but stricter, control characters +// (newline and tab included), bidirectional controls and bytes that are not +// UTF-8 are replaced with '?', so that a path cannot drive the terminal it +// is printed to, nor forge a line or disguise itself. The fields of the +// errors, and Entry, Origin and Key, hold the raw data: quote them before +// printing them yourself. var ( // ErrNotFound is returned when the section or subsection to rename is // absent. Removing something absent is not an error. @@ -84,7 +95,7 @@ type KeyError struct { } func (e *KeyError) Error() string { - return fmt.Sprintf("%v: %q", e.Err, e.Key) + return sanitize(fmt.Sprintf("%v: %q", e.Err, e.Key)) } func (e *KeyError) Unwrap() error { @@ -104,7 +115,8 @@ func (e *FileError) Error() string { if se, ok := errors.AsType[*SyntaxError](e.Err); ok && se.File == e.Path { return se.Error() } - return fmt.Sprintf("gitconfig: %s: %v", e.Path, e.Err) + // The wrapped error may name a path too (*fs.PathError). + return sanitize(fmt.Sprintf("gitconfig: %s: %v", e.Path, e.Err)) } func (e *FileError) Unwrap() error { @@ -121,7 +133,7 @@ type EnvError struct { } func (e *EnvError) Error() string { - return fmt.Sprintf("gitconfig: %s: %v", e.Var, e.Err) + return sanitize(fmt.Sprintf("gitconfig: %s: %v", e.Var, e.Err)) } func (e *EnvError) Unwrap() error { @@ -138,7 +150,7 @@ type SyntaxError struct { } func (e *SyntaxError) Error() string { - return fmt.Sprintf("gitconfig: %s line %d in file %s", e.Msg, e.Line, e.File) + return sanitize(fmt.Sprintf("gitconfig: %s line %d in file %s", e.Msg, e.Line, e.File)) } // ValueError reports a value that cannot be read as the requested type. Its @@ -167,9 +179,34 @@ func (e *ValueError) Error() string { if e.Err != nil { msg += ": " + e.Err.Error() } - return msg + return sanitize(msg) } func (e *ValueError) Unwrap() error { return e.Err } + +// sanitize replaces with '?' what could drive a terminal or disguise a +// path: control characters (C0, DEL, C1), bidirectional controls, and bytes +// that are not UTF-8. git does this for C0 and DEL only, but keeps tab and +// newline; no message of this package spans lines, and a newline in a path +// could forge one. +func sanitize(msg string) string { + i := 0 + for i < len(msg) && ' ' <= msg[i] && msg[i] < 0x7f { + i++ + } + if i == len(msg) { + return msg + } + var sb strings.Builder + sb.WriteString(msg[:i]) + for _, r := range msg[i:] { + // An invalid byte reads as utf8.RuneError, which is printed as '?' too. + if r < ' ' || 0x7f <= r && r <= 0x9f || r == utf8.RuneError || unicode.Is(unicode.Bidi_Control, r) { + r = '?' + } + sb.WriteRune(r) + } + return sb.String() +} diff --git a/errors_test.go b/errors_test.go index fb1ff0e..9b3c07e 100644 --- a/errors_test.go +++ b/errors_test.go @@ -1,6 +1,8 @@ package gitconfig_test import ( + "fmt" + "io/fs" "path/filepath" "testing" @@ -58,6 +60,52 @@ func TestErrorMessagesEscapeControlCharacters(t *testing.T) { assert.NotContains(t, err.Error(), "\r") }) + // Paths are not quoted: as in git, their control characters become '?', + // and also newline and tab, C1 and bidirectional controls, and bytes that + // are not UTF-8. + const evilPath = "p\x1b]52;c;cHduZWQ=\x07\x1b[2J\x7f\t\n\u009b\u202e\x9b é" + const shown = "p?]52;c;cHduZWQ=??[2J?????? é" + + t.Run("SyntaxError", func(t *testing.T) { + _, err := gitconfig.Parse(evilPath, []byte("[a\n")) + require.Error(t, err) + assert.Contains(t, err.Error(), "in file "+shown) + }) + + t.Run("ValueError file", func(t *testing.T) { + e := gitconfig.Entry{Key: gitconfig.Key{Section: "a", Name: "k"}, Value: "x", Origin: gitconfig.Origin{File: evilPath}} + _, err := e.Bool() + require.Error(t, err) + assert.Contains(t, err.Error(), "in file "+shown) + }) + + t.Run("FileError", func(t *testing.T) { + err := error(&gitconfig.FileError{Path: evilPath, Err: gitconfig.ErrLocked}) + assert.Equal(t, "gitconfig: "+shown+": "+gitconfig.ErrLocked.Error(), err.Error()) + + // A wrapped *fs.PathError names the path again. It is made by hand: + // opening such a name fails differently on each system. + err = &gitconfig.FileError{Path: "f", Err: &fs.PathError{Op: "open", Path: evilPath, Err: fs.ErrNotExist}} + require.ErrorIs(t, err, fs.ErrNotExist) + assert.NotContains(t, err.Error(), "\x1b") + assert.Contains(t, err.Error(), shown) + }) + + t.Run("include path", func(t *testing.T) { + // Only the deepest file includes the path, and the depth limit + // stops it there: it is never opened, which Windows would refuse. + dir := tempDir(t) + for i := range 10 { + writeFile(t, filepath.Join(dir, fmt.Sprintf("f%d", i)), fmt.Sprintf("[include]\n\tpath = f%d\n", i+1)) + } + writeFile(t, filepath.Join(dir, "f10"), "[include]\n\tpath = \"x\x1b]52;c;cHduZWQ=\x07\"\n") + _, err := gitconfig.Env{GlobalConfig: []string{filepath.Join(dir, "f0")}}.Load() + require.ErrorIs(t, err, gitconfig.ErrIncludeDepth) + assert.NotContains(t, err.Error(), "\x1b") + assert.NotContains(t, err.Error(), "\x07") + assert.Contains(t, err.Error(), "x?]52;c;cHduZWQ=?") + }) + t.Run("EnvError", func(t *testing.T) { for _, name := range []string{"GIT_CONFIG_COUNT", "GIT_CONFIG_NOSYSTEM"} { cleanGitEnv(t)