diff --git a/cmd/nvidia-container-runtime-hook/main.go b/cmd/nvidia-container-runtime-hook/main.go index 7cdb0ad83..c6529c821 100644 --- a/cmd/nvidia-container-runtime-hook/main.go +++ b/cmd/nvidia-container-runtime-hook/main.go @@ -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 ( @@ -150,8 +151,8 @@ func doPrestart() { args = append(args, rootfs) 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) + args = oci.Escape(args) + err = syscall.Exec(args[0], args, env) //nolint:gosec log.Panicln("exec failed:", err) } diff --git a/cmd/nvidia-container-runtime/main_test.go b/cmd/nvidia-container-runtime/main_test.go index ab5ddee73..3a93590ee 100644 --- a/cmd/nvidia-container-runtime/main_test.go +++ b/cmd/nvidia-container-runtime/main_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "github.com/NVIDIA/nvidia-container-toolkit/internal/modifier" + "github.com/NVIDIA/nvidia-container-toolkit/internal/oci" "github.com/NVIDIA/nvidia-container-toolkit/internal/test" ) @@ -87,8 +88,7 @@ func TestBadInput(t *testing.T) { t.Fatal(err) } - //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection - cmdCreate := exec.Command(nvidiaRuntime, "create", "--bundle") + cmdCreate := exec.Command(oci.Escape([]string{nvidiaRuntime})[0], oci.Escape([]string{"create", "--bundle"})...) //nolint:gosec t.Logf("executing: %s\n", strings.Join(cmdCreate.Args, " ")) err = cmdCreate.Run() require.Error(t, err, "runtime should return an error") @@ -105,8 +105,8 @@ func TestGoodInput(t *testing.T) { t.Fatalf("error generating runtime spec: %v", err) } - //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection - cmdRun := exec.Command(nvidiaRuntime, "run", "--bundle", cfg.bundlePath(), "testcontainer") + //nolint:gosec + cmdRun := exec.Command(oci.Escape([]string{nvidiaRuntime})[0], oci.Escape([]string{"run", "--bundle", cfg.bundlePath(), "testcontainer"})...) t.Logf("executing: %s\n", strings.Join(cmdRun.Args, " ")) output, err := cmdRun.CombinedOutput() require.NoErrorf(t, err, "runtime should not return an error", "output=%v", string(output)) @@ -116,8 +116,8 @@ func TestGoodInput(t *testing.T) { require.NoError(t, err, "should be no errors when reading and parsing spec from config.json") require.Empty(t, spec.Hooks, "there should be no hooks in config.json") - //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection - cmdCreate := exec.Command(nvidiaRuntime, "create", "--bundle", cfg.bundlePath(), "testcontainer") + //nolint:gosec + cmdCreate := exec.Command(oci.Escape([]string{nvidiaRuntime})[0], oci.Escape([]string{"create", "--bundle", cfg.bundlePath(), "testcontainer"})...) t.Logf("executing: %s\n", strings.Join(cmdCreate.Args, " ")) err = cmdCreate.Run() require.NoError(t, err, "runtime should not return an error") @@ -161,8 +161,8 @@ func TestDuplicateHook(t *testing.T) { } // Test how runtime handles already existing prestart hook in config.json - //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection - cmdCreate := exec.Command(nvidiaRuntime, "create", "--bundle", cfg.bundlePath(), "testcontainer") + //nolint:gosec + cmdCreate := exec.Command(oci.Escape([]string{nvidiaRuntime})[0], oci.Escape([]string{"create", "--bundle", cfg.bundlePath(), "testcontainer"})...) t.Logf("executing: %s\n", strings.Join(cmdCreate.Args, " ")) output, err := cmdCreate.CombinedOutput() require.NoErrorf(t, err, "runtime should not return an error", "output=%v", string(output)) @@ -230,8 +230,8 @@ func (c testConfig) generateNewRuntimeSpec() error { return err } - //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection - cmd := exec.Command("cp", c.unmodifiedSpecFile(), c.specFilePath()) + //nolint:gosec + cmd := exec.Command("cp", oci.Escape([]string{c.unmodifiedSpecFile(), c.specFilePath()})...) err = cmd.Run() if err != nil { return err diff --git a/cmd/nvidia-ctk-installer/container/container.go b/cmd/nvidia-ctk-installer/container/container.go index 0583e051a..08290b0dc 100644 --- a/cmd/nvidia-ctk-installer/container/container.go +++ b/cmd/nvidia-ctk-installer/container/container.go @@ -24,6 +24,7 @@ import ( "github.com/sirupsen/logrus" "github.com/NVIDIA/nvidia-container-toolkit/cmd/nvidia-ctk-installer/container/operator" + "github.com/NVIDIA/nvidia-container-toolkit/internal/oci" "github.com/NVIDIA/nvidia-container-toolkit/pkg/config/engine" ) @@ -147,8 +148,8 @@ func (o Options) SystemdRestart(service string) error { 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:]...) + args = oci.Escape(args) + cmd := exec.Command(args[0], args[1:]...) //nolint:gosec cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr err := cmd.Run() diff --git a/internal/ldconfig/safe-exec_linux.go b/internal/ldconfig/safe-exec_linux.go index 09b6cc22c..a430e122c 100644 --- a/internal/ldconfig/safe-exec_linux.go +++ b/internal/ldconfig/safe-exec_linux.go @@ -25,20 +25,20 @@ import ( "syscall" "github.com/opencontainers/runc/libcontainer/exeseal" + + "github.com/NVIDIA/nvidia-container-toolkit/internal/oci" ) // SafeExec attempts to clone the specified binary (as an memfd, for example) before executing it. func SafeExec(path string, args []string, envv []string) error { safeExe, err := cloneBinary(path) if err != nil { - //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection - return syscall.Exec(path, args, envv) + return syscall.Exec(path, oci.Escape(args), envv) //nolint:gosec } defer safeExe.Close() exePath := "/proc/self/fd/" + strconv.Itoa(int(safeExe.Fd())) - //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection - return syscall.Exec(exePath, args, envv) + return syscall.Exec(exePath, oci.Escape(args), envv) //nolint:gosec } func cloneBinary(path string) (*os.File, error) { diff --git a/internal/ldconfig/safe-exec_other.go b/internal/ldconfig/safe-exec_other.go index 3d0176458..245c1e207 100644 --- a/internal/ldconfig/safe-exec_other.go +++ b/internal/ldconfig/safe-exec_other.go @@ -18,11 +18,14 @@ package ldconfig -import "syscall" +import ( + "syscall" + + "github.com/NVIDIA/nvidia-container-toolkit/internal/oci" +) // SafeExec is not implemented on non-linux systems and forwards directly to the // Exec syscall. func SafeExec(path string, args []string, envv []string) error { - //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection - return syscall.Exec(path, args, envv) + return syscall.Exec(path, oci.Escape(args), envv) //nolint:gosec } diff --git a/internal/nvsandboxutils/cgo_helpers_static.go b/internal/nvsandboxutils/cgo_helpers_static.go index 5924d6227..34e7d2e17 100644 --- a/internal/nvsandboxutils/cgo_helpers_static.go +++ b/internal/nvsandboxutils/cgo_helpers_static.go @@ -19,7 +19,7 @@ package nvsandboxutils var cgoAllocsUnknown = new(struct{}) func clen(n []byte) int { - for i := 0; i < len(n); i++ { + for i := range n { if n[i] == 0 { return i } diff --git a/internal/nvsandboxutils/zz_generated.api.go b/internal/nvsandboxutils/zz_generated.api.go index 631b2b05a..1f444a39c 100644 --- a/internal/nvsandboxutils/zz_generated.api.go +++ b/internal/nvsandboxutils/zz_generated.api.go @@ -20,13 +20,13 @@ package nvsandboxutils // The variables below represent package level methods from the library type. var ( - ErrorString = libnvsandboxutils.ErrorString + ErrorString = libnvsandboxutils.ErrorString GetDriverVersion = libnvsandboxutils.GetDriverVersion - GetFileContent = libnvsandboxutils.GetFileContent - GetGpuResource = libnvsandboxutils.GetGpuResource - Init = libnvsandboxutils.Init - LookupSymbol = libnvsandboxutils.LookupSymbol - Shutdown = libnvsandboxutils.Shutdown + GetFileContent = libnvsandboxutils.GetFileContent + GetGpuResource = libnvsandboxutils.GetGpuResource + Init = libnvsandboxutils.Init + LookupSymbol = libnvsandboxutils.LookupSymbol + Shutdown = libnvsandboxutils.Shutdown ) // Interface represents the interface for the library type. diff --git a/internal/oci/runtime_syscall_exec.go b/internal/oci/runtime_syscall_exec.go index 349edf86e..a5feb2c0c 100644 --- a/internal/oci/runtime_syscall_exec.go +++ b/internal/oci/runtime_syscall_exec.go @@ -19,16 +19,28 @@ 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$\\`'\"" + +// metacharRegex matches any shell metacharcter. +var metacharRegex = regexp.MustCompile(`([` + regexp.QuoteMeta(shellMetachars) + `])`) + type syscallExec struct{} var _ Runtime = (*syscallExec)(nil) 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) } @@ -41,3 +53,21 @@ func (r syscallExec) Exec(args []string) error { func (r syscallExec) String() string { return "exec" } + +// escapeArg escapes shell metacharacters in a single command-line argument. +func escapeArg(arg string) string { + if strings.ContainsAny(arg, shellMetachars) { + return metacharRegex.ReplaceAllString(arg, `\$1`) + } + 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] = escapeArg(args[i]) + } + return escaped +} diff --git a/internal/oci/runtime_syscall_exec_test.go b/internal/oci/runtime_syscall_exec_test.go new file mode 100644 index 000000000..fa83e5af9 --- /dev/null +++ b/internal/oci/runtime_syscall_exec_test.go @@ -0,0 +1,55 @@ +/* +# Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +*/ + +package oci + +import ( + "reflect" + "testing" +) + +func TestEscape(t *testing.T) { + testCases := []struct { + name string + input []string + expected []string + }{ + { + name: "Empty Slice", + input: []string{}, + expected: []string{}, + }, + { + name: "Slice with no Metacharacters", + input: []string{"ls", "-l", "/home/user"}, + expected: []string{"ls", "-l", "/home/user"}, + }, + { + name: "Slice with some Metacharacters", + input: []string{"echo", "Hello World", "and", "goodbye | cat"}, + expected: []string{"echo", `Hello\ World`, `and`, `goodbye\ \|\ cat`}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + actual := Escape(tc.input) + if !reflect.DeepEqual(actual, tc.expected) { + t.Errorf("Escape(%q) = %q; want %q", tc.input, actual, tc.expected) + } + }) + } +}