pdptrace(2)

[kib]

Summary

capsicum: add CAP_PTRACE

The capability will allow the ptrace(2) on the procdesc.


pdfork(2): add PD_PTRACE_CAP flag

If the flag is not specified, the process descriptor returned by either
pdfork(2) or pdopenpid(2) has the CAP_PTRACE capability disabled.


pdptrace(2)


ptrace.2: document PT_PROCDESC

[kib]

This patch adds a capability for ptrace, and allows to do ptrace(req, procdesc, addr, data) instead of ptrace(req, pid, addr, data).

By default, the capability is not enabled, the caller of pdfork(2) or pdopenpid(2) must explicitly prepare process descriptor for debugging.
This way, the existing code that creates process descriptors and passes it somewhere, does not suddenly allow the recipient to debug the process, but it can allow if wanted.

[inquire_JohnEricson.me]

From a quick glance, this looks good to me. I am going to do something similar for procctl inspired by it.

[inquire_JohnEricson.me]

BTW I had to do this to get it to build on the latest main

diff --git a/sys/kern/kern_descrip.c b/sys/kern/kern_descrip.c
index 8272d6da9b0a..1d66581efb4c 100644
--- a/sys/kern/kern_descrip.c
+++ b/sys/kern/kern_descrip.c
@@ -1906,7 +1906,7 @@ filecaps_move(struct filecaps *src, struct filecaps *dst)
 /*
  * Fill the given filecaps structure with full rights.
  */
-static void
+void
 filecaps_fill(struct filecaps *fcaps)
 {

diff --git a/sys/kern/sys_process.c b/sys/kern/sys_process.c
index 98a453f1b029..c3e402ef2040 100644
--- a/sys/kern/sys_process.c
+++ b/sys/kern/sys_process.c
@@ -1120,7 +1120,7 @@ kern_ptrace(struct thread *td, bool pd_mode, int req, int pid, void *addr,
                        if (!proctree_locked)
                                sx_slock(&proctree_lock);
                        error = fget_procdesc(td, pid, &cap_ptrace_rights,
-                           &pfp, NULL, &p);
+                           EBADF, &pfp, NULL, &p);
                        if (!proctree_locked)
                                sx_sunlock(&proctree_lock);
                        if (error != 0)
diff --git a/sys/sys/filedesc.h b/sys/sys/filedesc.h
index c6499a18b884..31a3bc44d4aa 100644
--- a/sys/sys/filedesc.h
+++ b/sys/sys/filedesc.h
@@ -243,6 +243,7 @@ filecaps_init(struct filecaps *fcaps)
 bool   filecaps_copy(const struct filecaps *src, struct filecaps *dst,
            bool locked);
 void   filecaps_move(struct filecaps *src, struct filecaps *dst);
+void   filecaps_fill(struct filecaps *fcaps);
 void   filecaps_free(struct filecaps *fcaps);

 int    closef(struct file *fp, struct thread *td);

[inquire_JohnEricson.me]

Oh I suppose rights.4 needs to be updated, too?

[kib]

In D58586#1346635, @inquire_JohnEricson.me wrote:

BTW I had to do this to get it to build on the latest main

Yeah, I did not uploaded the updated diff yesterday.

In D58586#1346615, @inquire_JohnEricson.me wrote:

From a quick glance, this looks good to me. I am going to do something similar for procctl inspired by it.

Well, procctl(2) might be the target but might be in some future.
More interesting and with the immediate applications is the sysctl kern.proc. MIB tree.
See D58094 for the consumer, and the pdrfork branch at github.com/kostikbel/freebsd-src for the whole patch series.

I only hacked kern.proc.sv_name, to demonstrate the working truss(1) as consumer. But the real solution there would be something like this:

int pdkinfo(int fd, int mib, uintptr_t param, void *buf, size_t *bufsize);

with the mib parameter having the same namespace as kern.proc.VAL.

[kib]

Rebase after the fget_procdesc() signature change.
Document CAP_PTRACE in rights.4.

[inquire_JohnEricson.me]

In D58586#1346656, @kib wrote:

Well, procctl(2) might be the target but might be in some future.
More interesting and with the immediate applications is the sysctl kern.proc. MIB tree.

FWIW I started on procctl because I thought I would need it for reimplementing posix_spawn on top of my stuff, but perhaps that is my misunderstanding.

See D58094 for the consumer, and the pdrfork branch at github.com/kostikbel/freebsd-src for the whole patch series.

OK great. And in general, I do want to make sure that I am not needlessly duplicating your work. I saw you were doing a bunch of pd-related stuff earlier, but I only just now learned that posix_spawn in capability mode was also your end target. I suppose this is really more a conversation for D58688, but I would be happy to yield and just wait for your patches to land, if that helps.

In D58586#1346663, @kib wrote:

Rebase after the fget_procdesc() signature change.
Document CAP_PTRACE in rights.4.

Looks good now!

[kib]

Ping?

[markj]

In D58586#1349779, @kib wrote:

Ping?

Sorry for the delay. I will be away for the next few days, I will come back to this (and some other procdesc-related reviews) on Monday.

[markj]

sys/amd64/linux/linux_machdep.c

329 ↗

(On Diff #183541)

As usual, I really don't like the naked bool flag everywhere. Can we please add a separate kern_pdptrace() or so, so that existing callers do not need to be updated? Or, can we add P_PROCDESC to idtype_t and specify the desired object with a <id, n> tuple?

sys/kern/sys_process.c

1102–1103

This can be flatted: } else if (pd_mode) { ... } else if (pid <= PID_MAX) { ... } ...

1111

Is it correct to set tid here? That's different from the pid <= PID_MAX case. Further below we set td2 only if tid == 0, so this looks wrong.

[kib]

Remove pd_mode arg from kern_ptrace(), add kern_pdptrace().
Do not set tid to pid for pdmode.
Reindent the big if().

[inquire_JohnEricson.me]

Is this or a follow-up on one getting the corresponding man page changes?

(https://reviews.freebsd.org/D58694#inline-348427 I sorta meant as a note to self that for procctl I should end up mimicking whatever you end up doing man-page-wise.)

[inquire_JohnEricson.me]

sys/kern/sys_process.c

2045–2047

it's not morally a pid_t in this case (even if that is also int underneath the hood) right?

[kib]

In D58586#1351950, @inquire_JohnEricson.me wrote:

Is this or a follow-up on one getting the corresponding man page changes?

(https://reviews.freebsd.org/D58694#inline-348427 I sorta meant as a note to self that for procctl I should end up mimicking whatever you end up doing man-page-wise.)

I do not understand the question. There is the man pages update in the review.

[kib]

Rename kern_pdptrace() pfd arg.

[inquire_JohnEricson.me]

In D58586#1352006, @kib wrote:

I do not understand the question. There is the man pages update in the review.

Sorry, not sure how I missed that its there now!

---

Tempted to push the branching into the kern_ * wrappers, but that isn't so nice without a third kern_selftrace for symmetry, and PT_TRACE_ME is already a thing.

[markj]

sys/kern/kern_fork.c

1081

What if the call fails earlier? That is, why are we freeing the caps (really, the ioctl list) here?

sys/kern/sys_process.c

747–748

This might not be a PID now. (Same problem in compat32.)

1126

So, it's impossible to select a particular thread when specifying a procdesc. Isn't this a significant limitation? How can truss+capmode work with multi-threaded debugees?

[kib]

sys/kern/sys_process.c

1126

There is no place to put lwpid together with pfd. In the next review, I enable the pid-based ops if the target is the child (so it is either the direct child or the debugee).

In fact, there is a relatively small number of interfaces that needs that, for truss I only use it for PT_LWPINFO. The pid-based ops can be limited to the white list, but I do not see a strong reason to do that.

[kib]

Free fr_pd_fcaps if not consumed.
Distinguish between pid and pfd for audit of ptrace(2).

[markj]

sys/kern/sys_process.c

1126

So the idea is that, in capability mode:

• PT_ATTACH requires a procdesc (with CAP_PTRACE)
• all other ops are allowed so long as pid refers to a (reparented?) child of the current process

?

If so, then why not implement PT_ATTACH_PD as you originally proposed? It is kind of weird to allow procdescs to be used for all ptrace ops except those which target a specific thread.

[kib]

sys/kern/sys_process.c

1126

If the code needs pfd to attach anyway, it is much more convenient to use it systematically.

WRT the PT_LWPINFO, there is an idea that I want to pursue, but I do not want to spent too much efforts before it makes sense to do so.
My goal was to produce the architecturally sane capsicumization of the truss(1) that proves that procdesc enablement for debugging is feasible.

One more thing that is required for the capsicumized truss(1) is sysctl kern.proc.sv_name access. Right now I hacked the sysctl handler to use p_canopen(9) to grant access to the sysctl in the cap mode. IMO it is not the best approach.

The idea is to implement pdkinfo(2) syscall that would take procdesc and return information like sysctl kern.proc for the single process.

int pdkinfo(int pfd, int req, void *buf, size_t bufsz);

It makes sense to add LWPINFO there.

[markj]

sys/kern/sys_process.c

1126

If the code needs pfd to attach anyway, it is much more convenient to use it systematically.

But you can't use it systematically: any ptrace command which wants to target a specific thread, e.g., to fetch registers, can't do that. I don't think we should extend ptrace() only just enough to satisfy truss' requirements. The extension should be more general.

The idea is to implement pdkinfo(2) syscall that would take procdesc and return information like sysctl kern.proc

For a while now I've thought we should have some notion of a sysctl fd: a new syscall can open a sysctl OID and return the fd, and the fd can be used to access any sysctl below the opened OID. That would also be useful here. It would allow cheaper lookups too since we would not have to traverse the whole tree each time, if the sysctl is accessed frequently.

Or, a simpler idea: define KERN_PROC_PROCDESC, and let the caller pass a procdesc to sysctl(2) instead of a PID. Handle it in sysctl_kern_proc(), and flag all sysctls under kern.proc as CTLFLAG_CAPRD. If a capmode process tries to access kern.proc without specifying KERN_PROC_PROCDESC, return ENOTCAPABLE.

I think the idea of a pdkinfo() is also interesting, but the interface seems insufficient: there is no way to query the output length, so userspace has to guess and hope for the best. I suspect the functionality can be obtained without a new syscall, though.

[kib]

sys/kern/sys_process.c

1126

If the code needs pfd to attach anyway, it is much more convenient to use it systematically.

But you can't use it systematically: any ptrace command which wants to target a specific thread, e.g., to fetch registers, can't do that. I don't think we should extend ptrace() only just enough to satisfy truss' requirements. The extension should be more general.

It is not only about truss. I used truss as the working example, but I do not think it is fair to say that what I designed is only enough to satisfy truss porting to cap mode. I found it much more convenient to use pfd when possible, instead of continuing using the pid. Sure, consistently using pfd for ptrace would be ideal, but there is a balance between conversion and avoiding adding a single-purpose API call.

Would we get a need to extend ptrace(2) for other reasons, then we can also reformat the API to make space for pfd/additional parameters. For now I think it is useful change, more convenient that only adding PT_ATTACH_PD.

The idea is to implement pdkinfo(2) syscall that would take procdesc and return information like sysctl kern.proc

For a while now I've thought we should have some notion of a sysctl fd: a new syscall can open a sysctl OID and return the fd, and the fd can be used to access any sysctl below the opened OID. That would also be useful here. It would allow cheaper lookups too since we would not have to traverse the whole tree each time, if the sysctl is accessed frequently.

Or, a simpler idea: define KERN_PROC_PROCDESC, and let the caller pass a procdesc to sysctl(2) instead of a PID. Handle it in sysctl_kern_proc(), and flag all sysctls under kern.proc as CTLFLAG_CAPRD. If a capmode process tries to access kern.proc without specifying KERN_PROC_PROCDESC, return ENOTCAPABLE.

sysctl kern.proc should not be unconditionally enabled in cap mode IMO, it allows too wide access to the process namespace. For instance, kern.proc.procdesc would need to filter out the enumeration mibs. In general, I think that 'MIB' fds is not a safe idea. If added, it requires auditing the addition of a MIB in any hierarchy because it would suddenly provide additional privileges to any code that has an mibfd to access that hierarchy.

I think the idea of a pdkinfo() is also interesting, but the interface seems insufficient: there is no way to query the output length, so userspace has to guess and hope for the best. I suspect the functionality can be obtained without a new syscall, though.

I do not claim that this is the final form of the interface. Besides the way to measure the output buffer size which you pointed out, there is a need to provide flexible additional parameters for the cal, like lwpid or resource id, or both etc. I am mostly testing the idea of syscall that makes the current ad-hoc process information MIB tree more systematic and less hap-hazard. It would require some design, and a lot of refactoring, which is why I am discussing it before doing the code changes.

[kib]

Bite the bullet and add the pdptrace(2) syscall.
Documentation is not updated, and audit bits are missed.

But this allows me to run capsicumised truss using only procfd based pdptrace(2) calls. One exception is ptrace(PT_GET_CHILDREN, getpid()) which is not ptrace call on a debugging target.

[kib]

sys/compat/freebsd32/freebsd32_misc.c

1019

The compat32 copyinout is wrong, I will fix it later.

[markj]

sys/kern/sys_process.c

1126

It is not only about truss. I used truss as the working example, but I do not think it is fair to say that what I designed is only enough to satisfy truss porting to cap mode.

Maybe gcore should be considered as a target as well?

sysctl kern.proc should not be unconditionally enabled in cap mode IMO, it allows too wide access to the process namespace.

To be clear, I am not proposing unconditionally enabling it in capmode. sysctl_kern_proc() would have to carefully filter out subtrees when running in capability mode. Maybe something like:

mib[0] = CTL_KERN;
mib[1] = KERN_PROCDESC;
mib[2] = KERN_PROC_SV_NAME;
mib[3] = pd;

Then have a capmode-enabled sysctl_kern_procdesc() to handle children of [CTL_KERN, KERN_PROCDESC]. It would validate the procdesc, and use it to look up the target process.

I do not claim that this is the final form of the interface. Besides the way to measure the output buffer size which you pointed out, there is a need to provide flexible additional parameters for the cal, like lwpid or resource id, or both etc. I am mostly testing the idea of syscall that makes the current ad-hoc process information MIB tree more systematic and less hap-hazard.

I guess my question is, what would you change about the current MIB implementation if you didn't need to worry about backwards compatibility? That is, what makes it so ad-hoc that we should not extend it and design a new interface instead?

sysctl() is already very general, it lets you dispatch some arbitrary tuple of ints to a kernel function. As long as it's possible to encode selectors (like lwpids or resource IDs) as ints in the MIB array, then sysctl() can provide the required functionality without a new syscall. And libc or libutil can provide friendly wrappers around it.

I don't object to adding a new system call, but I also do not really understand what problem it solves.

[kib]

sys/kern/sys_process.c

1126

It is not only about truss. I used truss as the working example, but I do not think it is fair to say that what I designed is only enough to satisfy truss porting to cap mode.

Maybe gcore should be considered as a target as well?

For gcore, the full scope of kern.proc is needed. It might be easily done for '-k' mode, but I do not see it as a reasonable prize to be proud of.

Then of course there is a question what would be the attack surface we are trying to isolate the gcore code against. It works with the data formatted by kernel. It is unlike truss, where code directly parses the data read from the target process memory, which opens some interesting ways to manipulate the truss itself.

sysctl kern.proc should not be unconditionally enabled in cap mode IMO, it allows too wide access to the process namespace.

To be clear, I am not proposing unconditionally enabling it in capmode. sysctl_kern_proc() would have to carefully filter out subtrees when running in capability mode. Maybe something like:

mib[0] = CTL_KERN;
mib[1] = KERN_PROCDESC;
mib[2] = KERN_PROC_SV_NAME;
mib[3] = pd;

Then have a capmode-enabled sysctl_kern_procdesc() to handle children of [CTL_KERN, KERN_PROCDESC]. It would validate the procdesc, and use it to look up the target process.

This is exactly why I am thinking that mibfd is not that great idea, same as adding kern.procdesc. Somebody has to be very careful with adding new mibs.

And then, why doing it as sysctl at all? The purpose of sysctl was to provide configuration and state reporting. Now it becomes the escape hatch for all kind of controls that we did not managed to properly provide as the syscalls. People greatly dislike ioctl(2) but like the same pile of syscall backdoors with sysctl. I do not understand this.

I do not claim that this is the final form of the interface. Besides the way to measure the output buffer size which you pointed out, there is a need to provide flexible additional parameters for the cal, like lwpid or resource id, or both etc. I am mostly testing the idea of syscall that makes the current ad-hoc process information MIB tree more systematic and less hap-hazard.

I guess my question is, what would you change about the current MIB implementation if you didn't need to worry about backwards compatibility? That is, what makes it so ad-hoc that we should not extend it and design a new interface instead?

I would keep sysctl to its original purpose, that is system configuration. kern.proc is the greatest example of what should have not been added to sysctl, It is not much different from doing fork(2) as sysctl kern.proc.fork.

sysctl() is already very general, it lets you dispatch some arbitrary tuple of ints to a kernel function. As long as it's possible to encode selectors (like lwpids or resource IDs) as ints in the MIB array, then sysctl() can provide the required functionality without a new syscall. And libc or libutil can provide friendly wrappers around it.

Same as ioctl.

I don't object to adding a new system call, but I also do not really understand what problem it solves.

[kib]

Improve compat32 for pdptrace(2).
Rewrite documentation.

[markj]

sys/kern/sys_process.c

1126

Then of course there is a question what would be the attack surface we are trying to isolate the gcore code against.

There is not much. It is just the simplest ptrace() user I can think of. We could instead consider what's needed to sandbox gdb.

And then, why doing it as sysctl at all? The purpose of sysctl was to provide configuration and state reporting. Now it becomes the escape hatch for all kind of controls that we did not managed to properly provide as the syscalls. People greatly dislike ioctl(2) but like the same pile of syscall backdoors with sysctl. I do not understand this.

That's fair. I do think sysctl is a bit nicer than ioctl: it is possible to enumerate sysctls, whereas it is impossible to quickly answer the question, "which ioctls can be applied to <fd>?". sysctls are also arranged in a tree, while ioctls are completely flat.

I would keep sysctl to its original purpose, that is system configuration. kern.proc is the greatest example of what should have not been added to sysctl, It is not much different from doing fork(2) as sysctl kern.proc.fork.

I think I agree, but, kern.proc already exists. So I was trying to see if it possible to extend it, rather than invent something new.

IMO sysctl is okay for read-only queries. Or, to put it another way, I like the idea of a pdkinfo(), it's just that I suspect pdkinfo() will end up looking similar to sysctl().

Anyway, I am not trying to be contrarian, just trying to evaluate different directions. I agree that it's useful to provide ptrace()/procctl()/kinfo services for capsicumized programs.

[kib]

sys/kern/sys_process.c

1126

There is not much. It is just the simplest ptrace() user I can think of. We could instead consider what's needed to sandbox gdb.

gdb needs full filesystem access to be able to open and parse the modules loaded at runtime, and their symbol files. Perhaps the same is true for scripts. So gdb probably needs to keep the '/' opened.

On the other hand, gdb does not need much network access except for the communication with a remote target' gdb server, if any.

This sounds somewhat intrusive.

[kib]

kern_ptrace() must use copyinoutptrace(), because linuxolator passes userspace pointers to kern_ptrace().

[kib]

Revert previous change.
Harmonize function names.

[kib]

Upload the right patch