optim(utils): sanitize and clamp subpaths to handle path escaping - #6159
Conversation
…oots Signed-off-by: 东伝 <dongyun.xzh@alibaba-inc.com>
TrafalgarZZZ
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6159 +/- ##
==========================================
+ Coverage 65.13% 65.15% +0.01%
==========================================
Files 485 486 +1
Lines 34039 34144 +105
==========================================
+ Hits 22171 22246 +75
- Misses 10127 10149 +22
- Partials 1741 1749 +8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Sanitizes Fluid dataset subpaths to prevent lexical parent-directory traversal.
Changes:
- Adds reusable subpath normalization with unit tests.
- Applies normalization across dataset references, runtime metadata, CSI mounts, and sidecar injection.
- Rejects absolute CSI subpaths.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pkg/utils/mount.go |
Adds subpath normalization helper. |
pkg/utils/mount_test.go |
Tests normalization cases. |
pkg/ddc/base/runtime_helper.go |
Normalizes PV subpaths. |
pkg/ddc/base/dataset.go |
Normalizes referenced-dataset subpaths. |
pkg/ddc/base/dataset_test.go |
Tests reference-path clamping. |
pkg/csi/plugins/nodeserver.go |
Validates and clamps CSI subpaths. |
pkg/application/inject/fuse/mutator/mutator_default.go |
Safely joins sidecar host paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- 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: 东伝 <dongyun.xzh@alibaba-inc.com>
Signed-off-by: 东伝 <dongyun.xzh@alibaba-inc.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
pkg/ddc/base/dataset.go:61
- This error is emitted when there is exactly one mount but its scheme is not
dataset://, so saying the dataset should have one mount is inaccurate and hides the actual validation failure.
return "", fmt.Errorf("the dataset \"%s/%s\" should only have one mount", virtualDataset.Namespace, virtualDataset.Name)
|
|
||
| // 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 { |
| 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) { |
| return errors.Wrapf(err, "failed to get mount root for validating %s \"%s\"", attrName, path) | ||
| } | ||
|
|
||
| if !utils.IsSubPath(mountRoot, path) { |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
I checked this one by running it rather than only reading it, both against the base branch and at head 076f0ba9. The harness and captured output are on cheyang/fluid branch verify/pr6159-subpath-clamp, under docs/verification/pr6159-subpath-clamp/.
The premise holds and this change is worth landing. On base, a fluid_sub_path of ../../../../../hostonly makes NodePublishVolume publish a mount source that resolves outside the FUSE root, and the pod then reads the host file's contents. Both the original clamp and the current reject close that. So everything below is about gaps in the fix, not a case against it.
Switching from silent clamping to rejecting at each boundary is the right call, and dropping CleanSubPath with it removed a function whose comment promised containment it could not deliver. Two things still block.
1. The new symlink guard only inspects the last path component. checkSymlinkFile calls os.Lstat(mountPath), so a subpath whose earlier component is a symlink still gets through:
subPath "evil" -> rejected: "reject mounting path .../alluxio-fuse/evil because it is a symlink"
subPath "evil/inner" -> published .../alluxio-fuse/evil/inner
resolves to /tmp/fl6159579890907/inner
pod reads "HOST-ONLY-CONTENT"
Both values pass filepath.IsLocal. The spec added in this PR exercises only the single-component case, which is why the gap survived. More inline.
2. A fluid_sub_path persisted before the upgrade is now refused. dataset://ns/ds//sub-c (double slash) parses to /sub-c via SplitAfterN, and that mounted fine on base because POSIX collapses the extra slash. It is now rejected in three places, so existing PVs and existing Datasets both break on upgrade, and referencedataset/volume.go writes the attribute only at PV creation so nothing rewrites it.
Two pre-existing problems belong in the same conversation, because they sit inside what this PR is trying to fix and filepath.IsLocal addresses neither: the postStart hook still interpolates the subpath into a bash -c string, and the sidecar's subpath existence check can be made to pass without checking anything. I confirmed both by execution, and I'd rather they became a tracked issue than grew this PR.
On scope: I reproduced the escape at the CSI layer by running the code. That the value is tenant-controllable end to end I got from reading (GetPhysicalDatasetSubPath is the only producer of the attribute, mountPoint carries only MinLength=5, and there is no validating webhook on Dataset), not from a live cluster. Someone should confirm that before deciding whether this needs a backport or an advisory.
Last thing: the title still says "sanitize and clamp", but nothing is clamped now and pkg/utils is net -36 lines. Worth retitling before merge.
Detail: the sidecar subpath check
No inline anchor for this one, the file is not in the diff.
poststart/check_fuse_default.go:90, in the shipped script:
while [ ! -e $ConditionPathIsMountPoint/*/$SubPath ]Unquoted, and with a literal glob. A subpath containing a space makes [ receive too many operands and return 2, so the while condition is false, the loop never runs, and the script exits 0, reporting the subpath as present without having checked it. Running the shipped script:
subPath "pr6159-definitely-missing" -> exit 2, "timed out checking sub path" (works)
subPath "pr6159 definitely missing" -> exit 0, "[: too many arguments"
"succeed in checking mount point" (bypassed)
Such a value passes filepath.IsLocal, so nothing upstream stops it.
Note that csi/shell/check_mount.sh:48 is the CSI-side copy of this same check and was already hardened to test -e "$ConditionPathIsMountPoint/$SubPath" with grep -F and argv passing. The two copies have drifted apart, and only the unhardened one is in this PR's blast radius. No inline anchor since the file is not in the diff.
| 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) | ||
| } | ||
| mountPath = filepath.Join(mountPath, subPath) |
There was a problem hiding this comment.
The three specs added here carry real assertions on status.Code(err), which covers the substance of the earlier request. Three gaps left: the pre-existing "should append subpath to fluid path" spec still discards both results (_ = resp; _ = err), so it passes regardless of what this code does; nothing covers the legacy absolute-subpath case above; and the symlink spec stops at one component. The first gap is why the regression above shipped with all checks green and codecov reporting 50% patch coverage on this file.
Signed-off-by: 东伝 <dongyun.xzh@alibaba-inc.com>
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Re-ran the harness against 6f66e509. The symlink guard and the postStart sink are both properly fixed, and pinning the fd closes the check-then-use window too, which is better than what I suggested. Your resolveMountSource specs pass and pkg/csi/plugins is 132/132. The duplicated error message is gone.
Two left, both inline. The legacy fluid_sub_path one is the only thing this PR introduces rather than inherits, and it has not been touched in five pushes.
Harness and captured runs: cheyang/fluid, branch verify/pr6159-subpath-clamp.
| mountPath = fluidPath + "/" + 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) { |
There was a problem hiding this comment.
Still here after five pushes, so in more detail.
The value comes from our own parser, not from someone writing an absolute path. SplitAfterN keeps the separator with the preceding element, so parts[2] starts after the second /. Double that slash and the character it starts on is the third /:
dataset://ns-a/ds-b/sub-c -> parts[2] = "sub-c" IsLocal=true
dataset://ns-a/ds-b//sub-c -> parts[2] = "/sub-c" IsLocal=false
referencedataset/volume.go:94 stores it as is. On base it mounted fine, because fluidPath + "/" + "/sub-c" collapses in POSIX, so the typo was invisible. Those PVs can exist today.
Upgrading does not clean them up: volume.go:76 only writes the attribute when the PV is created. Nothing breaks at upgrade time either, since running pods keep their mount, so it shows up on the next restart or reschedule.
base 05f06659 : PASS -> published .../alluxio-fuse//sub-c, marker readable
6f66e509 : FAIL -> "fluid_sub_path must be a relative path that does not escape the mount point, but got \"/sub-c\""
It is also rejected now at runtime_helper.go:112 and dataset.go:77, and that last one stops the Dataset reconciling rather than just failing the mount.
Normalizing would fix it and make all four sites agree:
subPath = strings.TrimPrefix(filepath.Clean("/"+subPath), "/")
if subPath != "" && !filepath.IsLocal(subPath) { ... }Which is what CleanSubPath did. It was fine for this, it just was not a security boundary.
I only reproduced the CSI path. dataset.go:77 needs one more entry in your table if you want it covered.
| 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} |
There was a problem hiding this comment.
Anchored here because line 89 of this file is outside the diff. That is the real spot:
while [ ! -e $ConditionPathIsMountPoint/*/$SubPath ]Unquoted, plus a literal glob. A value with a space becomes extra operands, [ returns 2, and a while condition only has to be non-zero to be false, so the loop is skipped and the script exits 0 without checking anything. set -e does not apply to conditions.
"pr6159-definitely-missing" -> exit 2, "timed out checking sub path [...]"
"pr6159 definitely missing" -> exit 0, "[: too many arguments" / "succeed in checking mount point"
The first line is the control: the gate works normally, it only breaks on the crafted value, and that value passes filepath.IsLocal on the way in.
Your fix moved this one rather than helped it. bash -c used to split the value before the script saw it, so the gate checked a truncated path. Now it arrives whole and splits at line 89 instead, which skips the check.
Quoting alone will not do it, since a glob matching two directories has the same problem. This works, crafted case exits 2 and the ordinary one stays green:
while ! ls -d "$ConditionPathIsMountPoint"/*/"$SubPath" >/dev/null 2>&1csi/shell/check_mount.sh:48 is the other copy of this check and was already hardened. Only the unhardened one is reachable from here. Line 75 has the same unquoted grep.
Signed-off-by: TzZtzt <trafalgarz@outlook.com>
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Ⅰ. Describe what this PR does
Ⅱ. Does this pull request fix one issue?
fixes #XXXX
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews