Merge pull request #8672 from dokku/cron-command-injection-in-docker-local-scheduler
Prevent host shell injection from app.json cron commands
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