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

feat: Introduce shell metacharacter escaping for exec #491

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
5 changes: 3 additions & 2 deletions cmd/nvidia-container-runtime-hook/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/NVIDIA/nvidia-container-toolkit/internal/info"
"github.com/NVIDIA/nvidia-container-toolkit/internal/logger"
"github.com/NVIDIA/nvidia-container-toolkit/internal/lookup"
"github.com/NVIDIA/nvidia-container-toolkit/internal/oci"
)

var (
Expand Down Expand Up @@ -143,10 +144,10 @@ func doPrestart() {

args = append(args, fmt.Sprintf("--pid=%s", strconv.FormatUint(uint64(container.Pid), 10)))
args = append(args, rootfs)
args = oci.Escape(args)

env := append(os.Environ(), cli.Environment...)
//nolint:gosec // TODO: Can we harden this so that there is less risk of command injection?
err = syscall.Exec(args[0], args, env)
err = syscall.Exec(args[0], args, env) //nolint:gosec
log.Panicln("exec failed:", err)
}

Expand Down
4 changes: 2 additions & 2 deletions cmd/nvidia-ctk/hook/update-ldcache/update-ldcache.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,9 @@ func (m command) run(c *cli.Context, cfg *options) error {
// Explicitly specify using /etc/ld.so.conf since the host's ldconfig may
// be configured to use a different config file by default.
args = append(args, "-f", "/etc/ld.so.conf")
args = oci.Escape(args)

//nolint:gosec // TODO: Can we harden this so that there is less risk of command injection
return syscall.Exec(ldconfigPath, args, nil)
return syscall.Exec(ldconfigPath, args, nil) //nolint:gosec
}

type root string
Expand Down
35 changes: 33 additions & 2 deletions internal/oci/runtime_syscall_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,47 @@ package oci
import (
"fmt"
"os"
"regexp"
"strings"
"syscall"
)

// shellMetachars represents a set of shell metacharacters that are commonly
// used for shell scripting and may lead to security vulnerabilities if not
// properly handled.
//
// These metacharacters include: | & ; ( ) < > \t \n $ \ `
const shellMetachars = "|&;()<> \t\n$\\`"

type syscallExec struct{}

var _ Runtime = (*syscallExec)(nil)

// Escape1 escapes shell metacharacters in a single command-line argument.
func Escape1(arg string) string {
if strings.ContainsAny(arg, shellMetachars) {
// Argument contains shell metacharacters. Double quote the
// argument, and backslash-escape any characters that still have
// meaning inside of double quotes.
e := regexp.MustCompile("([$`\"\\\\])").ReplaceAllString(arg, `\$1`)
return fmt.Sprintf(`"%s"`, e)
}
return arg
}

// Escape escapes shell metacharacters in a slice of command-line arguments
// and returns a new slice containing the escaped arguments.
func Escape(args []string) []string {
escaped := make([]string, len(args))
for i := range args {
escaped[i] = Escape1(args[i])
}
return escaped
}

func (r syscallExec) Exec(args []string) error {
//nolint:gosec // TODO: Can we harden this so that there is less risk of command injection
err := syscall.Exec(args[0], args, os.Environ())
args = Escape(args)
err := syscall.Exec(args[0], args, os.Environ()) //nolint:gosec
if err != nil {
return fmt.Errorf("could not exec '%v': %v", args[0], err)
}
Expand Down
5 changes: 3 additions & 2 deletions tools/container/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/sirupsen/logrus"
"github.com/urfave/cli/v2"

"github.com/NVIDIA/nvidia-container-toolkit/internal/oci"
"github.com/NVIDIA/nvidia-container-toolkit/pkg/config/engine"
"github.com/NVIDIA/nvidia-container-toolkit/tools/container/operator"
)
Expand Down Expand Up @@ -155,11 +156,11 @@ func (o Options) SystemdRestart(service string) error {
args = append(args, "chroot", o.HostRootMount)
}
args = append(args, "systemctl", "restart", service)
args = oci.Escape(args)

logrus.Infof("Restarting %v%v using systemd: %v", service, msg, args)

//nolint:gosec // TODO: Can we harden this so that there is less risk of command injection
cmd := exec.Command(args[0], args[1:]...)
cmd := exec.Command(args[0], args[1:]...) //nolint:gosec
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
err := cmd.Run()
Expand Down
3 changes: 1 addition & 2 deletions tools/container/nvidia-toolkit/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,7 @@ func installToolkit(o *options) error {
filepath.Join(o.root, toolkitSubDir),
}

//nolint:gosec // TODO: Can we harden this so that there is less risk of command injection
cmd := exec.Command("sh", "-c", strings.Join(cmdline, " "))
cmd := exec.Command("sh", "-c", strings.Join(cmdline, " ")) //nolint:gosec
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
err := cmd.Run()
Expand Down