Skip to content

Commit

Permalink
fix(core): properly escape shell commands (#5811)
Browse files Browse the repository at this point in the history
* fix(core): properly escape shell commands

Shell commands need to be carefully constructed to avoid command
injection issues, and also to avoid issues with the shell interpreting
input strings, e.g. parenthesis or glob characters.

If possible, we should pass a command list for executing commands.

For untrusted input values, one should use environment variables and
access them from the shell script. Make sure to access them wrapped
around double quotes, otherwise the script is prone to command injection
attacks.

Sometimes it is needed to construct a shell script to chain multuiple
commands together. In this case, this commit introduces a utility
function `commandListToShellScript`.

It wraps every parameter in single quotes, escaping contained single quotes (for use in bash scripts). Joins the elements with a space character.

 Examples:

```
// returns `echo 'hello world'`
commandListToShellScript(["echo", "hello world"])

// returns `echo 'hello'"'"'world'`
commandListToShellScript(["echo", "hello'world"])

// returns `echo ''"'"'; exec ls /'`
commandListToShellScript(["echo", "'; exec ls /"])
```

Caveat: This is only safe if the command is directly executed. It is not safe, if you wrap the output of this in double quotes, for instance.

```
// SAFE
exec(["sh", "-c", ${commandListToShellScript(["some", "command", "--with" untrustedInput])}])
exec(["sh", "-c", dedent`
    set -e
    echo "running command..."
    ${commandListToShellScript(["some", "command", "--with" untrustedInput])}
    echo "done"
`])

// UNSAFE! don't do this

const commandWithUntrustedInput = commandListToShellScript(["some", "command", "--with" untrustedInput])
exec(["sh", "-c", `some_var="${commandWithUntrustedInput}"; echo "$some_var"`])
```

The second is UNSAFE, because we can't know that the /double quotes/ need to be escaped here.

If you can, use environment variables instead of this, to pass untrusted values to shell scripts, e.g. if you do not need to construct a command with untrusted input.

```
// SAFE
exec(["sh", "-c", `some_var="$UNTRUSTED_INPUT"; echo "$some_var"`], { env: { UNTRUSTED_INPUT: untrustedInput } })
```

* fix: tests

* revert: undo the escape of double quotes introduced in #5712

Double quotes in buildkit commands don't need to be escaped as they are not interpreted by the shell anymore now.

* fix: tests

* fix: allow empty values for build args #4081

* test: attempt fixing tests

* fix: remove unnecessary JSON escape to fix artifacts

* fix: allow globs in artifact source paths

* revert: undo unnecessary changes

* test: fix remaining tests
  • Loading branch information
stefreak committed Mar 11, 2024
1 parent dba5b06 commit a6d6534
Show file tree
Hide file tree
Showing 12 changed files with 194 additions and 60 deletions.
27 changes: 17 additions & 10 deletions core/src/plugins/container/build.ts
Expand Up @@ -155,14 +155,21 @@ export function getDockerBuildArgs(version: string, specBuildArgs: PrimitiveMap)
...specBuildArgs,
}

return Object.entries(buildArgs).map(([key, value]) => {
// 0 is falsy
if (value || value === 0) {
return `${key}=${value}`
} else {
// If the value of a build-arg is null, Docker pulls it from
// the environment: https://docs.docker.com/engine/reference/commandline/build/
return key
}
})
return Object.entries(buildArgs)
.map(([key, value]) => {
// If the value is empty, we simply don't pass it to docker
if (value === "") {
return undefined
}

// 0 is falsy
if (value || value === 0) {
return `${key}=${value}`
} else {
// If the value of a build-arg is null, Docker pulls it from
// the environment: https://docs.docker.com/engine/reference/commandline/build/
return key
}
})
.filter((x): x is string => !!x)
}
5 changes: 3 additions & 2 deletions core/src/plugins/kubernetes/container/build/buildkit.ts
Expand Up @@ -46,6 +46,7 @@ import { k8sGetContainerBuildActionOutputs } from "../handlers.js"
import { stringifyResources } from "../util.js"
import { styles } from "../../../../logger/styles.js"
import type { ResolvedBuildAction } from "../../../../actions/build.js"
import { commandListToShellScript } from "../../../../util/escape.js"

const AWS_ECR_REGEX = /^([^\.]+\.)?dkr\.ecr\.([^\.]+\.)amazonaws\.com\//i // AWS Elastic Container Registry

Expand Down Expand Up @@ -290,7 +291,7 @@ export function makeBuildkitBuildCommand({
...getBuildkitFlags(action),
]

return ["sh", "-c", `cd ${contextPath} && ${buildctlCommand.join(" ")}`]
return ["sh", "-c", `cd ${contextPath} && ${commandListToShellScript(buildctlCommand)}`]
}

export function getBuildkitFlags(action: Resolved<ContainerBuildAction>) {
Expand Down Expand Up @@ -337,7 +338,7 @@ export function getBuildkitImageFlags(
deploymentRegistryExtraSpec = ",registry.insecure=true"
}

args.push("--output", `type=image,\\"name=${imageNames.join(",")}\\",push=true${deploymentRegistryExtraSpec}`)
args.push("--output", `type=image,"name=${imageNames.join(",")}",push=true${deploymentRegistryExtraSpec}`)

for (const cache of cacheConfig) {
const cacheImageName = getCacheImageName(moduleOutputs, cache)
Expand Down
44 changes: 28 additions & 16 deletions core/src/plugins/kubernetes/container/build/kaniko.ts
Expand Up @@ -45,6 +45,7 @@ import { makePodName } from "../../util.js"
import type { ContainerBuildAction } from "../../../container/config.js"
import { defaultDockerfileName } from "../../../container/config.js"
import { styles } from "../../../../logger/styles.js"
import { commandListToShellScript } from "../../../../util/escape.js"

export const DEFAULT_KANIKO_FLAGS = ["--cache=true"]

Expand Down Expand Up @@ -249,7 +250,7 @@ export function getKanikoBuilderPodManifest({
imagePullSecrets,
sourceUrl,
podName,
commandStr,
kanikoCommand,
}: {
provider: KubernetesProvider
kanikoNamespace: string
Expand All @@ -260,7 +261,7 @@ export function getKanikoBuilderPodManifest({
}[]
sourceUrl: string
podName: string
commandStr: string
kanikoCommand: string[]
}) {
const kanikoImage = provider.config.kaniko?.image || defaultKanikoImageName
const kanikoTolerations = [...(provider.config.kaniko?.tolerations || []), builderToleration]
Expand Down Expand Up @@ -292,19 +293,29 @@ export function getKanikoBuilderPodManifest({
"/bin/sh",
"-c",
dedent`
echo "Copying from ${sourceUrl} to ${contextPath}"
mkdir -p ${contextPath}
echo "Copying from $SYNC_SOURCE_URL to $SYNC_CONTEXT_PATH"
mkdir -p "$SYNC_CONTEXT_PATH"
n=0
until [ "$n" -ge 30 ]
do
rsync ${syncArgs.join(" ")} && break
rsync ${commandListToShellScript(syncArgs)} && break
n=$((n+1))
sleep 1
done
echo "Done!"
`,
],
imagePullPolicy: "IfNotPresent",
env: [
{
name: "SYNC_SOURCE_URL",
value: sourceUrl,
},
{
name: "SYNC_CONTEXT_PATH",
value: contextPath,
},
],
volumeMounts: [
{
name: sharedVolumeName,
Expand All @@ -317,7 +328,16 @@ export function getKanikoBuilderPodManifest({
{
name: "kaniko",
image: kanikoImage,
command: ["sh", "-c", commandStr],
command: [
"/bin/sh",
"-c",
dedent`
${commandListToShellScript(kanikoCommand)};
export exitcode=$?;
${commandListToShellScript(["touch", `${sharedMountPath}/done`])};
exit $exitcode;
`,
],
volumeMounts: [
{
name: authSecretName,
Expand Down Expand Up @@ -364,15 +384,7 @@ async function runKaniko({

const podName = makePodName("kaniko", action.name)

// Escape the args so that we can safely interpolate them into the kaniko command
const argsStr = args.map((arg) => JSON.stringify(arg)).join(" ")

const commandStr = dedent`
/kaniko/executor ${argsStr};
export exitcode=$?;
touch ${sharedMountPath}/done;
exit $exitcode;
`
const kanikoCommand = ["/kaniko/executor", ...args]

const utilHostname = `${utilDeploymentName}.${utilNamespace}.svc.cluster.local`
const sourceUrl = `rsync://${utilHostname}:${utilRsyncPort}/volume/${ctx.workingCopyId}/${action.name}/`
Expand All @@ -391,7 +403,7 @@ async function runKaniko({
sourceUrl,
syncArgs,
imagePullSecrets,
commandStr,
kanikoCommand,
kanikoNamespace,
authSecretName,
})
Expand Down
2 changes: 1 addition & 1 deletion core/src/plugins/kubernetes/jib-container.ts
Expand Up @@ -168,7 +168,7 @@ async function buildAndPushViaRemote(params: BuildActionParams<"build", Containe

const { log: skopeoLog } = await runner.exec({
log,
command: ["sh", "-c", syncCommand.join(" ")],
command: syncCommand,
timeoutSec: pushTimeout + 5,
containerName: utilContainerName,
buffer: true,
Expand Down
39 changes: 24 additions & 15 deletions core/src/plugins/kubernetes/run.ts
Expand Up @@ -44,6 +44,7 @@ import type { RunResult } from "../../plugin/base.js"
import { LogLevel } from "../../logger/logger.js"
import { getResourceEvents } from "./status/events.js"
import stringify from "json-stringify-safe"
import { commandListToShellScript } from "../../util/escape.js"

// ref: https://kubernetes.io/docs/reference/labels-annotations-taints/#kubectl-kubernetes-io-default-container
export const K8_POD_DEFAULT_CONTAINER_ANNOTATION_KEY = "kubectl.kubernetes.io/default-container"
Expand Down Expand Up @@ -452,7 +453,7 @@ exec 2<&-
exec 1<>/tmp/output
exec 2>&1
${cmd.join(" ")}
${commandListToShellScript(cmd)}
`
}

Expand All @@ -466,16 +467,31 @@ ${cmd.join(" ")}
*/
function getArtifactsTarScript(artifacts: ArtifactSpec[]) {
const directoriesToCreate = artifacts.map((a) => a.target).filter((target) => !!target && target !== ".")
const tmpPath = "/tmp/.garden-artifacts-" + randomString(8)
const tmpPath = commandListToShellScript(["/tmp/.garden-artifacts-" + randomString(8)])

const createDirectoriesCommands = directoriesToCreate.map((target) =>
commandListToShellScript(["mkdir", "-p", target])
)

const copyArtifactsCommands = artifacts.map(({ source, target }) => {
const escapedTarget = commandListToShellScript([target || "."])

// Allow globs (*) in the source path
// Note: This works because `commandListToShellScript` wraps every parameter in single quotes, escaping contained single quotes.
// The string `bin/*` will be transformed to `'bin/*'` by `commandListToShellScript`. The shell would treat `*` as literal and not expand it.
// `replaceAll` transforms that string then to `'bin/'*''`, which allows the shell to expand the glob, everything else is treated as literal.
const escapedSource = commandListToShellScript([source]).replaceAll("*", "'*'")

return `cp -r ${escapedSource} ${escapedTarget} >/dev/null || true`
})

// TODO: escape the user paths somehow?
return `
rm -rf ${tmpPath} >/dev/null || true
mkdir -p ${tmpPath}
cd ${tmpPath}
touch .garden-placeholder
${directoriesToCreate.map((target) => `mkdir -p ${target}`).join("\n")}
${artifacts.map(({ source, target }) => `cp -r ${source} ${target || "."} >/dev/null || true`).join("\n")}
${createDirectoriesCommands.join("\n")}
${copyArtifactsCommands.join("\n")}
tar -c -z -f - . | cat
rm -rf ${tmpPath} >/dev/null || true
`
Expand Down Expand Up @@ -570,7 +586,7 @@ async function runWithArtifacts({
// using that (tar is not statically compiled so we can't copy that directly). Keeping this snippet around
// for that:
// await runner.exec({
// command: ["sh", "-c", `sed -n 'w ${arcPath}'; chmod +x ${arcPath}`],
// command: ["/bin/sh", "-c", `mkdir -p /.garden/bin/ && sed -n 'w /.garden/bin/arc' && chmod +x /.garden/bin/arc`],
// container: containerName,
// ignoreError: false,
// input: <binary>,
Expand All @@ -586,16 +602,11 @@ async function runWithArtifacts({
})
}

// Escape the command, so that we can safely pass it as a single string
const cmd = [...command!, ...(args || [])].map((s) => JSON.stringify(s))

try {
const commandScript = getCommandExecutionScript(cmd)

const res = await runner.exec({
// Pipe the output from the command to the /tmp/output pipe, including stderr. Some shell voodoo happening
// here, but this was the only working approach I could find after a lot of trial and error.
command: ["sh", "-c", commandScript],
command: ["sh", "-c", getCommandExecutionScript([...command!, ...(args || [])])],
containerName: mainContainerName,
log,
stdout,
Expand All @@ -617,8 +628,6 @@ async function runWithArtifacts({
})
}

const tarScript = getArtifactsTarScript(artifacts)

// Copy the artifacts
const tmpDir = await tmp.dir({ unsafeCleanup: true })

Expand Down Expand Up @@ -646,7 +655,7 @@ async function runWithArtifacts({
// Tarball the requested files and stream to the above extractor.
runner
.exec({
command: ["sh", "-c", tarScript],
command: ["sh", "-c", getArtifactsTarScript(artifacts)],
containerName: mainContainerName,
log,
stdout: extractor,
Expand Down
3 changes: 2 additions & 1 deletion core/src/plugins/kubernetes/sync.ts
Expand Up @@ -70,6 +70,7 @@ import type { GetSyncStatusResult, SyncState, SyncStatus } from "../../plugin/ha
import { ConfigurationError } from "../../exceptions.js"
import { gardenEnv } from "../../constants.js"
import { styles } from "../../logger/styles.js"
import { commandListToShellScript } from "../../util/escape.js"

export const builtInExcludes = ["/**/*.git", "**/*.garden"]

Expand Down Expand Up @@ -457,7 +458,7 @@ export async function configureSyncMode({
const initContainer = {
name: "garden-dev-init",
image: k8sSyncUtilImageName,
command: ["/bin/sh", "-c", "cp /usr/local/bin/mutagen-agent " + mutagenAgentPath],
command: ["/bin/sh", "-c", commandListToShellScript(["cp", "/usr/local/bin/mutagen-agent", mutagenAgentPath])],
imagePullPolicy: "IfNotPresent",
volumeMounts: [gardenVolumeMount],
}
Expand Down
4 changes: 3 additions & 1 deletion core/src/server/server.ts
Expand Up @@ -51,6 +51,7 @@ import { defaultServerPort } from "../commands/serve.js"
import type PTY from "@homebridge/node-pty-prebuilt-multiarch"
import pty from "@homebridge/node-pty-prebuilt-multiarch"
import { styles } from "../logger/styles.js"
import { commandListToShellScript } from "../util/escape.js"

const skipLogsForCommands = ["autocomplete"]
const serverLogName = "garden-server"
Expand Down Expand Up @@ -589,7 +590,8 @@ export class GardenServer extends EventEmitter {
}

try {
proc = pty.spawn("sh", ["-c", `sleep 1; ${command} ${args.join(" ")}`], {
// FIXME: Why do we need to sleep for 1 second?
proc = pty.spawn("sh", ["-c", `sleep 1; ${commandListToShellScript([command, ...args])}`], {
name: "xterm-256color",
cols: columns,
rows,
Expand Down
52 changes: 52 additions & 0 deletions core/src/util/escape.ts
@@ -0,0 +1,52 @@
/*
* Copyright (C) 2018-2024 Garden Technologies, Inc. <info@garden.io>
*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/.
*/

/**
* Wraps every parameter in single quotes, escaping contained single quotes (for use in bash scripts). Joins the elements with a space character.
*
* Examples:
*
* // returns `echo 'hello world'`
* commandListToShellScript(["echo", "hello world"])
*
* // returns `echo 'hello'"'"'world'`
* commandListToShellScript(["echo", "hello'world"])
*
* // returns `echo ''"'"'; exec ls /'`
* commandListToShellScript(["echo", "'; exec ls /"])
*
* Caveat: This is only safe if the command is directly executed. It is not safe, if you wrap the output of this in double quotes, for instance.
*
* // SAFE
* exec(["sh", "-c", ${commandListToShellScript(["some", "command", "--with" untrustedInput])}])
* exec(["sh", "-c", dedent`
* set -e
* echo "running command..."
* ${commandListToShellScript(["some", "command", "--with" untrustedInput])}
* echo "done"
* `])
*
* // UNSAFE! don't do this
*
* const commandWithUntrustedInput = commandListToShellScript(["some", "command", "--with" untrustedInput])
* exec(["sh", "-c", `some_var="${commandWithUntrustedInput}"; echo "$some_var"`])
*
* The second is UNSAFE, because we can't know that the /double quotes/ need to be escaped here.
*
* If you can, use environment variables instead of this, to pass untrusted values to shell scripts, e.g. if you do not need to construct a command with untrusted input.
*
* // SAFE
*
* exec(["sh", "-c", `some_var="$UNTRUSTED_INPUT"; echo "$some_var"`], { env: { UNTRUSTED_INPUT: untrustedInput } })
*
* @param command array of command line arguments
* @returns string to be used as shell script statement to execute the given command.
*/
export function commandListToShellScript(command: string[]) {
return command.map((c) => `'${c.replaceAll("'", `'"'"'`)}'`).join(" ")
}
Expand Up @@ -452,7 +452,7 @@ describe("kubernetes container deployment handlers", () => {
{
name: "garden-dev-init",
image: getK8sSyncUtilImageName(),
command: ["/bin/sh", "-c", "cp /usr/local/bin/mutagen-agent /.garden/mutagen-agent"],
command: ["/bin/sh", "-c", "'cp' '/usr/local/bin/mutagen-agent' '/.garden/mutagen-agent'"],
imagePullPolicy: "IfNotPresent",
volumeMounts: [
{
Expand Down

0 comments on commit a6d6534

Please sign in to comment.