Merge pull request #8590 from dokku/command-injection-via-app-name

plugins/common/common.go

@@ -923,26 +923,26 @@ func IsValidAppName(appName string) error {
 		return errors.New("Please specify an app to run the command on")
 	}
 
-	r, _ := regexp.Compile("^[a-z0-9][^/:_A-Z]*$")
+	r, _ := regexp.Compile("^[a-z0-9][a-z0-9.-]*$")
 	if r.MatchString(appName) {
 		return nil
 	}
 
-	return errors.New("App name must begin with lowercase alphanumeric character, and cannot include uppercase characters, colons, or underscores")
+	return errors.New("App name must begin with lowercase alphanumeric character, and may only contain lowercase alphanumerics, dots, and hyphens")
 }
 
-// isValidAppNameOld verifies that the app name matches the old naming restrictions
-func isValidAppNameOld(appName string) error {
+// IsValidAppNameOld verifies that the app name matches the old naming restrictions
+func IsValidAppNameOld(appName string) error {
 	if appName == "" {
 		return errors.New("Please specify an app to run the command on")
 	}
 
-	r, _ := regexp.Compile("^[a-z0-9][^/:A-Z]*$")
+	r, _ := regexp.Compile("^[a-z0-9][a-z0-9._-]*$")
 	if r.MatchString(appName) {
 		return nil
 	}
 
-	return errors.New("App name must begin with lowercase alphanumeric character, and cannot include uppercase characters, or colons")
+	return errors.New("App name must begin with lowercase alphanumeric character, and may only contain lowercase alphanumerics, dots, hyphens, and underscores")
 }
 
 // AppDoesNotExist wraps error to include the app name
@@ -981,7 +981,7 @@ func VarArgs(arguments []string, skip int) []string {
 // naming conventions exists
 func VerifyAppName(appName string) error {
 	newErr := IsValidAppName(appName)
-	oldErr := isValidAppNameOld(appName)
+	oldErr := IsValidAppNameOld(appName)
 	if newErr != nil && oldErr != nil {
 		return newErr
 	}

plugins/common/common_test.go

@@ -77,6 +77,90 @@ func TestCommonVerifyAppNameInvalid(t *testing.T) {
 	Expect(VerifyAppName("1994testApp")).To(HaveOccurred())
 }
 
+func TestCommonIsValidAppName(t *testing.T) {
+	RegisterTestingT(t)
+	validNames := []string{
+		"myapp",
+		"my-app",
+		"my.app",
+		"app123",
+		"123app",
+		"my-app.prod",
+		"a",
+		"0",
+	}
+	for _, name := range validNames {
+		Expect(IsValidAppName(name)).To(Succeed(), "expected %q to be a valid app name", name)
+	}
+}
+
+func TestCommonIsValidAppNameOld(t *testing.T) {
+	RegisterTestingT(t)
+	validNames := []string{
+		"myapp",
+		"my-app",
+		"my.app",
+		"my_app",
+		"legacy_app_name",
+		"app_v2",
+	}
+	for _, name := range validNames {
+		Expect(IsValidAppNameOld(name)).To(Succeed(), "expected %q to be a valid app name", name)
+	}
+
+	invalidNames := []string{
+		"app;id",
+		"app$cmd",
+		"app|cat",
+		"app/cmd",
+		"app:cmd",
+		"App",
+		"-app",
+		"_app",
+		"",
+	}
+	for _, name := range invalidNames {
+		Expect(IsValidAppNameOld(name)).To(HaveOccurred(), "expected %q to be rejected", name)
+	}
+}
+
+func TestCommonIsValidAppNameRejectsShellMetacharacters(t *testing.T) {
+	RegisterTestingT(t)
+	invalidNames := []string{
+		"app;id",
+		"app$cmd",
+		"app`whoami`",
+		"app|cat",
+		"app&background",
+		"app>file",
+		"app<file",
+		"app(cmd)",
+		"app{cmd}",
+		"app[cmd]",
+		"app*",
+		"app?",
+		"app!",
+		"app#",
+		"app\\cmd",
+		"app\"quote",
+		"app'quote",
+		"app cmd",
+		"app\tcmd",
+		"app\ncmd",
+		"app~cmd",
+		"app/cmd",
+		"app:cmd",
+		"app_name",
+		"App",
+		"-app",
+		".app",
+		"",
+	}
+	for _, name := range invalidNames {
+		Expect(IsValidAppName(name)).To(HaveOccurred(), "expected %q to be rejected", name)
+	}
+}
+
 func TestCommonVerifyAppName(t *testing.T) {
 	RegisterTestingT(t)
 	Expect(setupTests()).To(Succeed())

plugins/common/functions

@@ -222,26 +222,14 @@ fn-is-valid-app-name() {
   declare desc="verify that the app name matches naming restrictions"
   local APP="$1"
   [[ -z "$APP" ]] && dokku_log_fail "Please specify an app to run the command on"
-  if [[ "$APP" =~ ^[a-z].* ]] || [[ "$APP" =~ ^[0-9].* ]]; then
-    if [[ ! $APP =~ [A-Z] ]] && [[ ! $APP =~ [:] ]] && [[ ! $APP =~ [_] ]]; then
-      return 0
-    fi
-  fi
-
-  return 1
+  "$PLUGIN_CORE_AVAILABLE_PATH/common/common" --quiet is-valid-app-name "$APP"
 }
 
 fn-is-valid-app-name-old() {
   declare desc="verify that the app name matches the old naming restrictions"
   local APP="$1"
   [[ -z "$APP" ]] && dokku_log_fail "Please specify an app to run the command on"
-  if [[ "$APP" =~ ^[a-z].* ]] || [[ "$APP" =~ ^[0-9].* ]]; then
-    if [[ ! $APP =~ [A-Z] ]] && [[ ! $APP =~ [:] ]]; then
-      return 0
-    fi
-  fi
-
-  return 1
+  "$PLUGIN_CORE_AVAILABLE_PATH/common/common" --quiet is-valid-app-name-old "$APP"
 }
 
 fn-plugn-trigger-exists() {
@@ -265,7 +253,7 @@ is_valid_app_name() {
     return
   fi
 
-  dokku_log_fail "App name must begin with lowercase alphanumeric character, and cannot include uppercase characters, colons, or underscores"
+  dokku_log_fail "App name must begin with lowercase alphanumeric character, and may only contain lowercase alphanumerics, dots, and hyphens"
 }
 
 is_valid_app_name_old() {
@@ -276,7 +264,7 @@ is_valid_app_name_old() {
     return
   fi
 
-  dokku_log_fail "App name must begin with lowercase alphanumeric character, and cannot include uppercase characters, or colons"
+  dokku_log_fail "App name must begin with lowercase alphanumeric character, and may only contain lowercase alphanumerics, dots, and hyphens"
 }
 
 verify_app_name() {

plugins/common/src/common/common.go

@@ -86,6 +86,12 @@ func main() {
 		} else {
 			fmt.Print("false")
 		}
+	case "is-valid-app-name":
+		appName := flag.Arg(1)
+		err = common.IsValidAppName(appName)
+	case "is-valid-app-name-old":
+		appName := flag.Arg(1)
+		err = common.IsValidAppNameOld(appName)
 	case "verify-app-name":
 		appName := flag.Arg(1)
 		err = common.VerifyAppName(appName)

plugins/git/internal-functions

@@ -464,7 +464,7 @@ fn-git-create-hook() {
 set -e
 set -o pipefail
 
-cat | DOKKU_ROOT="$DOKKU_ROOT" dokku git-hook $APP
+cat | DOKKU_ROOT="$DOKKU_ROOT" dokku git-hook "$APP"
 EOF
   chmod +x "$PRERECEIVE_HOOK"
 }

tests/unit/apps_1.bats

@@ -96,6 +96,26 @@ teardown() {
   echo "status: $status"
   assert_failure
 
+  run /bin/bash -c "dokku apps:create 'test;id'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
+  run /bin/bash -c "dokku apps:create 'test\$cmd'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
+  run /bin/bash -c "dokku apps:create 'test|cmd'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
+  run /bin/bash -c "dokku apps:create 'test&cmd'"
+  echo "output: $output"
+  echo "status: $status"
+  assert_failure
+
   run /bin/bash -c "dokku --app $TEST_APP apps:create"
   echo "output: $output"
   echo "status: $status"
@@ -156,7 +176,7 @@ teardown() {
   echo "output: $output"
   echo "status: $status"
   assert_success
-  assert_output_contains "git-hook $TEST_APP"
+  assert_output_contains "git-hook \"$TEST_APP\""
   run /bin/bash -c "dokku apps:rename $TEST_APP great-test-name"
   echo "output: $output"
   echo "status: $status"
@@ -165,7 +185,7 @@ teardown() {
   echo "output: $output"
   echo "status: $status"
   assert_success
-  assert_output_contains "git-hook great-test-name"
+  assert_output_contains "git-hook \"great-test-name\""
   run /bin/bash -c "dokku apps:list | grep $TEST_APP"
   echo "output: $output"
   echo "status: $status"