virtio/net: fix ENAMETOOLONG on long gvproxy socket paths (macOS)#697
virtio/net: fix ENAMETOOLONG on long gvproxy socket paths (macOS)#697arewm wants to merge 1 commit into
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a potential ENAMETOOLONG error on macOS when creating local unixgram socket paths by using a combination of the process ID and a global atomic counter instead of appending a suffix to the peer path. It also adds a test case to verify this behavior with long socket paths. Feedback on the changes highlights a security vulnerability (CWE-377) caused by hardcoding /tmp for the temporary socket path, which could lead to a local Denial of Service (DoS). It is recommended to use std::env::temp_dir() instead to leverage secure, user-specific temporary directories.
| let local_path = PathBuf::from(format!( | ||
| "/tmp/krun-net-{}-{}.sock", | ||
| process::id(), | ||
| NET_SOCK_COUNTER.fetch_add(1, Ordering::Relaxed), | ||
| )); |
There was a problem hiding this comment.
Hardcoding /tmp as the temporary directory introduces a local Denial of Service (DoS) vulnerability (CWE-377: Insecure Temporary File). Since /tmp is a shared directory with the sticky bit set, any other local user can pre-create the predictable socket path /tmp/krun-net-<pid>-<counter>.sock. When libkrun attempts to start, the unlink call on line 97 will fail with EACCES or EPERM because the file is owned by another user. Since the error from unlink is ignored, the subsequent bind call will fail, preventing the VM from starting.
Using std::env::temp_dir() instead of hardcoding /tmp is much more secure. On macOS, std::env::temp_dir() points to a secure, user-specific temporary directory (typically under /var/folders/) which is not accessible by other users, completely mitigating this local DoS vector while still keeping the path length well under the 104-byte limit.
| let local_path = PathBuf::from(format!( | |
| "/tmp/krun-net-{}-{}.sock", | |
| process::id(), | |
| NET_SOCK_COUNTER.fetch_add(1, Ordering::Relaxed), | |
| )); | |
| let local_path = std::env::temp_dir().join(format!( | |
| "krun-net-{}-{}.sock", | |
| process::id(), | |
| NET_SOCK_COUNTER.fetch_add(1, Ordering::Relaxed), | |
| )); |
There was a problem hiding this comment.
I'll force push a new change to remediate this issue.
473b4db to
5166a30
Compare
macOS imposes a 104-byte limit on unix socket paths. The gvproxy peer socket path can be long enough that appending a suffix to derive the local bind address exceeds this limit, triggering ENAMETOOLONG. This surfaced as BadActivate from the net device, panicking the vCPU thread and leaving the VM frozen at 0% CPU while krunkit stayed alive. Place the local bind socket in the same directory as the peer using a short PID + per-process atomic counter filename (krun-net-<pid>-<n>.sock). The peer filename always contains the machine name, making it longer than our fixed-format name for any reasonably-named machine. PID + counter guarantees uniqueness across concurrent krunkit processes and multiple virtio-net devices within a single process. Add a net-gvproxy-long-path integration test that constructs a peer socket path long enough that the old suffix-based approach would exceed the 104-byte limit, and verifies that the VM starts and achieves network connectivity through gvproxy. Assisted-by: Claude Code (Sonnet 4.6) Signed-off-by: arewm <arewm@users.noreply.github.com>
5166a30 to
d45b2a7
Compare
Assisted-by: Claude Code (Sonnet 4.6)
Problem
On macOS,
Unixgram::openderives the local bind address by appending-krun.sockto the peer (gvproxy) socket path:macOS enforces a 104-byte unix socket path limit. The gvproxy peer path can be long enough that appending any suffix pushes the local path over this limit.
UnixAddr::newreturnsENAMETOOLONG, which propagates asBadActivateand panics the vCPU thread. krunkit stays alive but the VM is frozen at 0% CPU indefinitely.This affects any deployment where the gvproxy socket path is long enough that a 9-byte suffix would exceed the limit — common with default podman machine naming on macOS.
Fix
Place the local bind socket in the same directory as the peer, using a short PID + per-process atomic counter filename (
krun-net-<pid>-<n>.sock). The peer filename always contains the machine name, making it longer than our fixed-format name for any reasonably-named machine. PID ensures uniqueness across concurrent krunkit processes; the counter handles multiple virtio-net devices within a single process. The existingunlink()beforebind()handles stale sockets from crashed previous runs.Test
Adds
net-gvproxy-long-path: exercises the gvproxy backend with a peer socket path long enough that the old suffix-based approach would have exceeded the 104-byte macOS limit. Verifies the VM starts and achieves network connectivity.Fixes: #689