Make security.bsd.unprivileged_proc_debug per-jail

[jamie]

Summary

In hardened systems, where the security.bsd.unprivileged_proc_debug sysctl node is set, allow setting security.bsd.unprivileged_proc_debug per-jail. In part, this is needed to create jails in which the Address Sanitizer (ASAN) fully works as ASAN utilizes libkvm to inspect the virtual address space. Instead of having to allow unprivileged process debugging for the entire system, allow setting it on a per-jail basis.

The sysctl node is still security.bsd.unprivileged_proc_debug and the jail(8) param is allow.unprivileged_proc_debug. The sysctl code is now a sysctl proc rather than a sysctl int. This allows us to determine setting the flag for the corresponding jail (or prison0).

As part of the change, the dynamic allow.* API needed to be modified to take into account pr_allow flags which may now be disabled in prison0. This prevents conflicts with new pr_allow flags (like that of vmm(4)) that are added (and removed) dynamically.

Also teach the jail creation KPI to allow differences for certain pr_allow flags between the parent and child jail. This can happen when unprivileged process debugging is disabled in the parent prison, but enabled in the child.

Sponsored-by: HardenedBSD and G2, Inc
Obtained-from: HardenedBSD (45b3625edba0f73b3e3890b1ec3d0d1e95fd47e1, deba0b5078cef0faae43cbdafed3035b16587afc, ab21eeb3b4c72f2500987c96ff603ccf3b6e7de8)
Relnotes: yes

[jamie]

Since this bit is under the full control of the prison itself, does it belong in pr_allow? On the plus side, that lets the system create a jail with this turned on, but that can be just as easily done in the jail's sysctl.conf. It's something of a departure from the idea of this being something the jail is or isn't allowed to do. If you forgo the ability to set it as a jail parameter, then the bit can go into pr_flags and you won't have to bother noting which PR_ALLOW bits are allowed to be set .

It may make sense to have a two bits per jail: one in pr_flags for whether the prison has (via sysctl) elected to allow unprivileged debug, and another in pr_allow for whether the jail's creator has (via jail_set) given the jail permission to make such an election.

sys/kern/kern_prot.c

1643

It would be a little neater to replace with "val = prison_allow(req->td->td_ucred, PR_ALLOW_UNPRIV_DEBUG) != 0" (though see my comment above)

1690–1691

Since you're calling priv_check anyway, this test may be better placed in the "case PRIV_DEBUG_UNPRIV" part of prison_priv_check().

[lattera-gmail.com]

In D18319#388761, @jamie wrote:

Since this bit is under the full control of the prison itself, does it belong in pr_allow? On the plus side, that lets the system create a jail with this turned on, but that can be just as easily done in the jail's sysctl.conf.

The reason being is that by default, ASAN will crash the application if it can't inspect its virtual address space. So running sysctl security.bsd.unprivileged_proc_debug=1 inside the jail won't work. The reason for using an allow.* flag is to allow setting the flag prior to any application being run inside the jail, thus allowing ASAN to work.

It's something of a departure from the idea of this being something the jail is or isn't allowed to do. If you forgo the ability to set it as a jail parameter, then the bit can go into pr_flags and you won't have to bother noting which PR_ALLOW bits are allowed to be set .

I could set the CTLFLAG_SECURE on the sysctl node, such that when securelevel >= 1, the jail cannot modify it.

It may make sense to have a two bits per jail: one in pr_flags for whether the prison has (via sysctl) elected to allow unprivileged debug, and another in pr_allow for whether the jail's creator has (via jail_set) given the jail permission to make such an election.

I'm not sure I'd go that route. The design I took here keeps the simplicity of the jail mechanism. I think setting securelevel for the jail (with CTLFLAG_SECURE on the sysctl node) would be the better way to control it.

[lattera-gmail.com]

sys/kern/kern_prot.c

1643

Good call.

1690–1691

I'll take a look. Thanks for the suggestion!

[jamie]

OK, if the jail needs to have that bit set before anything is run, then yes it needs to be a parameter.

As far as its semantic fit with allow.*, I wonder if there's even a case for the jailed root changing this permission. What are the ramifications of leaving CTLFLAG_PRISON off of security.bsd.unprivileged_proc_debug?

[lattera-gmail.com]

In D18319#388774, @jamie wrote:

OK, if the jail needs to have that bit set before anything is run, then yes it needs to be a parameter.

As far as its semantic fit with allow.*, I wonder if there's even a case for the jailed root changing this permission. What are the ramifications of leaving CTLFLAG_PRISON off of security.bsd.unprivileged_proc_debug?

Then the jail must simply obey the existing state of unprivileged process debugging. We could go that route, but I wanted to make it flexible. I think setting CTLFLAG_SECURE is a good compromise between flexibility and security.

[jamie]

In D18319#388775, @lattera-gmail.com wrote:

Then the jail must simply obey the existing state of unprivileged process debugging. We could go that route, but I wanted to make it flexible. I think setting CTLFLAG_SECURE is a good compromise between flexibility and security.

Yes, that sounds reasonable.

[lattera-gmail.com]

Implement Jamie Gritton's suggestions. Use the priv_check API for checking the underlying debug privilege. Make brief that which is verbose.

[jamie]

priv_check_cred() in kern_priv.c isn't the right place to make the check, but prison_priv_check() in kern_jail.c. PRIV_DEBUG_UNPRIV is already in that function's list, in the part that lets jails do things, and it needs to be moved to the bottom part of the function where you'll see a number of other cases where a certain privilege checks a certain pr_allow bit.

[lattera-gmail.com]

In D18319#388815, @jamie wrote:

priv_check_cred() in kern_priv.c isn't the right place to make the check, but prison_priv_check() in kern_jail.c. PRIV_DEBUG_UNPRIV is already in that function's list, in the part that lets jails do things, and it needs to be moved to the bottom part of the function where you'll see a number of other cases where a certain privilege checks a certain pr_allow bit.

Whoops! Good catch! I'll fix that soon.

[lattera-gmail.com]

In D18319#388815, @jamie wrote:

priv_check_cred() in kern_priv.c isn't the right place to make the check, but prison_priv_check() in kern_jail.c. PRIV_DEBUG_UNPRIV is already in that function's list, in the part that lets jails do things, and it needs to be moved to the bottom part of the function where you'll see a number of other cases where a certain privilege checks a certain pr_allow bit.

It appears it needs to be in both places, since the very top of prison_priv_check has this:

if (!jailed(ucred))
    return (0);

[lattera-gmail.com]

In D18319#388857, @lattera-gmail.com wrote:

In D18319#388815, @jamie wrote:

priv_check_cred() in kern_priv.c isn't the right place to make the check, but prison_priv_check() in kern_jail.c. PRIV_DEBUG_UNPRIV is already in that function's list, in the part that lets jails do things, and it needs to be moved to the bottom part of the function where you'll see a number of other cases where a certain privilege checks a certain pr_allow bit.

It appears it needs to be in both places, since the very top of prison_priv_check has this:

if (!jailed(ucred))
    return (0);

Because of this, having the check in sys/kern/kern_priv.c is the right place. There's no real need to duplicate the logic to prison_priv_check. I can still add it, if you want, but I believe it would be a waste of cycles.

[jamie]

In D18319#388858, @lattera-gmail.com wrote:

Because of this, having the check in sys/kern/kern_priv.c is the right place. There's no real need to duplicate the logic to prison_priv_check. I can still add it, if you want, but I believe it would be a waste of cycles.

Good point. No need to do it twice, since prison_priv_check() only exists to be called by priv_check_cred().

[lattera-gmail.com]

Update jail.8 manage to document allow.unprivileged_proc_debug

[lattera-gmail.com]

In D18319#389013, @jamie wrote:

In D18319#388858, @lattera-gmail.com wrote:

Because of this, having the check in sys/kern/kern_priv.c is the right place. There's no real need to duplicate the logic to prison_priv_check. I can still add it, if you want, but I believe it would be a waste of cycles.

Good point. No need to do it twice, since prison_priv_check() only exists to be called by priv_check_cred().

Awesome. I believe I've addressed all concerns.

[bcr]

Approved by manpages, too.

[lattera-gmail.com]

Note that I'm not a committer, so I can't commit the patch. ;)

[jamie]

Sorry for a post-acceptance note, but on trying it out I noticed that jails are created by default with allow.nounprivileged_proc_debug. That's an easy fix - the bit needs to be added to PW_DEFAULT_ALLOW in kern_jail.h. I'm apparently unable to change the diff in this revision, so instead of creating a new revision I'll just mention that's what I'll be committing.

With that change, things work as expected: a new jail has user-level debugging if its parent does, and doesn't if its parent doesn't. Once the jail is created, the sysctl will change the behavior only inside that jail.

[lattera-gmail.com]

In D18319#389678, @jamie wrote:

Sorry for a post-acceptance note, but on trying it out I noticed that jails are created by default with allow.nounprivileged_proc_debug. That's an easy fix - the bit needs to be added to PW_DEFAULT_ALLOW in kern_jail.h. I'm apparently unable to change the diff in this revision, so instead of creating a new revision I'll just mention that's what I'll be committing.

With that change, things work as expected: a new jail has user-level debugging if its parent does, and doesn't if its parent doesn't. Once the jail is created, the sysctl will change the behavior only inside that jail.

Good catch! I don't know why you can't update this patch, as it's editable by all users. I can update it if you'd like.

[emaste]

In D18319#389890, @lattera-gmail.com wrote:

I don't know why you can't update this patch, as it's editable by all users.

I believe only the author of the patch can upload a new patch, but @jamie can comandeer the review to take over authorship and then upload a new version.

[jamie]

Huh - so I can. I didn't know of (or even suspect) such a possibility.

[jamie]

New jails are now created with the unprivileged_proc_debug bit inherited from the parent unless otherwise specified.

[lattera-gmail.com]

LGTM