feat: introduce shell metacharacter escaping for exec

Signed-off-by: Nate Bracy <nate@bracy.dev>

Author: servusdei2018

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)
 }
 

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.Escape1(nvidiaRuntime), 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.Escape1(nvidiaRuntime), 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.Escape1(nvidiaRuntime), 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.Escape1(nvidiaRuntime), 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(oci.Escape1("cp"), oci.Escape([]string{c.unmodifiedSpecFile(), c.specFilePath()})...)
 	err = cmd.Run()
 	if err != nil {
 		return err

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()

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) {

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
 }

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.

internal/oci/runtime_syscall_exec.go

@@ -19,16 +19,25 @@ 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)
 
 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 +50,22 @@ func (r syscallExec) Exec(args []string) error {
 func (r syscallExec) String() string {
 	return "exec"
 }
+
+// Escape1 escapes shell metacharacters in a single command-line argument.
+func Escape1(arg string) string {
+	if strings.ContainsAny(arg, shellMetachars) {
+		e := regexp.MustCompile(`([|&;()<> \t\n$\\`+"`"+`])`).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
+}

internal/oci/runtime_syscall_exec_test.go

@@ -0,0 +1,55 @@
+/*
+# Copyright (c) 2021, 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)
+			}
+		})
+	}
+}