Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor git command package to improve security and maintainability #22678

Merged
merged 8 commits into from
Feb 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ linters-settings:
- github.com/unknwon/com: "use gitea's util and replacements"
- io/ioutil: "use os or io instead"
- golang.org/x/exp: "it's experimental and unreliable."
- code.gitea.io/gitea/modules/git/internal: "do not use the internal package, use AddXxx function instead"

issues:
max-issues-per-linter: 0
Expand Down
97 changes: 69 additions & 28 deletions modules/git/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,20 @@ import (
"time"
"unsafe"

"code.gitea.io/gitea/modules/git/internal" //nolint:depguard // only this file can use the internal type CmdArg, other files and packages should use AddXxx functions
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/process"
"code.gitea.io/gitea/modules/util"
)

// TrustedCmdArgs returns the trusted arguments for git command.
// It's mainly for passing user-provided and trusted arguments to git command
// In most cases, it shouldn't be used. Use AddXxx function instead
type TrustedCmdArgs []internal.CmdArg

var (
// globalCommandArgs global command args for external package setting
globalCommandArgs []CmdArg
globalCommandArgs TrustedCmdArgs

// defaultCommandExecutionTimeout default command execution timeout duration
defaultCommandExecutionTimeout = 360 * time.Second
Expand All @@ -42,8 +48,6 @@ type Command struct {
brokenArgs []string
}

type CmdArg string

func (c *Command) String() string {
if len(c.args) == 0 {
return c.name
Expand All @@ -53,7 +57,7 @@ func (c *Command) String() string {

// NewCommand creates and returns a new Git Command based on given command and arguments.
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommand(ctx context.Context, args ...CmdArg) *Command {
func NewCommand(ctx context.Context, args ...internal.CmdArg) *Command {
// Make an explicit copy of globalCommandArgs, otherwise append might overwrite it
cargs := make([]string, 0, len(globalCommandArgs)+len(args))
for _, arg := range globalCommandArgs {
Expand All @@ -70,15 +74,9 @@ func NewCommand(ctx context.Context, args ...CmdArg) *Command {
}
}

// NewCommandNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommandNoGlobals(args ...CmdArg) *Command {
return NewCommandContextNoGlobals(DefaultContext, args...)
}

// NewCommandContextNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommandContextNoGlobals(ctx context.Context, args ...CmdArg) *Command {
func NewCommandContextNoGlobals(ctx context.Context, args ...internal.CmdArg) *Command {
cargs := make([]string, 0, len(args))
for _, arg := range args {
cargs = append(cargs, string(arg))
Expand All @@ -96,27 +94,70 @@ func (c *Command) SetParentContext(ctx context.Context) *Command {
return c
}

// SetDescription sets the description for this command which be returned on
// c.String()
// SetDescription sets the description for this command which be returned on c.String()
func (c *Command) SetDescription(desc string) *Command {
c.desc = desc
return c
}

// AddArguments adds new git argument(s) to the command. Each argument must be safe to be trusted.
// User-provided arguments should be passed to AddDynamicArguments instead.
func (c *Command) AddArguments(args ...CmdArg) *Command {
// isSafeArgumentValue checks if the argument is safe to be used as a value (not an option)
func isSafeArgumentValue(s string) bool {
return s == "" || s[0] != '-'
}

// isValidArgumentOption checks if the argument is a valid option (starting with '-').
// It doesn't check whether the option is supported or not
func isValidArgumentOption(s string) bool {
return s != "" && s[0] == '-'
}

// AddArguments adds new git arguments (option/value) to the command. It only accepts string literals, or trusted CmdArg.
// Type CmdArg is in the internal package, so it can not be used outside of this package directly,
// it makes sure that user-provided arguments won't cause RCE risks.
// User-provided arguments should be passed by other AddXxx functions
func (c *Command) AddArguments(args ...internal.CmdArg) *Command {
for _, arg := range args {
c.args = append(c.args, string(arg))
}
return c
}

// AddDynamicArguments adds new dynamic argument(s) to the command.
// The arguments may come from user input and can not be trusted, so no leading '-' is allowed to avoid passing options
// AddOptionValues adds a new option with a list of non-option values
// For example: AddOptionValues("--opt", val) means 2 arguments: {"--opt", val}.
// The values are treated as dynamic argument values. It equals to: AddArguments("--opt") then AddDynamicArguments(val).
func (c *Command) AddOptionValues(opt internal.CmdArg, args ...string) *Command {
if !isValidArgumentOption(string(opt)) {
c.brokenArgs = append(c.brokenArgs, string(opt))
return c
}
c.args = append(c.args, string(opt))
c.AddDynamicArguments(args...)
return c
}

// AddOptionFormat adds a new option with a format string and arguments
// For example: AddOptionFormat("--opt=%s %s", val1, val2) means 1 argument: {"--opt=val1 val2"}.
func (c *Command) AddOptionFormat(opt string, args ...any) *Command {
if !isValidArgumentOption(opt) {
c.brokenArgs = append(c.brokenArgs, opt)
return c
}
// a quick check to make sure the format string matches the number of arguments, to find low-level mistakes ASAP
if strings.Count(strings.ReplaceAll(opt, "%%", ""), "%") != len(args) {
c.brokenArgs = append(c.brokenArgs, opt)
return c
}
s := fmt.Sprintf(opt, args...)
c.args = append(c.args, s)
return c
}

// AddDynamicArguments adds new dynamic argument values to the command.
// The arguments may come from user input and can not be trusted, so no leading '-' is allowed to avoid passing options.
// TODO: in the future, this function can be renamed to AddArgumentValues
func (c *Command) AddDynamicArguments(args ...string) *Command {
for _, arg := range args {
if arg != "" && arg[0] == '-' {
if !isSafeArgumentValue(arg) {
c.brokenArgs = append(c.brokenArgs, arg)
}
}
Expand All @@ -137,14 +178,14 @@ func (c *Command) AddDashesAndList(list ...string) *Command {
return c
}

// CmdArgCheck checks whether the string is safe to be used as a dynamic argument.
// It panics if the check fails. Usually it should not be used, it's just for refactoring purpose
// deprecated
func CmdArgCheck(s string) CmdArg {
if s != "" && s[0] == '-' {
panic("invalid git cmd argument: " + s)
// ToTrustedCmdArgs converts a list of strings (trusted as argument) to TrustedCmdArgs
// In most cases, it shouldn't be used. Use AddXxx function instead
func ToTrustedCmdArgs(args []string) TrustedCmdArgs {
ret := make(TrustedCmdArgs, len(args))
for i, arg := range args {
ret[i] = internal.CmdArg(arg)
}
return CmdArg(s)
return ret
}

// RunOpts represents parameters to run the command. If UseContextTimeout is specified, then Timeout is ignored.
Expand Down Expand Up @@ -364,9 +405,9 @@ func (c *Command) RunStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunS
}

// AllowLFSFiltersArgs return globalCommandArgs with lfs filter, it should only be used for tests
func AllowLFSFiltersArgs() []CmdArg {
func AllowLFSFiltersArgs() TrustedCmdArgs {
// Now here we should explicitly allow lfs filters to run
filteredLFSGlobalArgs := make([]CmdArg, len(globalCommandArgs))
filteredLFSGlobalArgs := make(TrustedCmdArgs, len(globalCommandArgs))
j := 0
for _, arg := range globalCommandArgs {
if strings.Contains(string(arg), "lfs") {
Expand Down
11 changes: 11 additions & 0 deletions modules/git/command_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,14 @@ func TestRunWithContextStd(t *testing.T) {
assert.Empty(t, stderr)
assert.Contains(t, stdout, "git version")
}

func TestGitArgument(t *testing.T) {
assert.True(t, isValidArgumentOption("-x"))
assert.True(t, isValidArgumentOption("--xx"))
assert.False(t, isValidArgumentOption(""))
assert.False(t, isValidArgumentOption("x"))

assert.True(t, isSafeArgumentValue(""))
assert.True(t, isSafeArgumentValue("x"))
assert.False(t, isSafeArgumentValue("-x"))
}
18 changes: 9 additions & 9 deletions modules/git/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"bytes"
"context"
"errors"
"fmt"
"io"
"os/exec"
"strconv"
Expand Down Expand Up @@ -91,8 +90,8 @@ func AddChanges(repoPath string, all bool, files ...string) error {
}

// AddChangesWithArgs marks local changes to be ready for commit.
func AddChangesWithArgs(repoPath string, globalArgs []CmdArg, all bool, files ...string) error {
cmd := NewCommandNoGlobals(append(globalArgs, "add")...)
func AddChangesWithArgs(repoPath string, globalArgs TrustedCmdArgs, all bool, files ...string) error {
cmd := NewCommandContextNoGlobals(DefaultContext, globalArgs...).AddArguments("add")
if all {
cmd.AddArguments("--all")
}
Expand All @@ -111,27 +110,28 @@ type CommitChangesOptions struct {
// CommitChanges commits local changes with given committer, author and message.
// If author is nil, it will be the same as committer.
func CommitChanges(repoPath string, opts CommitChangesOptions) error {
cargs := make([]CmdArg, len(globalCommandArgs))
cargs := make(TrustedCmdArgs, len(globalCommandArgs))
copy(cargs, globalCommandArgs)
return CommitChangesWithArgs(repoPath, cargs, opts)
}

// CommitChangesWithArgs commits local changes with given committer, author and message.
// If author is nil, it will be the same as committer.
func CommitChangesWithArgs(repoPath string, args []CmdArg, opts CommitChangesOptions) error {
cmd := NewCommandNoGlobals(args...)
func CommitChangesWithArgs(repoPath string, args TrustedCmdArgs, opts CommitChangesOptions) error {
cmd := NewCommandContextNoGlobals(DefaultContext, args...)
if opts.Committer != nil {
cmd.AddArguments("-c", CmdArg("user.name="+opts.Committer.Name), "-c", CmdArg("user.email="+opts.Committer.Email))
cmd.AddOptionValues("-c", "user.name="+opts.Committer.Name)
cmd.AddOptionValues("-c", "user.email="+opts.Committer.Email)
}
cmd.AddArguments("commit")

if opts.Author == nil {
opts.Author = opts.Committer
}
if opts.Author != nil {
cmd.AddArguments(CmdArg(fmt.Sprintf("--author='%s <%s>'", opts.Author.Name, opts.Author.Email)))
cmd.AddOptionFormat("--author='%s <%s>'", opts.Author.Name, opts.Author.Email)
}
cmd.AddArguments("-m").AddDynamicArguments(opts.Message)
cmd.AddOptionValues("-m", opts.Message)

_, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
// No stderr but exit status 1 means nothing to commit.
Expand Down
2 changes: 1 addition & 1 deletion modules/git/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,6 @@ func configUnsetAll(key, value string) error {
}

// Fsck verifies the connectivity and validity of the objects in the database
func Fsck(ctx context.Context, repoPath string, timeout time.Duration, args ...CmdArg) error {
func Fsck(ctx context.Context, repoPath string, timeout time.Duration, args TrustedCmdArgs) error {
return NewCommand(ctx, "fsck").AddArguments(args...).Run(&RunOpts{Timeout: timeout, Dir: repoPath})
}
9 changes: 9 additions & 0 deletions modules/git/internal/cmdarg.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Copyright 2023 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package internal

// CmdArg represents a command argument for git command, and it will be used for the git command directly without any further processing.
// In most cases, you should use the "AddXxx" functions to add arguments, but not use this type directly.
// Casting a risky (user-provided) string to CmdArg would cause security issues if it's injected with a "--xxx" argument.
type CmdArg string
2 changes: 1 addition & 1 deletion modules/git/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func Clone(ctx context.Context, from, to string, opts CloneRepoOptions) error {
}

// CloneWithArgs original repository to target path.
func CloneWithArgs(ctx context.Context, args []CmdArg, from, to string, opts CloneRepoOptions) (err error) {
func CloneWithArgs(ctx context.Context, args TrustedCmdArgs, from, to string, opts CloneRepoOptions) (err error) {
toDir := path.Dir(to)
if err = os.MkdirAll(toDir, os.ModePerm); err != nil {
return err
Expand Down
4 changes: 2 additions & 2 deletions modules/git/repo_archive.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ func (repo *Repository) CreateArchive(ctx context.Context, format ArchiveType, t

cmd := NewCommand(ctx, "archive")
if usePrefix {
cmd.AddArguments(CmdArg("--prefix=" + filepath.Base(strings.TrimSuffix(repo.Path, ".git")) + "/"))
cmd.AddOptionFormat("--prefix=%s", filepath.Base(strings.TrimSuffix(repo.Path, ".git"))+"/")
}
cmd.AddArguments(CmdArg("--format=" + format.String()))
cmd.AddOptionFormat("--format=%s", format.String())
cmd.AddDynamicArguments(commitID)

var stderr strings.Builder
Expand Down
40 changes: 19 additions & 21 deletions modules/git/repo_attribute.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
type CheckAttributeOpts struct {
CachedOnly bool
AllAttributes bool
Attributes []CmdArg
Attributes []string
Filenames []string
IndexFile string
WorkTree string
Expand Down Expand Up @@ -48,7 +48,7 @@ func (repo *Repository) CheckAttribute(opts CheckAttributeOpts) (map[string]map[
} else {
for _, attribute := range opts.Attributes {
if attribute != "" {
cmd.AddArguments(attribute)
cmd.AddDynamicArguments(attribute)
}
}
}
Expand Down Expand Up @@ -95,7 +95,7 @@ func (repo *Repository) CheckAttribute(opts CheckAttributeOpts) (map[string]map[
// CheckAttributeReader provides a reader for check-attribute content that can be long running
type CheckAttributeReader struct {
// params
Attributes []CmdArg
Attributes []string
Repo *Repository
IndexFile string
WorkTree string
Expand All @@ -111,19 +111,6 @@ type CheckAttributeReader struct {

// Init initializes the CheckAttributeReader
func (c *CheckAttributeReader) Init(ctx context.Context) error {
cmdArgs := []CmdArg{"check-attr", "--stdin", "-z"}

if len(c.IndexFile) > 0 {
cmdArgs = append(cmdArgs, "--cached")
c.env = append(c.env, "GIT_INDEX_FILE="+c.IndexFile)
}

if len(c.WorkTree) > 0 {
c.env = append(c.env, "GIT_WORK_TREE="+c.WorkTree)
}

c.env = append(c.env, "GIT_FLUSH=1")

if len(c.Attributes) == 0 {
lw := new(nulSeparatedAttributeWriter)
lw.attributes = make(chan attributeTriple)
Expand All @@ -134,11 +121,22 @@ func (c *CheckAttributeReader) Init(ctx context.Context) error {
return fmt.Errorf("no provided Attributes to check")
}

cmdArgs = append(cmdArgs, c.Attributes...)
cmdArgs = append(cmdArgs, "--")

c.ctx, c.cancel = context.WithCancel(ctx)
c.cmd = NewCommand(c.ctx, cmdArgs...)
c.cmd = NewCommand(c.ctx, "check-attr", "--stdin", "-z")

if len(c.IndexFile) > 0 {
c.cmd.AddArguments("--cached")
c.env = append(c.env, "GIT_INDEX_FILE="+c.IndexFile)
}

if len(c.WorkTree) > 0 {
c.env = append(c.env, "GIT_WORK_TREE="+c.WorkTree)
}

c.env = append(c.env, "GIT_FLUSH=1")

// The empty "--" comes from #16773 , and it seems unnecessary because nothing else would be added later.
c.cmd.AddDynamicArguments(c.Attributes...).AddArguments("--")
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved

var err error

Expand Down Expand Up @@ -294,7 +292,7 @@ func (repo *Repository) CheckAttributeReader(commitID string) (*CheckAttributeRe
}

checker := &CheckAttributeReader{
Attributes: []CmdArg{"linguist-vendored", "linguist-generated", "linguist-language", "gitlab-language"},
Attributes: []string{"linguist-vendored", "linguist-generated", "linguist-language", "gitlab-language"},
Repo: repo,
IndexFile: indexFilename,
WorkTree: worktree,
Expand Down
8 changes: 5 additions & 3 deletions modules/git/repo_blame.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@

package git

import "fmt"
import (
"fmt"
)

// FileBlame return the Blame object of file
func (repo *Repository) FileBlame(revision, path, file string) ([]byte, error) {
Expand All @@ -14,8 +16,8 @@ func (repo *Repository) FileBlame(revision, path, file string) ([]byte, error) {
// LineBlame returns the latest commit at the given line
func (repo *Repository) LineBlame(revision, path, file string, line uint) (*Commit, error) {
res, _, err := NewCommand(repo.Ctx, "blame").
AddArguments(CmdArg(fmt.Sprintf("-L %d,%d", line, line))).
AddArguments("-p").AddDynamicArguments(revision).
AddOptionFormat("-L %d,%d", line, line).
AddOptionValues("-p", revision).
AddDashesAndList(file).RunStdString(&RunOpts{Dir: path})
if err != nil {
return nil, err
Expand Down
Loading