fix(security): prevent symlink LPE in GPU allocation - #2605
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: KunwarSidhu47 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesGPU allocation security
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
72d92bf to
f706387
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go`:
- Around line 845-857: Update the directory validation around
cacheFileHostDirectory and /tmp/vgpulock to fail closed: use an atomic no-follow
validation where supported, propagate and wrap every Lstat or Chmod failure, and
reject symlinks or non-directories before either path becomes a mount source.
Call PodAllocationFailed before each filesystem-error return so the allocation
lock is released.
- Around line 841-844: Update the cache-directory setup near MkdirAll to call
PodAllocationFailed(nodename, current, NodeLockNvidia) before every new
filesystem-error return. Make path validation fail closed when Lstat fails or
either path is not a directory, handle Chmod errors explicitly, and avoid
continuing with an unsafe path; prefer descriptor-based validation/permission
changes where needed to prevent replacement races.
- Around line 845-846: Replace the pathname-based Lstat/Chmod sequence around
cacheFileHostDirectory with an atomic no-follow directory-open and
descriptor-based chmod, preserving the directory-only and non-symlink
validation. Apply the same descriptor-based approach to both permission changes
in the surrounding plugin server logic, using the opened directory handle to
prevent replacement races.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a3ec7fb-a485-41c6-9ce2-59fa47edd9cd
📒 Files selected for processing (1)
pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
| if err := os.MkdirAll(cacheFileHostDirectory, 0777); err != nil { | ||
| klog.Errorf("Failed to create cache directory: %v", err) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 '\bPodAllocationFailed\b' pkg/device-plugin/nvidiadevice/nvinternal/pluginRepository: Project-HAMi/HAMi
Length of output: 15429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- allocation path ---'
sed -n '720,925p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
printf '%s\n' '--- cleanup implementation and tests ---'
sed -n '350,430p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go
sed -n '280,335p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/util_test.go
printf '%s\n' '--- cache and lock path operations ---'
rg -n -C 12 'cacheFileHostDirectory|cacheFileHostPath|os\.Lstat|os\.Chmod|MkdirAll|NodeLockNvidia' pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.goRepository: Project-HAMi/HAMi
Length of output: 23105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os
import stat
import tempfile
def cache_check(path):
try:
info = os.lstat(path)
except OSError:
return "continue"
if stat.S_ISDIR(info.st_mode) and not stat.S_ISLNK(info.st_mode):
return "chmod-and-continue"
return "continue"
def lock_check(path):
try:
info = os.lstat(path)
except OSError:
return "continue"
if stat.S_ISDIR(info.st_mode) and not stat.S_ISLNK(info.st_mode):
return "chmod-and-continue"
return "return-validation-error"
with tempfile.TemporaryDirectory() as root:
missing = os.path.join(root, "missing")
regular = os.path.join(root, "regular")
symlink = os.path.join(root, "symlink")
os.mkdir(regular)
open(os.path.join(root, "file"), "w").close()
os.symlink(os.path.join(root, "file"), symlink)
print("cache missing:", cache_check(missing))
print("cache directory:", cache_check(regular))
print("cache symlink:", cache_check(symlink))
print("lock missing:", lock_check(missing))
print("lock directory:", lock_check(regular))
print("lock symlink:", lock_check(symlink))
source = open("pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go").read()
block = source[source.index("if err := os.MkdirAll(cacheFileHostDirectory"):source.index("response.Mounts = append", source.index("if err := os.MkdirAll(cacheFileHostDirectory"))]
print("cache cleanup call:", "PodAllocationFailed" in block)
print("lock cleanup call:", "PodAllocationFailed" in block)
PYRepository: Project-HAMi/HAMi
Length of output: 386
Preserve allocation-failure cleanup and fail closed on path validation.
Call PodAllocationFailed(nodename, current, NodeLockNvidia) before each new filesystem-error return. This marks the allocation failed and releases the node lock.
Return an error when Lstat fails or either path is not a directory. Handle Chmod errors instead of ignoring them. The current checks can continue with an unsafe path, and Lstat followed by path-based Chmod remains vulnerable to replacement races.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go` around lines 841
- 844, Update the cache-directory setup near MkdirAll to call
PodAllocationFailed(nodename, current, NodeLockNvidia) before every new
filesystem-error return. Make path validation fail closed when Lstat fails or
either path is not a directory, handle Chmod errors explicitly, and avoid
continuing with an unsafe path; prefer descriptor-based validation/permission
changes where needed to prevent replacement races.
| if info, err := os.Lstat(cacheFileHostDirectory); err == nil && info.Mode().IsDir() && info.Mode()&os.ModeSymlink == 0 { | ||
| _ = os.Chmod(cacheFileHostDirectory, 0777) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'os\.Lstat|os\.Chmod' pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
go doc os.Lstat
go doc os.ChmodRepository: Project-HAMi/HAMi
Length of output: 2342
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- server.go outline ---'
ast-grep outline pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
printf '%s\n' '--- allocation context ---'
sed -n '780,875p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
printf '%s\n' '--- applyMutex and related paths ---'
rg -n -C 5 'applyMutex|cacheFileHostDirectory|vgpulock|PodAllocationFailed' pkg/device-plugin/nvidiadevice/nvinternal/plugin
printf '%s\n' '--- module and platform context ---'
sed -n '1,120p' go.mod
find pkg/device-plugin/nvidiadevice/nvinternal/plugin -maxdepth 2 -type f -print | sortRepository: Project-HAMi/HAMi
Length of output: 39654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete Allocate tail ---'
sed -n '737,922p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go
printf '%s\n' '--- lock and allocation-failure implementation ---'
sed -n '1,240p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/lock.go
sed -n '360,425p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go
printf '%s\n' '--- hostHookPath sources ---'
rg -n -C 4 'hostHookPath\s*=|hostHookPath:|HostHook|HOST_HOOK|host-hook' pkg/device-plugin/nvidiadevice/nvinternal/plugin
printf '%s\n' '--- Go standard-library semantics and Linux flags ---'
go doc os.File.Chmod
go doc syscall.Open
go doc syscall.O_NOFOLLOW
go doc syscall.O_DIRECTORY
go env GOROOTRepository: Project-HAMi/HAMi
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import os
import re
import subprocess
server = Path("pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go").read_text()
checks = [
("cache", "cacheFileHostDirectory"),
("lock", '"/tmp/vgpulock"'),
]
for name, path_expr in checks:
lstat = re.search(rf'os\.Lstat\({re.escape(path_expr)}\)', server)
chmod = re.search(rf'os\.Chmod\({re.escape(path_expr)},\s*0777\)', server)
if name == "cache":
lstat = re.search(r'os\.Lstat\(cacheFileHostDirectory\)', server)
chmod = re.search(r'os\.Chmod\(cacheFileHostDirectory,\s*0777\)', server)
assert lstat and chmod, f"missing {name} Lstat/Chmod pair"
assert lstat.start() < chmod.start(), f"unexpected {name} call order"
print(f"{name}: Lstat and Chmod are separate pathname operations")
assert "applyMutex sync.Mutex" in server
print("synchronization: applyMutex is an in-process sync.Mutex")
goroot = Path(subprocess.check_output(["go", "env", "GOROOT"], text=True).strip())
matches = []
for p in (goroot / "src/os").glob("*.go"):
text = p.read_text(errors="replace")
if re.search(r"func Chmod\(name string", text) and "syscall.Chmod" in text:
matches.append((p, "Chmod", "syscall.Chmod(name"))
if re.search(r"func Lstat\(name string", text) and "syscall.Lstat" in text:
matches.append((p, "Lstat", "syscall.Lstat(name"))
assert {kind for _, kind, _ in matches} == {"Lstat", "Chmod"}
for p, kind, needle in matches:
print(f"{kind}: {p.relative_to(goroot)} delegates by pathname")
PYRepository: Project-HAMi/HAMi
Length of output: 411
🏁 Script executed:
#!/bin/bash
set -euo pipefail
GOROOT="$(go env GOROOT)"
rg -n -C 5 'func (Lstat|Chmod)|syscall\.(Lstat|Chmod)|Fchmod|fchmod' "$GOROOT/src/os" "$GOROOT/src/syscall" | head -n 160Repository: Project-HAMi/HAMi
Length of output: 10563
Make the no-symlink check atomic with the permission change. os.Lstat and path-based os.Chmod use separate pathname lookups. Another local process can replace the checked directory with a symlink between these calls, and os.Chmod follows that symlink. Open the directory with no-follow and directory flags, then chmod the open descriptor, or use an equivalent atomic Linux primitive. Apply this to both permission changes.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 845-845: File mode grants world-writable permission; restrict the mode so other users cannot modify the file (e.g. 0o644 / 0o600).
Context: os.Chmod(cacheFileHostDirectory, 0777)
Note: [CWE-276] Incorrect Default Permissions.
(world-writable-chmod-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go` around lines 845
- 846, Replace the pathname-based Lstat/Chmod sequence around
cacheFileHostDirectory with an atomic no-follow directory-open and
descriptor-based chmod, preserving the directory-only and non-symlink
validation. Apply the same descriptor-based approach to both permission changes
in the surrounding plugin server logic, using the opened directory handle to
prevent replacement races.
| if info, err := os.Lstat(cacheFileHostDirectory); err == nil && info.Mode().IsDir() && info.Mode()&os.ModeSymlink == 0 { | ||
| _ = os.Chmod(cacheFileHostDirectory, 0777) | ||
| } | ||
|
|
||
| if err := os.MkdirAll("/tmp/vgpulock", 0777); err != nil { | ||
| klog.Errorf("Failed to create lock directory: %v", err) | ||
| return nil, err | ||
| } | ||
| if info, err := os.Lstat("/tmp/vgpulock"); err == nil && info.Mode().IsDir() && info.Mode()&os.ModeSymlink == 0 { | ||
| _ = os.Chmod("/tmp/vgpulock", 0777) | ||
| } else if err == nil { | ||
| return nil, fmt.Errorf("/tmp/vgpulock is a symlink or not a directory") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file='pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go'
printf '%s\n' '--- target function map ---'
ast-grep outline "$file" | sed -n '1,220p'
printf '%s\n' '--- target region ---'
sed -n '780,900p' "$file"
printf '%s\n' '--- allocation/error and cleanup references ---'
rg -n -C 4 'PodAllocationFailed|cacheFileHostDirectory|vgpulock|func \(.*\) Allocate|func .*Allocate' "$file" pkg/device-plugin/nvidiadevice/nvinternal/plugin
printf '%s\n' '--- changed-file summary ---'
git diff --statRepository: Project-HAMi/HAMi
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -eu
file='pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go'
printf '%s\n' '--- Allocate entry, defers, and exits ---'
sed -n '737,785p' "$file"
sed -n '890,918p' "$file"
printf '%s\n' '--- lock release implementation ---'
sed -n '340,430p' pkg/device-plugin/nvidiadevice/nvinternal/plugin/util.go
rg -n -C 6 'applyMutex|updatePodAnnotationsAndReleaseLock|DeviceBindFailed|PodAllocationTrySuccess' \
pkg/device-plugin/nvidiadevice/nvinternal/plugin
printf '%s\n' '--- exact diff for the target file ---'
git diff --unified=30 -- "$file" | sed -n '1,260p'Repository: Project-HAMi/HAMi
Length of output: 30940
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- pending-pod lookup and lock acquisition ---'
rg -n -C 12 'getPendingPod|func .*PendingPod|NodeLockNvidia|DeviceBind' \
pkg/device-plugin/nvidiadevice/nvinternal/plugin \
pkg | sed -n '1,320p'
printf '%s\n' '--- allocation tests covering filesystem setup and failure handling ---'
rg -n -C 12 'cache directory|vgpulock|MkdirAll|Chmod|RemoveAll|Allocate\(' \
pkg/device-plugin/nvidiadevice/nvinternal/plugin/*_test.go | sed -n '1,360p'
printf '%s\n' '--- all Allocate returns in the target function ---'
python3 - <<'PY'
from pathlib import Path
p = Path('pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go')
lines = p.read_text().splitlines()
for i in range(736, 916):
if 'return ' in lines[i]:
print(f'{i+1}: {lines[i]}')
PYRepository: Project-HAMi/HAMi
Length of output: 50373
Fail closed on directory validation and permission errors.
If os.Lstat or os.Chmod fails for cacheFileHostDirectory or /tmp/vgpulock, return a wrapped error before adding the path as a mount source. The current code ignores os.Chmod errors and allows os.Lstat errors to fall through.
Call PodAllocationFailed before these error returns. Otherwise, the pod allocation lock can remain held after a filesystem failure. Replace the Lstat/Chmod sequence with an atomic no-follow validation where supported.
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 845-845: File mode grants world-writable permission; restrict the mode so other users cannot modify the file (e.g. 0o644 / 0o600).
Context: os.Chmod(cacheFileHostDirectory, 0777)
Note: [CWE-276] Incorrect Default Permissions.
(world-writable-chmod-go)
[warning] 848-848: File mode grants world-writable permission; restrict the mode so other users cannot modify the file (e.g. 0o644 / 0o600).
Context: os.MkdirAll("/tmp/vgpulock", 0777)
Note: [CWE-276] Incorrect Default Permissions.
(world-writable-chmod-go)
[warning] 853-853: File mode grants world-writable permission; restrict the mode so other users cannot modify the file (e.g. 0o644 / 0o600).
Context: os.Chmod("/tmp/vgpulock", 0777)
Note: [CWE-276] Incorrect Default Permissions.
(world-writable-chmod-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go` around lines 845
- 857, Update the directory validation around cacheFileHostDirectory and
/tmp/vgpulock to fail closed: use an atomic no-follow validation where
supported, propagate and wrap every Lstat or Chmod failure, and reject symlinks
or non-directories before either path becomes a mount source. Call
PodAllocationFailed before each filesystem-error return so the allocation lock
is released.
Signed-off-by: KunwarSidhu47 <kunwarsidhu1947@gmail.com>
f706387 to
13d743a
Compare
|
You can view the relevant rule here. |
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
2 participants
Title: fix(security): prevent symlink LPE and add error handling in GPU allocation
Description:
What this PR does / why we need it:
This PR fixes a critical local privilege escalation (LPE) vulnerability and missing error handling within the Nvidia device plugin's
Allocatefunction (pkg/device-plugin/nvidiadevice/nvinternal/plugin/server.go).Previously, the plugin blindly created directories and modified their permissions without checking for errors or verifying the file type. Because the device plugin runs as
rootand/tmpis world-writable, a malicious local user or unprivileged pod could pre-create a symlink at/tmp/vgpulockpointing to a sensitive file (e.g.,/etc/shadow). Since Go'sos.Chmodfollows symlinks, allocating a GPU would silently change the permissions of/etc/shadowto0777, granting the attacker full control over the node.Additionally, because the return values of
os.MkdirAllandos.Chmodwere completely ignored, any failure to create these directories would be silently swallowed, leading to unpredictable volume mount failures downstream.Which issue(s) this PR fixes:
Fixes #2604
Special notes for your reviewer:
if err != nilchecks aroundos.MkdirAllto ensure we fail gracefully if directory creation fails.os.Lstatprior toos.Chmodto explicitly verify that the target path is an actual directory andos.ModeSymlinkis NOT set. This mitigates the symlink TOCTOU attack vector while preserving the required0777permission enforcement.Does this PR introduce a user-facing change?:
No, internal security and error handling improvement.
Summary by CodeRabbit