Details

Reviewers

brooks
rwatson

Group Reviewers

capsicum

Commits

rGd0675399d09f: capsicum: allow subset of wait4(2) functionality

Summary

The usual way of handling process exit exit in capsicum(4) mode is by using process
descriptors (pdfork(2)) instead of the traditional fork(2)/wait4(2) API. But most apps
hadn't been converted this way, and many cannot because the wait is hidden behind
a library APIs that revolve around PID numbers and not descriptors; GLib's
g_spawn_check_wait_status(3) is one example.

Thus, provide backwards compatibility by allowing the wait(2) family of functions
in Capsicum mode, except for child processes created by pdfork(2).

One practical result is that with this patch, a capsicated irssi(1) is no longer
leaving zombies around.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 58216
Build 55104: arc lint + arc unit

Event Timeline

trasz created this revision.Mar 15 2024, 12:22 PM

Herald added a reviewer: brooks. · View Herald TranscriptMar 15 2024, 12:22 PM

Herald added subscribers: olce, jonathan, imp. · View Herald Transcript

trasz requested review of this revision.Mar 15 2024, 12:22 PM

Harbormaster completed remote builds in B56624: Diff 135782.Mar 15 2024, 12:22 PM

trasz added a reviewer: capsicum.Mar 15 2024, 12:24 PM

I think this behavior should be documented in the man pages (capsicum/wait), as it isn't obvious.

Also, do you have any particular cases or example of application where you want to use it?

trasz mentioned this in D44373: Capsicum vs the Pathnames, a PoC.Mar 15 2024, 12:48 PM

What about wait6?

Should there be a way for a program to disable this?

I agree this should be documented somewhere, but at the moment wait(2) doesn't mention Capsicum at all, and capsicum(4) doesn't mention wait(2). Perhaps a paragraph in pdfork(2), something along the lines of "processes created with pdfork cannot be waited for by a parent running in capsicum(4) mode"?

Regarding use case - sh(1) when running with D44373 in place. Also either bmake(1) or clang(1) in a similar circumstances.

As for wait6(2) - definitely; I'll update the patch. Should I also handle wait(2) similarly, or is that just for backward compatibility?

As for disabling - I'm not sure. Are there cases where we'd need to?

Fix panic which occured when the PID is specified explictly.
Also handle wait6(2). Add some documentation. Pacify a test.

Harbormaster completed remote builds in B56722: Diff 136036.Mar 21 2024, 1:33 PM

trasz edited the summary of this revision. (Show Details)Apr 22 2024, 1:37 PM

trasz edited the summary of this revision. (Show Details)May 1 2024, 6:15 PM

@brooks Are you happy with wait6?

brooks added inline comments.May 13 2024, 6:47 PM

lib/libsys/wait.2
606–615	I found the edit hard to read, particularly the second sentence. Perhaps this option is better.

Man page fix from Brooks.

Harbormaster completed remote builds in B57701: Diff 138530.May 14 2024, 10:23 AM

Sigh, a typo.

Harbormaster completed remote builds in B57703: Diff 138532.May 14 2024, 10:25 AM

Hey folks,

Sorry to Statler / Waldorf, but I have a student who's been bumping up against wait4 limitations when doing some transparent/oblivious sandboxing stuff, so perhaps this is timely feedback! :)

If we're going to allow wait4 and/or wait6, I'm wondering if it would make sense to maintain the disallowance of access to the (global) PID and process group ID namespaces. Allowing "wait for any of my children" would help with a lot of processes, whereas opening up any global namespace access seems to weaken the model in ways that I don't (yet) understand...

Jon

sys/kern/kern_exit.c
1209	If `IN_CAPABILITY_MODE`, would it make sense to check and confirm that `pid == WAIT_ANY` (to avoid using global PID namespace)?
1340	Would it also make sense to check that `idtype` is `P_ALL`? That way, the process isn't using the PID or process group ID namespaces...

Sorry to Statler / Waldorf

Hah!

If we're going to allow wait4 and/or wait6, I'm wondering if it would make sense to maintain the disallowance of access to the (global) PID and process group ID namespaces. Allowing "wait for any of my children" would help with a lot of processes, whereas opening up any global namespace access seems to weaken the model in ways that I don't (yet) understand...

Yes - I was uneasy with this proposal (this review) but am happy with it, with your suggestion included. Disallowing unless P_ALL, or with id == 0 P_PID or P_PGID makes sense to me.

I might be wrong, but isn’t this restriction already there, inherent to wait(2) APIs? You need to use kqueue to wait for non-children?

(And also an earlier version of this did exactly that wrt idtype, that’s why the title still mentions the “limited subset”; only after that I’ve discovered that you can’t wait for arbitrary PIDs anyway.)

I think this change looks OK as long as wait6(2) is limited to checking children [and perhaps their children, etc]. It doesn’t look to me like it can ever originate the list of processes from allproc or similar, so I believe this is OK?

(The implicit argument is that we could have used pdwait(2) on any child of the current process, as a result of having the option to call pdfork(2) to create them, so as long as it’s children we are able to wait for, no matter how named, it should be OK. Perhaps we should write this not down somewhere?)

lib/libsys/wait.2
610–614	I find the double negative in this sentence very confusing. Can we find a way to reword this?

This revision now requires changes to proceed.May 21 2024, 1:09 PM

trasz added inline comments.May 27 2024, 9:25 AM

lib/libsys/wait.2
610–614	How about something like: By default, the wait family of functions will not return a child process created with pdfork. They can be directed to do so by specifying its process ID; this is not allowed in Capsicum capability mode.

Man page rewording from gnn@

Harbormaster completed remote builds in B58216: Diff 139947.Jun 18 2024, 10:51 AM

Drop wait6(2) for now

There's certain... uneasiness about idtypes other than P_ALL; while this
is just a filter, let's lean on the safe side and just drop wait6(2) for
now. We can get back to it in a followup review.

Harbormaster completed remote builds in B58786: Diff 141269.Jul 23 2024, 12:33 PM

brooks accepted this revision.Jul 23 2024, 4:50 PM

This revision was not accepted when it landed; it landed in state Needs Review.Aug 27 2024, 3:27 PM

Closed by commit rGd0675399d09f: capsicum: allow subset of wait4(2) functionality (authored by trasz, committed by oshogbo). · Explain Why

This revision was automatically updated to reflect the committed changes.

oshogbo added a commit: rGd0675399d09f: capsicum: allow subset of wait4(2) functionality.

Allow subset of wait4(2) functionality in Capsicum mode
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 139947

contrib/capsicum-test/capmode.cc

lib/libsys/wait.2

sys/kern/kern_exit.c

sys/kern/syscalls.master

Allow subset of wait4(2) functionality in Capsicum modeClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 139947

contrib/capsicum-test/capmode.cc

lib/libsys/wait.2

sys/kern/kern_exit.c

sys/kern/syscalls.master

Allow subset of wait4(2) functionality in Capsicum mode
ClosedPublic
Actions

Revision Contents
Changeset List