optim(utils): sanitize and clamp subpaths to handle path escaping

pkg/application/inject/fuse/injector_test.go

@@ -264,7 +264,7 @@ var _ = Describe("Application Injector Related Tests", Label("pkg.application.in
 		// Verify post start hook
 		Expect(out.Spec.Containers[containerIdx].Lifecycle.PostStart).To(Equal(&corev1.LifecycleHandler{
 			Exec: &corev1.ExecAction{
-				Command: []string{"bash", "-c", fmt.Sprintf("time /check-mount.sh /runtime-mnt/thin/%s/%s/ thin ", dataset.Namespace, dataset.Name)},
+				Command: []string{"bash", "-c", `time "$0" "$@"`, "/check-mount.sh", fmt.Sprintf("/runtime-mnt/thin/%s/%s/", dataset.Namespace, dataset.Name), "thin", ""},
 			},
 		}))
 

pkg/application/inject/fuse/mutator/mutator_default.go

@@ -159,7 +159,7 @@ func defaultMutateDatasetVolumes(helper *helperData) (err error) {
 	}
 
 	if helper.template.FuseMountInfo.SubPath != "" {
-		mountPath = mountPath + "/" + helper.template.FuseMountInfo.SubPath
+		mountPath = filepath.Join(mountPath, helper.template.FuseMountInfo.SubPath)
 	}
 
 	mutatedDatasetVolume := corev1.Volume{

pkg/application/inject/fuse/poststart/check_fuse_app.go

@@ -17,8 +17,6 @@ limitations under the License.
 package poststart
 
 import (
-	"fmt"
-
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
@@ -145,7 +143,8 @@ func (a *ScriptGeneratorForApp) getConfigmapName() string {
 func (a *ScriptGeneratorForApp) GetPostStartCommand(mountPaths string, mountTypes string) (handler *corev1.LifecycleHandler) {
 	// Return non-null post start command only when PostStartInjeciton is enabled
 	// https://github.com/kubernetes/kubernetes/issues/25766
-	cmd := []string{"bash", "-c", fmt.Sprintf("time %s %s %s", appScriptPath, mountPaths, mountTypes)}
+	// Arguments are passed as positional parameters so that they are never re-parsed by the shell.
+	cmd := []string{"bash", "-c", `time "$0" "$@"`, appScriptPath, mountPaths, mountTypes}
 	handler = &corev1.LifecycleHandler{
 		Exec: &corev1.ExecAction{Command: cmd},
 	}

pkg/application/inject/fuse/poststart/check_fuse_default.go

@@ -17,7 +17,6 @@ limitations under the License.
 package poststart
 
 import (
-	"fmt"
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
@@ -73,7 +72,7 @@ fi
 
 count=1
 limit=30
-while ! cat /proc/self/mountinfo | grep $ConditionPathIsMountPoint | grep $MountType
+while ! cat /proc/self/mountinfo | grep -F "$ConditionPathIsMountPoint" | grep -F "$MountType"
 do
     sleep 1
     count=¬expr $count + 1¬
@@ -87,7 +86,7 @@ done
 # different with csi, as here the mount point is the parent dir of the fuse mount point, 
 subpath_check_count=1
 subpath_check_limit=30
-while [ ! -e  $ConditionPathIsMountPoint/*/$SubPath ]
+while ! ls -d "$ConditionPathIsMountPoint"/*/"$SubPath" >/dev/null 2>&1
 do
     sleep 1
     subpath_check_count=¬expr $subpath_check_count + 1¬
@@ -129,7 +128,9 @@ func NewDefaultPostStartScriptGenerator() *defaultPostStartScriptGenerator {
 
 func (g *defaultPostStartScriptGenerator) GetPostStartCommand(mountPath, mountType, subPath string) (handler *corev1.LifecycleHandler) {
 	// https://github.com/kubernetes/kubernetes/issues/25766
-	cmd := []string{"bash", "-c", fmt.Sprintf("time %s %s %s %s", g.scriptMountPath, mountPath, mountType, subPath)}
+	// Arguments are passed as positional parameters so that user-controlled values (e.g. subPath)
+	// are never re-parsed by the shell.
+	cmd := []string{"bash", "-c", `time "$0" "$@"`, g.scriptMountPath, mountPath, mountType, subPath}
 
 	return &corev1.LifecycleHandler{
 		Exec: &corev1.ExecAction{Command: cmd},

pkg/application/inject/fuse/poststart/script_gen_helper_test.go

@@ -17,8 +17,6 @@ limitations under the License.
 package poststart
 
 import (
-	"fmt"
-
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -339,15 +337,18 @@ var _ = Describe("ScriptGeneratorForApp", func() {
 
 			Expect(handler).NotTo(BeNil())
 			Expect(handler.Exec).NotTo(BeNil())
-			expectedCmd := fmt.Sprintf("time %s %s %s", appScriptPath, "/data1:/data2", "alluxio:jindo")
-			Expect(handler.Exec.Command).To(Equal([]string{"bash", "-c", expectedCmd}))
+			Expect(handler.Exec.Command).To(Equal([]string{
+				"bash", "-c", `time "$0" "$@"`, appScriptPath, "/data1:/data2", "alluxio:jindo",
+			}))
 		})
 
-		It("should include the script path in the command", func() {
+		It("should pass arguments as argv instead of interpolating them into the shell string", func() {
 			g := NewScriptGeneratorForApp("default")
-			handler := g.GetPostStartCommand("/mnt/data", "juicefs")
+			handler := g.GetPostStartCommand("/mnt/data; touch /tmp/injected", "juicefs")
 
-			Expect(handler.Exec.Command[2]).To(ContainSubstring(appScriptPath))
+			Expect(handler.Exec.Command[2]).NotTo(ContainSubstring("touch"))
+			Expect(handler.Exec.Command[3]).To(Equal(appScriptPath))
+			Expect(handler.Exec.Command[4]).To(Equal("/mnt/data; touch /tmp/injected"))
 		})
 	})
 

pkg/csi/plugins/nodeserver.go

@@ -134,9 +134,21 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 		mountType = common.AlluxioMountType
 	}
 
+	if !filepath.IsAbs(fluidPath) {
+		return nil, status.Errorf(codes.InvalidArgument, "%s must be an absolute path, but got \"%s\"", common.VolumeAttrFluidPath, fluidPath)
+	}
+	fluidPath = filepath.Clean(fluidPath)
+	if err := checkPathUnderMountRoot(common.VolumeAttrFluidPath, fluidPath); err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}
+
 	mountPath := fluidPath
 	if subPath != "" {
-		mountPath = fluidPath + "/" + subPath
+		subPath, err = utils.NormalizeSubPath(subPath)
+		if err != nil {
+			return nil, status.Errorf(codes.InvalidArgument, "invalid %s: %v", common.VolumeAttrFluidSubPath, err)
+		}
+		mountPath = filepath.Join(mountPath, subPath)
 	}
 
 	// 1. Wait the runtime fuse ready and check the sub path existence
@@ -159,6 +171,17 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 		}
 	}
 
+	// 2. Resolve the bind mount source below the FUSE mount point. Every component of subPath is
+	// opened without following symlinks, so a symlink planted anywhere under the mount point cannot
+	// redirect the mount to an arbitrary path on the host.
+	mountSource, closeMountSource, err := resolveMountSource(fluidPath, subPath)
+	if err != nil {
+		return nil, status.Error(codes.InvalidArgument, err.Error())
+	}
+	// The source pins the resolved inode only while the descriptor is open, so it must outlive the
+	// mount call below.
+	defer closeMountSource()
+
 	// use symlink
 	if useSymlink(req) {
 		if err := utils.CreateSymlink(targetPath, mountPath); err != nil {
@@ -174,9 +197,9 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis
 	// }
 
 	if readOnly {
-		args = append(args, "-o", "ro", mountPath, targetPath)
+		args = append(args, "-o", "ro", mountSource, targetPath)
 	} else {
-		args = append(args, mountPath, targetPath)
+		args = append(args, mountSource, targetPath)
 	}
 	command, err := cmdguard.Command("mount", args...)
 	if err != nil {
@@ -392,6 +415,22 @@ func (ns *nodeServer) NodeGetCapabilities(ctx context.Context, req *csi.NodeGetC
 	}, nil
 }
 
+// checkPathUnderMountRoot rejects a path that does not live under the mount root configured via
+// the MOUNT_ROOT env. The path comes from PV volume attributes, which are not under CSI's control,
+// so an arbitrary host path such as "/etc" must not be accepted for mounting.
+func checkPathUnderMountRoot(attrName, path string) error {
+	mountRoot, err := utils.GetMountRoot()
+	if err != nil {
+		return errors.Wrapf(err, "failed to get mount root for validating %s \"%s\"", attrName, path)
+	}
+
+	if !utils.IsSubPath(mountRoot, path) {
+		return fmt.Errorf("%s \"%s\" must be under the mount root \"%s\"", attrName, path, mountRoot)
+	}
+
+	return nil
+}
+
 // getRuntimeNamespacedName first checks volume context for runtime's namespace and name as a fast path.
 // If not found, it takes a fallback to query API Server and to parse the PV information.
 func (ns *nodeServer) getRuntimeNamespacedName(volumeContext map[string]string, volumeId string) (namespace string, name string, err error) {

pkg/csi/plugins/nodeserver_test.go

@@ -35,6 +35,8 @@
 	csicommon "github.com/kubernetes-csi/drivers/pkg/csi-common"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -294,6 +296,102 @@
 			})
 		})
 
+		Context("when validating fluid_path against traversal", func() {
+			It("should reject a relative fluid_path", func() {
+				req := &csi.NodePublishVolumeRequest{
+					VolumeId:   testVolumeID,
+					TargetPath: testTargetPath,
+					VolumeContext: map[string]string{
+						common.VolumeAttrFluidPath: "runtime/test-dataset",
+					},
+				}
+
+				isMountedPatch := gomonkey.ApplyFunc(utils.IsMounted, func(absPath string) (bool, error) {
+					return false, os.ErrNotExist
+				})
+				defer isMountedPatch.Reset()
+
+				_, err := ns.NodePublishVolume(context.Background(), req)
+				Expect(err).To(HaveOccurred())
+				Expect(status.Code(err)).To(Equal(codes.InvalidArgument))
+			})
+
+			It("should reject an absolute fluid_path outside the mount root", func() {
+				tempDir, err := os.MkdirTemp("", "node-publish-escape-*")
+				Expect(err).NotTo(HaveOccurred())
+				DeferCleanup(func() {
+					Expect(os.RemoveAll(tempDir)).To(Succeed())
+				})
+
+				Expect(os.Setenv(utils.MountRoot, filepath.Join(tempDir, "runtime-mnt"))).To(Succeed())
+				DeferCleanup(func() {
+					Expect(os.Unsetenv(utils.MountRoot)).To(Succeed())
+				})
+
+				isMountedPatch := gomonkey.ApplyFunc(utils.IsMounted, func(absPath string) (bool, error) {
+					return false, os.ErrNotExist
+				})
+				defer isMountedPatch.Reset()
+
+				req := &csi.NodePublishVolumeRequest{
+					VolumeId:   testVolumeID,
+					TargetPath: testTargetPath,
+					VolumeContext: map[string]string{
+						common.VolumeAttrFluidPath: "/etc",
+					},
+				}
+
+				_, err = ns.NodePublishVolume(context.Background(), req)
+				Expect(err).To(HaveOccurred())
+				Expect(status.Code(err)).To(Equal(codes.InvalidArgument))
+			})
+
+			It("should reject a mount path that is a symlink", func() {
+				tempDir, err := os.MkdirTemp("", "node-publish-symlink-*")
+				Expect(err).NotTo(HaveOccurred())
+				DeferCleanup(func() {
+					Expect(os.RemoveAll(tempDir)).To(Succeed())
+				})
+
+				mountRoot := filepath.Join(tempDir, "runtime-mnt")
+				fluidPath := filepath.Join(mountRoot, testName, "fuse")
+				Expect(os.MkdirAll(fluidPath, 0750)).To(Succeed())
+
+				// Plant a symlink under the FUSE mount point.
+				target := filepath.Join(tempDir, "target-dir")
+				Expect(os.MkdirAll(target, 0750)).To(Succeed())
+				Expect(os.Symlink(target, filepath.Join(fluidPath, "evil"))).To(Succeed())
+
+				Expect(os.Setenv(utils.MountRoot, mountRoot)).To(Succeed())
+				DeferCleanup(func() {
+					Expect(os.Unsetenv(utils.MountRoot)).To(Succeed())
+				})
+
+				isMountedPatch := gomonkey.ApplyFunc(utils.IsMounted, func(absPath string) (bool, error) {
+					return false, os.ErrNotExist
+				})
+				defer isMountedPatch.Reset()
+
+				mountReadyPatch := gomonkey.ApplyFunc(utils.CheckMountReadyAndSubPathExist, func(fluidPath string, mountType string, subPath string) error {
+					return nil
+				})
+				defer mountReadyPatch.Reset()
+
+				req := &csi.NodePublishVolumeRequest{
+					VolumeId:   testVolumeID,
+					TargetPath: filepath.Join(tempDir, "target"),
+					VolumeContext: map[string]string{
+						common.VolumeAttrFluidPath:    fluidPath,
+						common.VolumeAttrFluidSubPath: "evil",
+					},
+				}
+
+				_, err = ns.NodePublishVolume(context.Background(), req)
+				Expect(err).To(HaveOccurred())
+				Expect(status.Code(err)).To(Equal(codes.InvalidArgument))
+			})
+		})
+
 		Context("when bind mounting succeeds", func() {
 			It("should return success after creating the target path", func() {
 				tempDir, err := os.MkdirTemp("", "node-publish-success-*")
@@ -307,6 +405,12 @@
 				fakeMountPath := filepath.Join(tempDir, "mount")
 				originalPath := os.Getenv("PATH")
 
+				Expect(os.MkdirAll(fluidPath, 0750)).To(Succeed())
+				Expect(os.Setenv(utils.MountRoot, tempDir)).To(Succeed())
+				DeferCleanup(func() {
+					Expect(os.Unsetenv(utils.MountRoot)).To(Succeed())
+				})
+
 				isMountedPatch := gomonkey.ApplyFunc(utils.IsMounted, func(absPath string) (bool, error) {
 					return false, os.ErrNotExist
 				})
@@ -337,6 +441,54 @@
 				Expect(resp).NotTo(BeNil())
 				Expect(targetPath).To(BeADirectory())
 			})
+			It("should publish a subPath with a redundant leading separator", func() {
+				tempDir, err := os.MkdirTemp("", "node-publish-legacy-subpath-*")
+				Expect(err).NotTo(HaveOccurred())
+				DeferCleanup(func() {
+					Expect(os.RemoveAll(tempDir)).To(Succeed())
+				})
+
+				fluidPath := filepath.Join(tempDir, "runtime", testName)
+				targetPath := filepath.Join(tempDir, "target")
+				fakeMountPath := filepath.Join(tempDir, "mount")
+				originalPath := os.Getenv("PATH")
+
+				Expect(os.MkdirAll(filepath.Join(fluidPath, "sub-c"), 0750)).To(Succeed())
+				Expect(os.Setenv(utils.MountRoot, tempDir)).To(Succeed())
+				DeferCleanup(func() {
+					Expect(os.Unsetenv(utils.MountRoot)).To(Succeed())
+				})
+
+				isMountedPatch := gomonkey.ApplyFunc(utils.IsMounted, func(absPath string) (bool, error) {
+					return false, os.ErrNotExist
+				})
+				defer isMountedPatch.Reset()
+
+				mountReadyPatch := gomonkey.ApplyFunc(utils.CheckMountReadyAndSubPathExist, func(fluidPath string, mountType string, subPath string) error {
+					return nil
+				})
+				defer mountReadyPatch.Reset()
+
+				Expect(os.WriteFile(fakeMountPath, []byte("#!/bin/sh\nexit 0\n"), 0755)).To(Succeed())
+				Expect(os.Setenv("PATH", tempDir+string(os.PathListSeparator)+originalPath)).To(Succeed())
+				DeferCleanup(func() {
+					Expect(os.Setenv("PATH", originalPath)).To(Succeed())
+				})
+
+				req := &csi.NodePublishVolumeRequest{
+					VolumeId:   testVolumeID,
+					TargetPath: targetPath,
+					VolumeContext: map[string]string{
+						common.VolumeAttrFluidPath:    fluidPath,
+						common.VolumeAttrFluidSubPath: "/sub-c",
+					},
+				}
+
+				resp, err := ns.NodePublishVolume(context.Background(), req)
+
+				Expect(err).NotTo(HaveOccurred())
+				Expect(resp).NotTo(BeNil())
+			})
 		})
 
 		Context("when skip check mount ready is set", func() {