From 70d75f12e3fa4dd770ecc37f573db916d0792f85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=9C=E4=BC=9D?= Date: Wed, 12 Aug 2026 15:10:38 +0800 Subject: [PATCH 1/7] optim(utils): sanitize and clamp subpaths to prevent escaping mount roots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: 东伝 --- .../inject/fuse/mutator/mutator_default.go | 2 +- pkg/csi/plugins/nodeserver.go | 9 +++++-- pkg/ddc/base/dataset.go | 3 ++- pkg/ddc/base/dataset_test.go | 25 +++++++++++++++--- pkg/ddc/base/runtime_helper.go | 2 +- pkg/utils/mount.go | 10 +++++++ pkg/utils/mount_test.go | 26 +++++++++++++++++++ 7 files changed, 69 insertions(+), 8 deletions(-) diff --git a/pkg/application/inject/fuse/mutator/mutator_default.go b/pkg/application/inject/fuse/mutator/mutator_default.go index 2e562d4ae00..85cc3f58d43 100644 --- a/pkg/application/inject/fuse/mutator/mutator_default.go +++ b/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, utils.CleanSubPath(helper.template.FuseMountInfo.SubPath)) } mutatedDatasetVolume := corev1.Volume{ diff --git a/pkg/csi/plugins/nodeserver.go b/pkg/csi/plugins/nodeserver.go index d27cdf882a2..d51e646e4d0 100644 --- a/pkg/csi/plugins/nodeserver.go +++ b/pkg/csi/plugins/nodeserver.go @@ -134,9 +134,14 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis mountType = common.AlluxioMountType } - mountPath := fluidPath + mountPath := filepath.Clean(fluidPath) if subPath != "" { - mountPath = fluidPath + "/" + subPath + if filepath.IsAbs(subPath) { + return nil, status.Errorf(codes.InvalidArgument, "%s must be a relative path, but got \"%s\"", common.VolumeAttrFluidSubPath, subPath) + } + // Clamp subPath so that it cannot escape the FUSE mount point + subPath = utils.CleanSubPath(subPath) + mountPath = filepath.Join(mountPath, subPath) } // 1. Wait the runtime fuse ready and check the sub path existence diff --git a/pkg/ddc/base/dataset.go b/pkg/ddc/base/dataset.go index dbcec71d35a..543feca9a7c 100644 --- a/pkg/ddc/base/dataset.go +++ b/pkg/ddc/base/dataset.go @@ -22,6 +22,7 @@ import ( datav1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1" "github.com/fluid-cloudnative/fluid/pkg/common" + "github.com/fluid-cloudnative/fluid/pkg/utils" transformerutils "github.com/fluid-cloudnative/fluid/pkg/utils/transformer" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -57,7 +58,7 @@ func GetPhysicalDatasetSubPath(virtualDataset *datav1alpha1.Dataset) []string { datasetPath := strings.TrimPrefix(mount.MountPoint, string(common.RefSchema)) splitsStrings := strings.SplitAfterN(datasetPath, "/", 3) if len(splitsStrings) == 3 { - paths = append(paths, splitsStrings[2]) + paths = append(paths, utils.CleanSubPath(splitsStrings[2])) } } } diff --git a/pkg/ddc/base/dataset_test.go b/pkg/ddc/base/dataset_test.go index a866309bea5..03f64acccc4 100644 --- a/pkg/ddc/base/dataset_test.go +++ b/pkg/ddc/base/dataset_test.go @@ -100,17 +100,36 @@ var _ = Describe("Dataset", func() { Expect(GetPhysicalDatasetSubPath(dataset)).To(Equal(expected)) }, Entry("returns nested subpaths", - &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ns-b/sub-c/sub-d"}}}}, + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/sub-c/sub-d"}}}}, []string{"sub-c/sub-d"}, ), + Entry("trims the trailing slash of a subpath", + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/sub-c/"}}}}, + []string{"sub-c"}, + ), Entry("returns an empty subpath when path ends with slash", - &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ns-b/"}}}}, + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/"}}}}, []string{""}, ), Entry("returns nil when no subpath exists", - &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ns-b"}}}}, + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b"}}}}, nil, ), + Entry("returns cleaned subpath", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/../sub-c/./inner/../sub-d"}}}}, + []string{"sub-c/sub-d"}, + ), + Entry("clamps a subpath escaping the dataset root", + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/../../../etc"}}}}, + []string{"etc"}, + ), + Entry("clamps a subpath escaping the dataset root from a nested segment", + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/sub-c/../../../sub-d"}}}}, + []string{"sub-d"}, + ), + Entry("returns an empty subpath when the subpath resolves to the dataset root", + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/.."}}}}, + []string{""}, + ), ) }) diff --git a/pkg/ddc/base/runtime_helper.go b/pkg/ddc/base/runtime_helper.go index 3c835612155..cb1e1b29ab9 100644 --- a/pkg/ddc/base/runtime_helper.go +++ b/pkg/ddc/base/runtime_helper.go @@ -107,7 +107,7 @@ func (info *RuntimeInfo) getMountInfo() (path, mountType, subpath string, err er if pv.Spec.CSI != nil && len(pv.Spec.CSI.VolumeAttributes) > 0 { path = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidPath] mountType = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrMountType] - subpath = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidSubPath] + subpath = utils.CleanSubPath(pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidSubPath]) } else { err = fmt.Errorf("the pv %s is not created by fluid", pv.Name) } diff --git a/pkg/utils/mount.go b/pkg/utils/mount.go index a1fe1e03556..ac0cd0c420f 100644 --- a/pkg/utils/mount.go +++ b/pkg/utils/mount.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "strings" "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" @@ -41,6 +42,15 @@ func GetMountRoot() (string, error) { return mountRoot, nil } +// CleanSubPath clamps an untrusted sub path into a cleaned relative path. Anchoring the path at +// the root before cleaning makes any leading ".." elements collapse, so the result can never +// escape the directory it is later joined to. An empty string is returned for a sub path that +// resolves to the root itself. +func CleanSubPath(subPath string) string { + cleaned := filepath.Join(string(filepath.Separator), subPath) + return strings.TrimPrefix(cleaned, string(filepath.Separator)) +} + func CheckMountReadyAndSubPathExist(fluidPath string, mountType string, subPath string) (err error) { glog.Infof("Try to check if the mount target %s is ready", fluidPath) if fluidPath == "" { diff --git a/pkg/utils/mount_test.go b/pkg/utils/mount_test.go index 7ef2d6ddde6..1d60d8bd507 100644 --- a/pkg/utils/mount_test.go +++ b/pkg/utils/mount_test.go @@ -73,6 +73,32 @@ func TestMountRootWithoutEnvSet(t *testing.T) { } } +func TestCleanSubPath(t *testing.T) { + var testCases = []struct { + name string + input string + expected string + }{ + {"empty subPath", "", ""}, + {"plain subPath", "sub", "sub"}, + {"nested subPath", "sub/path", "sub/path"}, + {"redundant separators", "sub//path/", "sub/path"}, + {"dot elements", "./sub/./path", "sub/path"}, + {"inner parent element", "sub/inner/../path", "sub/path"}, + {"leading parent elements", "../../etc", "etc"}, + {"escaping from a nested segment", "sub/../../../path", "path"}, + {"resolves to the parent itself", "..", ""}, + {"absolute subPath", "/sub/path", "sub/path"}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if got := CleanSubPath(tc.input); got != tc.expected { + t.Errorf("CleanSubPath(%q) = %q, expected %q", tc.input, got, tc.expected) + } + }) + } +} + func TestCheckMountReady(t *testing.T) { Convey("TestCheckMountReady", t, func() { Convey("CheckMountReady success", func() { From 6432fdfe31103def45271d49e8bbd133143ceed0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=9C=E4=BC=9D?= Date: Thu, 13 Aug 2026 13:54:43 +0800 Subject: [PATCH 2/7] optim(nodeserver): validate fluid_path and reject symlink mount paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reject relative fluid_path values to ensure absolute paths - Validate fluid_path to be within the configured mount root only - Disallow fluid_path paths outside mount root Signed-off-by: 东伝 --- pkg/csi/plugins/nodeserver.go | 58 ++++++++++++++-- pkg/csi/plugins/nodeserver_test.go | 104 +++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 5 deletions(-) diff --git a/pkg/csi/plugins/nodeserver.go b/pkg/csi/plugins/nodeserver.go index d51e646e4d0..8919b9db422 100644 --- a/pkg/csi/plugins/nodeserver.go +++ b/pkg/csi/plugins/nodeserver.go @@ -134,13 +134,21 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis mountType = common.AlluxioMountType } - mountPath := filepath.Clean(fluidPath) + 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 != "" { - if filepath.IsAbs(subPath) { - return nil, status.Errorf(codes.InvalidArgument, "%s must be a relative path, but got \"%s\"", common.VolumeAttrFluidSubPath, subPath) + // filepath.IsLocal rejects an absolute subPath or one that escapes the FUSE mount point + // (e.g. contains "../"), so it cannot be used to break out of fluidPath. + if !filepath.IsLocal(subPath) { + return nil, status.Errorf(codes.InvalidArgument, "%s must be a relative path that does not escape the mount point, but got \"%s\"", common.VolumeAttrFluidSubPath, subPath) } - // Clamp subPath so that it cannot escape the FUSE mount point - subPath = utils.CleanSubPath(subPath) mountPath = filepath.Join(mountPath, subPath) } @@ -164,6 +172,14 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis } } + // 2. Reject mountPath if it is a symlink. A symlink planted under the FUSE mount point could + // otherwise redirect the bind mount or the symlink to an arbitrary path on the host. + if isSymlinkFile, err := checkSymlinkFile(mountPath); err != nil { + return nil, status.Error(codes.Internal, err.Error()) + } else if isSymlinkFile { + return nil, status.Errorf(codes.InvalidArgument, "reject mounting path %s because it is a symlink", mountPath) + } + // use symlink if useSymlink(req) { if err := utils.CreateSymlink(targetPath, mountPath); err != nil { @@ -397,6 +413,38 @@ 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 +} + +// checkSymlinkFile reports whether path is a symlink. A non-existent path or a corrupted mount point +// is treated as not a symlink. Contents under the FUSE mount point are controlled by the dataset, so +// a symlink there must not be used as the bind mount source or the target symlink, otherwise it could +// redirect to an arbitrary path on the host. +func checkSymlinkFile(path string) (bool, error) { + fi, err := os.Lstat(path) + if err != nil { + if os.IsNotExist(err) || mount.IsCorruptedMnt(err) { + return false, nil + } + return false, errors.Wrapf(err, "failed to lstat path %s", path) + } + + return fi.Mode()&os.ModeSymlink != 0, 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) { diff --git a/pkg/csi/plugins/nodeserver_test.go b/pkg/csi/plugins/nodeserver_test.go index 002e8ec8969..daaae5b667c 100644 --- a/pkg/csi/plugins/nodeserver_test.go +++ b/pkg/csi/plugins/nodeserver_test.go @@ -35,6 +35,8 @@ import ( 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 @@ var _ = Describe("NodeServer", func() { }) }) + 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 @@ var _ = Describe("NodeServer", func() { 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 }) From 076f0ba9578e260de88f5264d3f04e51f2b6fabd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=9C=E4=BC=9D?= Date: Thu, 13 Aug 2026 15:05:21 +0800 Subject: [PATCH 3/7] fix comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: 东伝 --- .../inject/fuse/mutator/mutator_default.go | 2 +- pkg/ddc/base/dataset.go | 40 +++++++++---- pkg/ddc/base/dataset_test.go | 58 +++++++++++++------ pkg/ddc/base/runtime_helper.go | 7 ++- pkg/ddc/thin/referencedataset/engine.go | 5 ++ pkg/ddc/thin/referencedataset/volume.go | 11 ++-- pkg/utils/mount.go | 10 ---- pkg/utils/mount_test.go | 26 --------- 8 files changed, 84 insertions(+), 75 deletions(-) diff --git a/pkg/application/inject/fuse/mutator/mutator_default.go b/pkg/application/inject/fuse/mutator/mutator_default.go index 85cc3f58d43..dd3873bbf45 100644 --- a/pkg/application/inject/fuse/mutator/mutator_default.go +++ b/pkg/application/inject/fuse/mutator/mutator_default.go @@ -159,7 +159,7 @@ func defaultMutateDatasetVolumes(helper *helperData) (err error) { } if helper.template.FuseMountInfo.SubPath != "" { - mountPath = filepath.Join(mountPath, utils.CleanSubPath(helper.template.FuseMountInfo.SubPath)) + mountPath = filepath.Join(mountPath, helper.template.FuseMountInfo.SubPath) } mutatedDatasetVolume := corev1.Volume{ diff --git a/pkg/ddc/base/dataset.go b/pkg/ddc/base/dataset.go index 543feca9a7c..6e6ea36b7d4 100644 --- a/pkg/ddc/base/dataset.go +++ b/pkg/ddc/base/dataset.go @@ -18,11 +18,11 @@ package base import ( "fmt" + "path/filepath" "strings" datav1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1" "github.com/fluid-cloudnative/fluid/pkg/common" - "github.com/fluid-cloudnative/fluid/pkg/utils" transformerutils "github.com/fluid-cloudnative/fluid/pkg/utils/transformer" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -51,18 +51,34 @@ func GetPhysicalDatasetFromMounts(mounts []datav1alpha1.Mount) []types.Namespace return physicalNamespacedName } -func GetPhysicalDatasetSubPath(virtualDataset *datav1alpha1.Dataset) []string { - var paths []string - for _, mount := range virtualDataset.Spec.Mounts { - if common.IsFluidRefSchema(mount.MountPoint) { - datasetPath := strings.TrimPrefix(mount.MountPoint, string(common.RefSchema)) - splitsStrings := strings.SplitAfterN(datasetPath, "/", 3) - if len(splitsStrings) == 3 { - paths = append(paths, utils.CleanSubPath(splitsStrings[2])) - } - } +func GetPhysicalDatasetSubPath(virtualDataset *datav1alpha1.Dataset) (string, error) { + if len(virtualDataset.Spec.Mounts) != 1 { + return "", fmt.Errorf("the dataset \"%s/%s\" should only have one mount", virtualDataset.Namespace, virtualDataset.Name) } - return paths + + mount := virtualDataset.Spec.Mounts[0] + if !common.IsFluidRefSchema(mount.MountPoint) { + return "", fmt.Errorf("the dataset \"%s/%s\" should only have one mount", virtualDataset.Namespace, virtualDataset.Name) + } + + datasetPath := strings.TrimPrefix(mount.MountPoint, string(common.RefSchema)) + splitsStrings := strings.SplitAfterN(datasetPath, "/", 3) + if len(splitsStrings) != 3 { + return "", nil + } + + subPath := splitsStrings[2] + if subPath == "" { + return "", nil + } + + // The raw subPath must not escape the physical dataset's mount root. filepath.IsLocal rejects + // an absolute path or one that contains a "../" escape, so it cannot be used to break out. + if !filepath.IsLocal(subPath) { + return "", fmt.Errorf("the dataset \"%s/%s\" has an invalid subPath %q: must be a relative path that does not escape the mount point", virtualDataset.Namespace, virtualDataset.Name, subPath) + } + + return subPath, nil } func CheckReferenceDataset(dataset *datav1alpha1.Dataset) (check bool, err error) { diff --git a/pkg/ddc/base/dataset_test.go b/pkg/ddc/base/dataset_test.go index 03f64acccc4..c783f595118 100644 --- a/pkg/ddc/base/dataset_test.go +++ b/pkg/ddc/base/dataset_test.go @@ -95,40 +95,60 @@ var _ = Describe("Dataset", func() { }) Describe("GetPhysicalDatasetSubPath", func() { - DescribeTable("extracts dataset subpaths", - func(dataset *datav1alpha1.Dataset, expected []string) { - Expect(GetPhysicalDatasetSubPath(dataset)).To(Equal(expected)) + DescribeTable("extracts and validates the dataset subpath", + func(dataset *datav1alpha1.Dataset, expected string, expectErr bool) { + got, err := GetPhysicalDatasetSubPath(dataset) + if expectErr { + Expect(err).To(HaveOccurred()) + return + } + Expect(err).NotTo(HaveOccurred()) + Expect(got).To(Equal(expected)) }, - Entry("returns nested subpaths", + Entry("returns a nested subpath", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/sub-c/sub-d"}}}}, - []string{"sub-c/sub-d"}, + "sub-c/sub-d", + false, ), - Entry("trims the trailing slash of a subpath", + Entry("keeps the trailing slash of a subpath", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/sub-c/"}}}}, - []string{"sub-c"}, + "sub-c/", + false, ), Entry("returns an empty subpath when path ends with slash", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/"}}}}, - []string{""}, + "", + false, ), - Entry("returns nil when no subpath exists", + Entry("returns an empty subpath when no subpath exists", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b"}}}}, - nil, - ), - Entry("returns cleaned subpath", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/../sub-c/./inner/../sub-d"}}}}, - []string{"sub-c/sub-d"}, + "", + false, ), - Entry("clamps a subpath escaping the dataset root", + Entry("rejects a subpath escaping the dataset root", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/../../../etc"}}}}, - []string{"etc"}, + "", + true, ), - Entry("clamps a subpath escaping the dataset root from a nested segment", + Entry("rejects a subpath escaping the dataset root from a nested segment", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/sub-c/../../../sub-d"}}}}, - []string{"sub-d"}, + "", + true, ), - Entry("returns an empty subpath when the subpath resolves to the dataset root", + Entry("rejects a subpath that resolves to the dataset root", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/.."}}}}, - []string{""}, + "", + true, + ), + Entry("rejects a dataset with more than one mount", + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/sub-c"}, {MountPoint: "dataset://ns-a/ds-c"}}}}, + "", + true, + ), + Entry("rejects a dataset whose only mount is not a fluid ref", + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "http://ns-a/ds-b"}}}}, + "", + true, ), ) }) diff --git a/pkg/ddc/base/runtime_helper.go b/pkg/ddc/base/runtime_helper.go index cb1e1b29ab9..ded21377b7c 100644 --- a/pkg/ddc/base/runtime_helper.go +++ b/pkg/ddc/base/runtime_helper.go @@ -18,6 +18,7 @@ package base import ( "fmt" + "path/filepath" "time" "github.com/fluid-cloudnative/fluid/pkg/common" @@ -107,7 +108,11 @@ func (info *RuntimeInfo) getMountInfo() (path, mountType, subpath string, err er if pv.Spec.CSI != nil && len(pv.Spec.CSI.VolumeAttributes) > 0 { path = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidPath] mountType = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrMountType] - subpath = utils.CleanSubPath(pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidSubPath]) + subpath = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidSubPath] + if len(subpath) != 0 && !filepath.IsLocal(subpath) { + err = fmt.Errorf("the pv %s has an invalid subPath %q: must be a relative path that does not escape the mount point", pv.Name, subpath) + return + } } else { err = fmt.Errorf("the pv %s is not created by fluid", pv.Name) } diff --git a/pkg/ddc/thin/referencedataset/engine.go b/pkg/ddc/thin/referencedataset/engine.go index 94c5da5a4b2..46d3f88bb8a 100644 --- a/pkg/ddc/thin/referencedataset/engine.go +++ b/pkg/ddc/thin/referencedataset/engine.go @@ -255,5 +255,10 @@ func (e *ReferenceDatasetEngine) checkDatasetMountSupport(dataset *v1alpha1.Data return fmt.Errorf("ThinRuntime with no profile name can only handle dataset only mounting one dataset") } + // the subPath in the reference mount point must not escape the physical dataset's mount root + if _, err := base.GetPhysicalDatasetSubPath(dataset); err != nil { + return err + } + return nil } diff --git a/pkg/ddc/thin/referencedataset/volume.go b/pkg/ddc/thin/referencedataset/volume.go index 1428815225a..ae7f8856abb 100644 --- a/pkg/ddc/thin/referencedataset/volume.go +++ b/pkg/ddc/thin/referencedataset/volume.go @@ -18,7 +18,6 @@ package referencedataset import ( "context" - "fmt" "reflect" datav1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1" @@ -87,12 +86,12 @@ func createFusePersistentVolume(ctx context.Context, client client.Client, virtu return accessModes, err } // set the sub path attribute - subPaths := base.GetPhysicalDatasetSubPath(virtualDataset) - if len(subPaths) > 1 { - return accessModes, fmt.Errorf("the dataset is not validated, only support dataset mounts which expects 1") + subPath, err := base.GetPhysicalDatasetSubPath(virtualDataset) + if err != nil { + return accessModes, err } - if len(subPaths) == 1 && subPaths[0] != "" { - copiedPvSpec.CSI.VolumeAttributes[common.VolumeAttrFluidSubPath] = subPaths[0] + if subPath != "" { + copiedPvSpec.CSI.VolumeAttributes[common.VolumeAttrFluidSubPath] = subPath } // set the accessModes diff --git a/pkg/utils/mount.go b/pkg/utils/mount.go index ac0cd0c420f..a1fe1e03556 100644 --- a/pkg/utils/mount.go +++ b/pkg/utils/mount.go @@ -21,7 +21,6 @@ import ( "fmt" "os" "os/exec" - "path/filepath" "strings" "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" @@ -42,15 +41,6 @@ func GetMountRoot() (string, error) { return mountRoot, nil } -// CleanSubPath clamps an untrusted sub path into a cleaned relative path. Anchoring the path at -// the root before cleaning makes any leading ".." elements collapse, so the result can never -// escape the directory it is later joined to. An empty string is returned for a sub path that -// resolves to the root itself. -func CleanSubPath(subPath string) string { - cleaned := filepath.Join(string(filepath.Separator), subPath) - return strings.TrimPrefix(cleaned, string(filepath.Separator)) -} - func CheckMountReadyAndSubPathExist(fluidPath string, mountType string, subPath string) (err error) { glog.Infof("Try to check if the mount target %s is ready", fluidPath) if fluidPath == "" { diff --git a/pkg/utils/mount_test.go b/pkg/utils/mount_test.go index 1d60d8bd507..7ef2d6ddde6 100644 --- a/pkg/utils/mount_test.go +++ b/pkg/utils/mount_test.go @@ -73,32 +73,6 @@ func TestMountRootWithoutEnvSet(t *testing.T) { } } -func TestCleanSubPath(t *testing.T) { - var testCases = []struct { - name string - input string - expected string - }{ - {"empty subPath", "", ""}, - {"plain subPath", "sub", "sub"}, - {"nested subPath", "sub/path", "sub/path"}, - {"redundant separators", "sub//path/", "sub/path"}, - {"dot elements", "./sub/./path", "sub/path"}, - {"inner parent element", "sub/inner/../path", "sub/path"}, - {"leading parent elements", "../../etc", "etc"}, - {"escaping from a nested segment", "sub/../../../path", "path"}, - {"resolves to the parent itself", "..", ""}, - {"absolute subPath", "/sub/path", "sub/path"}, - } - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - if got := CleanSubPath(tc.input); got != tc.expected { - t.Errorf("CleanSubPath(%q) = %q, expected %q", tc.input, got, tc.expected) - } - }) - } -} - func TestCheckMountReady(t *testing.T) { Convey("TestCheckMountReady", t, func() { Convey("CheckMountReady success", func() { From 5b640364bbfa7151da926bea290f225c25d0e998 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=B8=9C=E4=BC=9D?= Date: Fri, 14 Aug 2026 10:27:53 +0800 Subject: [PATCH 4/7] fix comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: 东伝 --- pkg/csi/plugins/nodeserver.go | 35 +++------- pkg/csi/plugins/subpath_linux.go | 113 +++++++++++++++++++++++++++++++ pkg/csi/plugins/subpath_other.go | 62 +++++++++++++++++ pkg/csi/plugins/subpath_test.go | 85 +++++++++++++++++++++++ 4 files changed, 271 insertions(+), 24 deletions(-) create mode 100644 pkg/csi/plugins/subpath_linux.go create mode 100644 pkg/csi/plugins/subpath_other.go create mode 100644 pkg/csi/plugins/subpath_test.go diff --git a/pkg/csi/plugins/nodeserver.go b/pkg/csi/plugins/nodeserver.go index 8919b9db422..4c5f41de203 100644 --- a/pkg/csi/plugins/nodeserver.go +++ b/pkg/csi/plugins/nodeserver.go @@ -172,13 +172,16 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis } } - // 2. Reject mountPath if it is a symlink. A symlink planted under the FUSE mount point could - // otherwise redirect the bind mount or the symlink to an arbitrary path on the host. - if isSymlinkFile, err := checkSymlinkFile(mountPath); err != nil { - return nil, status.Error(codes.Internal, err.Error()) - } else if isSymlinkFile { - return nil, status.Errorf(codes.InvalidArgument, "reject mounting path %s because it is a symlink", mountPath) + // 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) { @@ -195,9 +198,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 { @@ -429,22 +432,6 @@ func checkPathUnderMountRoot(attrName, path string) error { return nil } -// checkSymlinkFile reports whether path is a symlink. A non-existent path or a corrupted mount point -// is treated as not a symlink. Contents under the FUSE mount point are controlled by the dataset, so -// a symlink there must not be used as the bind mount source or the target symlink, otherwise it could -// redirect to an arbitrary path on the host. -func checkSymlinkFile(path string) (bool, error) { - fi, err := os.Lstat(path) - if err != nil { - if os.IsNotExist(err) || mount.IsCorruptedMnt(err) { - return false, nil - } - return false, errors.Wrapf(err, "failed to lstat path %s", path) - } - - return fi.Mode()&os.ModeSymlink != 0, 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) { diff --git a/pkg/csi/plugins/subpath_linux.go b/pkg/csi/plugins/subpath_linux.go new file mode 100644 index 00000000000..6b90c1b85e4 --- /dev/null +++ b/pkg/csi/plugins/subpath_linux.go @@ -0,0 +1,113 @@ +/* +Copyright 2026 The Fluid Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package plugins + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "syscall" + + "github.com/pkg/errors" + "golang.org/x/sys/unix" +) + +// openFDFlags names a path component without following it if it turns out to be a symlink. +// O_PATH keeps the open cheap and side effect free: the descriptor only names the inode and is +// never read from. Note that O_PATH|O_NOFOLLOW does not fail on a symlink, it returns a descriptor +// for the link itself, so every component is stat'ed to reject one. +const openFDFlags = unix.O_NOFOLLOW | unix.O_PATH | unix.O_CLOEXEC + +// resolveMountSource resolves subPath below base and returns a path that is safe to hand to +// mount(8) as the bind mount source, together with a closer to call once the mount is done. +// +// Every component of subPath is opened with O_NOFOLLOW relative to its parent descriptor, so a +// symlink anywhere along the way - not only in the last component - is rejected. The returned +// source is a /proc/self/fd entry for the descriptor that was verified, which pins the resolved +// inode: swapping a component for a symlink after the check no longer changes what gets mounted. +func resolveMountSource(base, subPath string) (source string, closer func(), err error) { + if subPath == "" { + return base, func() {}, nil + } + + fd, err := openBeneathNoSymlinks(base, subPath) + if err != nil { + return "", nil, err + } + + return fmt.Sprintf("/proc/%d/fd/%d", os.Getpid(), fd), func() { _ = syscall.Close(fd) }, nil +} + +// openBeneathNoSymlinks walks subPath one component at a time below base and returns a descriptor +// for the final component. It fails if base or any component is a symlink. The caller owns the +// returned descriptor. +func openBeneathNoSymlinks(base, subPath string) (int, error) { + if !filepath.IsLocal(subPath) { + return -1, fmt.Errorf("subPath %q must be a relative path that does not escape %s", subPath, base) + } + + parentFD, err := syscall.Open(base, openFDFlags, 0) + if err != nil { + return -1, errors.Wrapf(err, "failed to open mount point %s", base) + } + + succeeded := false + defer func() { + if !succeeded { + _ = syscall.Close(parentFD) + } + }() + + if err := rejectSymlinkFD(parentFD, base); err != nil { + return -1, err + } + + // filepath.Clean collapses the "." and ".." elements IsLocal tolerates as long as they resolve + // below base, so the remaining segments are all plain directory entries. + for _, segment := range strings.Split(filepath.Clean(subPath), string(filepath.Separator)) { + childFD, err := syscall.Openat(parentFD, segment, openFDFlags, 0) + if err != nil { + return -1, errors.Wrapf(err, "failed to open %q of subPath %q below mount point %s", segment, subPath, base) + } + + _ = syscall.Close(parentFD) + parentFD = childFD + + if err := rejectSymlinkFD(parentFD, filepath.Join(base, segment)); err != nil { + return -1, err + } + } + + succeeded = true + return parentFD, nil +} + +// rejectSymlinkFD returns an error if fd names a symlink. O_PATH|O_NOFOLLOW succeeds on a symlink +// and yields a descriptor for the link itself, so the check has to be explicit. +func rejectSymlinkFD(fd int, name string) error { + var stat unix.Stat_t + if err := unix.Fstat(fd, &stat); err != nil { + return errors.Wrapf(err, "failed to stat %s", name) + } + + if stat.Mode&syscall.S_IFMT == syscall.S_IFLNK { + return fmt.Errorf("%s is a symlink, which is not allowed for mounting", name) + } + + return nil +} diff --git a/pkg/csi/plugins/subpath_other.go b/pkg/csi/plugins/subpath_other.go new file mode 100644 index 00000000000..95b61c94ee2 --- /dev/null +++ b/pkg/csi/plugins/subpath_other.go @@ -0,0 +1,62 @@ +//go:build !linux + +/* +Copyright 2026 The Fluid Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package plugins + +import ( + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/pkg/errors" + "k8s.io/utils/mount" +) + +// resolveMountSource resolves subPath below base and returns the bind mount source. +// +// The CSI plugin only runs on Linux; this build exists so the package stays buildable and testable +// elsewhere. It checks every component of subPath for a symlink instead of pinning a descriptor, so +// unlike the Linux implementation it cannot rule out a component being swapped after the check. +func resolveMountSource(base, subPath string) (source string, closer func(), err error) { + if subPath == "" { + return base, func() {}, nil + } + + if !filepath.IsLocal(subPath) { + return "", nil, fmt.Errorf("subPath %q must be a relative path that does not escape %s", subPath, base) + } + + current := base + for _, segment := range strings.Split(filepath.Clean(subPath), string(filepath.Separator)) { + current = filepath.Join(current, segment) + + fi, err := os.Lstat(current) + if err != nil { + if mount.IsCorruptedMnt(err) { + return "", nil, fmt.Errorf("mount point %s is corrupted", base) + } + return "", nil, errors.Wrapf(err, "failed to lstat %q of subPath %q below mount point %s", segment, subPath, base) + } + if fi.Mode()&os.ModeSymlink != 0 { + return "", nil, fmt.Errorf("subPath %q of mount point %s contains a symlink at %q, which is not allowed", subPath, base, segment) + } + } + + return current, func() {}, nil +} diff --git a/pkg/csi/plugins/subpath_test.go b/pkg/csi/plugins/subpath_test.go new file mode 100644 index 00000000000..9d3b8fa0193 --- /dev/null +++ b/pkg/csi/plugins/subpath_test.go @@ -0,0 +1,85 @@ +/* +Copyright 2026 The Fluid Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package plugins + +import ( + "os" + "path/filepath" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("resolveMountSource", func() { + var ( + tempDir string + fluidPath string + outside string + ) + + BeforeEach(func() { + var err error + tempDir, err = os.MkdirTemp("", "resolve-mount-source-*") + Expect(err).NotTo(HaveOccurred()) + DeferCleanup(func() { + Expect(os.RemoveAll(tempDir)).To(Succeed()) + }) + + fluidPath = filepath.Join(tempDir, "runtime-mnt", "fuse") + outside = filepath.Join(tempDir, "outside") + Expect(os.MkdirAll(filepath.Join(fluidPath, "sub", "nested"), 0750)).To(Succeed()) + Expect(os.MkdirAll(outside, 0750)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(outside, "passwd"), []byte("secret"), 0600)).To(Succeed()) + }) + + It("returns the mount point itself for an empty subPath", func() { + source, closer, err := resolveMountSource(fluidPath, "") + Expect(err).NotTo(HaveOccurred()) + defer closer() + Expect(source).To(Equal(fluidPath)) + }) + + It("resolves a nested subPath", func() { + source, closer, err := resolveMountSource(fluidPath, "sub/nested") + Expect(err).NotTo(HaveOccurred()) + defer closer() + Expect(source).NotTo(BeEmpty()) + }) + + It("rejects a subPath escaping the mount point", func() { + _, _, err := resolveMountSource(fluidPath, "../../outside") + Expect(err).To(HaveOccurred()) + }) + + It("rejects a subPath whose last component is a symlink", func() { + Expect(os.Symlink(outside, filepath.Join(fluidPath, "evil"))).To(Succeed()) + + _, _, err := resolveMountSource(fluidPath, "evil") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("symlink")) + }) + + // Regression test: an Lstat of the joined path reports "passwd" as a regular file and misses + // that the "link" component already led out of the mount point. + It("rejects a subPath whose intermediate component is a symlink", func() { + Expect(os.Symlink(outside, filepath.Join(fluidPath, "link"))).To(Succeed()) + + _, _, err := resolveMountSource(fluidPath, "link/passwd") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("symlink")) + }) +}) From 273fc0cf4cc86f292e90de7c1ceda2662f144c9f Mon Sep 17 00:00:00 2001 From: TzZtzt Date: Fri, 14 Aug 2026 11:04:06 +0800 Subject: [PATCH 5/7] fix comment Signed-off-by: TzZtzt --- pkg/application/inject/fuse/injector_test.go | 2 +- .../inject/fuse/poststart/check_fuse_app.go | 5 ++--- .../inject/fuse/poststart/check_fuse_default.go | 5 +++-- .../fuse/poststart/script_gen_helper_test.go | 15 ++++++++------- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/pkg/application/inject/fuse/injector_test.go b/pkg/application/inject/fuse/injector_test.go index 038ec4a8578..d96d06f5d4a 100644 --- a/pkg/application/inject/fuse/injector_test.go +++ b/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", ""}, }, })) diff --git a/pkg/application/inject/fuse/poststart/check_fuse_app.go b/pkg/application/inject/fuse/poststart/check_fuse_app.go index a1505844032..22739fb49dc 100644 --- a/pkg/application/inject/fuse/poststart/check_fuse_app.go +++ b/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}, } diff --git a/pkg/application/inject/fuse/poststart/check_fuse_default.go b/pkg/application/inject/fuse/poststart/check_fuse_default.go index 8e222632f09..7bab2950d39 100644 --- a/pkg/application/inject/fuse/poststart/check_fuse_default.go +++ b/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" @@ -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}, diff --git a/pkg/application/inject/fuse/poststart/script_gen_helper_test.go b/pkg/application/inject/fuse/poststart/script_gen_helper_test.go index 822223aa400..eb31256fc49 100644 --- a/pkg/application/inject/fuse/poststart/script_gen_helper_test.go +++ b/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")) }) }) From 6f66e509cdeaa29faf3dc9fff5f31893c88ff3cf Mon Sep 17 00:00:00 2001 From: TzZtzt Date: Fri, 14 Aug 2026 11:46:00 +0800 Subject: [PATCH 6/7] fix comment Signed-off-by: TzZtzt --- pkg/ddc/base/dataset.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ddc/base/dataset.go b/pkg/ddc/base/dataset.go index 6e6ea36b7d4..a3cdb328856 100644 --- a/pkg/ddc/base/dataset.go +++ b/pkg/ddc/base/dataset.go @@ -58,7 +58,7 @@ func GetPhysicalDatasetSubPath(virtualDataset *datav1alpha1.Dataset) (string, er mount := virtualDataset.Spec.Mounts[0] if !common.IsFluidRefSchema(mount.MountPoint) { - return "", fmt.Errorf("the dataset \"%s/%s\" should only have one mount", virtualDataset.Namespace, virtualDataset.Name) + return "", fmt.Errorf("the dataset \"%s/%s\" mountpoint should follow the schema \"%s\"", virtualDataset.Namespace, virtualDataset.Name, common.RefSchema) } datasetPath := strings.TrimPrefix(mount.MountPoint, string(common.RefSchema)) From 55ca45547699b05d4cd0797ab607b1c6e3210e13 Mon Sep 17 00:00:00 2001 From: TzZtzt Date: Fri, 14 Aug 2026 17:40:27 +0800 Subject: [PATCH 7/7] fix comment Signed-off-by: TzZtzt --- .../fuse/poststart/check_fuse_default.go | 4 +- pkg/csi/plugins/nodeserver.go | 7 ++- pkg/csi/plugins/nodeserver_test.go | 48 +++++++++++++++++++ pkg/ddc/base/dataset.go | 11 ++--- pkg/ddc/base/dataset_test.go | 14 +++++- pkg/ddc/base/runtime_helper.go | 10 ++-- pkg/ddc/base/runtime_helper_test.go | 45 +++++++++++++++++ pkg/utils/mount.go | 21 ++++++++ pkg/utils/mount_test.go | 38 +++++++++++++++ 9 files changed, 180 insertions(+), 18 deletions(-) diff --git a/pkg/application/inject/fuse/poststart/check_fuse_default.go b/pkg/application/inject/fuse/poststart/check_fuse_default.go index 7bab2950d39..9fd060f31dd 100644 --- a/pkg/application/inject/fuse/poststart/check_fuse_default.go +++ b/pkg/application/inject/fuse/poststart/check_fuse_default.go @@ -72,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¬ @@ -86,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¬ diff --git a/pkg/csi/plugins/nodeserver.go b/pkg/csi/plugins/nodeserver.go index 4c5f41de203..dab6ddda786 100644 --- a/pkg/csi/plugins/nodeserver.go +++ b/pkg/csi/plugins/nodeserver.go @@ -144,10 +144,9 @@ func (ns *nodeServer) NodePublishVolume(ctx context.Context, req *csi.NodePublis mountPath := fluidPath if subPath != "" { - // filepath.IsLocal rejects an absolute subPath or one that escapes the FUSE mount point - // (e.g. contains "../"), so it cannot be used to break out of fluidPath. - if !filepath.IsLocal(subPath) { - return nil, status.Errorf(codes.InvalidArgument, "%s must be a relative path that does not escape the mount point, but got \"%s\"", common.VolumeAttrFluidSubPath, 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) } diff --git a/pkg/csi/plugins/nodeserver_test.go b/pkg/csi/plugins/nodeserver_test.go index daaae5b667c..1fce98be98c 100644 --- a/pkg/csi/plugins/nodeserver_test.go +++ b/pkg/csi/plugins/nodeserver_test.go @@ -441,6 +441,54 @@ var _ = Describe("NodeServer", func() { 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() { diff --git a/pkg/ddc/base/dataset.go b/pkg/ddc/base/dataset.go index a3cdb328856..8d0895dbb5b 100644 --- a/pkg/ddc/base/dataset.go +++ b/pkg/ddc/base/dataset.go @@ -18,11 +18,11 @@ package base import ( "fmt" - "path/filepath" "strings" datav1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1" "github.com/fluid-cloudnative/fluid/pkg/common" + "github.com/fluid-cloudnative/fluid/pkg/utils" transformerutils "github.com/fluid-cloudnative/fluid/pkg/utils/transformer" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -72,13 +72,12 @@ func GetPhysicalDatasetSubPath(virtualDataset *datav1alpha1.Dataset) (string, er return "", nil } - // The raw subPath must not escape the physical dataset's mount root. filepath.IsLocal rejects - // an absolute path or one that contains a "../" escape, so it cannot be used to break out. - if !filepath.IsLocal(subPath) { - return "", fmt.Errorf("the dataset \"%s/%s\" has an invalid subPath %q: must be a relative path that does not escape the mount point", virtualDataset.Namespace, virtualDataset.Name, subPath) + normalized, err := utils.NormalizeSubPath(subPath) + if err != nil { + return "", fmt.Errorf("the dataset \"%s/%s\" has an invalid subPath: %w", virtualDataset.Namespace, virtualDataset.Name, err) } - return subPath, nil + return normalized, nil } func CheckReferenceDataset(dataset *datav1alpha1.Dataset) (check bool, err error) { diff --git a/pkg/ddc/base/dataset_test.go b/pkg/ddc/base/dataset_test.go index c783f595118..1f72328a5b3 100644 --- a/pkg/ddc/base/dataset_test.go +++ b/pkg/ddc/base/dataset_test.go @@ -110,9 +110,19 @@ var _ = Describe("Dataset", func() { "sub-c/sub-d", false, ), - Entry("keeps the trailing slash of a subpath", + Entry("trims the trailing slash of a subpath", &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b/sub-c/"}}}}, - "sub-c/", + "sub-c", + false, + ), + Entry("normalizes a doubled separator before the subpath", + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b//sub-c"}}}}, + "sub-c", + false, + ), + Entry("normalizes redundant separators before a nested subpath", + &datav1alpha1.Dataset{Spec: datav1alpha1.DatasetSpec{Mounts: []datav1alpha1.Mount{{MountPoint: "dataset://ns-a/ds-b///sub-c/sub-d"}}}}, + "sub-c/sub-d", false, ), Entry("returns an empty subpath when path ends with slash", diff --git a/pkg/ddc/base/runtime_helper.go b/pkg/ddc/base/runtime_helper.go index ded21377b7c..a798bdf7e06 100644 --- a/pkg/ddc/base/runtime_helper.go +++ b/pkg/ddc/base/runtime_helper.go @@ -18,7 +18,6 @@ package base import ( "fmt" - "path/filepath" "time" "github.com/fluid-cloudnative/fluid/pkg/common" @@ -109,9 +108,12 @@ func (info *RuntimeInfo) getMountInfo() (path, mountType, subpath string, err er path = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidPath] mountType = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrMountType] subpath = pv.Spec.CSI.VolumeAttributes[common.VolumeAttrFluidSubPath] - if len(subpath) != 0 && !filepath.IsLocal(subpath) { - err = fmt.Errorf("the pv %s has an invalid subPath %q: must be a relative path that does not escape the mount point", pv.Name, subpath) - return + if len(subpath) != 0 { + subpath, err = utils.NormalizeSubPath(subpath) + if err != nil { + err = errors.Wrapf(err, "the pv %s has an invalid subPath", pv.Name) + return + } } } else { err = fmt.Errorf("the pv %s is not created by fluid", pv.Name) diff --git a/pkg/ddc/base/runtime_helper_test.go b/pkg/ddc/base/runtime_helper_test.go index 92e85a7224c..e37e7970e37 100644 --- a/pkg/ddc/base/runtime_helper_test.go +++ b/pkg/ddc/base/runtime_helper_test.go @@ -165,6 +165,31 @@ var _ = Describe("RuntimeHelper", func() { VolumeName: "default-fluid-dataset-subpath", }, }, + &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fluid-dataset-legacy-subpath", + Namespace: testNamespace, + Annotations: common.GetExpectedFluidAnnotations(), + }, + Spec: corev1.PersistentVolumeClaimSpec{ + VolumeName: "default-fluid-dataset-legacy-subpath", + }, + }, + &corev1.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{Name: "default-fluid-dataset-legacy-subpath"}, + Spec: corev1.PersistentVolumeSpec{ + PersistentVolumeSource: corev1.PersistentVolumeSource{ + CSI: &corev1.CSIPersistentVolumeSource{ + Driver: "fuse.csi.fluid.io", + VolumeAttributes: map[string]string{ + common.VolumeAttrFluidPath: "/runtime-mnt/jindo/big-data/nofounddataset/jindofs-fuse", + common.VolumeAttrMountType: common.JindoRuntime, + common.VolumeAttrFluidSubPath: "/subtest", + }, + }, + }, + }, + }, &corev1.PersistentVolume{ ObjectMeta: metav1.ObjectMeta{Name: "default-fluid-dataset"}, Spec: corev1.PersistentVolumeSpec{ @@ -300,5 +325,25 @@ var _ = Describe("RuntimeHelper", func() { Expect(subpath).To(Equal("subtest")) }) }) + + Context("when the PV has a subpath with a redundant leading separator", func() { + BeforeEach(func() { + fakeClient := fake.NewFakeClientWithScheme(scheme, objs...) + runtimeInfo = RuntimeInfo{ + name: "fluid-dataset-legacy-subpath", + namespace: testNamespace, + runtimeType: common.JindoRuntime, + apiReader: fakeClient, + } + }) + + It("should normalize the subpath instead of failing", func() { + path, mountType, subpath, err := runtimeInfo.getMountInfo() + Expect(err).NotTo(HaveOccurred()) + Expect(path).To(Equal("/runtime-mnt/jindo/big-data/nofounddataset/jindofs-fuse")) + Expect(mountType).To(Equal(common.JindoRuntime)) + Expect(subpath).To(Equal("subtest")) + }) + }) }) }) diff --git a/pkg/utils/mount.go b/pkg/utils/mount.go index a1fe1e03556..33a1429b5e3 100644 --- a/pkg/utils/mount.go +++ b/pkg/utils/mount.go @@ -21,6 +21,7 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "strings" "github.com/fluid-cloudnative/fluid/pkg/utils/cmdguard" @@ -41,6 +42,26 @@ func GetMountRoot() (string, error) { return mountRoot, nil } +// NormalizeSubPath cleans a Fluid subPath and rejects one that escapes its mount root. +// +// Leading separators are stripped before validation: GetPhysicalDatasetSubPath derives the subPath +// with strings.SplitAfterN, so a mount point written as "dataset://ns/ds//sub" yields "/sub". Such a +// value used to mount fine because the extra separator collapsed on concatenation, meaning PVs +// carrying it already exist and must keep working. Traversal that still escapes after cleaning is +// returned as an error instead of being clamped, so the caller can surface it. +func NormalizeSubPath(subPath string) (string, error) { + cleaned := filepath.Clean(strings.TrimLeft(subPath, "/")) + if cleaned == "." { + return "", nil + } + + if !filepath.IsLocal(cleaned) { + return "", fmt.Errorf("subPath %q must be a relative path that does not escape the mount point", subPath) + } + + return cleaned, nil +} + func CheckMountReadyAndSubPathExist(fluidPath string, mountType string, subPath string) (err error) { glog.Infof("Try to check if the mount target %s is ready", fluidPath) if fluidPath == "" { diff --git a/pkg/utils/mount_test.go b/pkg/utils/mount_test.go index 7ef2d6ddde6..54c14215368 100644 --- a/pkg/utils/mount_test.go +++ b/pkg/utils/mount_test.go @@ -73,6 +73,44 @@ func TestMountRootWithoutEnvSet(t *testing.T) { } } +func TestNormalizeSubPath(t *testing.T) { + testCases := []struct { + subPath string + expected string + expectErr bool + }{ + {subPath: "sub-c", expected: "sub-c"}, + {subPath: "sub-c/sub-d", expected: "sub-c/sub-d"}, + // A mount point written as "dataset://ns/ds//sub-c" yields a leading separator + {subPath: "/sub-c", expected: "sub-c"}, + {subPath: "//sub-c", expected: "sub-c"}, + {subPath: "/sub-c/sub-d", expected: "sub-c/sub-d"}, + {subPath: "sub-c/", expected: "sub-c"}, + {subPath: "./sub-c", expected: "sub-c"}, + {subPath: "", expected: ""}, + {subPath: "/", expected: ""}, + {subPath: "../etc", expectErr: true}, + {subPath: "sub-c/../../etc", expectErr: true}, + } + + for _, tc := range testCases { + got, err := NormalizeSubPath(tc.subPath) + if tc.expectErr { + if err == nil { + t.Errorf("NormalizeSubPath(%q) expected an error, but got none", tc.subPath) + } + continue + } + if err != nil { + t.Errorf("NormalizeSubPath(%q) got unexpected error %v", tc.subPath, err) + continue + } + if got != tc.expected { + t.Errorf("NormalizeSubPath(%q) = %q, expected %q", tc.subPath, got, tc.expected) + } + } +} + func TestCheckMountReady(t *testing.T) { Convey("TestCheckMountReady", t, func() { Convey("CheckMountReady success", func() {