-
Notifications
You must be signed in to change notification settings - Fork 379
linux: initial hardening for /proc paths #1910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis patch hardens all File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@kolyshkin PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/libcrun/utils.c:2855` </location>
<code_context>
+ if (proc_fd < 0)
+ return proc_fd;
+
+ return TEMP_FAILURE_RETRY (openat (proc_fd, path, flags | O_CLOEXEC));
+}
+
</code_context>
<issue_to_address>
**suggestion:** Always adding O_CLOEXEC may override caller intent.
Check if O_CLOEXEC is already set in flags before adding it, or clarify in documentation that this function enforces O_CLOEXEC regardless of caller intent.
```suggestion
if (!(flags & O_CLOEXEC))
flags |= O_CLOEXEC;
return TEMP_FAILURE_RETRY (openat (proc_fd, path, flags));
```
</issue_to_address>
### Comment 2
<location> `src/libcrun/utils.c:2843` </location>
<code_context>
+ if (container->proc_fd < 0)
+ return crun_make_error (err, errno, "open `/proc`");
+ }
+ return container->proc_fd;
+}
+
</code_context>
<issue_to_address>
**question (bug_risk):** proc_fd lifetime may not match container usage.
If the container is used after fork or exec, proc_fd may be invalid. Please clarify its intended lifetime or ensure it is revalidated as needed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/libcrun/utils.c
Outdated
| if (proc_fd < 0) | ||
| return proc_fd; | ||
|
|
||
| return TEMP_FAILURE_RETRY (openat (proc_fd, path, flags | O_CLOEXEC)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Always adding O_CLOEXEC may override caller intent.
Check if O_CLOEXEC is already set in flags before adding it, or clarify in documentation that this function enforces O_CLOEXEC regardless of caller intent.
| return TEMP_FAILURE_RETRY (openat (proc_fd, path, flags | O_CLOEXEC)); | |
| if (!(flags & O_CLOEXEC)) | |
| flags |= O_CLOEXEC; | |
| return TEMP_FAILURE_RETRY (openat (proc_fd, path, flags)); |
| if (container->proc_fd < 0) | ||
| return crun_make_error (err, errno, "open `/proc`"); | ||
| } | ||
| return container->proc_fd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question (bug_risk): proc_fd lifetime may not match container usage.
If the container is used after fork or exec, proc_fd may be invalid. Please clarify its intended lifetime or ensure it is revalidated as needed.
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
2339fd4 to
a4941ad
Compare
Signed-off-by: Giuseppe Scrivano <[email protected]>
Signed-off-by: Giuseppe Scrivano <[email protected]>
a4941ad to
50e9898
Compare
|
@flouthoc PTAL |
flouthoc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
not an exhaustive list but implement some hardening for opens under
/procSummary by Sourcery
Harden filesystem interactions under /proc by introducing container-scoped directory file descriptors and replacing all direct
/procpath opens and writes with secureopenat-based helpers.Enhancements:
proc_fdto container context and utility functionslibcrun_get_cached_proc_fd,libcrun_open_proc_file, andlibcrun_open_proc_pid_filefor safer/procaccess./proc/*file open and write operations (uid_map, gid_map, setgroups, keycreate, caps, selinux, apparmor, sysctl, oom_score_adj, etc.) to use the new helpers andsafe_writeto mitigate TOCTOU and path-based vulnerabilities.containerparameter and leverage the hardened/procaccess model./procdirectory file descriptor when the container is freed.