cr_canseejailproc(): New privilege, no direct check for UID 0

[olce]

Summary

Use priv_check_cred() with a new privilege (PRIV_SEEJAILPROC) instead of
explicitly testing for UID 0 (the former has been the rule for almost 20
years).

As a consequence, cr_canseejailproc() now abides by the
'security.bsd.suser_enabled' sysctl and MAC policies.

Update the MAC policies Biba and LOMAC, and prison_priv_check() so that
they don't deny this privilege. This preserves the existing behavior
(the 'root' user is not restricted, even when jailed, unless
'security.bsd.suser_enabled' is not 0) and is consistent with what is
done for the related policies/privileges (PRIV_SEEOTHERGIDS,
PRIV_SEEOTHERUIDS).

[emaste]

sys/kern/kern_prot.c

1433–1435

Minor point but I find the ternary operator somewhat obfuscates cases like this; I think something like this is clearer:

if (see_jail_proc != 0 || u1->cr_prison == u2->cr_prison || priv_check_cred(u1, PRIV_SEEJAILPROC) == 0)
        return (0);
return (ESRCH);

(I don't insist it be done like this though)

[jamie]

Looks good to me. But then the original that did the direct cr_uid check looked apparently good to me too, so take it for what it's worth.

[olce]

sys/kern/kern_prot.c

1433–1435

I don't have strong feelings about that. It's just that I usually prefer the ternary operator when both results are to be used in the same statements.

Would you find it clearer like this:

	return ((see_jail_proc != 0 || u1->cr_prison == u2->cr_prison ||
	    priv_check_cred(u1, PRIV_SEEJAILPROC) == 0) ?
            0 : ESRCH);

(i.e., making the ? stand out at the end of the line)? If you don't feel it's enough, I'll convert it to your proposal.

Also, I put parentheses around the guard expression to make things explicit. Personally, I don't really consider it to be ambiguous (the ternary operator is of very low priority, I assume this is something everyone knows, at least intuitively), so I'd gladly remove them if you're not opposed to.

[olce]

Remove superfluous parentheses, make ? stand out

[zlei]

sys/kern/kern_prot.c

1431

The original logic:

if (u1->cr_uid == 0 ||  /* root ? */
    see_jail_proc != 0 ||  /* MIB permitted */
    u1->cr_prison == u2->cr_prison) /* Obviously */
        return 0;

return (ESRCH);

sys/sys/priv.h

108

PRIV_SEEJAILPROC is newly introduced. I do not see any logic add this privilege to ucred.

I have not tested this patch yet, is it complete ?

[emaste]

@zlei I added you to subscribers on this first one in the series - click on Stack to see the rest.

[zlei]

In D40626#928850, @emaste wrote:

@zlei I added you to subscribers on this first one in the series - click on Stack to see the rest.

Got a huge Stack, I'll check them at my best ;)

[olce]

sys/sys/priv.h

108

PRIV_SEEJAILPROC is newly introduced. I do not see any logic add this privilege to ucred.

You don't because there is no need of additional logic. Please see priv_check_cred() and the if (suser_enabled(cred)) block, and in particular the default case of switch(priv) which handles any privilege not explicitly matched with the default policy of testing that the (effective) UID is 0.

I have not tested this patch yet, is it complete ?

Yes, it is complete and well-tested (I would have marked it as a draft if it wasn't).

[olce]

In D40626#928851, @zlei wrote:

Got a huge Stack, I'll check them at my best ;)

Don't be afraid... Of the 19 differentials, only 8 contain code changes (the others are man pages, not sure if you want to review them; would be great), of which 3 are non-functional ones (renames, function split). The remaining 5 are not rocket science, and you've already stumbled on probably the only "surprise" (the new priv declaration but without additional code).

[emaste]

sys/kern/kern_prot.c

1433–1435

I don't think the different wrapping improves clarity. To me the formatting/style in @zlei's "The original logic:" makes it very clear -- in these cases it can be seen, otherwise we fall through to the error.

[emaste]

Simplest change here would be just if (u1->cr_uid == 0) -> if (priv_check_cred(u1, PRIV_SEEJAILPROC) == 0)

[olce]

Change form and formatting.

Update the commit message.

[olce]

In D40626#934738, @emaste wrote:

Simplest change here would be just if (u1->cr_uid == 0) -> if (priv_check_cred(u1, PRIV_SEEJAILPROC) == 0)

Indeed, but the current ordering of tests doesn't seem very logical. Moreover, now that there is a function to call instead of just checking for equality, determining if some user is privileged will be comparatively longer than just checking for a flag, which penalizes those that don't use the policy (which is disabled by default). So I'm using the opportunity to improve that as well.

[emaste]

In D40626#934796, @olce.freebsd_certner.fr wrote:

Indeed, but the current ordering of tests doesn't seem very logical. Moreover, now that there is a function to call instead of just checking for equality, determining if some user is privileged will be comparatively longer than just checking for a flag, which penalizes those that don't use the policy (which is disabled by default). So I'm using the opportunity to improve that as well.

All fair points.

On a related note, what do you think about making security.bsd.see_jail_proc 0 by default?

[olce]

In D40626#934868, @emaste wrote:

On a related note, what do you think about making security.bsd.see_jail_proc 0 by default?

I think we should default it to 0.

Regular users seeing processes/sockets in sub-jails with same UID seem to me more a consequence of the jails implementation than something done deliberately. I think it is much more likely that admins delegate a sub-jail to another admin who is not even a regular user in the upper jail or the raw machine. In other words, I expect that in most cases, the set of users in a jail and that in a sub-jail are totally independent. Also, server software/packages/ports usually reserve some user with particular UID for their use, which leads to (usually unwanted) user conflation between jails and sub-jails using the same software.

[mhorne]

sys/kern/kern_prot.c

1429–1431

[emaste]

sys/kern/kern_prot.c

1429–1431

Changing the sense?

[mhorne]

sys/kern/kern_prot.c

1429–1431

oh, duh, I thought something seemed wrong.

I meant: if (see_jail_proc ||

[olce]

sys/kern/kern_prot.c

1429–1431

Ah yes, the integers that are in fact booleans... Forgot about it once more.

I'll change that (tomorrow).

[olce]

Simplify testing a boolean.

[olce]

Re-validated the revision after this minor change.

[zlei]

Sorry for excessively late response, I'd like to test these serials of changes, I'll report later.

BTW, I urge @olce.freebsd_certner.fr to re-arrange the diffs, I suggested as following (in general):

1. Functional changes (fixes)
2. Document changes
3. Breaking changes
4. Document of the breaking changes

then they are basically atomic changes, and for 1 and 2 it will be much easier to MFC to stable branches.

[olce]

Make sure to preserve the original behavior of MAC policies Biba and LOMAC and that of jailed root users, which is consistent with how the other process visibility policies are treated.

This only involved adding some additional case statements with PRIV_SEEJAILPROC to existing ones with PRIV_SEEOTHERGIDS/PRIV_SEEOTHERUIDS.

[mhorne]

Good job identifying the behaviour change.

Have you given any thought about how we might write tests for any of these functions/policies? They are deeply layered enough that it would seem difficult to do completely and efficiently, and yet it would be valuable to be able to verify that future changes do not change the behaviour.

[olce]

In D40626#957701, @mhorne wrote:

Have you given any thought about how we might write tests for any of these functions/policies? They are deeply layered enough that it would seem difficult to do completely and efficiently, and yet it would be valuable to be able to verify that future changes do not change the behaviour.

My first thought is to have simple tests that launch several processes with unprivileged users at different levels of jail nesting (0, 1 or 2) and have them perform and check accesses to other known-to-exist processes, which is basically automatizing the tests I performed by hand.

I "just" have to get familiar with the test infrastructure (I only know "stress2" for now) and then figure out a not too complicated way to synchronize the above processes to ensure coexistence at the moment of access attempts. So I need more time, but yes, it's planned.