setusercontext(): Better error messages when priority is not set correctly

[olce]

Summary

Polish the syslog messages to contain readily useful information.

Behavior of capability 'priority' is inconsistent with what is done for
all other contexts: 'umask', 'cpumask', resource limits, etc., where an
absence of capability means to inherit the value. It is currently
preserved for compatibility, but is subject to change on a future major
release.

PR: 271749

[olce]

Commit message: Mention LOGIN_DEFPRI not used out-of-tree; Add PR number

[andrew_tao173.riddles.org.uk]

I'm not at all sure I approve of the logic of this one. If the caller asked for LOGIN_SETPRIORITY, they may well be explicitly intending that the current process priority not be inherited by the eventual user's process.

[olce]

In D40349#921585, @andrew_tao173.riddles.org.uk wrote:

I'm not at all sure I approve of the logic of this one. If the caller asked for LOGIN_SETPRIORITY, they may well be explicitly intending that the current process priority not be inherited by the eventual user's process.

The logic of LOGIN_SETPRIORITY (as well as the other flags) is to apply the system (and possibly user, but this is another story) configuration to the current process for a (new) login. There's no change here to that part.

By contrast, what this changes is that the system administrator now can forgo imposing a priority for some classes of users. This is entirely in his control, by configuring a class not to have the priority capability.

The (unchanged) default login.conf has priority for the default class (with value 0), so by default the priority is still effectively reset as before (since all other defined classes inherit from default).

[kib]

I think I lean to agree with Andrew. You are introducing a new feature by dropping previous behavior. Might be, it makes more sense to provide some new cap or a specific value for the lc cap (can it be non-numeric) to request not changing the priority at all.

lib/libutil/login_class.c

488

Indents are wrong.

[olce]

Fix indent.

[olce]

In D40349#921706, @kib wrote:

I think I lean to agree with Andrew. You are introducing a new feature by dropping previous behavior. Might be, it makes more sense to provide some new cap or a specific value for the lc cap (can it be non-numeric) to request not changing the priority at all.

This is indeed a new feature but also a fix for the incoherent behavior of 'priority' vs. all the other non-essential capabilities (including 'umask', but also resource limits, paths, and other environment variables; "non-essential" in a technical sense, policy is another matter). An absence of the latter means the corresponding context is left unchanged (and thus inherited), and it is pretty much confusing (besides illogical) that priority would be the only capability not behaving this way. It's also a fix vis-a-vis what is documented (no default value for priority).

The only slight incompatibility is that this changes behavior for configurations where 'priority' has been removed from the default login.conf. Besides not affecting the default configuration, it's very unlikely that people would tweak that (before this change, removing ':priority=0:' has no effect). So I think it is worth going ahead instead of adding another capability, and of course document the incompatibility in the release notes (again, a priori I don't think anyone will be affected; I might be wrong).

[kib]

I do not propose to add yet another capability. My question is, can you specify either keyword or a numeric for lc?

[olce]

In D40349#921930, @kib wrote:

I do not propose to add yet another capability.

In D40349#921706, @kib wrote:

I think I lean to agree with Andrew. You are introducing a new feature by dropping previous behavior. Might be, it makes more sense to provide some new cap or a specific value for the lc cap (can it be non-numeric) to request not changing the priority at all.

(Emphasis mine.)

In D40349#921930, @kib wrote:

I do not propose to add yet another capability. My question is, can you specify either keyword or a numeric for lc?

Currently, it's not possible to input anything other than an integer for capability priority (anything else triggers the error path, with in the end LOGIN_DEFPRI being used before this patch, or only a more informative error message being printed with the patch).

But we could certainly make it so.

[kib]

In D40349#921968, @olce.freebsd_certner.fr wrote:

In D40349#921930, @kib wrote:

I do not propose to add yet another capability.

In D40349#921706, @kib wrote:

I think I lean to agree with Andrew. You are introducing a new feature by dropping previous behavior. Might be, it makes more sense to provide some new cap or a specific value for the lc cap (can it be non-numeric) to request not changing the priority at all.

(Emphasis mine.)

In D40349#921930, @kib wrote:

I do not propose to add yet another capability. My question is, can you specify either keyword or a numeric for lc?

Currently, it's not possible to input anything other than an integer for capability priority (anything else triggers the error path, with in the end LOGIN_DEFPRI being used before this patch, or only a more informative error message being printed with the patch).

But we could certainly make it so.

I mean, is it possible to modify the code to accept both keyword and number. If yes, then I see it as the best route to achieve what you want and keep compatibility. By introducing a keyword for priority value that means 'do not change prio' and still set the priority for default if not specified.

[olce]

In D40349#923223, @kib wrote:

I mean, is it possible to modify the code to accept both keyword and number. If yes, then I see it as the best route to achieve what you want and keep compatibility. By introducing a keyword for priority value that means 'do not change prio' and still set the priority for default if not specified.

Yes, it is indeed the best route, but as long as we absolutely want to keep compatibility in this case.

Just to be sure that there is no misunderstanding, I of course value backwards compatibility a lot (and POLA). However, my point in this particular case was that keeping compatibility for what is very likely a non-existent use case appeared to me less important than the consistency of the login.conf machinery, and in particular for users of login.conf considering what is in the documentation and has been for years (i.e., almost all capabilities are "inherited", including priority according to the man page, umask being an exception there (but, as we saw in the code and in a previous review, umask is actually also inherited)).

To give other data points, it is true that OpenBSD and NetBSD reset priority to 0 and umask to 022. By contrast, we don't for umask (since 1997; DragonflyBSD inherited the same code). On the other hand, illumos resets the umask but not the priority (for the login command; there does not seem to be anything equivalent to login.conf there, the context is arguably a bit different). On Linux, there are the pam_umask and pam_limits modules (most distros should be using them). pam_limits is the one that manages the process priority, and doesn't change it if priority is not explicitly specified in limits.conf. pam_umask also doesn't change the umask if not explicitly specified (as an argument to the module, in /etc/login.defs or in /etc/default/login in that order). I think this behavior, being more in line with the traditional UNIX way (inherit and then possibly override), is the least surprising.

I really did not want to sound too nitpicky on this, but I think consistency and predictability and POLA are especially important when it comes to things having potential security implications. Given all the additional considerations above, would you still not accept the initial patch (and with it, the extremely low risk that someone would have deleted :priority=0: from the default login.conf, the only case of a change in behavior)?

If you want me to persist on the keyword route regardless, then here are the next steps:

1. I'm going to propose code to have priority accept a keyword (e.g., inherit).
2. I'll be considering revising the umask part then, again for consistency. Perhaps the pre-1997 behavior should be restored (for compatibility with other BSDs), and a keyword defined there as well to indicate inheritance is desired? Or you'd prefer not to change it again (similarly to priority, I don't think anyone would simply remove :umask=022: from their login.conf)?

Thanks for your patience and advices/guidance.

[kib]

I think that the login.conf stuff, besides being a security-sensitive, is also quite obscure, both for users in devs. For the 'devs' part, I mean that developers really do not have good insight how this interface is (mis-)used. This is my reason to be such jerk when discussing behavior changes, whatever logical and streamlining they seems to be.

[olce]

Ha ha... I don't think you've been a jerk. Generally speaking, you may be right about unforeseen misuse, but I don't think it applies in such a simple case. And I can only hope that you've not lost all hope concerning changing legacy code (only if there are excellent reasons to do so).

Anyway, after pondering this a bit more and with my security hat on, I think that having an explicit value to indicate inheritance is really the best move. And this doesn't preclude a future change of default behavior (which well could be, instead of inheriting in absence of priority, just throwing an error, except perhaps for root).

I'll submit a new change tomorrow.

Thanks.

[olce]

Code is done, but not yet tested correctly. So I'll submit it tomorrow instead, sorry for the delay.

[olce]

Revert to setting the priority to 0 when priority is not specified.

[olce]

Delay removing LOGIN_DEFPRI in another commit (not to be MFCed now). Arguably, the removal didn't belong to this commit anyway.

[emaste]

lib/libutil/login_class.c

521

as with comment in some previous review, I prefer braces around all if-else blocks if any have them

[imp]

lib/libutil/login_class.c

521

style(9) tries to make this point, but is a hopeless. on clearity in its striving for brevity:

Space after keywords (if, while, for, return, switch).  Two styles of
 braces (‘{’ and ‘}’) are allowed for single line statements.  Either they
 are used for all single statements, or they are used only where needed
 for clarity.  Usage within a function should be consistent.  Forever

which can be read many ways. Certainly within a single if statement, I prefer what you're advocating
too and that seemed to be the consensus of the discussions when I added the above text, but it's not 100% clear.