Capsicumise truss(1)

[kib]

In D58094#1332492, @markj wrote:

Even if the debugee is also in capability mode, the debugger would still potentially be able to obtain capabilities it doesn't already have, violating the sandbox rules.

For this, the debugger needs to get the procdesc to the target with enough cap allowed. If something provided it, then it was the decision.

[kib]

Add fget_procdesc().
Fix several bugs.

I did not converted sys_pdgetpid() to fget_procdesc() because I require proctree_lock for the call. In principle this can be relaxed if we do not return p, and then sys_procdesc() can use fget_procdesc().

[kevans]

In D58094#1332533, @markj wrote:

Given that LSAN is a testing tool, why do we care about having it work properly in capability mode at all? The sanitizer runtime could intercept cap_enter()/caph_enter() and prevent the target process from entering a sandbox when running under LSAN.

Another fair point- from my perspective, we probably don't particularly care about it working properly in capability mode, but we should care about the fact that capability mode would causes us to lose this incredibly useful functionality.

That can be solved in the majority of capsicumized things today by intercepting cap_enter, as you note- that doesn't help if an instrumented program was exec'd already in a sandbox by an uninstrumented parent, but to my knowledge we don't really have any case like that in base. I don't think "disabling capsicum on a larger scale if you want to use lsan" (the logical next step to address the last case) is a great approach if we can find a way to make it work safely in that context.

[kib]

Convert sys_pdgetpid().
Enable ptrace(2) for cap mode, but unconditionally disable PT_ATTACH.

[markj]

In D58094#1332534, @kib wrote:

In D58094#1332492, @markj wrote:

Even if the debugee is also in capability mode, the debugger would still potentially be able to obtain capabilities it doesn't already have, violating the sandbox rules.

For this, the debugger needs to get the procdesc to the target with enough cap allowed. If something provided it, then it was the decision.

... or a sandbox escape bug, if someone forgot to limit rights on the procdesc. There is at least one example in the tree. In particular, this is not a decision made by any existing code, since currently a process descriptor does not give you access to the target process' capabilities.

In D58094#1332543, @kevans wrote:

In D58094#1332533, @markj wrote:

Given that LSAN is a testing tool, why do we care about having it work properly in capability mode at all? The sanitizer runtime could intercept cap_enter()/caph_enter() and prevent the target process from entering a sandbox when running under LSAN.

Another fair point- from my perspective, we probably don't particularly care about it working properly in capability mode, but we should care about the fact that capability mode would causes us to lose this incredibly useful functionality.

That can be solved in the majority of capsicumized things today by intercepting cap_enter, as you note- that doesn't help if an instrumented program was exec'd already in a sandbox by an uninstrumented parent, but to my knowledge we don't really have any case like that in base. I don't think "disabling capsicum on a larger scale if you want to use lsan" (the logical next step to address the last case) is a great approach if we can find a way to make it work safely in that context.

I suppose so, but extending the kernel to support this rather esoteric case, and in so doing increasing the attack surface available to any sandboxed process owning a procdesc with CAP_PTRACE_ATTACH, seems like a dubious tradeoff...

[kevans]

In D58094#1332550, @markj wrote:

In D58094#1332534, @kib wrote:

In D58094#1332492, @markj wrote:

Even if the debugee is also in capability mode, the debugger would still potentially be able to obtain capabilities it doesn't already have, violating the sandbox rules.

For this, the debugger needs to get the procdesc to the target with enough cap allowed. If something provided it, then it was the decision.

... or a sandbox escape bug, if someone forgot to limit rights on the procdesc. There is at least one example in the tree. In particular, this is not a decision made by any existing code, since currently a process descriptor does not give you access to the target process' capabilities.

In D58094#1332543, @kevans wrote:

In D58094#1332533, @markj wrote:

Given that LSAN is a testing tool, why do we care about having it work properly in capability mode at all? The sanitizer runtime could intercept cap_enter()/caph_enter() and prevent the target process from entering a sandbox when running under LSAN.

Another fair point- from my perspective, we probably don't particularly care about it working properly in capability mode, but we should care about the fact that capability mode would causes us to lose this incredibly useful functionality.

That can be solved in the majority of capsicumized things today by intercepting cap_enter, as you note- that doesn't help if an instrumented program was exec'd already in a sandbox by an uninstrumented parent, but to my knowledge we don't really have any case like that in base. I don't think "disabling capsicum on a larger scale if you want to use lsan" (the logical next step to address the last case) is a great approach if we can find a way to make it work safely in that context.

I suppose so, but extending the kernel to support this rather esoteric case, and in so doing increasing the attack surface available to any sandboxed process owning a procdesc with CAP_PTRACE_ATTACH, seems like a dubious tradeoff...

This is why the latest round of my proposal was that it probably has to be an active decision to bless the relationship (like PT_TRACE_ME), rather than something inherently allowed (if we choose to allow it). Maybe the shape of that is that you have to opt-in for the rights(4) with a new PD_* flag or something.

[kib]

Add a global knob required to enable ptrace in cap mode.

[kib]

PD_ATTACH_CAP.
Remove CAP_PTRACE from the default list of caps.

[markj]

I don't think this patch is a good idea. It adds a bunch of complexity and state to a security layer in order to support a debugging feature, as I understand it.

Sanitizers and debuggers don't work properly with ASLR, so they just turn off ASLR. Why isn't that the approach here?

[kevans]

In D58094#1332688, @markj wrote:

I don't think this patch is a good idea. It adds a bunch of complexity and state to a security layer in order to support a debugging feature, as I understand it.

Sanitizers and debuggers don't work properly with ASLR, so they just turn off ASLR. Why isn't that the approach here?

Because they actually can disable ASLR, but you can't escape a sandbox that your parent placed you in- the blast radius is wider than it strictly needs to be and requires more intervention.

[kib]

In D58094#1332688, @markj wrote:

I don't think this patch is a good idea. It adds a bunch of complexity and state to a security layer in order to support a debugging feature, as I understand it.

Sanitizers and debuggers don't work properly with ASLR, so they just turn off ASLR. Why isn't that the approach here?

Well, ASLR is the feature of the target, not of the debugger. There, the proposed feature should allow the debugger to run with less privileges, even if it potentially could get some privileges of the target, after more work.

The model I had in mind was gdb, or better, gdb remote server. There we have a process that ptrace-es the target, and also talks to a socket, executing ptrace actions in response to messages. It does not need to open files, or establish new network connections, or access the global process namespace after initial attach (not quite apparently, see below).

But then I realized that there is much better example, which we have in the tree. Consider truss(1): after attach, it only writes to trace file, besides executing the ptrace actions. But, it parses a lot of data from the target. Did anybody looked what could a malicious target trigger in the truss parsers? Having truss executing in cap mode would be beneficial IMO.

The problem that is not solved by this patch is that truss needs to attach to children to follow them, I think that to make it functional, this approach needs to be extended to allow opening a child when we have ptrace-able procdesc on the parent.

Lets return to the patch at hand. For now, I propose to split out fget_procdesc() and consider just it. IMO it is the most delicate part of the series, everything else looks relatively simple (putting the architectural views aside). I put it into the separate review D58117.

[kib]

Capsicumise truss.

This is not a proposed commit, I will gradually push the reviews from this branch.
I want to show the usefulness of the proposed extension.

[kib]

Rebase after fget_procdesc() and NOTE_PDSIGCHLD commits.

[kib]

The actual patch.

[kib]

Some restructuring and man pages updates.
Introduce p_canopen() to centralize pdopenpid() policy check.

Note that further testing shown the problem with the truss patches. Truss steals zombie' status from the real parent, because it opens the procdescs. This can be worked around by special handling for the wait*() syscalls, I believe, but I did not implemented it.

[kib]

This is still not intended for review, I just demonstrate the (IMO reasonable) full branch where the end is the ability to run truss in cap mode.
I am posting individual commits from the branch for review still.

[kib]

Use pdptrace(2)
Remove changes submitted as other reviews.

[kib]

Wrap more ptrace(2) calls.

[kib]

Use PT_GET_ABI_NAME. No kernel patches in this review left.

[kib]

Rebase.
Use pdptrace(2) without syscall wrapper, now that the syscall is upstreamed.

This is the final version of the patch, IMO ready for review.

[kib]

Rebase

[markj]

Why is capability mode optional?

usr.bin/truss/setup.c

222

We already have caph_enter() in capsicum_helpers.h, it will swallow ENOSYS from cap_enter(2). It is widely used in the src tree, why not use it here?

I don't really see the need for the force_cap_mode flag. If cap_enter() fails with ENOSYS we should just keep going, as the user configured their system to disable capsicum.

240

Why not use procdescs unconditionally?

usr.bin/truss/truss.1

79

81

[kib]

In D58094#1361090, @markj wrote:

Why is capability mode optional?
Why not use procdescs unconditionally?

Answers to both questions come from the same motivations:

• to allow users to work around the potential bugs in the new implementation
• to allow in-field debugging, where users can easily compare ptrace(2) vs pdptrace(2)-based code paths.

After your question, I think that the description of the '-y' option should more clearly explain that the difference is not only in the cap mode enter, but mainly in the use of different control facilities: kqueue(EVFILT_PROCDESC) + pdptrace(2) vs. waitpid(2) + ptrace(2).

[kib]

Use caph_enter().
Remove -Y option.
Update man page for -y.

[kib]

Remove one more instance of 'Y'.

[markj]

Some test cases were added to usr.bin/truss/tests recently. I wonder if they pass when truss is running in capmode?

In D58094#1361107, @kib wrote:

In D58094#1361090, @markj wrote:

Why is capability mode optional?
Why not use procdescs unconditionally?

Answers to both questions come from the same motivations:

• to allow users to work around the potential bugs in the new implementation
• to allow in-field debugging, where users can easily compare ptrace(2) vs pdptrace(2)-based code paths.

After your question, I think that the description of the '-y' option should more clearly explain that the difference is not only in the cap mode enter, but mainly in the use of different control facilities: kqueue(EVFILT_PROCDESC) + pdptrace(2) vs. waitpid(2) + ptrace(2).

The sense of the flag should be inverted: otherwise the capability mode functionality will not get much testing IMHO. In the long run, we should remove the flag entirely.

usr.bin/truss/setup.c

408

This doesn't match the prototype near the beginning of the file, the pfd parameter is in the wrong place.

916

Don't you need to set np here?

921

Is this incomplete?

[emaste]

The sense of the flag should be inverted: otherwise the capability mode functionality will not get much testing IMHO. In the long run, we should remove the flag entirely.

Agreed. If we really want to be conservative with the introduction we can do:
a. Make capmode opt-in
b. Switch capmode to opt-out
c. Remove non-capmode
but I think that's unnecessary here. I hope we're sufficiently confident that it can be enabled by default, and the option is just an escape hatch.

[kib]

Invert meaning of -y.
Fix new_proc() prototype.
Remove #if 0 block.
Initialize np var on new process.

[markj]

usr.bin/truss/setup.c

219

Why not let it be a fatal condition?

290–291

Is it correct to pass WCONTINUED here?

972

Is it possible for the number of children to change between here and the first call? Shouldn't we check that cnt did not change?

982

Doesn't eventloop_handle_trapped() also report the new child?

[kib]

usr.bin/truss/setup.c

219

Why ENOSYS is not fatal, but hypothetical other reasons would be? IMO it is not consistent, ENOSYS is the most important.

290–291

In the sense 'would it break the logic of the code' yes, I think it is correct. At worst it would cause more returns from truss_wait() which are filtered in the loop anyway.

I think that using WFLAGS there is fine and consistent. I can change it if you insist.

972

We cannot check that cnt did not change in the sense that it is fine to change, as far as we get some report. We only should care about ENOMEM. I added a retry.

982

There is a herald_printed check in the report_new_child() that prevents double reporting.

NOTE_FORK and PT_GET_CHILDREN enumeration are only used in the cap mode, so the report in eventloop() is needed for the classic mode. report_new_child() is bound to info->curthread, so it is not easy to remove either call.

[kib]

Retry after PT_GET_CHILDREN returned ENOMEM.

[markj]

usr.bin/truss/setup.c

219

ENOSYS means the admin configured the system to make cap_enter() a no-op. It is not really an error, but it would be wrong to return 0 there, since the guarantees of capability mode are not provided in that case.

Other errors are unexpected and should be fatal, IMO.

435

437

Why is ok to ignore the return value here?

[kib]

usr.bin/truss/setup.c

219

But the outcome with both cases is same. Also, the person who disabled capabilities in the kernel config, and the person who runs truss are not necessarily the same.

If this is the last stumbling point for agreeing with the patch, I will change it.

[kib]

Some polishing to the new_proc() handler.

[kib]

err() if caph_enter() failed

[markj]

usr.bin/truss/setup.c

219

If the administrator disabled capsicum globally, then programs should respect that (in general[*]). Any other errors from cap_enter() are unexpected, so why ignore them? Especially here, where the user has an option to disable the use of capsicum using a command-line flag.

I think I understand your point: cap_enter() errors should not be fatal since the program's correctness does not depend on being in capability mode (again see [*]). But we consistently treat these as fatal error elsewhere, so IMO there should be consistency. Maybe all programs in src should be changed to make cap_enter() failures non-fatal.

\[*] It is possible for the correctness of the program depends on capability mode. For instance, consider a server which lets a client upload files to the server filesystem, with a client-supplied path. Suppose the server uses openat(dirfd, <client path>) to open the file and begin the upload. Then the server probably wants O_RESOLVE_BENEATH semantics for that openat(), which are provided automatically in capability mode (capsicum existed before O_RESOLVE_BENEATH). In this case, the program should not operate if cap_enter() returns ENOSYS, since otherwise a client can upload a file anywhere it wants.