Page MenuHomeFreeBSD

jail: introduce per jail suser_enabled setting
ClosedPublic

Authored by oshogbo on Nov 6 2020, 6:23 PM.

Details

Summary

The suser_enable sysctl allows to remove a privliged rights from uid 0.
This change introduce per jail setting which allow to make root a
normal user.

Diff Detail

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

Event Timeline

oshogbo created this revision.
oshogbo retitled this revision from jail2: introduce per jail suser_enabled setting to jail: introduce per jail suser_enabled setting.Nov 7 2020, 1:08 AM

This looks good to me; I'm kind of wondering if this shouldn't also have some effect on setuid/setgid binaries (i.e. don't honor it). As it stands, you can strip root of suser in the jail but they can still setuid /bin/sh and "help" a user on the host escalate that way.

(edit: sorry, just realized this can't possibly work... meh.)

usr.sbin/jail/jail.8
420 ↗(On Diff #79277)

"automatically", and it might be good to add in a little bit of discussion under "Configuring the Jail" below to describe the kind of setups that can use this since, e.g., the default jail(8) setup of running /etc/rc may not work so well if suser is disabled upon creation.

Please upload with context next time. Haven't reviewed in detail yet but I think this is a reasonable thing to do.

I would prefer allow.suser instead of suser_enabled. It's the logical place for such flags, and you can take advantage of existing code that manages disallowing adding a permission the parent lacks, and passing the restriction on to child jails. Then much of this diff can be collapsed into adding to pr_flag_allow, JAIL_DEFAULT_ALLOW, and SYSCTL_JAIL_PARAM(_allow, ...).

usr.sbin/jail/jailp.h
138 ↗(On Diff #79277)

This isn't necessary. The KP_* flags
are only useful for parameters that are referred to in special ways
(such as KP_SECURELEVEL which is set by the "-s" flag).

I may be handy to allow jailed root to control its own security.bsd.suser_enabled.

1: It could either work one-way, with the ability to set clear one's own permission bit but not set it (except in the case of prison0).
2: More flexible but complicated, there could be separate suser_enabled and allow.suser bits, though that's only useful in the probably-rare case of wanting to regain suser ability.

I like option 2 better: easier to code, easier for admins to understand, less likely to cause a problem.

I may be handy to allow jailed root to control its own security.bsd.suser_enabled.

1: It could either work one-way, with the ability to set clear one's own permission bit but not set it (except in the case of prison0).
2: More flexible but complicated, there could be separate suser_enabled and allow.suser bits, though that's only useful in the probably-rare case of wanting to regain suser ability.

I like option 2 better: easier to code, easier for admins to understand, less likely to cause a problem.

can you elaborate on what that rare case of wanting to regain suser ability?

@kevans Thank you for the review. :) I fixed the typo.
@emaste Sorry, fixed.
@jamie
If I understand correctly - the allow.* and the suser has a reverted values. You can disable suser, which by default is enabled. I wanted to made it exactly the same as sysctl on the hosts system, but I don't have strong opinion here.
I'm not sure if I understand. Do you suggest to have allow.suser which allow you to change the suser sysctl?
There should be no possibility to get back the suser priviliged inside the jail.
In the scenario I tested you can give/retrieve the suser from the host.

can you elaborate on what that rare case of wanting to regain suser ability?

I can't really - by "rare" I meant "I can't think of a reason offhand." All the more reason to discount my #2 option.

If I understand correctly - the allow.* and the suser has a reverted values. You can disable suser, which by default is enabled. I wanted to made it exactly the same as sysctl on the hosts system, but I don't have strong opinion here.
I'm not sure if I understand. Do you suggest to have allow.suser which allow you to change the suser sysctl?
There should be no possibility to get back the suser priviliged inside the jail.
In the scenario I tested you can give/retrieve the suser from the host.

The semantics of an allow.* bit (at least of one included in JAIL_DEFAULT_ALLOW) are exactly the same as you used in the suser_enabled parameter: enabled by default when a top-level jail is created, set to the parent jail's value by default when child jail is created, and cleared in all child jails whenever it's cleared in an existing parent jail. OK, not exactly - your loop to set the child jails will re-add the bit to children if re-added to a parent jail, which is against the general rule of restrictions cascading while easing those restrictions do not.

When I was talking about getting the suser privilege back, it was in the context of having separate parallel host-imposed and jail-controlled bits, both of which must be enabled to work. But that's clearly a dead end - especially now that I consider that the sysctl to revert security.bsd.suser_enabled is unavailable to a suser-disabled system.

On the subject of a jail being able to clear (and only clear) its own suser_enabled bit via sysctl, I think the static suser_enabled variable in kern_priv.c is redundant. You already have something in prison0 which does the same job. Removing the redundant variable would add a touch of complexity, in that the sysctl would need code to change the child jails. But I think that's cleaner than having a similar (but not quite the same) value in two different places.

This is similar to how securelevel became prison0.pr_securelevel, and how sysctl_kern_securelvl() changes that for child jails as needed.

If I understand correctly - the allow.* and the suser has a reverted values. You can disable suser, which by default is enabled. I wanted to made it exactly the same as sysctl on the hosts system, but I don't have strong opinion here.
I'm not sure if I understand. Do you suggest to have allow.suser which allow you to change the suser sysctl?
There should be no possibility to get back the suser priviliged inside the jail.
In the scenario I tested you can give/retrieve the suser from the host.

The semantics of an allow.* bit (at least of one included in JAIL_DEFAULT_ALLOW) are exactly the same as you used in the suser_enabled parameter: enabled by default when a top-level jail is created, set to the parent jail's value by default when child jail is created, and cleared in all child jails whenever it's cleared in an existing parent jail. OK, not exactly - your loop to set the child jails will re-add the bit to children if re-added to a parent jail, which is against the general rule of restrictions cascading while easing those restrictions do not.

When I was talking about getting the suser privilege back, it was in the context of having separate parallel host-imposed and jail-controlled bits, both of which must be enabled to work. But that's clearly a dead end - especially now that I consider that the sysctl to revert security.bsd.suser_enabled is unavailable to a suser-disabled system.

Sorry, I'm really hope I'm not annoying. Correct me if I'm wrong, I'm very new to this code.
The allow.* is something that we enable in jail. For example we are enabling raw sockets.
In case of suser (at least in my code) the suser by default is enabled (set to 1), and we would like to have option to be explicite disable.
I think this is additional protection that user may want but it can really limit usability of the jails (a lot of things doesn't work like setuid, chroot, initgroups etc.).
So at least for me the allow.* flags suggest that we are giving additional permission but actually we would like to disallow something.
This is why I was looking more into the 'securelevel' then 'allow.*' flags.

Or are you suggesting that the suser by default was disabled, and we enable it per jail? But this won't be something to annoying?

Sorry, I'm really hope I'm not enjoying. Correct me if I'm wrong, I'm very new to this code.
The allow.* is something that we enable in jail. For example we are enabling raw sockets.
In case of suser (at least in my code) the suser by default is enabled (set to 1), and we would like to have option to be explicite disable.
I think this is additional protection that user may want but it can really limit usability of the jails (a lot of things doesn't work like setuid, chroot, initgroups etc.).
So at least for me the allow.* flags suggest that we are giving additional permission but actually we would like to disallow something.
This is why I was looking more into the 'securelevel' then 'allow.*' flags.

Or are you suggesting that the suser by default was disabled, and we enable it per jail? But this won't be something to annoying?

The semantics of most allow.* parameters are as you describe. But the semantics of allow.parameters that are included in JAIL_DEFAULT_ALLOW the same as what you're doing with suser_enabled.

Consider allow.set_hostname. PR_ALLOW_SET_HOSTNAME is part of JAIL_DEFAULT_ALLOW. By default, jails may set their own hostnames. Only when the the jail is explicitly created with allow.noset_hostname, or when that is set in an existing jail, do they lose that ability. In the same way, by adding PR_ALLOW_SUSER to JAIL_DEFAULT_ALLOW, jails would be created by default with their current ability to use suser permissions. Only by explicitly setting allow.nosuser would that permission be taken away.

Gotcha, thank you I will refactor the code.

BTW I used allow.suser in my example, but of course it could as easily be allow.suser_enabled. To me the "allow" and "enabled" sound redundant together, but allow.suser_enabled still might be a better name because it's the same as security.bsd.suser_enabled. So either choice is fine.

sys/kern/kern_jail.c
3821 ↗(On Diff #79477)

Cut & paste nit here: Should be "Processes".

sys/kern/kern_priv.c
66 ↗(On Diff #79477)

Since almost all use of suser_enabled() is in the form:
(cred->cr_uid == 0 && suser_enabled(cred))
It might be worth putting that "cred->cr_uid == 0" test into suser_enabled() itself, and thus simplifying most of the code that uses it.

The only tricky part is then you wouldn't be able to use the function in sysctl_kern_suser_enabled().

94 ↗(On Diff #79477)

I think this check is redundant with the one sysctl has already made (via priv_check()) before you get to this point.

103 ↗(On Diff #79477)

Child jails should only ever have their security implicitly made more strict, never less. It's possible for a system that's already suser-enabled to "sysctl -w suser_enabled=1", which in this case would erase any suser restrictions that may have been set in any jails.

And then if you're not changing any child jails in the enabled=true case, and you're not changing the base system (or current jail) because you're not even allowed to run the sysctl unless it's currently suser_enabled, then there's nothing at all that needs to be done for enabled=true.

oshogbo added inline comments.
sys/kern/kern_jail.c
3821 ↗(On Diff #79477)

Fixed.

sys/kern/kern_priv.c
66 ↗(On Diff #79477)

Yea that true, but for know I would live as it is.

94 ↗(On Diff #79477)

fixed.

103 ↗(On Diff #79477)

Thanks, fixed.

usr.sbin/jail/jail.8
594 ↗(On Diff #79477)

s/suser/super-user/, ditto below.

sys/kern/kern_jail.c
3821 ↗(On Diff #79506)

This needs to be "B" and not "I", or else the allow.nosuser parameter doesn't exist.

oshogbo edited the summary of this revision. (Show Details)

Changes after markj@ and jamie@ review.

It looks to be working correctly on a quick run-through.

This revision is now accepted and ready to land.Nov 13 2020, 11:58 PM

It looks to be working correctly on a quick run-through.

i'd still like to see an example in the man page, like @kevans suggests

This revision now requires changes to proceed.Nov 14 2020, 7:09 AM

me_igalic.co I will created for this separated review.

This revision was not accepted when it landed; it landed in state Needs Revision.Nov 18 2020, 9:07 PM
This revision was automatically updated to reflect the committed changes.