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

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

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.

In D27128#606151, @jamie wrote:

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.

In D27128#606196, @me_igalic.co wrote:

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.

In D27128#606198, @oshogbo wrote:

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.

In D27128#606252, @jamie wrote:

In D27128#606198, @oshogbo wrote:

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?

In D27128#606254, @oshogbo wrote:

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.

me_igalic.co I will created for this separated review.