fix: prevent host shell injection from app.json cron commands
The docker-local scheduler wrote each app.json cron command verbatim into the dokku user's crontab, where cron's `bash -c` interpreted any shell metacharacters in the command on the host as the dokku user rather than inside the container. The crontab line is now `dokku cron:run <app> <cron-id>`, and the command is resolved from app.json and exec'd inside the container at run time. Commands containing shell operators are also rejected when app.json is validated at deploy time.
This commit is contained in:
Jose Diaz-Gonzalez
@@ -59,6 +59,7 @@ When running scheduled cron tasks, there are a few items to be aware of:
|
||||
- Each scheduled task is executed within a one-off `run` container, and thus inherit any docker-options specified for `run` containers. Resources are never shared between scheduled tasks.
|
||||
- Scheduled cron tasks are supported on a per-scheduler basis, and are currently only implemented by the `docker-local` scheduler.
|
||||
- Tasks for _all_ apps managed by the `docker-local` scheduler are written to a single crontab file owned by the `dokku` user. The `dokku` user's crontab should be considered reserved for this purpose.
|
||||
- The `command` is tokenized and exec'd directly inside the container. Shell features such as `;`, `&&`, `|`, and `>` are _not_ interpreted. Commands that contain a bare shell operator are rejected when `app.json` is validated at deploy time, so a malformed cron command will fail the deploy rather than silently fail to run. If shell semantics are required, wrap the command explicitly, for example `"sh -c 'do-thing > /var/log/x.log'"`.
|
||||
|
||||
|
||||
### Changing cron management settings
|
||||
|
||||
@@ -10,8 +10,20 @@ import (
|
||||
|
||||
"github.com/multiformats/go-base36"
|
||||
cronparser "github.com/robfig/cron/v3"
|
||||
"mvdan.cc/sh/v3/shell"
|
||||
)
|
||||
|
||||
// ValidateCronCommand returns an error if the command cannot be tokenised
|
||||
// as a sequence of shell words. cron:run dispatches with the same parser,
|
||||
// so a command that fails here will also fail to execute - validating at
|
||||
// deploy time surfaces the error before the schedule fires.
|
||||
func ValidateCronCommand(command string) error {
|
||||
_, err := shell.Fields(command, func(name string) string {
|
||||
return ""
|
||||
})
|
||||
return err
|
||||
}
|
||||
|
||||
var (
|
||||
// DefaultProperties is a map of all valid cron properties with corresponding default property values
|
||||
DefaultProperties = map[string]string{
|
||||
@@ -75,7 +87,7 @@ func (t CronTask) DokkuRunCommand() string {
|
||||
return t.AltCommand
|
||||
}
|
||||
|
||||
return fmt.Sprintf("dokku run --concurrency-policy %s --cron-id %s %s %s", t.ConcurrencyPolicy, t.ID, t.App, t.Command)
|
||||
return fmt.Sprintf("dokku cron:run %s %s", t.App, t.ID)
|
||||
}
|
||||
|
||||
// FetchCronTasksInput is the input for the FetchCronTasks function
|
||||
@@ -138,6 +150,10 @@ func FetchCronTasks(input FetchCronTasksInput) ([]CronTask, error) {
|
||||
return tasks, fmt.Errorf("Invalid cron schedule for app %s (schedule %s): %s", appName, c.Schedule, err.Error())
|
||||
}
|
||||
|
||||
if err := ValidateCronCommand(c.Command); err != nil {
|
||||
return tasks, fmt.Errorf("Invalid cron command for app %s (command %q): %s", appName, c.Command, err.Error())
|
||||
}
|
||||
|
||||
cronID := GenerateCommandID(appName, c)
|
||||
maintenance := c.Maintenance
|
||||
if value, ok := properties[MaintenancePropertyPrefix+cronID]; ok {
|
||||
|
||||
102
plugins/cron/cron_test.go
Normal file
102
plugins/cron/cron_test.go
Normal file
@@ -0,0 +1,102 @@
|
||||
package cron
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestDokkuRunCommandAppTaskDispatchesViaCronRun(t *testing.T) {
|
||||
task := CronTask{
|
||||
App: "myapp",
|
||||
ID: "abc123",
|
||||
Command: "echo CRON_OK; echo hi > /tmp/appjson-test.txt",
|
||||
Schedule: "* * * * *",
|
||||
ConcurrencyPolicy: "allow",
|
||||
}
|
||||
|
||||
got := task.DokkuRunCommand()
|
||||
want := "dokku cron:run myapp abc123"
|
||||
if got != want {
|
||||
t.Errorf("DokkuRunCommand() = %q, want %q", got, want)
|
||||
}
|
||||
|
||||
if strings.Contains(got, task.Command) {
|
||||
t.Errorf("DokkuRunCommand() leaked user command into crontab line: %q", got)
|
||||
}
|
||||
if strings.ContainsAny(got, ";>|&`$") {
|
||||
t.Errorf("DokkuRunCommand() contains shell metacharacters: %q", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDokkuRunCommandPlainCommandStillUsesCronRun(t *testing.T) {
|
||||
task := CronTask{
|
||||
App: "myapp",
|
||||
ID: "abc123",
|
||||
Command: "npm run send-email",
|
||||
Schedule: "@daily",
|
||||
ConcurrencyPolicy: "forbid",
|
||||
}
|
||||
|
||||
got := task.DokkuRunCommand()
|
||||
want := "dokku cron:run myapp abc123"
|
||||
if got != want {
|
||||
t.Errorf("DokkuRunCommand() = %q, want %q", got, want)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDokkuRunCommandAltCommandUnchanged(t *testing.T) {
|
||||
task := CronTask{
|
||||
ID: "abc123",
|
||||
AltCommand: "/usr/bin/some-internal-task --flag",
|
||||
}
|
||||
|
||||
got := task.DokkuRunCommand()
|
||||
want := "/usr/bin/some-internal-task --flag"
|
||||
if got != want {
|
||||
t.Errorf("DokkuRunCommand() = %q, want %q", got, want)
|
||||
}
|
||||
}
|
||||
|
||||
func TestValidateCronCommandAcceptsValidCommands(t *testing.T) {
|
||||
cases := []string{
|
||||
"python3 task.py schedule",
|
||||
"npm run send-email",
|
||||
"sh -c 'echo CRON_OK; echo hi > /tmp/x.txt'",
|
||||
`node -e 'console.log(1)'`,
|
||||
"true",
|
||||
}
|
||||
for _, cmd := range cases {
|
||||
if err := ValidateCronCommand(cmd); err != nil {
|
||||
t.Errorf("ValidateCronCommand(%q) returned error: %v", cmd, err)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestValidateCronCommandRejectsShellOperators(t *testing.T) {
|
||||
cases := []string{
|
||||
"echo CRON_OK; echo hi > /tmp/x.txt",
|
||||
"cmd1 && cmd2",
|
||||
"cmd | other",
|
||||
"cmd > file",
|
||||
"cmd $(other)",
|
||||
}
|
||||
for _, cmd := range cases {
|
||||
if err := ValidateCronCommand(cmd); err == nil {
|
||||
t.Errorf("ValidateCronCommand(%q) accepted a command containing a shell operator", cmd)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestDokkuRunCommandAltCommandWithLogFile(t *testing.T) {
|
||||
task := CronTask{
|
||||
ID: "abc123",
|
||||
AltCommand: "/usr/bin/some-internal-task",
|
||||
LogFile: "/var/log/dokku/internal-task.log",
|
||||
}
|
||||
|
||||
got := task.DokkuRunCommand()
|
||||
want := "/usr/bin/some-internal-task &>> /var/log/dokku/internal-task.log"
|
||||
if got != want {
|
||||
t.Errorf("DokkuRunCommand() = %q, want %q", got, want)
|
||||
}
|
||||
}
|
||||
@@ -52,7 +52,7 @@ func CommandList(appName string, format string) error {
|
||||
maintenance = "true (app)"
|
||||
}
|
||||
}
|
||||
output = append(output, fmt.Sprintf("%s | %s | %s | %t | %s", task.ID, task.Schedule, task.ConcurrencyPolicy, maintenance, task.Command))
|
||||
output = append(output, fmt.Sprintf("%s | %s | %s | %s | %s", task.ID, task.Schedule, task.ConcurrencyPolicy, maintenance, task.Command))
|
||||
}
|
||||
|
||||
result := columnize.SimpleFormat(output)
|
||||
@@ -139,6 +139,7 @@ func CommandRun(appName string, cronID string, detached bool) error {
|
||||
os.Setenv("DOKKU_CONCURRENCY_POLICY", concurrencyPolicy)
|
||||
os.Setenv("DOKKU_CRON_ID", cronID)
|
||||
os.Setenv("DOKKU_RM_CONTAINER", "1")
|
||||
os.Setenv("DOKKU_RUN_TTL_SECONDS", "86400")
|
||||
scheduler := common.GetAppScheduler(appName)
|
||||
args := append([]string{scheduler, appName, "0", "--"}, fields...)
|
||||
_, err = common.CallPlugnTrigger(common.PlugnTriggerInput{
|
||||
|
||||
@@ -82,11 +82,14 @@ teardown() {
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
|
||||
cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')"
|
||||
|
||||
run /bin/bash -c "cat /var/spool/cron/crontabs/dokku"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output_contains "python3 task.py schedule"
|
||||
assert_output_contains "dokku cron:run $TEST_APP $cron_id"
|
||||
assert_output_contains "python3 task.py schedule" 0
|
||||
}
|
||||
|
||||
@test "(cron) create [single-short]" {
|
||||
@@ -95,11 +98,14 @@ teardown() {
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
|
||||
cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')"
|
||||
|
||||
run /bin/bash -c "cat /var/spool/cron/crontabs/dokku"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output_contains "python3 task.py daily"
|
||||
assert_output_contains "dokku cron:run $TEST_APP $cron_id"
|
||||
assert_output_contains "python3 task.py daily" 0
|
||||
}
|
||||
|
||||
@test "(cron:report) --global --format json" {
|
||||
@@ -253,12 +259,17 @@ teardown() {
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
|
||||
first_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')"
|
||||
second_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[1].id')"
|
||||
|
||||
run /bin/bash -c "cat /var/spool/cron/crontabs/dokku"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output_contains "python3 task.py first"
|
||||
assert_output_contains "python3 task.py second"
|
||||
assert_output_contains "dokku cron:run $TEST_APP $first_id"
|
||||
assert_output_contains "dokku cron:run $TEST_APP $second_id"
|
||||
assert_output_contains "python3 task.py first" 0
|
||||
assert_output_contains "python3 task.py second" 0
|
||||
}
|
||||
|
||||
@test "(cron) injected entries" {
|
||||
@@ -417,38 +428,27 @@ teardown() {
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
|
||||
first_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')"
|
||||
second_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[1].id')"
|
||||
|
||||
run /bin/bash -c "cat /var/spool/cron/crontabs/dokku"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output_contains "python3 task.py first"
|
||||
assert_output_contains "python3 task.py second"
|
||||
assert_output_contains "dokku cron:run $TEST_APP $first_id"
|
||||
assert_output_contains "dokku cron:run $TEST_APP $second_id"
|
||||
|
||||
cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')"
|
||||
run /bin/bash -c "echo $cron_id"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output_exists
|
||||
|
||||
first_command="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].command')"
|
||||
run /bin/bash -c "echo $first_command"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output_exists
|
||||
|
||||
run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$cron_id"
|
||||
run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$first_id"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_failure
|
||||
|
||||
run /bin/bash -c "dokku cron:suspend $TEST_APP $cron_id"
|
||||
run /bin/bash -c "dokku cron:suspend $TEST_APP $first_id"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
|
||||
run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$cron_id"
|
||||
run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$first_id"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
@@ -458,15 +458,15 @@ teardown() {
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output_contains "python3 task.py first" 0
|
||||
assert_output_contains "python3 task.py second"
|
||||
assert_output_contains "dokku cron:run $TEST_APP $first_id" 0
|
||||
assert_output_contains "dokku cron:run $TEST_APP $second_id"
|
||||
|
||||
run /bin/bash -c "dokku cron:resume $TEST_APP $cron_id"
|
||||
run /bin/bash -c "dokku cron:resume $TEST_APP $first_id"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
|
||||
run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$cron_id"
|
||||
run /bin/bash -c "dokku cron:report $TEST_APP --cron-maintenance-$first_id"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_failure
|
||||
@@ -475,8 +475,112 @@ teardown() {
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output_contains "python3 task.py first"
|
||||
assert_output_contains "python3 task.py second"
|
||||
assert_output_contains "dokku cron:run $TEST_APP $first_id"
|
||||
assert_output_contains "dokku cron:run $TEST_APP $second_id"
|
||||
}
|
||||
|
||||
@test "(cron) invalid [command]" {
|
||||
run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP template_cron_file_injection
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_failure
|
||||
}
|
||||
|
||||
@test "(cron) crontab format [no raw command leakage]" {
|
||||
run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP template_cron_file_valid_single
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
|
||||
cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')"
|
||||
|
||||
run /bin/bash -c "cat /var/spool/cron/crontabs/dokku"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output_contains "dokku cron:run $TEST_APP $cron_id"
|
||||
# User command must never appear verbatim in the crontab - the crontab
|
||||
# only references the cron ID, and cron:run resolves the command at run
|
||||
# time and exec's it inside the container.
|
||||
assert_output_contains "python3 task.py schedule" 0
|
||||
}
|
||||
|
||||
@test "(cron) container labels regression" {
|
||||
run deploy_app python dokku@$DOKKU_DOMAIN:$TEST_APP template_cron_file_long_running
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
|
||||
cron_id="$(dokku cron:list $TEST_APP --format json | jq -r '.[0].id')"
|
||||
|
||||
run /bin/bash -c "dokku cron:run $TEST_APP $cron_id --detach"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
|
||||
run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.cron-id\" }}'"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output "$cron_id"
|
||||
|
||||
run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.concurrency-policy\" }}'"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output "allow"
|
||||
|
||||
run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.container-type\" }}'"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output "cron"
|
||||
|
||||
run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.app-name\" }}'"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output "$TEST_APP"
|
||||
|
||||
run /bin/bash -c "docker ps --filter \"label=com.dokku.cron-id=$cron_id\" -q | xargs docker inspect -f '{{ index .Config.Labels \"com.dokku.active-deadline-seconds\" }}'"
|
||||
echo "output: $output"
|
||||
echo "status: $status"
|
||||
assert_success
|
||||
assert_output "86400"
|
||||
}
|
||||
|
||||
template_cron_file_long_running() {
|
||||
local APP="$1"
|
||||
local APP_REPO_DIR="$2"
|
||||
[[ -z "$APP" ]] && local APP="$TEST_APP"
|
||||
echo "injecting long-running cron app.json -> $APP_REPO_DIR/app.json"
|
||||
cat <<EOF >"$APP_REPO_DIR/app.json"
|
||||
{
|
||||
"cron": [
|
||||
{
|
||||
"command": "sleep 30",
|
||||
"schedule": "0 0 * * *"
|
||||
}
|
||||
]
|
||||
}
|
||||
EOF
|
||||
}
|
||||
|
||||
template_cron_file_injection() {
|
||||
local APP="$1"
|
||||
local APP_REPO_DIR="$2"
|
||||
[[ -z "$APP" ]] && local APP="$TEST_APP"
|
||||
echo "injecting injection-attempt cron app.json -> $APP_REPO_DIR/app.json"
|
||||
cat <<EOF >"$APP_REPO_DIR/app.json"
|
||||
{
|
||||
"cron": [
|
||||
{
|
||||
"command": "echo CRON_OK; echo hi > /tmp/appjson-injection-test.txt",
|
||||
"schedule": "* * * * *"
|
||||
}
|
||||
]
|
||||
}
|
||||
EOF
|
||||
}
|
||||
|
||||
template_cron_file_invalid() {
|
||||
|
||||
Reference in New Issue
Block a user