Why not just use ptrace(PT_ATTACH, pdgetpid(), ...)? I can see some argument for PT_ATTACH_PD if you are planning to make ptrace() available in capability mode.

In D58094#1332276, @markj wrote:

Why not just use ptrace(PT_ATTACH, pdgetpid(), ...)? I can see some argument for PT_ATTACH_PD if you are planning to make ptrace() available in capability mode.

Exactly what Kyle said above.

In D58094#1332272, @kevans wrote:

Nice, this seems reasonable to me. I have a proposal that I'm gonig to send to a list, but effectively, I'd like to do a few things in this area:

• Add a convenience pdopenpid(0, ...) -> similar to what we do with setpgrp/setpggid, special-case 0 to allow self-pdopenpid without a getpid() call
• Mark pdopenpid(2) CAPENABLED, allow it in capability mode only for self-requests
• Mark ptrace(2) CAPENABLED, only allow: PT_ATTACH_PD, PT_DETACH, PT_GETNUMLWPS, PT_GETLWPLIST, PT_GETREGS

PT_VM_TIMESTAMP, TP_VM_ENTRY, PT_GETREGSET, PT_GET_SC_ARGS, PT_GET_SC_RET, and MD options.

But I really thought that if you have the pfd with the capability, then you can debug the target as you want. pdopenpid() should be not allowed lightly for capsicum, but you could get the pfd by other means, e.g. by passing it from some fully privileged wrapper.

I expect the ptrace(2) one to be the most contentious, but I think with a procdesc and very limited set of commands (and maybe some other restrictions?) it doesn't necessarily violate the spirit of capsicum. LeakSanitizer is the motivation (CC @aokblast) -- the best way we have today to reliably 'stop the world' *and* collect the necessary information is to rfork(RFPROC | RFMEM) and then ptrace(2) the parent to halt it.

The current version that hasn't landed anywhere obviously can't work in capability mode, but just the above operations are sufficient for it to figure out all of the stack bounds and then analyze its own address space. I'd love for us to be able to enable this pretty widely and add the same value for capsicumized things, even if it's pretty clear that capability-mode ptrace(2) is virtually useless everywhere except this one niche.

In D58094#1332294, @kib wrote:

In D58094#1332276, @markj wrote:

Why not just use ptrace(PT_ATTACH, pdgetpid(), ...)? I can see some argument for PT_ATTACH_PD if you are planning to make ptrace() available in capability mode.

Exactly what Kyle said above.

In D58094#1332272, @kevans wrote:

Nice, this seems reasonable to me. I have a proposal that I'm gonig to send to a list, but effectively, I'd like to do a few things in this area:

• Add a convenience pdopenpid(0, ...) -> similar to what we do with setpgrp/setpggid, special-case 0 to allow self-pdopenpid without a getpid() call
• Mark pdopenpid(2) CAPENABLED, allow it in capability mode only for self-requests
• Mark ptrace(2) CAPENABLED, only allow: PT_ATTACH_PD, PT_DETACH, PT_GETNUMLWPS, PT_GETLWPLIST, PT_GETREGS

PT_VM_TIMESTAMP, TP_VM_ENTRY, PT_GETREGSET, PT_GET_SC_ARGS, PT_GET_SC_RET, and MD options.

But I really thought that if you have the pfd with the capability, then you can debug the target as you want. pdopenpid() should be not allowed lightly for capsicum, but you could get the pfd by other means, e.g. by passing it from some fully privileged wrapper.

I think my concern is that you can easily exfiltrate or manage resources from the global namespace if you can get ahold of a pdfd for a process that isn't also in capability mode. Even without remote syscall capability directly, you can still drive arbitrary execution with proper stepping and reg swapping. We probably want to be pretty conservative in what we allow to limit the possible shenanigans.

Re: obtaining pdfd, in an lsan context we don't really have any choice- we're effectively running in a hostile environment, and we may not get the chance to execute anything privileged (because our image may infact even be exec'd that way). We allow getpid() in capability mode, though, so I didn't really see a problem with allowing pdopenpid for the process itself.

We might introduce two kinds of ptrace caps: one full, as in the patch, and one ro, with the consequences as you specified. But I am somewhat sceptical about this ro ptrace mode, even if we do not introduce the full mode at all.
First, you can read all the memory of the target (it is impractical not to allow the memory reads), and second, the target is reparented to the debugger.

In D58094#1332300, @kevans wrote:

In D58094#1332294, @kib wrote:

In D58094#1332276, @markj wrote:

Why not just use ptrace(PT_ATTACH, pdgetpid(), ...)? I can see some argument for PT_ATTACH_PD if you are planning to make ptrace() available in capability mode.

Exactly what Kyle said above.

In D58094#1332272, @kevans wrote:

Nice, this seems reasonable to me. I have a proposal that I'm gonig to send to a list, but effectively, I'd like to do a few things in this area:

• Add a convenience pdopenpid(0, ...) -> similar to what we do with setpgrp/setpggid, special-case 0 to allow self-pdopenpid without a getpid() call
• Mark pdopenpid(2) CAPENABLED, allow it in capability mode only for self-requests
• Mark ptrace(2) CAPENABLED, only allow: PT_ATTACH_PD, PT_DETACH, PT_GETNUMLWPS, PT_GETLWPLIST, PT_GETREGS

PT_VM_TIMESTAMP, TP_VM_ENTRY, PT_GETREGSET, PT_GET_SC_ARGS, PT_GET_SC_RET, and MD options.

But I really thought that if you have the pfd with the capability, then you can debug the target as you want. pdopenpid() should be not allowed lightly for capsicum, but you could get the pfd by other means, e.g. by passing it from some fully privileged wrapper.

I think my concern is that you can easily exfiltrate or manage resources from the global namespace if you can get ahold of a pdfd for a process that isn't also in capability mode. Even without remote syscall capability directly, you can still drive arbitrary execution with proper stepping and reg swapping. We probably want to be pretty conservative in what we allow to limit the possible shenanigans.

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.

Re: obtaining pdfd, in an lsan context we don't really have any choice- we're effectively running in a hostile environment, and we may not get the chance to execute anything privileged (because our image may infact even be exec'd that way). We allow getpid() in capability mode, though, so I didn't really see a problem with allowing pdopenpid for the process itself.

Sorry, what does LSAN have to do with capability mode?

In D58094#1332492, @markj wrote:

In D58094#1332300, @kevans wrote:

In D58094#1332294, @kib wrote:

In D58094#1332276, @markj wrote:

Why not just use ptrace(PT_ATTACH, pdgetpid(), ...)? I can see some argument for PT_ATTACH_PD if you are planning to make ptrace() available in capability mode.

Exactly what Kyle said above.

In D58094#1332272, @kevans wrote:

Nice, this seems reasonable to me. I have a proposal that I'm gonig to send to a list, but effectively, I'd like to do a few things in this area:

• Add a convenience pdopenpid(0, ...) -> similar to what we do with setpgrp/setpggid, special-case 0 to allow self-pdopenpid without a getpid() call
• Mark pdopenpid(2) CAPENABLED, allow it in capability mode only for self-requests
• Mark ptrace(2) CAPENABLED, only allow: PT_ATTACH_PD, PT_DETACH, PT_GETNUMLWPS, PT_GETLWPLIST, PT_GETREGS

PT_VM_TIMESTAMP, TP_VM_ENTRY, PT_GETREGSET, PT_GET_SC_ARGS, PT_GET_SC_RET, and MD options.

But I really thought that if you have the pfd with the capability, then you can debug the target as you want. pdopenpid() should be not allowed lightly for capsicum, but you could get the pfd by other means, e.g. by passing it from some fully privileged wrapper.

I think my concern is that you can easily exfiltrate or manage resources from the global namespace if you can get ahold of a pdfd for a process that isn't also in capability mode. Even without remote syscall capability directly, you can still drive arbitrary execution with proper stepping and reg swapping. We probably want to be pretty conservative in what we allow to limit the possible shenanigans.

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.

Fair point.

Re: obtaining pdfd, in an lsan context we don't really have any choice- we're effectively running in a hostile environment, and we may not get the chance to execute anything privileged (because our image may infact even be exec'd that way). We allow getpid() in capability mode, though, so I didn't really see a problem with allowing pdopenpid for the process itself.

Sorry, what does LSAN have to do with capability mode?

There is no relation, except that LSAN cannot work if a program is in capability mode because it needs to momentarily freeze the program and collect current state about it to detect leaks. Right now, only ptrace(2) has the right facilities for this (it uses just the set that I enumerated above). Given your other point about even the target being in capability mode may be unsafe, it may be the case that we need something like PT_TRACE_ME to bless the relationship or declare it safe or something? I don't think you necessarily want to set the precedence that a child is able to trace its parent in the normal case, since the parent may have dropped resources after forking.

pdopenpid() should be not allowed lightly for capsicum

The pdopenpid's pid arg is accessing a global namespace and thus not permitted in Capsicum (other than potentially 0 for self), but as you note there are several ways to obtain the pd.

In D58094#1332531, @kevans wrote:

In D58094#1332492, @markj wrote:

In D58094#1332300, @kevans wrote:

Re: obtaining pdfd, in an lsan context we don't really have any choice- we're effectively running in a hostile environment, and we may not get the chance to execute anything privileged (because our image may infact even be exec'd that way). We allow getpid() in capability mode, though, so I didn't really see a problem with allowing pdopenpid for the process itself.

Sorry, what does LSAN have to do with capability mode?

There is no relation, except that LSAN cannot work if a program is in capability mode because it needs to momentarily freeze the program and collect current state about it to detect leaks. Right now, only ptrace(2) has the right facilities for this (it uses just the set that I enumerated above). Given your other point about even the target being in capability mode may be unsafe, it may be the case that we need something like PT_TRACE_ME to bless the relationship or declare it safe or something? I don't think you necessarily want to set the precedence that a child is able to trace its parent in the normal case, since the parent may have dropped resources after forking.

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.

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.

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.

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...

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.

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?

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.

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.