From 93cc54a1b56e3829800b5c64220f7874a5a2e8e0 Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Tue, 7 Jul 2026 10:39:51 -0400 Subject: [PATCH 1/2] fix/snapshot: prevent OS command injection via targets file --- cmd/src/snapshot_databases.go | 22 ++- cmd/src/snapshot_restore.go | 12 +- internal/pgdump/pgdump.go | 211 +++++++++++++++++++++---- internal/pgdump/pgdump_command_test.go | 107 +++++++++++++ 4 files changed, 307 insertions(+), 45 deletions(-) create mode 100644 internal/pgdump/pgdump_command_test.go diff --git a/cmd/src/snapshot_databases.go b/cmd/src/snapshot_databases.go index c614f3d21e..ae880cca9a 100644 --- a/cmd/src/snapshot_databases.go +++ b/cmd/src/snapshot_databases.go @@ -4,7 +4,6 @@ import ( "flag" "fmt" "os" - "os/exec" "strings" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -14,6 +13,16 @@ import ( "github.com/sourcegraph/src-cli/internal/pgdump" ) +// renderCommands renders a set of commands for display as copy-pasteable, +// safely shell-quoted strings. +func renderCommands(commands []pgdump.Command) []string { + lines := make([]string, len(commands)) + for i, c := range commands { + lines[i] = c.String() + } + return lines +} + func init() { usage := `'src snapshot databases' generates commands to export Sourcegraph database dumps. Note that these commands are intended for use as reference - you may need to adjust the commands for your deployment. @@ -83,19 +92,18 @@ TARGETS FILES if *run { for _, c := range commands { - out.WriteLine(output.Emojif(output.EmojiInfo, "Running command: %q", c)) - command := exec.Command("bash", "-c", c) - output, err := command.CombinedOutput() - out.Write(string(output)) + out.WriteLine(output.Emojif(output.EmojiInfo, "Running command: %q", c.String())) + combined, err := c.Run() + out.Write(string(combined)) if err != nil { - return errors.Wrapf(err, "failed to run command: %q", c) + return errors.Wrapf(err, "failed to run command: %q", c.String()) } } out.WriteLine(output.Emoji(output.EmojiSuccess, "Successfully completed dump commands")) } else { b := out.Block(output.Emoji(output.EmojiSuccess, "Run these commands to generate the required database dumps:")) - b.Write("\n" + strings.Join(commands, "\n")) + b.Write("\n" + strings.Join(renderCommands(commands), "\n")) b.Close() out.WriteLine(output.Styledf(output.StyleSuggestion, "Note that you may need to do some additional setup, such as authentication, beforehand.")) diff --git a/cmd/src/snapshot_restore.go b/cmd/src/snapshot_restore.go index 9ae941e609..7acc979e50 100644 --- a/cmd/src/snapshot_restore.go +++ b/cmd/src/snapshot_restore.go @@ -4,7 +4,6 @@ import ( "flag" "fmt" "os" - "os/exec" "strings" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -81,12 +80,11 @@ TARGETS FILES } if *run { for _, c := range commands { - out.WriteLine(output.Emojif(output.EmojiInfo, "Running command: %q", c)) - command := exec.Command("bash", "-c", c) - output, err := command.CombinedOutput() - out.Write(string(output)) + out.WriteLine(output.Emojif(output.EmojiInfo, "Running command: %q", c.String())) + combined, err := c.Run() + out.Write(string(combined)) if err != nil { - return errors.Wrapf(err, "failed to run command: %q", c) + return errors.Wrapf(err, "failed to run command: %q", c.String()) } } @@ -94,7 +92,7 @@ TARGETS FILES out.WriteLine(output.Styledf(output.StyleSuggestion, "It may be necessary to restart your Sourcegraph instance after restoring")) } else { b := out.Block(output.Emoji(output.EmojiSuccess, "Run these commands to restore the databases:")) - b.Write("\n" + strings.Join(commands, "\n")) + b.Write("\n" + strings.Join(renderCommands(commands), "\n")) b.Close() out.WriteLine(output.Styledf(output.StyleSuggestion, "Note that you may need to do some additional setup, such as authentication, beforehand.")) diff --git a/internal/pgdump/pgdump.go b/internal/pgdump/pgdump.go index 05d916b16b..7a7b81c4a8 100644 --- a/internal/pgdump/pgdump.go +++ b/internal/pgdump/pgdump.go @@ -1,8 +1,11 @@ package pgdump import ( - "fmt" + "bytes" + "os" + "os/exec" "path/filepath" + "strings" "github.com/sourcegraph/sourcegraph/lib/errors" ) @@ -30,24 +33,114 @@ type Target struct { Password string `yaml:"password"` } -// RestoreCommand generates a psql command that can be used for migrations. -func RestoreCommand(t Target) string { - dump := fmt.Sprintf("psql --username=%s --dbname=%s 1>/dev/null", - t.Username, t.DBName) - if t.Password == "" { - return dump +// Command represents a command to run as an argv array rather than a shell +// string. Values that originate from user-controlled targets are always passed +// as discrete arguments (or safely shell-quoted when they must be embedded in a +// nested shell command), never interpolated into a string that is handed to +// "bash -c". This prevents shell injection via malicious targets files. +type Command struct { + // Env holds additional environment variables in "KEY=value" form. + Env []string + // Args is the argv to execute; Args[0] is the executable. + Args []string + // InputFile, if set, is opened and connected to the command's stdin. + InputFile string + // OutputFile, if set, receives the command's stdout. When empty, stdout is + // discarded (matching the original "1>/dev/null" behaviour for restores). + OutputFile string +} + +// Run executes the command safely (without a shell), wiring up any input/output +// file redirection, and returns the combined stderr (and stdout when it is not +// redirected to a file) output. +func (c Command) Run() ([]byte, error) { + if len(c.Args) == 0 { + return nil, errors.New("no command to run") + } + + cmd := exec.Command(c.Args[0], c.Args[1:]...) + cmd.Env = append(os.Environ(), c.Env...) + + var combined bytes.Buffer + cmd.Stderr = &combined + + if c.InputFile != "" { + f, err := os.Open(c.InputFile) + if err != nil { + return nil, errors.Wrapf(err, "opening input file %q", c.InputFile) + } + defer f.Close() + cmd.Stdin = f + } + + if c.OutputFile != "" { + f, err := os.Create(c.OutputFile) + if err != nil { + return nil, errors.Wrapf(err, "creating output file %q", c.OutputFile) + } + defer f.Close() + cmd.Stdout = f + } else { + // No output file means stdout is not of interest (e.g. restores), so + // capture it alongside stderr for diagnostics. + cmd.Stdout = &combined + } + + err := cmd.Run() + return combined.Bytes(), err +} + +// String renders the command as a copy-pasteable, safely shell-quoted string. +// It is intended for display only; execution always goes through Run. +func (c Command) String() string { + var parts []string + for _, e := range c.Env { + parts = append(parts, shellQuoteEnv(e)) + } + for _, a := range c.Args { + parts = append(parts, shellQuote(a)) + } + s := strings.Join(parts, " ") + if c.OutputFile != "" { + s += " > " + shellQuote(c.OutputFile) } - return fmt.Sprintf("PGPASSWORD=%s %s", t.Password, dump) + if c.InputFile != "" { + s += " < " + shellQuote(c.InputFile) + } + return s } -// DumpCommand generates a pg_dump command that can be used for on-prem-to-Cloud migrations. -func DumpCommand(t Target) string { - dump := fmt.Sprintf("pg_dump --clean --format=plain --if-exists --no-acl --no-owner --quote-all-identifiers --username=%s --dbname=%s", - t.Username, t.DBName) - if t.Password == "" { - return dump +// RestoreCommand generates a psql invocation that can be used for migrations. +func RestoreCommand(t Target) (args []string, env []string) { + args = []string{ + "psql", + "--username=" + t.Username, + "--dbname=" + t.DBName, + } + if t.Password != "" { + env = append(env, "PGPASSWORD="+t.Password) } - return fmt.Sprintf("PGPASSWORD=%s %s", t.Password, dump) + return args, env +} + +// DumpCommand generates a pg_dump invocation that can be used for +// on-prem-to-Cloud migrations. +func DumpCommand(t Target) (args []string, env []string) { + args = []string{ + "pg_dump", + "--clean", + "--format=plain", + "--if-exists", + "--no-acl", + "--no-owner", + "--quote-all-identifiers", + "--username=" + t.Username, + "--dbname=" + t.DBName, + } + if t.Password != "" { + env = append(env, "PGPASSWORD="+t.Password) + } + return args, env } type Output struct { @@ -70,30 +163,37 @@ func Outputs(dir string, targets Targets) []Output { }} } -type CommandBuilder func(Target) (string, error) -type PGCommand func(Target) string +type CommandBuilder func(Target) (Command, error) + +// PGCommand generates the argv and environment for a base Postgres command +// (pg_dump or psql) targeting the given Target. +type PGCommand func(Target) (args []string, env []string) // Builder generates the CommandBuilder and targetKey for a given builder and PGCommand func Builder(builder string, command PGCommand) (commandBuilder CommandBuilder, targetKey string) { switch builder { case "pg_dump", "": targetKey = "local" - commandBuilder = func(t Target) (string, error) { - cmd := command(t) + commandBuilder = func(t Target) (Command, error) { + args, env := command(t) if t.Target != "" { - return fmt.Sprintf("%s --host=%s", cmd, t.Target), nil + args = append(args, "--host="+t.Target) } - return cmd, nil + return Command{Env: env, Args: args}, nil } case "docker": targetKey = "docker" - commandBuilder = func(t Target) (string, error) { - return fmt.Sprintf("docker exec -i %s sh -c '%s'", t.Target, command(t)), nil + commandBuilder = func(t Target) (Command, error) { + args, env := command(t) + inner := shellCommand(env, args) + return Command{Args: []string{"docker", "exec", "-i", t.Target, "sh", "-c", inner}}, nil } case "kubectl": targetKey = "k8s" - commandBuilder = func(t Target) (string, error) { - return fmt.Sprintf("kubectl exec -i %s -- bash -c '%s'", t.Target, command(t)), nil + commandBuilder = func(t Target) (Command, error) { + args, env := command(t) + inner := shellCommand(env, args) + return Command{Args: []string{"kubectl", "exec", "-i", t.Target, "--", "bash", "-c", inner}}, nil } default: return commandBuilder, targetKey @@ -103,8 +203,8 @@ func Builder(builder string, command PGCommand) (commandBuilder CommandBuilder, // BuildCommands generates commands that output Postgres dumps and sends them to predefined // files for each target database. -func BuildCommands(outDir string, commandBuilder CommandBuilder, targets Targets, dump bool) ([]string, error) { - var commands []string +func BuildCommands(outDir string, commandBuilder CommandBuilder, targets Targets, dump bool) ([]Command, error) { + var commands []Command for _, t := range Outputs(outDir, targets) { c, err := commandBuilder(t.Target) if err != nil { @@ -112,12 +212,61 @@ func BuildCommands(outDir string, commandBuilder CommandBuilder, targets Targets } if dump { - // When dumping use output redirection to dump command stdout to target file - commands = append(commands, fmt.Sprintf("%s > %s", c, t.Output)) + // When dumping, redirect command stdout to the target file. + c.OutputFile = t.Output } else { - // When restoring use input redirection to pass target file to command stdin - commands = append(commands, fmt.Sprintf("%s < %s", c, t.Output)) + // When restoring, feed the target file to the command's stdin. + c.InputFile = t.Output } + commands = append(commands, c) } return commands, nil } + +// shellCommand renders env and args as a single, safely shell-quoted string for +// use as the argument to a nested "sh -c"/"bash -c" (e.g. inside a container). +func shellCommand(env, args []string) string { + var parts []string + for _, e := range env { + parts = append(parts, shellQuoteEnv(e)) + } + for _, a := range args { + parts = append(parts, shellQuote(a)) + } + return strings.Join(parts, " ") +} + +// shellQuoteEnv quotes a "KEY=value" assignment, leaving the KEY= prefix +// unquoted (so the shell still treats it as an assignment) and quoting only the +// value. +func shellQuoteEnv(assignment string) string { + i := strings.IndexByte(assignment, '=') + if i < 0 { + return shellQuote(assignment) + } + return assignment[:i+1] + shellQuote(assignment[i+1:]) +} + +// shellQuote returns s quoted such that a POSIX shell will treat it as a single +// literal argument. +func shellQuote(s string) string { + if s == "" { + return "''" + } + safe := true + for _, r := range s { + if (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9') { + continue + } + switch r { + case '-', '_', '.', '/', ':', '=', '@', '%', '+', ',': + continue + } + safe = false + break + } + if safe { + return s + } + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} diff --git a/internal/pgdump/pgdump_command_test.go b/internal/pgdump/pgdump_command_test.go new file mode 100644 index 0000000000..8e8e2a241b --- /dev/null +++ b/internal/pgdump/pgdump_command_test.go @@ -0,0 +1,107 @@ +package pgdump + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// maliciousTarget is a target whose fields contain shell metacharacters, +// mirroring the injection payloads described in VULN-100. +func maliciousTarget() Target { + return Target{ + Target: "localhost; touch /tmp/pwned #", + DBName: "sg; rm -rf /", + Username: "sg`whoami`", + Password: "sg$(id)", + } +} + +func TestLocalCommandNoInjection(t *testing.T) { + builder, key := Builder("pg_dump", DumpCommand) + require.Equal(t, "local", key) + + cmd, err := builder(maliciousTarget()) + require.NoError(t, err) + + // Every untrusted value must be carried as a discrete argv element, never + // merged into another token where the shell could reinterpret it. + assert.Contains(t, cmd.Args, "--host=localhost; touch /tmp/pwned #") + assert.Contains(t, cmd.Args, "--dbname=sg; rm -rf /") + assert.Contains(t, cmd.Args, "--username=sg`whoami`") + assert.Contains(t, cmd.Env, "PGPASSWORD=sg$(id)") + + // pg_dump is invoked directly, not via a shell. + assert.Equal(t, "pg_dump", cmd.Args[0]) +} + +func TestDockerCommandNoInjection(t *testing.T) { + builder, key := Builder("docker", DumpCommand) + require.Equal(t, "docker", key) + + cmd, err := builder(maliciousTarget()) + require.NoError(t, err) + + // The target is passed as a discrete argv element to docker, so it cannot + // break out into the calling shell. + assert.Equal(t, []string{"docker", "exec", "-i"}, cmd.Args[:3]) + assert.Equal(t, "localhost; touch /tmp/pwned #", cmd.Args[3]) + assert.Equal(t, []string{"sh", "-c"}, cmd.Args[4:6]) + + // The nested "sh -c" command embeds values, but they are shell-quoted so + // they cannot alter control flow inside the container. + inner := cmd.Args[6] + assert.Contains(t, inner, `'--dbname=sg; rm -rf /'`) + assert.Contains(t, inner, "'--username=sg`whoami`'") + assert.Contains(t, inner, `PGPASSWORD='sg$(id)'`) +} + +func TestKubectlCommandNoInjection(t *testing.T) { + builder, key := Builder("kubectl", RestoreCommand) + require.Equal(t, "k8s", key) + + cmd, err := builder(maliciousTarget()) + require.NoError(t, err) + + assert.Equal(t, []string{"kubectl", "exec", "-i"}, cmd.Args[:3]) + assert.Equal(t, "localhost; touch /tmp/pwned #", cmd.Args[3]) + assert.Equal(t, []string{"--", "bash", "-c"}, cmd.Args[4:7]) + + inner := cmd.Args[7] + assert.Contains(t, inner, `'--dbname=sg; rm -rf /'`) +} + +func TestBuildCommandsRedirection(t *testing.T) { + builder, _ := Builder("pg_dump", DumpCommand) + targets := Targets{Pgsql: Target{DBName: "sg", Username: "sg"}} + + dumps, err := BuildCommands("out", builder, targets, true) + require.NoError(t, err) + require.NotEmpty(t, dumps) + // Dump commands write to the output file; nothing is read from stdin. + assert.Equal(t, "out/pgsql.sql", dumps[0].OutputFile) + assert.Empty(t, dumps[0].InputFile) + + restores, err := BuildCommands("out", builder, targets, false) + require.NoError(t, err) + require.NotEmpty(t, restores) + // Restore commands read from the input file; no shell redirection is used. + assert.Equal(t, "out/pgsql.sql", restores[0].InputFile) + assert.Empty(t, restores[0].OutputFile) +} + +func TestCommandStringQuotesMaliciousValues(t *testing.T) { + builder, _ := Builder("pg_dump", DumpCommand) + cmd, err := builder(maliciousTarget()) + require.NoError(t, err) + cmd.OutputFile = "out/pgsql.sql" + + s := cmd.String() + // The rendered (display-only) string quotes dangerous tokens so that even a + // copy-paste into a shell would not execute the injected commands. + assert.False(t, strings.Contains(s, "; touch /tmp/pwned # >"), + "unquoted injection leaked into rendered command: %s", s) + assert.Contains(t, s, `'--host=localhost; touch /tmp/pwned #'`) +} From f6997392e60afba6a5d75c442b35b7ffe18b2073 Mon Sep 17 00:00:00 2001 From: Carter Brainerd Date: Tue, 7 Jul 2026 10:50:25 -0400 Subject: [PATCH 2/2] fix test failing with windows paths --- internal/pgdump/pgdump_command_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/pgdump/pgdump_command_test.go b/internal/pgdump/pgdump_command_test.go index 8e8e2a241b..808a265546 100644 --- a/internal/pgdump/pgdump_command_test.go +++ b/internal/pgdump/pgdump_command_test.go @@ -1,6 +1,7 @@ package pgdump import ( + "path/filepath" "strings" "testing" @@ -77,18 +78,20 @@ func TestBuildCommandsRedirection(t *testing.T) { builder, _ := Builder("pg_dump", DumpCommand) targets := Targets{Pgsql: Target{DBName: "sg", Username: "sg"}} + wantPath := filepath.Join("out", "pgsql.sql") + dumps, err := BuildCommands("out", builder, targets, true) require.NoError(t, err) require.NotEmpty(t, dumps) // Dump commands write to the output file; nothing is read from stdin. - assert.Equal(t, "out/pgsql.sql", dumps[0].OutputFile) + assert.Equal(t, wantPath, dumps[0].OutputFile) assert.Empty(t, dumps[0].InputFile) restores, err := BuildCommands("out", builder, targets, false) require.NoError(t, err) require.NotEmpty(t, restores) // Restore commands read from the input file; no shell redirection is used. - assert.Equal(t, "out/pgsql.sql", restores[0].InputFile) + assert.Equal(t, wantPath, restores[0].InputFile) assert.Empty(t, restores[0].OutputFile) }