Page MenuHomeFreeBSD
Differential D40626

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

Authored by olce on Jun 20 2023, 1:44 PM.
Tags
None
Referenced Files
F109378586: D40626.diff
Tue, Feb 4, 7:12 AM
Unknown Object (File)
Tue, Jan 28, 11:07 PM
Unknown Object (File)
Tue, Jan 28, 6:37 AM
Unknown Object (File)
Mon, Jan 27, 11:12 AM
Unknown Object (File)
Mon, Jan 27, 11:01 AM
Unknown Object (File)
Sun, Jan 12, 2:28 AM
Unknown Object (File)
Sun, Jan 12, 2:28 AM
Unknown Object (File)
Sat, Jan 11, 4:35 PM
View All Files
Subscribers
emaste
imp
jamie
jrm
mhorne
zlei

Details

Reviewers
mjg
kib
dchagin
mhorne
olce
Commits
rGd40c6fa7db7d: cr_canseejailproc(): New privilege, no direct check for UID 0
rG63c01c18a8d3: cr_canseejailproc(): New privilege, no direct check for UID 0
rG9dad3ed1d15c: cr_canseejailproc(): New privilege, no direct check for UID 0
rG7974ca1cdbee: cr_canseejailproc(): New privilege, no direct check for UID 0
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).

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 53748
Build 50639: arc lint + arc unit

Event Timeline

olce created this revision.Jun 20 2023, 1:44 PM
Herald added a subscriber: imp. · View Herald TranscriptJun 20 2023, 1:44 PM
olce requested review of this revision.Jun 20 2023, 1:44 PM
Harbormaster completed remote builds in B52161: Diff 123492.Jun 20 2023, 1:44 PM
olce added a child revision: D40627: New cr_bsd_visible(): Whether BSD policies deny seeing subjects/objects.Jun 20 2023, 2:04 PM
olce added reviewers: mjg, kib, dchagin.Jun 20 2023, 9:03 PM
emaste added subscribers: jamie, emaste, zlei.Jun 29 2023, 3:39 PM
emaste added inline comments.Jun 29 2023, 3:46 PM
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)

jrm added a subscriber: jrm.Jun 29 2023, 3:53 PM
jamie added a comment.Jun 29 2023, 4:08 PM

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 added inline comments.Jun 29 2023, 4:43 PM
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 updated this revision to Diff 124002.Jun 29 2023, 8:36 PM

Remove superfluous parentheses, make ? stand out

Harbormaster completed remote builds in B52339: Diff 124002.Jun 29 2023, 8:36 PM
olce marked an inline comment as done.Jun 29 2023, 8:36 PM
olce marked an inline comment as not done.
zlei added inline comments.Jun 30 2023, 1:24 AM
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 added a comment.Jun 30 2023, 1:33 AM

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

zlei added a comment.Jun 30 2023, 1:36 AM
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 added inline comments.Jun 30 2023, 8:14 AM
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 added a comment.Jun 30 2023, 1:24 PM
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 added inline comments.Jul 17 2023, 1:45 PM
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 added a comment.Jul 17 2023, 1:58 PM

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

olce updated this revision to Diff 124761.Jul 17 2023, 4:02 PM
olce marked 4 inline comments as done.
olce edited the summary of this revision. (Show Details)

Change form and formatting.

Update the commit message.

Harbormaster completed remote builds in B52673: Diff 124761.Jul 17 2023, 4:02 PM
olce added a comment.Jul 17 2023, 4:03 PM
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.

olce mentioned this in D40627: New cr_bsd_visible(): Whether BSD policies deny seeing subjects/objects.Jul 17 2023, 4:38 PM
olce mentioned this in D40629: Make cr_bsd_visible()'s sub-functions internal.Jul 17 2023, 4:41 PM
olce mentioned this in D40630: cr_canseeotheruids(), cr_canseeothergids(): Man pages: Impacts of rename.Jul 17 2023, 4:50 PM
emaste added a comment.Jul 17 2023, 5:13 PM
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 added a comment.Jul 17 2023, 5:34 PM
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 accepted this revision.Jul 18 2023, 2:22 PM
mhorne added a subscriber: mhorne.
mhorne added inline comments.
sys/kern/kern_prot.c
1429–1431
This revision is now accepted and ready to land.Jul 18 2023, 2:22 PM
emaste added inline comments.Jul 18 2023, 2:32 PM
sys/kern/kern_prot.c
1429–1431

Changing the sense?

mhorne added inline comments.Jul 18 2023, 2:37 PM
sys/kern/kern_prot.c
1429–1431

oh, duh, I thought something seemed wrong.

I meant: if (see_jail_proc ||

olce added inline comments.Jul 18 2023, 4:50 PM
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 updated this revision to Diff 124880.Jul 19 2023, 6:11 PM

Simplify testing a boolean.

This revision now requires review to proceed.Jul 19 2023, 6:11 PM
Harbormaster completed remote builds in B52726: Diff 124880.Jul 19 2023, 6:11 PM
olce marked 3 inline comments as done.Jul 19 2023, 6:12 PM

Re-validated the revision after this minor change.

olce accepted this revision.Jul 19 2023, 6:12 PM
This revision is now accepted and ready to land.Jul 19 2023, 6:12 PM
zlei added a comment.Aug 16 2023, 1:19 AM

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 updated this revision to Diff 127874.Sep 27 2023, 5:27 PM
olce edited the summary of this revision. (Show Details)

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.

This revision now requires review to proceed.Sep 27 2023, 5:27 PM
Harbormaster completed remote builds in B53748: Diff 127874.Sep 27 2023, 5:27 PM
mhorne accepted this revision.Sep 27 2023, 5:52 PM

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.

This revision is now accepted and ready to land.Sep 27 2023, 5:52 PM
Closed by commit rG7974ca1cdbee: cr_canseejailproc(): New privilege, no direct check for UID 0 (authored by olce, committed by mhorne). · Explain WhySep 28 2023, 3:11 PM
This revision was automatically updated to reflect the committed changes.
mhorne added a commit: rG7974ca1cdbee: cr_canseejailproc(): New privilege, no direct check for UID 0.
olce added a comment.EditedOct 3 2023, 12:39 PM
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.

mhorne added a commit: rG9dad3ed1d15c: cr_canseejailproc(): New privilege, no direct check for UID 0.Oct 17 2023, 7:43 PM
mhorne added a commit: rG63c01c18a8d3: cr_canseejailproc(): New privilege, no direct check for UID 0.Oct 18 2023, 6:04 PM
olce added a commit: rGd40c6fa7db7d: cr_canseejailproc(): New privilege, no direct check for UID 0.Dec 21 2023, 1:45 PM

Revision Contents
Changeset List

PathSize
sys/
kern/
1 line
7 lines
security/
mac_biba/
1 line
mac_lomac/
1 line
sys/
1 line

Diff 127874

View Options

sys/kern/kern_jail.c

Loading...
View Options

sys/kern/kern_prot.c

Loading...
View Options

sys/security/mac_biba/mac_biba.c

Loading...
View Options

sys/security/mac_lomac/mac_lomac.c

Loading...
View Options

sys/sys/priv.h

Loading...