privsep: Don't open PF_INET socket for each ioctl#640
Conversation
Because we already have it open ....
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesPrivilege separation socket handling refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/privsep-root.c (1)
202-237:⚠️ Potential issue | 🔴 CriticalDecode ioctl request once; use the same value for both the allow-list and syscall.
The function receives
reqas an unsigned long (from the wire), but on systems whereIOCTL_REQUEST_TYPEis narrower, the allow-list check at line 209 validates the padded wire value while line 235 issues the decoded narrow value toioctl(). On big-endian targets, this mismatch allows the checked request and issued request to differ, potentially bypassing the allow-list. Decode the request once at function entry and use that decoded value for both the switch statement and the ioctl call.Proposed fix
static ssize_t ps_root_doioctl(int fd, unsigned long req, void *data, size_t len) { `#ifdef` IOCTL_REQUEST_TYPE - ioctl_request_t reqt; + ioctl_request_t ioreq; + + memcpy(&ioreq, &req, sizeof(ioreq)); +#else + unsigned long ioreq = req; `#endif` /* Only allow these ioctls */ - switch (req) { + switch (ioreq) { @@ `#ifdef` IOCTL_REQUEST_TYPE - memcpy(&reqt, &req, sizeof(reqt)); - return ioctl(fd, reqt, data, len); -#else - return ioctl(fd, req, data, len); `#endif` + return ioctl(fd, ioreq, data, len); }🤖 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 202 - 237, The function ps_root_doioctl has a security vulnerability where the allow-list validation and the ioctl syscall use different representations of the request value on systems with IOCTL_REQUEST_TYPE. On big-endian targets, the padded wire value checked in the switch statement may differ from the decoded narrow value used in the ioctl call, bypassing the allow-list. Decode the ioctl request value once at the beginning of the function immediately after entry (before the switch statement) by performing the memcpy operation that converts from unsigned long req to ioctl_request_t reqt. Then use this decoded reqt value in both the switch statement for validation and in the ioctl syscall, replacing the current conditional logic that currently applies the decoding only for the IOCTL_REQUEST_TYPE case.
🤖 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-root.c`:
- Around line 507-508: The code at line 507-508 in src/privsep-root.c uses
ctx->pf_inet_fd for ps_root_doioctl calls, but the socket initialization at line
649-650 only logs failures and continues without ensuring a valid fd is set,
causing subsequent EBADF errors when ctx->pf_inet_fd remains invalid. Either
make the if_opensockets failure fatal by calling it before entering the main
request-serving loop (so requests are never served with an invalid fd), or add a
one-shot recovery mechanism at the ps_root_doioctl call site that attempts to
open a socket if ctx->pf_inet_fd is -1 before proceeding with the ioctl
operation. Choose the approach that best fits the intended control flow and
error handling strategy.
---
Outside diff comments:
In `@src/privsep-root.c`:
- Around line 202-237: The function ps_root_doioctl has a security vulnerability
where the allow-list validation and the ioctl syscall use different
representations of the request value on systems with IOCTL_REQUEST_TYPE. On
big-endian targets, the padded wire value checked in the switch statement may
differ from the decoded narrow value used in the ioctl call, bypassing the
allow-list. Decode the ioctl request value once at the beginning of the function
immediately after entry (before the switch statement) by performing the memcpy
operation that converts from unsigned long req to ioctl_request_t reqt. Then use
this decoded reqt value in both the switch statement for validation and in the
ioctl syscall, replacing the current conditional logic that currently applies
the decoding only for the IOCTL_REQUEST_TYPE case.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
1 participant
Because we already have it open ....