Page MenuHomeFreeBSD

Make security.bsd.unprivileged_proc_debug per-jail
ClosedPublic

Authored by jamie on Nov 23 2018, 11:20 PM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

lattera-gmail.com edited the summary of this revision. (Show Details)

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 ↗(On Diff #51029)

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)

1692 ↗(On Diff #51029)

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

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 marked 2 inline comments as done.Nov 24 2018, 12:00 AM
lattera-gmail.com added inline comments.
sys/kern/kern_prot.c
1643 ↗(On Diff #51029)

Good call.

1692 ↗(On Diff #51029)

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

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?

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.

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 edited the summary of this revision. (Show Details)

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

lattera-gmail.com marked 2 inline comments as done.Nov 24 2018, 1:45 AM
jamie added a comment.Nov 24 2018, 4:56 AM

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.

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.

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);

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 added a comment.Nov 24 2018, 9:36 PM

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 edited the summary of this revision. (Show Details)

Update jail.8 manage to document allow.unprivileged_proc_debug

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.

jamie accepted this revision.Nov 24 2018, 11:50 PM
This revision is now accepted and ready to land.Nov 24 2018, 11:50 PM
bcr accepted this revision.Nov 25 2018, 11:08 AM
bcr added a subscriber: bcr.

Approved by manpages, too.

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

emaste added a subscriber: emaste.Nov 26 2018, 3:07 PM
jamie added a comment.Nov 27 2018, 3:50 AM

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.

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.

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 commandeered this revision.Nov 27 2018, 5:18 PM
jamie edited reviewers, added: lattera-gmail.com; removed: jamie.

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

jamie updated this revision to Diff 51222.Nov 27 2018, 5:20 PM

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

This revision now requires review to proceed.Nov 27 2018, 5:20 PM
This revision is now accepted and ready to land.Nov 27 2018, 5:39 PM
This revision was automatically updated to reflect the committed changes.