Whoops, thanks!

In D51865#1334998, @aokblast wrote:

In D51865#1334521, @kevans wrote:

In D51865#1334517, @adrian wrote:

hi!

So why are a whole lot of functions getting an fd added to them?

Do the FDs change during the runtime of the tool? Or are they something that could be set
once during some init phase (maybe in a new function which would call cap_enter, rather than
the owner having to call it) and then pulled out of some the libusb backend state?

IMO we really shouldn't have libraries entering capablity mode- libusb may be just one of many, and setting a precedence like that where some will insist on entering the sandbox is bound to end up with some really annoying circumstances.

I can somehow agree with you. But how can we integrate to bhyve without this as bhyve is capsicumized by default? We can turn off temporary but it adds complexity on bhyve.

Whack the remaining moles, fix OptionalObsoleteFiles to also remove the example

In D51865#1334517, @adrian wrote:

hi!

So why are a whole lot of functions getting an fd added to them?

Do the FDs change during the runtime of the tool? Or are they something that could be set
once during some init phase (maybe in a new function which would call cap_enter, rather than
the owner having to call it) and then pulled out of some the libusb backend state?

In D58152#1333589, @aokblast wrote:

In D58152#1333581, @kevans wrote:

Yes, I think @kib was at least mildly concerned when I raised the thought of doing the same, and we decided it was reasonably fine to just bypass the locks because we do dl_iterate_phdr, rfork (vfork) inside the handler, then ptrace the parent. The thread that holds the relevanr locks is suspended in rfork and safely not racing against what we do in the child.

In D58152#1333577, @aokblast wrote:

In D58152#1333568, @kevans wrote:

In D58152#1333559, @kib wrote:

You do understand, that the change is only changes something for the statically linked binaries?

We had already implemented a workaround in rtld, _dl_iterate_phdr_locked iirc. I'm not sure offhand why that wasn't sufficient here, I might need to re-check which lsan drop I sent to @aokblast

Yes, I see the comment from your brach and see both glibc and NetBSD (although very strange in their implemetation) has recursive dl_phdr_info_lock lock.

In D58152#1333559, @kib wrote:

You do understand, that the change is only changes something for the statically linked binaries?

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?

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

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.

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.

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.

In D58063#1332290, @sjg wrote:

I'll remove the |= bit of this patch and related commentary in the commit message. I won't change the kern_exec.c part of the patch, but I do want to think about whether it would be useful for imgp->interpreted to reflect all of the interpreters involved in the chain -- it could be that it's useful to set IMGACT_INTERP_ELF in imgp->interpreted back where __elfN(load_interp) is first called in the elf activator. I'm not sure I see how at the moment, though:

My understanding may be incorrect, but if there is an interpreter chain, I would think kern_exec is only dealing with them one after another no
so not sure |= is helping.

In D58063#1331879, @kevans wrote:

In D58063#1331523, @sjg wrote:

There are some diffs b/w imgact_elf.c in main and our tree (we're a bit behind), but wrt above it looks the same, and it works fine.

Interesting, OK- I don't see how imgp->interpreted could possibly preserve the bit in question until it gets to a MAC check, but I think I'll punt on that and just leave this patch as-is for now. In the short-to-medium term, my plan is to rewrite the imgact_binmisc configuration interface to allow rules scoped to jails; at that point, I'll write some tests for how all of these things work together and maybe tweak stuff further.

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:

In D58063#1331523, @sjg wrote:

In D58063#1331520, @kevans wrote:

In D58063#1331519, @sjg wrote:

In D58063#1331516, @kevans wrote:

Hmm, the last bit seems to be wrong because elf's imgact will set interpreted = 0 anyways. Actually, that seems to happen *after* we set it in load_interp_file -- was there something missing from the original patch?

I don't think so. The original problem was that when exec'ing ld-elf.so the 'interpreted' field was not set so if we set veriexec's indirect flag on ld-elf.so, mac_veriexec thinks it is a direct exec and blocks it - which is sub-optimal. Simply having imgp->interpreted != 0 is sufficient to address that.

Right, but as far as I can tell it won't work right in vanilla FreeBSD. We set imgp->interpreted = 0 here:

https://cgit.freebsd.org/src/tree/sys/kern/imgact_elf.c#n1461

After you would have set it non-zero up in load_interp -- I think clearing it needs to move above the interp != NULL branch, or the |= here changes back to = and we just clear it in the else block (interp == NULL).

There are some diffs b/w imgact_elf.c in main and our tree (we're a bit behind), but wrt above it looks the same, and it works fine.

Adding jail folks; this is an extension of ttps://github.com/freebsd/freebsd-src/pull/2108. At least one notable change since @jamie looked last is that we negotiated dropping the rights(4) at creation time, so use of the descriptor can check just the rights(4) rather than both rights + jdcred/fp->f_cred.

In D58063#1331519, @sjg wrote:

In D58063#1331516, @kevans wrote:

Hmm, the last bit seems to be wrong because elf's imgact will set interpreted = 0 anyways. Actually, that seems to happen *after* we set it in load_interp_file -- was there something missing from the original patch?

I don't think so. The original problem was that when exec'ing ld-elf.so the 'interpreted' field was not set so if we set veriexec's indirect flag on ld-elf.so, mac_veriexec thinks it is a direct exec and blocks it - which is sub-optimal. Simply having imgp->interpreted != 0 is sufficient to address that.

Hmm, the last bit seems to be wrong because elf's imgact will set interpreted = 0 anyways. Actually, that seems to happen *after* we set it in load_interp_file -- was there something missing from the original patch?

Unrelated, I'm wondering if there's any interest in slightly refactoring this into a prometheus_fbsd_exporter (for lack of better name). Move the bits specific to sysctl output into a separate file and retain a prometheus_sysctl_exporter link to it, but it seems like there's a good opportunity to collect more stats and re-use the same basic CLI interface.

Increase NUM_DSO to a number that reliably hits the guard page without ASAN.

With the guard page landed, I'm going to rework this a little and likely try posix_spawnp at each fdlopen (or every few) up until 256 to exercise both the old regression and the new transition point from alloca -> heap after your fix.

In D57955#1328646, @kib wrote:

The cost is +3 syscalls per spawn, but so be it.

Given that it's still not even at absurdly large numbers, I wonder if it's coincidentally just wiping out the mapped contents of these DSOs. Maybe I should re-design this to then call into every single one of them after the posix_spawnp.

In D57954#1328625, @kib wrote:

In D57954#1328620, @kevans wrote:

In D57954#1328617, @kib wrote:

If NUM_DSO is large enough, I believe it should trips without any sanitizer.

Right, it will still underflow, but the hard part is detecting that so that you can trip an assertion. I think we'd probably want a custom allocator to be able to influence where the rfork_thread stack is laid out, so that we can guarantee where the damage will occur and spot it. Otherwise, I think we have to spray a bunch of patterned buffers onto the heap and posix_spawn a bunch, then verify that all of our buffers are still intact?

If the test program segfaults, wouldn't kyua declare the test failing?
Or you could fork, detect the child exit due to a signal, and declare test failing.

Downgrade our shlib to RDONLY and MAP_SHARED, now that we're not trying to do
a unique SOVERSION for every fdlopen().

In D57954#1328617, @kib wrote:

If NUM_DSO is large enough, I believe it should trips without any sanitizer.

In D57755#1328574, @jrtc27 wrote:

Implementation-wise it seems reasonable, but every time bsdinstall hardening gains an option the security world cries out in pain over having sensible defaults and good UX :')

Perfect, thanks. I can confirm that this still fixes my test-case, I'll get that cleaned up and posted to Phabricator ~soon-ish.

Ping- this is relatively simple, so I may just commit it later this week, unless someone has time to give it a once-over.

Seems fine overall, just two nits

This version seems to have dropped the manpage revisions entirely

In D57908#1327731, @kib wrote:

In D57908#1327700, @zarychtam_plan-b.pwste.edu.pl wrote:

I am commenting here instead of PR for consistency. The most recent version of the patch doesn't introduce any breakage, but also doesn't solve bug 289425.

Try reducing DLP_ALLOCA_LIMIT twice until it helps.

I haven't had a chance to thoroughly review this yet, but I think it's a good idea regardless of the question about vfork.

Perfect, thanks!

Additional .Nm

error back to nerror

From TCG PC Client Specific TPM Interface
Specification (TIS) version 1.3, 5.6.12 "Status Register":

Thanks!

Perfect, thank you! I had forgotten about this, but I've also switched to amdgpu since the last time I was active on this and I also haven't run it without this applied. I'll redo my previous dtrace'ing here to see if this patch actually has any impact on amdgpu's usage at all -- if it doesn't hit either of the two busy- probes in this patch, then the patch is a net-0 for amdgpu and you'd have the same experience without it.

In D56308#1323543, @2khramtsov_gmail.com wrote:

FWIW: I cherry picked this into local src git branch for use with -CURRENT and drm-kmod 6.12-lts branch sometime in May. 1002:73bf (RDNA2 amdgpu) works stable (~1-2 week uptime, then I usually reboot for newer src) with this, I haven't seen a regression from this. I usually torture this machine with all-core CPU stress with poudriere while doing desktop tasks as well.

In D57697#1322962, @freebsd_igalic.co wrote:

what about bsdinstall's security screen?

In D57697#1322952, @markj wrote:

What sensitive things are being exposed through kenv? Is there some subsystem which should be scrubbing its kenv entries during boot but isn't?

(I have no objection to the change, just wondering.)