feat: Introduce shell metacharacter escaping for exec #491

servusdei2018 · 2024-05-13T00:19:34Z

In order to mitigate potential security vulnerabilities arising from shell injection attacks, this PR introduces a function to escape shell metacharacters which may be present in command-line arguments.

It's worth noting that two potential vulnerabilities still exist in

nvidia-container-toolkit/tools/container/nvidia-toolkit/run.go

Lines 249 to 252 in 973a663

    
           cmdline := fmt.Sprintf("%v setup %v %v\n", o.runtime, o.runtimeArgs, toolkitDir) 
        
           //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection 
        
           cmd := exec.Command("sh", "-c", cmdline)

and

nvidia-container-toolkit/tools/container/nvidia-toolkit/run.go

Lines 275 to 278 in 973a663

    
           cmdline := fmt.Sprintf("%v cleanup %v %v\n", o.runtime, o.runtimeArgs, toolkitDir) 
        
           //nolint:gosec // TODO: Can we harden this so that there is less risk of command injection 
        
           cmd := exec.Command("sh", "-c", cmdline)

where the string o.runtimeArgs is directly interpolated into cmdline, leaving it susceptible to injection attacks. To address this, a more comprehensive solution would involve reimplementing o.runtimeArgs as []string, allowing for proper sanitization using the introduced oci.Escape() function; however, this likely involves a breaking change.

servusdei2018 · 2024-09-03T17:48:13Z

cc @elezar

elezar

My one question would be whether the logic that we're implementing here exists in some third-pary package somewhere. I'm sure that runc for example needs to also check the arguments that are passed to its hooks.

servusdei2018 · 2024-10-13T18:14:00Z

Hmm I believe I did look at runc and couldn't find the exact logic they used. I also looked at Docker and they use shellescape internally.

ArangoGutierrez · 2025-07-10T10:19:28Z

Hey @servusdei2018, let's revise this valuable contribution. Recently, we have done a lot of hardening work, so it's worth revisiting your contribution, starting with a rebase

copy-pr-bot · 2025-07-11T02:46:00Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

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

servusdei2018 · 2025-07-11T03:36:39Z

Rebased @ArangoGutierrez

internal/oci/runtime_syscall_exec_test.go

internal/oci/runtime_syscall_exec.go

Copilot

Pull Request Overview

This PR introduces functions to escape shell metacharacters in command-line arguments and updates various exec invocations to use these escaping utilities to mitigate injection risks.

Adds EscapeArg and Escape functions with accompanying tests.
Updates syscall.Exec and exec.Command calls across multiple packages to use escaped arguments.
Includes a new test suite for argument escaping behavior.

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
internal/oci/runtime_syscall_exec.go	Implements `EscapeArg` and `Escape`; applies escaping in `Exec`
internal/oci/runtime_syscall_exec_test.go	Adds tests for the `Escape` function
internal/ldconfig/safe-exec_other.go	Applies `oci.Escape` to `syscall.Exec` in non-Linux SafeExec
internal/ldconfig/safe-exec_linux.go	Applies `oci.Escape` to both branches of SafeExec on Linux
cmd/nvidia-ctk-installer/.../container.go	Uses `oci.Escape` before building `exec.Command`
cmd/nvidia-container-runtime/main_test.go	Updates tests to use `oci.EscapeArg` and `oci.Escape`
cmd/nvidia-container-runtime-hook/main.go	Applies `oci.Escape` before `syscall.Exec` in hook logic

Comments suppressed due to low confidence (3)

internal/oci/runtime_syscall_exec_test.go:42

Current tests cover spaces and pipes but miss other metacharacters like backticks, dollar signs, semicolons, quotes, and newlines. Add cases for those to ensure EscapeArg handles all defined metacharacters.

			input:    []string{"echo", "Hello World", "and", "goodbye | cat"},

internal/oci/runtime_syscall_exec.go:65

[nitpick] Clarify in the docstring that Escape is intended for commands passed through a shell, not for direct use with syscall.Exec or exec.Command without a shell, to avoid confusion about its purpose.

// Escape escapes shell metacharacters in a slice of command-line arguments

internal/oci/runtime_syscall_exec.go:42

Escaping shell metacharacters is unnecessary when calling syscall.Exec directly, as execve bypasses the shell and does not interpret these characters. This transformation will corrupt arguments like spaces or dollar signs. Consider removing Escape here and relying on direct exec without shell escaping.

	args = Escape(args)

internal/oci/runtime_syscall_exec.go

ArangoGutierrez

Can you prepend to the commits that start with nit a prefix [no-relnote] , thanks

internal/oci/runtime_syscall_exec.go

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

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

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

cmd/nvidia-container-runtime/main_test.go

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

github-actions · 2025-11-05T19:35:53Z

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 30 days unless new comments are made or the stale label is removed. To skip these checks, apply the "lifecycle/frozen" label.

Copilot

Pull Request Overview

Copilot reviewed 8 out of 9 changed files in this pull request and generated 12 comments.

Copilot · 2025-11-06T04:02:52Z

internal/oci/runtime_syscall_exec.go

+// These metacharacters include: | & ; ( ) < > \t \n $ \ `
+const shellMetachars = "|&;()<> \t\n$\\`'\""
+
+// metacharRegex matches any shell metacharcter.


Corrected spelling of 'metacharcter' to 'metacharacter'.

Suggested change

// metacharRegex matches any shell metacharcter.

// metacharRegex matches any shell metacharacter.

Copilot · 2025-11-06T04:02:53Z

internal/oci/runtime_syscall_exec.go

 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)


Escaping arguments passed to syscall.Exec is incorrect. The syscall.Exec function directly invokes the execve system call, which does not interpret shell metacharacters - it passes arguments as-is to the new process. Adding backslash escaping will cause the literal backslashes to be passed to the program, breaking argument parsing. Shell escaping is only needed when passing commands through a shell interpreter (e.g., /bin/sh -c). Remove the Escape call here.

Suggested change

args = Escape(args)

// Do not escape arguments; syscall.Exec passes them as-is to the new process.

Copilot · 2025-11-06T04:02:54Z

internal/ldconfig/safe-exec_linux.go

 	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


Escaping arguments passed to syscall.Exec is incorrect. The syscall.Exec function directly invokes the execve system call without shell interpretation, so shell metacharacter escaping will cause literal backslashes to be passed to the program. Remove the oci.Escape call.

Copilot · 2025-11-06T04:02:54Z

internal/ldconfig/safe-exec_linux.go

 	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


Escaping arguments passed to syscall.Exec is incorrect. The syscall.Exec function directly invokes the execve system call without shell interpretation, so shell metacharacter escaping will cause literal backslashes to be passed to the program. Remove the oci.Escape call.

Copilot · 2025-11-06T04:02:54Z

internal/ldconfig/safe-exec_other.go

 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


Escaping arguments passed to syscall.Exec is incorrect. The syscall.Exec function directly invokes the execve system call without shell interpretation, so shell metacharacter escaping will cause literal backslashes to be passed to the program. Remove the oci.Escape call.

Suggested change

return syscall.Exec(path, oci.Escape(args), envv) //nolint:gosec

return syscall.Exec(path, args, envv) //nolint:gosec

Copilot · 2025-11-06T04:02:55Z

cmd/nvidia-container-runtime/main_test.go

-	//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"})...)


Escaping arguments passed to exec.Command is incorrect. The exec.Command function executes binaries directly without shell interpretation. The escaped strings will include literal backslashes, breaking the command. Remove the oci.Escape calls.

Copilot · 2025-11-06T04:02:55Z

cmd/nvidia-container-runtime/main_test.go

-	//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"})...)


Escaping arguments passed to exec.Command is incorrect. The exec.Command function executes binaries directly without shell interpretation. The escaped strings will include literal backslashes, breaking the command. Remove the oci.Escape calls.

Copilot · 2025-11-06T04:02:55Z

cmd/nvidia-container-runtime/main_test.go

-	//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"})...)


Escaping arguments passed to exec.Command is incorrect. The exec.Command function executes binaries directly without shell interpretation. The escaped strings will include literal backslashes, breaking the command. Remove the oci.Escape calls.

Copilot · 2025-11-06T04:02:55Z

cmd/nvidia-container-runtime/main_test.go

-	//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()})...)


Escaping arguments passed to exec.Command is incorrect. The exec.Command function executes binaries directly without shell interpretation. The escaped strings will include literal backslashes, which will be interpreted as part of the file paths, causing the cp command to fail. Remove the oci.Escape call.

Suggested change

cmd := exec.Command("cp", oci.Escape([]string{c.unmodifiedSpecFile(), c.specFilePath()})...)

cmd := exec.Command("cp", c.unmodifiedSpecFile(), c.specFilePath())

Copilot · 2025-11-06T04:02:56Z

cmd/nvidia-container-runtime-hook/main.go

 	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)


Escaping arguments passed to syscall.Exec is incorrect. The syscall.Exec function directly invokes the execve system call without shell interpretation, so shell metacharacter escaping will cause literal backslashes to be passed to the program. Remove the oci.Escape call.

Suggested change

args = oci.Escape(args)

elezar reviewed Oct 11, 2024

View reviewed changes

ArangoGutierrez requested review from ArangoGutierrez and Copilot July 10, 2025 10:18

ArangoGutierrez self-assigned this Jul 10, 2025

This comment was marked as outdated.

Sign in to view

servusdei2018 closed this Jul 11, 2025

servusdei2018 force-pushed the main branch from 5d16bae to 1a58392 Compare July 11, 2025 02:45

feat: introduce shell metacharacter escaping for exec

c27dd75

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

servusdei2018 reopened this Jul 11, 2025

servusdei2018 force-pushed the main branch from 13210f3 to a568372 Compare July 11, 2025 03:48

ArangoGutierrez requested review from Copilot and elezar July 14, 2025 09:44

This comment was marked as outdated.

Sign in to view

ArangoGutierrez requested changes Jul 14, 2025

View reviewed changes

servusdei2018 force-pushed the main branch from 9b05d8c to 6902279 Compare July 15, 2025 18:09

servusdei2018 requested a review from ArangoGutierrez July 15, 2025 18:10

servusdei2018 force-pushed the main branch from 0282b3b to b491a15 Compare July 15, 2025 20:18

ArangoGutierrez requested a review from Copilot July 16, 2025 13:22

Copilot AI reviewed Jul 16, 2025

View reviewed changes

internal/oci/runtime_syscall_exec.go Show resolved Hide resolved

ArangoGutierrez requested changes Jul 16, 2025

View reviewed changes

internal/oci/runtime_syscall_exec.go Outdated Show resolved Hide resolved

servusdei2018 added 4 commits July 16, 2025 11:44

[no-relnote] nit: move metacharRegex to package-level

9d08eac

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

[no-relnote] nit: address code review comments

8dcdf83

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

[no-relnote] nit: revert EscapeArg to global per usage outside of pac…

8c93cd8

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

[no-relnote] nit: update tests

9ab22e5

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

servusdei2018 force-pushed the main branch from b491a15 to 9ab22e5 Compare July 16, 2025 15:45

ArangoGutierrez reviewed Jul 17, 2025

View reviewed changes

cmd/nvidia-container-runtime/main_test.go Outdated Show resolved Hide resolved

[no-relnote] nit: revert oci.EscapeArg -> oci.escapeArg

ff28fcc

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

servusdei2018 requested a review from ArangoGutierrez July 26, 2025 21:23

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 5, 2025

servusdei2018 requested a review from Copilot November 6, 2025 03:55

Copilot AI reviewed Nov 6, 2025

View reviewed changes

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 6, 2025

	cmdline := fmt.Sprintf("%v setup %v %v\n", o.runtime, o.runtimeArgs, toolkitDir)

	//nolint:gosec // TODO: Can we harden this so that there is less risk of command injection
	cmd := exec.Command("sh", "-c", cmdline)

	cmdline := fmt.Sprintf("%v cleanup %v %v\n", o.runtime, o.runtimeArgs, toolkitDir)

	//nolint:gosec // TODO: Can we harden this so that there is less risk of command injection
	cmd := exec.Command("sh", "-c", cmdline)

	// metacharRegex matches any shell metacharcter.
	// metacharRegex matches any shell metacharacter.

	args = Escape(args)
	// Do not escape arguments; syscall.Exec passes them as-is to the new process.

	return syscall.Exec(path, oci.Escape(args), envv) //nolint:gosec
	return syscall.Exec(path, args, envv) //nolint:gosec

	cmd := exec.Command("cp", oci.Escape([]string{c.unmodifiedSpecFile(), c.specFilePath()})...)
	cmd := exec.Command("cp", c.unmodifiedSpecFile(), c.specFilePath())

feat: Introduce shell metacharacter escaping for exec #491

Are you sure you want to change the base?

feat: Introduce shell metacharacter escaping for exec #491

Conversation

servusdei2018 commented May 13, 2024

Uh oh!

servusdei2018 commented Sep 3, 2024

Uh oh!

elezar left a comment

Choose a reason for hiding this comment

Uh oh!

servusdei2018 commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArangoGutierrez commented Jul 10, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

copy-pr-bot bot commented Jul 11, 2025

Uh oh!

servusdei2018 commented Jul 11, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

ArangoGutierrez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Nov 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

servusdei2018 commented Oct 13, 2024 •

edited

Loading