fix: harden ephemeral mount option handling for inline volumes.#2460
fix: harden ephemeral mount option handling for inline volumes.#2460kapilupadhayay wants to merge 2 commits into
Conversation
Inline ephemeral CSI volumes allow Pod authors to supply arbitrary blobfuse2 CLI options via volumeAttributes.mountOptions. Because blobfuse2 runs as root, a malicious --tmp-path value (e.g. /etc/kubernetes/manifests) causes root-owned blob cache writes to land on sensitive host paths, enabling static Pod injection and host-level RCE. Block --tmp-path and --config-file from user-supplied mount options for ephemeral volumes only. --config-file is blocked as a bypass vector: it can contain file_cache.path which is equivalent to --tmp-path. Regular PVC mount options (from StorageClass/PV, admin-controlled) are unaffected.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kapilupadhayay The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
|
|
Welcome @kapilupadhayay! |
|
Hi @kapilupadhayay. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
andyzhangx
left a comment
There was a problem hiding this comment.
can you sign the easycla first? (select individual contributor option)
|
/ok-to-test |
There was a problem hiding this comment.
Pull request overview
Hardens inline ephemeral CSI volume mounts by stripping security-sensitive blobfuse2 options (--tmp-path and --config-file) supplied via volumeAttributes.mountOptions, preventing root-owned cache writes to attacker-chosen host paths (e.g. /etc/kubernetes/manifests) that could enable static Pod injection and host RCE. PVC/StorageClass-supplied options (admin-controlled) are not affected.
Changes:
- Adds
sanitizeMountOptionsand ablockedEphemeralMountOptionsdenylist inpkg/blob/blob.go. - Calls the sanitizer in
NodeStageVolumebefore joining ephemeral mount options. - Adds
TestSanitizeMountOptionscovering pass-through, blocking, and whitespace cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/blob/blob.go | Introduces denylist and prefix-based sanitization for ephemeral mount options. |
| pkg/blob/nodeserver.go | Sanitizes ephemeral mount options before merging with mountFlags. |
| pkg/blob/blob_test.go | Unit tests for the new sanitizer covering allowed/blocked/whitespace cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
--block-cache-path is the block-cache mode equivalent of --tmp-path: blobfuse2 persists downloaded blocks (attacker-controlled blob content) to this directory as root. An ephemeral volume specifying --block-cache-path=/etc/kubernetes/manifests reproduces the same static-Pod-injection / host RCE primitive as the original --tmp-path exploit. Also extend the --config-file comment to cover block_cache.path as an additional config-file bypass path, and add a dedicated test case for the new blocked option. Addresses review comment.
| mountOptions = util.JoinMountOptions(mountOptions, strings.Split(ephemeralVolMountOptions, ",")) | ||
| // Sanitize user-supplied mount options before use: strip options that are | ||
| // security-sensitive (e.g. --tmp-path) and must be driver-controlled. | ||
| sanitized := sanitizeMountOptions(strings.Split(ephemeralVolMountOptions, ",")) |
There was a problem hiding this comment.
Should it silently sanitize and mount or should it throw an error saying that it can't mount with provided options?
There was a problem hiding this comment.
I think I would prefer this failing with InvaidArgument error as the user isn't getting the requested behavior.
There was a problem hiding this comment.
Yes, its better to fail with InvalidArg.
| blocked := false | ||
| for _, blockedPrefix := range blockedEphemeralMountOptions { | ||
| if strings.HasPrefix(strings.TrimSpace(opt), blockedPrefix) { | ||
| klog.Warningf("mount option %q is not allowed for ephemeral volumes and will be ignored", opt) |
There was a problem hiding this comment.
is there a --temp-path*" mount that is ok? Any mount option starting with temp-path` will be blocked in this code path.
There was a problem hiding this comment.
what about the short form of these mount options?
There was a problem hiding this comment.
Made the check exact. None of the options have shorter forms.
|
Instead of keeping a deny-list of mounts, should we have a list of allowed mount options? |
Agreed, an allow list of safe mount options would improve security, |
I believe deny list is still better. It won't be a breaking change and new options when added in the future need not be maintained in the list. |
|
@kapilupadhayay pls also add |
Harden ephemeral mount option handling for inline volumes.
What type of PR is this?
/kind bug
What this PR does / why we need it:
Blocks the users to specify --tmp-path on ephemeral volumes
Which issue(s) this PR fixes:
Fixes #
Requirements:
Special notes for your reviewer:
Release note: