Skip to content
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

Tidy handling of privileged operation fds #666

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

smcv
Copy link
Collaborator

@smcv smcv commented Oct 30, 2024

Similar to #665, but for the socket pair that communicates privileged operations between the temporary unprivileged child and the privileged parent when we are setuid root.

  • Use PIPE_READ_END, PIPE_WRITE_END to clarify use of privileged op sockets

    Both sockets in this socket pair are technically bidirectional, but
    we're using them in a way that is close enough to unidirectional that
    using these symbolic constants is clearer than magic numbers.
    We send multi-byte requests into the write end, and read those requests
    from the read end (even though we also send a 1-byte reply to each request
    into the "read" end, and read it from the "write" end).

  • utils: Add steal_fd()

    This is inspired by g_steal_fd() in GLib, and lets us make it explicit
    that ownership of a fd is being moved, similar to steal_pointer().

  • Clarify ownership of privileged op sockets

    Setting the members of privsep_sockets[] to -1 when they have been
    closed or had their ownership transferred is clearer than leaving behind
    dangling references.

cc @refi64

smcv added 3 commits October 30, 2024 14:49
…kets

Both sockets in this socket pair are technically bidirectional, but
we're using them in a way that is close enough to unidirectional that
using these symbolic constants is clearer than magic numbers.
We send multi-byte requests into the write end, and read those requests
from the read end (even though we also send a 1-byte reply to each request
into the "read" end, and read it from the "write" end).

Signed-off-by: Simon McVittie <smcv@collabora.com>
This is inspired by g_steal_fd() in GLib, and lets us make it explicit
that ownership of a fd is being moved, similar to steal_pointer().

Signed-off-by: Simon McVittie <smcv@collabora.com>
Setting the members of privsep_sockets[] to -1 when they have been
closed or had their ownership transferred is clearer than leaving behind
dangling references.

Signed-off-by: Simon McVittie <smcv@collabora.com>
smcv added a commit to smcv/bubblewrap that referenced this pull request Oct 30, 2024
This avoids leaving dangling references to fds that no longer exist,
clarifying ownership.

This commit does not cover the socket pairs used to transfer the pid
of a descendant process (see containers#665 for that) and privilege-separated
operations (see containers#666).

Signed-off-by: Simon McVittie <smcv@collabora.com>
@smcv smcv marked this pull request as draft October 30, 2024 15:05
@smcv
Copy link
Collaborator Author

smcv commented Oct 30, 2024

Setting the members of privsep_sockets[] to -1

... is not (currently) something that cleanup_fdp() does, so this PR is wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant