refactor: safe exec commands (fluid-cloudnative#3699)

* Move security command to pkg/utils/security Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Fix build Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Fix unit tests Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Move Commands to cmdguard utils package Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Rename exec pipes Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Add unit tests for exec pipes Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Add unit tests for exec pipes Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Remove unused helm utility functions Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Fix illegal sequence to pass Juicefs's health check Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> * Rename allowedEnvs to allowedExpressions Signed-off-by: trafalgarzzz <trafalgarz@outlook.com> --------- Signed-off-by: trafalgarzzz <trafalgarz@outlook.com>
cheyang · Jan 23, 2024 · d4c82f1 · d4c82f1
1 parent c53ef20
commit d4c82f1
Show file tree

Hide file tree

Showing 24 changed files with 499 additions and 515 deletions.
diff --git a/pkg/csi/plugins/nodeserver.go b/pkg/csi/plugins/nodeserver.go
@@ -30,6 +30,7 @@ import (
 	"github.com/fluid-cloudnative/fluid/pkg/common"
 	"github.com/fluid-cloudnative/fluid/pkg/ddc/base"
 	"github.com/fluid-cloudnative/fluid/pkg/utils"
+	"github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard"
 	"github.com/fluid-cloudnative/fluid/pkg/utils/dataset/volume"
 	"github.com/pkg/errors"
 	v1 "k8s.io/api/core/v1"
@@ -161,7 +162,7 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	} else {
 		args = append(args, mountPath, targetPath)
 	}
-	command, err := utils.SimpleCommand("mount", args...)
+	command, err := cmdguard.Command("mount", args...)
 	if err != nil {
 		return nil, status.Error(codes.InvalidArgument, err.Error())
 	}
@@ -511,7 +512,7 @@ func checkMountInUse(volumeName string) (bool, error) {
 
 	// TODO: refer to https://github.com/kubernetes-sigs/alibaba-cloud-csi-driver/blob/4fcb743220371de82d556ab0b67b08440b04a218/pkg/oss/utils.go#L72
 	// for a better implementation
-	command, err := utils.SimpleCommand("/usr/local/bin/check_bind_mounts.sh", volumeName)
+	command, err := cmdguard.Command("/usr/local/bin/check_bind_mounts.sh", volumeName)
 	if err != nil {
 		return inUse, err
 	}

diff --git a/pkg/ddc/alluxio/operations/base.go b/pkg/ddc/alluxio/operations/base.go
@@ -25,8 +25,9 @@ import (
 	"strings"
 	"time"
 
+	"github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard"
 	"github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient"
-	securityutil "github.com/fluid-cloudnative/fluid/pkg/utils/security"
+	securityutils "github.com/fluid-cloudnative/fluid/pkg/utils/security"
 	"github.com/go-logr/logr"
 
 	"github.com/fluid-cloudnative/fluid/pkg/utils"
@@ -505,7 +506,7 @@ func (a AlluxioFileUtils) exec(command []string, verbose bool) (stdout string, s
 
 	select {
 	case <-ch:
-		a.log.Info("execute in time", "command", securityutil.FilterCommand(command))
+		a.log.Info("execute in time", "command", securityutils.FilterCommand(command))
 	case <-ctx.Done():
 		err = fmt.Errorf("timeout when executing %v", command)
 	}
@@ -515,7 +516,8 @@ func (a AlluxioFileUtils) exec(command []string, verbose bool) (stdout string, s
 
 // execWithoutTimeout
 func (a AlluxioFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) {
-	err = utils.ValidateCommandSlice(command)
+	err = cmdguard.ValidateCommandSlice(command)
+
 	if err != nil {
 		return
 	}

diff --git a/pkg/ddc/efc/operations/base.go b/pkg/ddc/efc/operations/base.go
@@ -23,8 +23,9 @@ import (
 	"time"
 
 	"github.com/fluid-cloudnative/fluid/pkg/common"
+	"github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard"
 	"github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient"
-	securityutil "github.com/fluid-cloudnative/fluid/pkg/utils/security"
+	securityutils "github.com/fluid-cloudnative/fluid/pkg/utils/security"
 	"github.com/go-logr/logr"
 )
 
@@ -57,7 +58,7 @@ func (a EFCFileUtils) exec(command []string, verbose bool) (stdout string, stder
 
 	select {
 	case <-ch:
-		a.log.Info("execute in time", "command", securityutil.FilterCommand(command))
+		a.log.Info("execute in time", "command", securityutils.FilterCommand(command))
 	case <-ctx.Done():
 		err = fmt.Errorf("timeout when executing %v", command)
 	}
@@ -67,6 +68,11 @@ func (a EFCFileUtils) exec(command []string, verbose bool) (stdout string, stder
 
 // execWithoutTimeout
 func (a EFCFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) {
+	err = cmdguard.ValidateCommandSlice(command)
+	if err != nil {
+		return
+	}
+
 	stdout, stderr, err = kubeclient.ExecCommandInContainer(a.podName, a.container, a.namespace, command)
 	if err != nil {
 		a.log.Info("Stdout", "Command", command, "Stdout", stdout)

diff --git a/pkg/ddc/goosefs/operations/base.go b/pkg/ddc/goosefs/operations/base.go
@@ -25,7 +25,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/fluid-cloudnative/fluid/pkg/utils"
+	"github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard"
 	"github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient"
 	"github.com/go-logr/logr"
 )
@@ -502,7 +502,7 @@ func (a GooseFSFileUtils) exec(command []string, verbose bool) (stdout string, s
 
 // execWithoutTimeout
 func (a GooseFSFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) {
-	err = utils.ValidateCommandSlice(command)
+	err = cmdguard.ValidateCommandSlice(command)
 	if err != nil {
 		return
 	}

diff --git a/pkg/ddc/jindo/operations/base.go b/pkg/ddc/jindo/operations/base.go
@@ -22,7 +22,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/fluid-cloudnative/fluid/pkg/utils"
+	"github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard"
 	"github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient"
 	"github.com/go-logr/logr"
 )
@@ -67,7 +67,7 @@ func (a JindoFileUtils) exec(command []string, verbose bool) (stdout string, std
 
 // execWithoutTimeout
 func (a JindoFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) {
-	err = utils.ValidateCommandSlice(command)
+	err = cmdguard.ValidateCommandSlice(command)
 	if err != nil {
 		return
 	}

diff --git a/pkg/ddc/jindocache/operations/base.go b/pkg/ddc/jindocache/operations/base.go
@@ -22,6 +22,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard"
 	"github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient"
 	"github.com/go-logr/logr"
 )
@@ -87,6 +88,11 @@ func (a JindoFileUtils) execWithTimeOut(command []string, verbose bool, second i
 
 // execWithoutTimeout
 func (a JindoFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) {
+	err = cmdguard.ValidateCommandSlice(command)
+	if err != nil {
+		return
+	}
+
 	stdout, stderr, err = kubeclient.ExecCommandInContainer(a.podName, a.container, a.namespace, command)
 	if err != nil {
 		a.log.Info("Stdout", "Command", command, "Stdout", stdout)

diff --git a/pkg/ddc/jindofsx/operations/base.go b/pkg/ddc/jindofsx/operations/base.go
@@ -22,7 +22,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/fluid-cloudnative/fluid/pkg/utils"
+	"github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard"
 	"github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient"
 	"github.com/go-logr/logr"
 )
@@ -88,7 +88,7 @@ func (a JindoFileUtils) execWithTimeOut(command []string, verbose bool, second i
 
 // execWithoutTimeout
 func (a JindoFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) {
-	err = utils.ValidateCommandSlice(command)
+	err = cmdguard.ValidateCommandSlice(command)
 	if err != nil {
 		return
 	}

diff --git a/pkg/ddc/juicefs/operations/base.go b/pkg/ddc/juicefs/operations/base.go
@@ -26,7 +26,7 @@ import (
 
 	"github.com/go-logr/logr"
 
-	"github.com/fluid-cloudnative/fluid/pkg/utils"
+	"github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard"
 	"github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient"
 )
 
@@ -239,7 +239,7 @@ func (j JuiceFileUtils) DeleteCacheDir(dir string) (err error) {
 // GetStatus get status of volume
 func (j JuiceFileUtils) GetStatus(source string) (status string, err error) {
 	var (
-		command = []string{"/bin/sh", "-c", fmt.Sprintf("juicefs status %s", source)}
+		command = []string{"sh", "-c", fmt.Sprintf("juicefs status %s", source)}
 		stdout  string
 		stderr  string
 	)
@@ -327,7 +327,7 @@ func (j JuiceFileUtils) exec(command []string) (stdout string, stderr string, er
 // execWithoutTimeout
 func (j JuiceFileUtils) execWithoutTimeout(command []string) (stdout string, stderr string, err error) {
 	// validate the pipe command with white list
-	err = utils.ValidateCommandSlice(command)
+	err = cmdguard.ValidateCommandSlice(command)
 	if err != nil {
 		return
 	}

diff --git a/pkg/ddc/thin/operations/base.go b/pkg/ddc/thin/operations/base.go
@@ -23,7 +23,7 @@ import (
 	"strings"
 	"time"
 
-	"github.com/fluid-cloudnative/fluid/pkg/utils"
+	"github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard"
 	"github.com/fluid-cloudnative/fluid/pkg/utils/kubeclient"
 	"github.com/go-logr/logr"
 )
@@ -157,7 +157,7 @@ func (t ThinFileUtils) exec(command []string, verbose bool) (stdout string, stde
 // execWithoutTimeout
 func (t ThinFileUtils) execWithoutTimeout(command []string, verbose bool) (stdout string, stderr string, err error) {
 	// validate the pipe command with white list
-	err = utils.ValidateCommandSlice(command)
+	err = cmdguard.ValidateCommandSlice(command)
 	if err != nil {
 		return
 	}

diff --git a/pkg/utils/exec.go → pkg/utils/cmdguard/exec.go b/pkg/utils/exec.go → pkg/utils/cmdguard/exec.go
@@ -14,31 +14,42 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package utils
+package cmdguard
 
 import (
 	"fmt"
 	"os/exec"
 	"path/filepath"
 	"strings"
+
+	"github.com/go-logr/logr"
+	ctrl "sigs.k8s.io/controller-runtime"
 )
 
-func init() {
-	allowPathlist = buildPathList(allowPathlist)
-}
+var log logr.Logger
 
-// allowPathlist of safe commands
-var allowPathlist = map[string]bool{
+// allowedPathList is a whitelist of safe commands
+var allowedPathList = map[string]bool{
 	// "helm":    true,
 	"kubectl":  true,
 	"ddc-helm": true,
 	// add other commands as needed
 }
 
+var shellCommandList = map[string]bool{
+	"bash": true,
+	"sh":   true,
+}
+
 // illegalChars to check
-// var illegalChars = []string{"&", "|", ";", "$", "'", "`", "(", ")", ">>"}
 var illegalChars = []rune{'&', '|', ';', '$', '\'', '`', '(', ')', '>'}
 
+func init() {
+	allowedPathList = buildPathList(allowedPathList)
+	shellCommandList = buildPathList(shellCommandList)
+	log = ctrl.Log.WithName("utils.security")
+}
+
 // buildPathList is a function that builds a map of paths for the given pathList.
 func buildPathList(pathList map[string]bool) (targetPath map[string]bool) {
 	targetPath = make(map[string]bool)
@@ -47,7 +58,6 @@ func buildPathList(pathList map[string]bool) (targetPath map[string]bool) {
 			path, err := exec.LookPath(name)
 			if err != nil {
 				log.Info("Failed to find path %s due to %v", path, err)
-
 			} else {
 				targetPath[path] = enabled
 			}
@@ -57,19 +67,37 @@ func buildPathList(pathList map[string]bool) (targetPath map[string]bool) {
 	return
 }
 
-// SimpleCommand checks the args before creating *exec.Cmd
-func SimpleCommand(name string, arg ...string) (cmd *exec.Cmd, err error) {
-	if allowPathlist[name] {
-		cmd = exec.Command(name, arg...)
-	} else {
-		err = checkCommandArgs(arg...)
-		if err != nil {
-			return
-		}
-		cmd = exec.Command(name, arg...)
+// Command checks the args before creating *exec.Cmd
+func Command(name string, arg ...string) (cmd *exec.Cmd, err error) {
+	commandSlice := append([]string{name}, arg...)
+
+	if err = ValidateCommandSlice(commandSlice); err != nil {
+		return
 	}
 
-	return
+	return exec.Command(name, arg...), nil
+}
+
+// ValidateCommandSlice validates all the commands in the commandSlice.
+// - For command in allowedPathList, it passes validation without further checks.
+// - For a possible shell command, it calls ValidateShellCommandSlice() for detailed checks.
+// - For any other command, it checks all the command args to ensure no illegal chars exists.
+func ValidateCommandSlice(commandSlice []string) (err error) {
+	if len(commandSlice) == 0 {
+		return nil
+	}
+
+	mainCmd := commandSlice[0]
+	if allowedPathList[mainCmd] {
+		return nil
+	}
+
+	if shellCommandList[mainCmd] {
+		return ValidateShellCommandSlice(commandSlice)
+	}
+
+	// For any other mainCmd, check illegal chars in command args
+	return checkCommandArgs(commandSlice[1:]...)
 }
 
 // CheckCommandArgs is check string is valid in args