Conversation
WalkthroughThis PR removes the separate unprivileged control socket, uses peer credentials and group membership for access classification, changes control messaging to ChangesUnified control socket and privsep privilege flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Control as control.c
participant Root as privsep-root.c
participant Proxy as privsep-control.c
participant Peers as control_fds
Client->>Control: send control command
Control->>Control: recvmsg and extract uid/gid
Control->>Root: ps_root_user_ingroup(uid, gid, grpid)
Root-->>Control: group permission result
Control->>Proxy: ps_ctl_sendmsg(msghdr)
Proxy->>Peers: dispatch and broadcast command or listen data
Peers-->>Client: queued response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/dhcpcd.c (1)
2570-2576: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicate "spawned manager process" log.
The same
logdebugx(...)now appears both before thestart_manager:label (line 2572) and after it (line 2576). On the daemonise fall-through path both fire, logging the manager PID twice; thegoto start_managerpath only hits 2576. Drop the pre-label copy so both paths log exactly once.🩹 Remove the duplicate
/* We have now forked, setsid, forked once more. * From this point on, we are the controlling daemon. */ - logdebugx("spawned manager process on PID %d", (int)getpid()); start_manager: logdebugx("spawned manager process on PID %d", (int)getpid());🤖 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 `@src/dhcpcd.c` around lines 2570 - 2576, The manager startup debug message is duplicated around the start_manager label in dhcpcd.c, causing the daemonise fall-through path to log the same PID twice. Remove the pre-label logdebugx call and keep the single logdebugx("spawned manager process on PID %d", ...) after start_manager so both the fall-through and goto start_manager paths emit the message exactly once.
🧹 Nitpick comments (4)
src/privsep-root.c (1)
328-338: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant reassignment of
p.
pis already initialized todataat Line 328, so thep = data;at Line 335 is redundant. Minor cleanup.♻️ Cleanup
- uint8_t *p = data; + uint8_t *p; if (len != sizeof(uid) + sizeof(gid)) { errno = EINVAL; return -1; } p = data;🤖 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 `@src/privsep-root.c` around lines 328 - 338, Remove the redundant reassignment of the local pointer in the privilege-separation parsing logic: in the function that reads `uid` and `gid` from `data`, `p` is already initialized before the length check, so the later `p = data;` is unnecessary. Keep the existing `memcpy` flow intact and simply delete the duplicate assignment while preserving the `uid`/`gid` extraction behavior.src/privsep-control.c (1)
40-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
PS_CTL_FLAGS_PRIVmacro
PS_CTL_FLAGS_PRIVis never referenced; privilege is already carried byPS_CTL_PRIV/PS_CTLinps_ctl_dispatch, so this macro can be dropped.🤖 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 `@src/privsep-control.c` around lines 40 - 42, Remove the unused PS_CTL_FLAGS_PRIV macro from privsep-control.c and keep the PS_CTL_FD helper intact. Verify that ps_ctl_dispatch and the existing PS_CTL_PRIV/PS_CTL macros still provide the privilege handling needed, and delete any related dead definition so the header/source only contains symbols that are actually referenced.src/control.c (2)
453-453: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
listen()backlog usessizeof(ctx->control_fds).
ctx->control_fdsis a TAILQ head, sosizeofyields the struct size (~16), not a meaningful connection backlog. It works but is misleading; consider a named constant.🤖 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 `@src/control.c` at line 453, The backlog passed to listen() in the control socket setup uses sizeof(ctx->control_fds), which is just the TAILQ head size and not a meaningful queue limit. Update the listen() call in the control initialization path to use a clear named constant or defined backlog value instead of sizeof(ctx->control_fds), keeping the existing fd/control_fds logic unchanged.
434-454: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the
SOCK_MODEmacro and confirm the world-writable socket is intended.
SOCK_MODEis defined at line 434 (same value,0666) butcontrol_start1()chmods with the literal0666, leaving the macro unused. Static analysis also flags the world-writable mode (CWE-732). Under the new credential model any user may connect and privilege is enforced viagetpeereid/FD_PRIV, so world-writable is plausibly intentional — but then thectx->control_groupchown no longer restricts who can connect. Please confirm0666is desired rather than0660+control_group, and either way replace the literal with the macro.♻️ Use the defined macro
if (bind(fd, (struct sockaddr *)&sa, len) == -1 || - chmod(sa.sun_path, 0666) == -1 || + chmod(sa.sun_path, SOCK_MODE) == -1 || (ctx->control_group && chown(sa.sun_path, geteuid(), ctx->control_group) == -1) || listen(fd, sizeof(ctx->control_fds)) == -1)🤖 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 `@src/control.c` around lines 434 - 454, In control_start1(), replace the hardcoded chmod mode with the existing SOCK_MODE macro so the defined socket permissions are used consistently. Also verify the intended access model for the control socket: if world-writable access is deliberate under the new credential/FD_PRIV checks, keep SOCK_MODE and leave a note or related adjustment; otherwise tighten the mode to a group-only setting and ensure ctx->control_group still meaningfully restricts access.Source: Linters/SAST tools
🤖 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 `@src/control.c`:
- Around line 642-676: In control_user_ispriv(), treat an unknown uid from
getpwuid() as non-privileged instead of returning -1, so control_handle_data()
does not drop read-only clients whose user is missing from the passwd database.
Update the early return path in control_user_ispriv() to return 0 for this case,
and make sure any error logging in the caller does not rely on stale errno from
getpwuid() when no real error occurred.
In `@src/privsep-control.c`:
- Around line 111-123: The PS_CTL_PRIV/PS_CTL handling in privsep control is
incorrectly closing the shared proxy work fd when control_recvmsg() fails.
Update the control_new()/control_free() flow so the wrapper is dropped without
closing ctx->ps_ctl->psp_work_fd on the error path, and keep the underlying
shared fd registered for later dispatches; use the PS_CTL_PRIV/PS_CTL case block
and the control_recvmsg() cleanup branch as the places to adjust.
---
Outside diff comments:
In `@src/dhcpcd.c`:
- Around line 2570-2576: The manager startup debug message is duplicated around
the start_manager label in dhcpcd.c, causing the daemonise fall-through path to
log the same PID twice. Remove the pre-label logdebugx call and keep the single
logdebugx("spawned manager process on PID %d", ...) after start_manager so both
the fall-through and goto start_manager paths emit the message exactly once.
---
Nitpick comments:
In `@src/control.c`:
- Line 453: The backlog passed to listen() in the control socket setup uses
sizeof(ctx->control_fds), which is just the TAILQ head size and not a meaningful
queue limit. Update the listen() call in the control initialization path to use
a clear named constant or defined backlog value instead of
sizeof(ctx->control_fds), keeping the existing fd/control_fds logic unchanged.
- Around line 434-454: In control_start1(), replace the hardcoded chmod mode
with the existing SOCK_MODE macro so the defined socket permissions are used
consistently. Also verify the intended access model for the control socket: if
world-writable access is deliberate under the new credential/FD_PRIV checks,
keep SOCK_MODE and leave a note or related adjustment; otherwise tighten the
mode to a group-only setting and ensure ctx->control_group still meaningfully
restricts access.
In `@src/privsep-control.c`:
- Around line 40-42: Remove the unused PS_CTL_FLAGS_PRIV macro from
privsep-control.c and keep the PS_CTL_FD helper intact. Verify that
ps_ctl_dispatch and the existing PS_CTL_PRIV/PS_CTL macros still provide the
privilege handling needed, and delete any related dead definition so the
header/source only contains symbols that are actually referenced.
In `@src/privsep-root.c`:
- Around line 328-338: Remove the redundant reassignment of the local pointer in
the privilege-separation parsing logic: in the function that reads `uid` and
`gid` from `data`, `p` is already initialized before the length check, so the
later `p = data;` is unnecessary. Keep the existing `memcpy` flow intact and
simply delete the duplicate assignment while preserving the `uid`/`gid`
extraction behavior.
🪄 Autofix (Beta)
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
Run ID: 0f1cec4a-8cc9-4b2c-99f5-c3d964783ef1
📒 Files selected for processing (11)
src/control.csrc/control.hsrc/dhcpcd.8.insrc/dhcpcd.csrc/dhcpcd.hsrc/privsep-control.csrc/privsep-control.hsrc/privsep-root.csrc/privsep-root.hsrc/privsep.csrc/privsep.h
Instead workout credentials from the connected socket by getpeereid(2). Remove the limit of only one connected control client. This needs some testing and is only expected to compile on the BSDs right now.
Add --isprivileged to return either true or false so the caller can work this out as we now just have the one socket. Bump version for this.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/privsep-control.c`:
- Around line 178-181: Update the recvmsg error handling around msglen in the
control proxy to detect EAGAIN, EWOULDBLOCK, and EINTR and return to the event
loop without calling eloop_exit for those recoverable failures. Preserve the
existing log-and-exit behavior for all other recvmsg errors.
- Around line 259-262: Update the socketpair creation in the privsep control
setup to preserve non-blocking behavior for both work_fd and listen_fd by
restoring the non-blocking socket flag alongside SOCK_CLOEXEC, unless
xsocketpair or the target platform already guarantees it. Keep both descriptors
suitable for use by the eloop event loop.
- Line 97: Replace the unbounded strcmp check for "--isprivileged" in the
control request handling branch with a length-bounded comparison using the
received len value, matching the surrounding bounded-payload logic and
preventing reads beyond the client buffer.
- Around line 211-230: Update the control-message reads in the receive path
around ps_ctl_listen() so they handle partial length-prefix and payload reads
without stalling the event loop: preserve non-blocking socket setup in
ps_ctl_start() using SOCK_CXNB, or replace these reads with recv(...,
MSG_WAITALL) while retaining appropriate EOF/error handling.
🪄 Autofix (Beta)
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
Run ID: 33712ca1-d250-4d7c-b5ba-e2bded8f15d6
📒 Files selected for processing (5)
src/control.csrc/control.hsrc/defs.hsrc/dhcpcd.csrc/privsep-control.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/control.c
| if (msglen == -1) { | ||
| logerr("%s: read", __func__); | ||
| eloop_exit(ctx->eloop, EXIT_FAILURE); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle non-fatal errors from recvmsg.
If recvmsg fails with EAGAIN, EWOULDBLOCK, or EINTR, terminating the control proxy process via eloop_exit is overly aggressive. This is especially problematic if the socket is reverted to non-blocking mode, where EAGAIN is expected.
Consider ignoring these recoverable errors so the event loop can retry.
🛠️ Proposed fix
if (msglen == -1) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR)
+ return;
logerr("%s: read", __func__);
eloop_exit(ctx->eloop, EXIT_FAILURE);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (msglen == -1) { | |
| logerr("%s: read", __func__); | |
| eloop_exit(ctx->eloop, EXIT_FAILURE); | |
| } | |
| if (msglen == -1) { | |
| if (errno == EAGAIN || errno == EWOULDBLOCK || errno == EINTR) | |
| return; | |
| logerr("%s: read", __func__); | |
| eloop_exit(ctx->eloop, EXIT_FAILURE); | |
| } |
🤖 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 `@src/privsep-control.c` around lines 178 - 181, Update the recvmsg error
handling around msglen in the control proxy to detect EAGAIN, EWOULDBLOCK, and
EINTR and return to the event loop without calling eloop_exit for those
recoverable failures. Preserve the existing log-and-exit behavior for all other
recvmsg errors.
| if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, work_fd) == | ||
| -1 || | ||
| xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, listen_fd) == | ||
| -1) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Retain non-blocking behavior for event-loop file descriptors.
Replacing SOCK_CXNB with SOCK_STREAM | SOCK_CLOEXEC drops the SOCK_NONBLOCK flag (assuming SOCK_CXNB provided it). Since work_fd and listen_fd are added to the eloop (lines 292 and 299), they should remain non-blocking to prevent any read() or recvmsg() calls from stalling the single-threaded event loop.
Unless SOCK_CLOEXEC natively implies non-blocking on this target, please restore the non-blocking flag.
🛠️ Proposed fix
- if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, work_fd) ==
- -1 ||
- xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, listen_fd) ==
- -1)
+ if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CXNB, 0, work_fd) ==
+ -1 ||
+ xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CXNB, 0, listen_fd) ==
+ -1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, work_fd) == | |
| -1 || | |
| xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, listen_fd) == | |
| -1) | |
| if (xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CXNB, 0, work_fd) == | |
| -1 || | |
| xsocketpair(AF_UNIX, SOCK_STREAM | SOCK_CXNB, 0, listen_fd) == | |
| -1) |
🤖 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 `@src/privsep-control.c` around lines 259 - 262, Update the socketpair creation
in the privsep control setup to preserve non-blocking behavior for both work_fd
and listen_fd by restoring the non-blocking socket flag alongside SOCK_CLOEXEC,
unless xsocketpair or the target platform already guarantees it. Keep both
descriptors suitable for use by the eloop event loop.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@compat/getpeereid.c`:
- Around line 32-35: Update compat/getpeereid.c to include the project or system
header that declares getpeereid, alongside the existing conditional ucred.h
include, so the function definition has a visible matching prototype and
eliminates the -Wmissing-prototypes warning.
In `@configure`:
- Around line 943-949: Update the _getpeereid configure probe to include
<unistd.h> alongside the existing headers before calling getpeereid, so macOS
and BSD systems compile the detection test successfully and select native
support.
🪄 Autofix (Beta)
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
Run ID: 28030e40-8769-4008-a769-556062710614
📒 Files selected for processing (4)
compat/getpeereid.ccompat/getpeereid.hconfiguresrc/control.c
💤 Files with no reviewable changes (1)
- src/control.c
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/eloop.c`:
- Around line 255-256: Preserve the caller’s original nsignals value before the
normalization at lines 243-245, then use that preserved zero-state to choose
EVFILT_SIGNAL’s EV_DELETE versus EV_ADD command in the EV_SET call. Keep the
existing normalized eloop->nsignals value for subsequent signal processing.
🪄 Autofix (Beta)
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
Run ID: 1c9fa012-e070-46f9-a827-a4f069f14078
📒 Files selected for processing (2)
compat/getpeereid.csrc/eloop.c
🚧 Files skipped from review as they are similar to previous changes (1)
- compat/getpeereid.c
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/control.c (1)
651-689: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider undefining the macro or using a function-like macro to prevent namespace pollution.
Defining a generic macro like
GIDmid-file without an#undefallows it to leak, which can cause unexpected macro expansions in code added later. Using a function-like macro also clarifies the intent since it visually resembles a cast or function call rather than a prefix operator.♻️ Proposed refactor
`#ifdef` __APPLE__ /* why is getgrouplist API is different ..... */ int *groups = NULL, *gp; -#define GID (int) +#define CAST_GID(x) ((int)(x)) `#else` gid_t *groups = NULL, *gp; -#define GID +#define CAST_GID(x) (x) `#endif` struct passwd *pw; int ngroups = 10, err = -1; pw = getpwuid(uid); if (pw == NULL) return 0; /* unknown user is not privileged */ groups = reallocarray(groups, (size_t)ngroups, sizeof(*groups)); if (groups == NULL) return -1; - if (getgrouplist(pw->pw_name, GID gid, groups, &ngroups) == -1) { + if (getgrouplist(pw->pw_name, CAST_GID(gid), groups, &ngroups) == -1) { gp = reallocarray(groups, (size_t)ngroups, sizeof(*groups)); if (gp == NULL) goto out; groups = gp; - if (getgrouplist(pw->pw_name, GID gid, groups, &ngroups) == -1) + if (getgrouplist(pw->pw_name, CAST_GID(gid), groups, &ngroups) == -1) goto out; } for (gp = groups; ngroups != 0; ngroups--, gp++) { - if (*gp == GID ctx->control_group) { + if (*gp == CAST_GID(ctx->control_group)) { err = 1; goto out; } } err = 0; out: + +#undef CAST_GID free(groups); return err;🤖 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 `@src/control.c` around lines 651 - 689, Replace the object-like GID macro in the getgrouplist logic with a function-like cast macro, or undefine it immediately after its final use, so it cannot affect later code. Preserve the platform-specific cast behavior for the getgrouplist and group comparison expressions.
🤖 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.
Nitpick comments:
In `@src/control.c`:
- Around line 651-689: Replace the object-like GID macro in the getgrouplist
logic with a function-like cast macro, or undefine it immediately after its
final use, so it cannot affect later code. Preserve the platform-specific cast
behavior for the getgrouplist and group comparison expressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d10855b1-e3d6-45ea-8e2f-0722494cfa49
📒 Files selected for processing (2)
src/control.csrc/eloop.c
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/privsep-control.c (1)
175-185: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn after the fatal receive error.
After
recvmsgreturns-1, execution reaches Line 184 and converts it toSIZE_MAX, causingcontrol_queuefto consume an invalid length frombuf.Proposed fix
if (msglen == -1) { logerr("%s: recvmsg", __func__); eloop_exit(ctx->eloop, EXIT_FAILURE); + return; }🤖 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 `@src/privsep-control.c` around lines 175 - 185, Return immediately after handling the fatal recvmsg error in the msglen == -1 branch, before iterating over control_fds or calling control_queuef. Preserve the existing logging and eloop_exit behavior.
🧹 Nitpick comments (1)
src/privsep-control.c (1)
111-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd credential-boundary regression coverage.
The PR objective calls out missing testing. Cover control-group access, readgroup-only access, rejected privileged commands, and concurrent control clients sharing the proxy.
🤖 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 `@src/privsep-control.c` around lines 111 - 126, Add regression tests for the control handling path around control_new and control_recvmsg covering control-group access, readgroup-only access, rejection of privileged commands, and concurrent control clients sharing the proxy. Verify both permitted and denied operations and preserve existing cleanup behavior for each client scenario.
🤖 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 `@src/dhcpcd.c`:
- Around line 1669-1682: Add BSD integration tests covering the authorization
matrix in the doauth path of dhcpcd: validate FD_CONTROL for normal requests,
FD_READ for DHCPCD_DUMPLEASE, and neither flag for both cases. Include
--getinterfaces framing checks and assert EPERM responses for unauthorized
peers, using the existing BSD integration-test helpers and peer-credential/group
setup.
- Around line 1893-1907: Update dhcpcd_readerror to accumulate exactly
sizeof(error) bytes from ctx->control_fd before decoding the error value,
handling partial reads and continuing until the complete frame is received.
Preserve the existing -1 failure behavior and EINVAL handling for an incomplete
or invalid stream response, then set errno and return the decoded error only
after the full frame is assembled.
- Around line 1602-1606: Initialize opts before the --getinterfaces branch in
the argument-processing flow, ensuring it has a defined zero value before the
opts |= DHCPCD_DUMPLEASE operation and subsequent doauth jump. Preserve the
existing option initialization semantics for all other argument paths.
In `@src/privsep-control.c`:
- Around line 84-97: Update ps_ctl_handleargs to use length-bounded command
comparisons based on len instead of strcmp, since data may not be
NUL-terminated. Preserve the existing matches and command-specific handling for
--version, --getconfigfile, --isprivileged, and --listen, including their
expected newline distinctions.
---
Outside diff comments:
In `@src/privsep-control.c`:
- Around line 175-185: Return immediately after handling the fatal recvmsg error
in the msglen == -1 branch, before iterating over control_fds or calling
control_queuef. Preserve the existing logging and eloop_exit behavior.
---
Nitpick comments:
In `@src/privsep-control.c`:
- Around line 111-126: Add regression tests for the control handling path around
control_new and control_recvmsg covering control-group access, readgroup-only
access, rejection of privileged commands, and concurrent control clients sharing
the proxy. Verify both permitted and denied operations and preserve existing
cleanup behavior for each client scenario.
🪄 Autofix (Beta)
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: fe92d6a4-70e1-4043-ba77-4f0f1cd03236
📒 Files selected for processing (13)
src/control.csrc/control.hsrc/dhcpcd.csrc/dhcpcd.conf.5.insrc/dhcpcd.hsrc/if-options.csrc/if-options.hsrc/privsep-control.csrc/privsep-control.hsrc/privsep-root.csrc/privsep-root.hsrc/privsep.hsrc/script.c
🚧 Files skipped from review as they are similar to previous changes (3)
- src/privsep-root.h
- src/privsep-control.h
- src/control.c
| } else if (strcmp(*argv, "--getinterfaces") == 0) { | ||
| oifind = argc = 0; | ||
| goto dumplease; | ||
| opts |= DHCPCD_DUMPLEASE; | ||
| do_getifs = true; | ||
| goto doauth; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/sh
rg -n -C3 'unsigned long long opts|opts \|= DHCPCD_DUMPLEASE|opts = 0' src/dhcpcd.cRepository: NetworkConfiguration/dhcpcd
Length of output: 1821
🏁 Script executed:
#!/bin/sh
sed -n '1576,1640p' src/dhcpcd.cRepository: NetworkConfiguration/dhcpcd
Length of output: 1950
🏁 Script executed:
#!/bin/sh
printf '%s\n' '--- doauth usage in handler ---'
sed -n '1660,1780p' src/dhcpcd.c
printf '\n--- DHCPCD_DUMPLEASE definition and uses ---'
rg -n 'DHCPCD_DUMPLEASE|do_getifs|opts \|=|opts =' src/dhcpcd.c src/privsep-control.c 2>/dev/null | sed -n '1,120p'Repository: NetworkConfiguration/dhcpcd
Length of output: 4630
Initialize opts before handling --getinterfaces.
opts is initialized at opts = 0 after the long-argument parsing, but this control-socket branch compound-assigns DHCPCD_DUMPLEASE before that and then jumps to doauth; this reads indeterminate storage.
Proposed fix
- opts |= DHCPCD_DUMPLEASE;
+ opts = DHCPCD_DUMPLEASE;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (strcmp(*argv, "--getinterfaces") == 0) { | |
| oifind = argc = 0; | |
| goto dumplease; | |
| opts |= DHCPCD_DUMPLEASE; | |
| do_getifs = true; | |
| goto doauth; | |
| } else if (strcmp(*argv, "--getinterfaces") == 0) { | |
| oifind = argc = 0; | |
| opts = DHCPCD_DUMPLEASE; | |
| do_getifs = true; | |
| goto doauth; |
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 1604-1604: Uninitialized variable
(legacyUninitvar)
🤖 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 `@src/dhcpcd.c` around lines 1602 - 1606, Initialize opts before the
--getinterfaces branch in the argument-processing flow, ensuring it has a
defined zero value before the opts |= DHCPCD_DUMPLEASE operation and subsequent
doauth jump. Preserve the existing option initialization semantics for all other
argument paths.
Source: Linters/SAST tools
| /* If we are not dumping the lease we must be able to control | ||
| * dhcpcd */ | ||
| doauth: | ||
| if (opts & DHCPCD_DUMPLEASE) { | ||
| if (fd->flags & FD_READ) | ||
| error = 0; | ||
| else | ||
| error = EPERM; | ||
| } else { | ||
| if (fd->flags & FD_CONTROL) | ||
| error = 0; | ||
| else | ||
| error = EPERM; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Add BSD integration coverage for the new authorization boundary.
Cover the FD_CONTROL / FD_READ / neither matrix, including --getinterfaces framing and EPERM replies. This PR moves access control to peer credentials and group membership, so regressions here become privilege-boundary failures.
🤖 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 `@src/dhcpcd.c` around lines 1669 - 1682, Add BSD integration tests covering
the authorization matrix in the doauth path of dhcpcd: validate FD_CONTROL for
normal requests, FD_READ for DHCPCD_DUMPLEASE, and neither flag for both cases.
Include --getinterfaces framing checks and assert EPERM responses for
unauthorized peers, using the existing BSD integration-test helpers and
peer-credential/group setup.
| static int | ||
| dhcpcd_readerror(struct dhcpcd_ctx *ctx) | ||
| { | ||
| int error = 0; | ||
| ssize_t len; | ||
|
|
||
| len = read(ctx->control_fd, &error, sizeof(error)); | ||
| if (len == -1) | ||
| return -1; | ||
| if (len != sizeof(error)) { | ||
| errno = EINVAL; | ||
| return -1; | ||
| } | ||
| errno = error; | ||
| return error; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Accumulate the complete error frame before decoding it.
SOCK_STREAM does not preserve write boundaries; read may return fewer than sizeof(error) bytes. A valid fragmented response is currently rejected with EINVAL.
Proposed fix
static int
dhcpcd_readerror(struct dhcpcd_ctx *ctx)
{
int error = 0;
- ssize_t len;
+ size_t off = 0;
- len = read(ctx->control_fd, &error, sizeof(error));
- if (len == -1)
- return -1;
- if (len != sizeof(error)) {
- errno = EINVAL;
- return -1;
+ while (off != sizeof(error)) {
+ ssize_t len = read(ctx->control_fd, (char *)&error + off,
+ sizeof(error) - off);
+
+ if (len == -1) {
+ if (errno == EINTR)
+ continue;
+ return -1;
+ }
+ if (len == 0) {
+ errno = EINVAL;
+ return -1;
+ }
+ off += (size_t)len;
}
errno = error;
return error;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static int | |
| dhcpcd_readerror(struct dhcpcd_ctx *ctx) | |
| { | |
| int error = 0; | |
| ssize_t len; | |
| len = read(ctx->control_fd, &error, sizeof(error)); | |
| if (len == -1) | |
| return -1; | |
| if (len != sizeof(error)) { | |
| errno = EINVAL; | |
| return -1; | |
| } | |
| errno = error; | |
| return error; | |
| static int | |
| dhcpcd_readerror(struct dhcpcd_ctx *ctx) | |
| { | |
| int error = 0; | |
| size_t off = 0; | |
| while (off != sizeof(error)) { | |
| ssize_t len = read(ctx->control_fd, (char *)&error + off, | |
| sizeof(error) - off); | |
| if (len == -1) { | |
| if (errno == EINTR) | |
| continue; | |
| return -1; | |
| } | |
| if (len == 0) { | |
| errno = EINVAL; | |
| return -1; | |
| } | |
| off += (size_t)len; | |
| } | |
| errno = error; | |
| return error; | |
| } |
🤖 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 `@src/dhcpcd.c` around lines 1893 - 1907, Update dhcpcd_readerror to accumulate
exactly sizeof(error) bytes from ctx->control_fd before decoding the error
value, handling partial reads and continuing until the complete frame is
received. Preserve the existing -1 failure behavior and EINVAL handling for an
incomplete or invalid stream response, then set errno and return the decoded
error only after the full frame is assembled.
| ps_ctl_handleargs(struct fd_list *fd, const char *data, __unused size_t len) | ||
| { | ||
| /* Make any change here in dhcpcd.c as well. */ | ||
| if (strncmp(data, "--version", MIN(strlen("--version"), len)) == 0) { | ||
| return control_queue(fd, UNCONST(VERSION), strlen(VERSION) + 1); | ||
| } else if (strncmp(data, "--getconfigfile", | ||
| MIN(strlen("--getconfigfile"), len)) == 0) { | ||
| return control_queue(fd, UNCONST(fd->ctx->cffile), | ||
| /* Make any change here in dhcpcd.c as well. | ||
| * --version is NOT terminated with \n. */ | ||
| if (strcmp(data, "--version") == 0) { | ||
| return control_queue(fd, VERSION, strlen(VERSION) + 1); | ||
| } else if (strcmp(data, "--getconfigfile\n") == 0) { | ||
| return control_queue(fd, fd->ctx->cffile, | ||
| strlen(fd->ctx->cffile) + 1); | ||
| } else if (strncmp(data, "--listen", MIN(strlen("--listen"), len)) == | ||
| 0) { | ||
| fd->flags |= FD_LISTEN; | ||
| return 0; | ||
| } | ||
| } else if (strcmp(data, "--isprivileged\n") == 0) { | ||
| const char *ret = fd->flags & FD_CONTROL ? "true" : "false"; | ||
| return control_queue(fd, ret, strlen(ret) + 1); | ||
| } else if (strcmp(data, "--listen\n") == 0) | ||
| return control_handle_listen(fd); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use length-bounded command matching.
data is raw recvmsg input and need not be NUL-terminated; each strcmp can read past len.
Proposed fix
-ps_ctl_handleargs(struct fd_list *fd, const char *data, __unused size_t len)
+ps_ctl_handleargs(struct fd_list *fd, const char *data, size_t len)
{
- if (strcmp(data, "--version") == 0) {
+ if (len == sizeof("--version") - 1 &&
+ memcmp(data, "--version", sizeof("--version") - 1) == 0) {
return control_queue(fd, VERSION, strlen(VERSION) + 1);
- } else if (strcmp(data, "--getconfigfile\n") == 0) {
+ } else if (len == sizeof("--getconfigfile\n") - 1 &&
+ memcmp(data, "--getconfigfile\n",
+ sizeof("--getconfigfile\n") - 1) == 0) {
return control_queue(fd, fd->ctx->cffile,
strlen(fd->ctx->cffile) + 1);
- } else if (strcmp(data, "--isprivileged\n") == 0) {
+ } else if (len == sizeof("--isprivileged\n") - 1 &&
+ memcmp(data, "--isprivileged\n",
+ sizeof("--isprivileged\n") - 1) == 0) {
const char *ret = fd->flags & FD_CONTROL ? "true" : "false";
return control_queue(fd, ret, strlen(ret) + 1);
- } else if (strcmp(data, "--listen\n") == 0)
+ } else if (len == sizeof("--listen\n") - 1 &&
+ memcmp(data, "--listen\n", sizeof("--listen\n") - 1) == 0)
return control_handle_listen(fd);🤖 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 `@src/privsep-control.c` around lines 84 - 97, Update ps_ctl_handleargs to use
length-bounded command comparisons based on len instead of strcmp, since data
may not be NUL-terminated. Preserve the existing matches and command-specific
handling for --version, --getconfigfile, --isprivileged, and --listen, including
their expected newline distinctions.
1 participant
Instead workout credentials from the connected socket by getpeereid(2).
Add
readgroupto dhcpcd.conf which controls who can read interface data from the control socket.Each control socket command will now return an errno after the command has been processed to indicate the result of the command - for example EPERM.
Remove the limit of only one connected control client.
This is a breaking change for socket consumers.