jailed() isn't the right test. It handles someone on the host system looking at jailed users' processes, but doesn't handle the sub-jail case. If a user in p1 is looking at processes, he shouldn't see anything from p2 which is a jail under p1. Yet, both creds will show up as "jailed".

The proper test would be to just compare u1->cr_prison to u2->cr_prison. That will work regardless of whether u1 is the base system or a higher-level jail.

In D10770#223172, @jamie wrote:

jailed() isn't the right test. It handles someone on the host system looking at jailed users' processes, but doesn't handle the sub-jail case. If a user in p1 is looking at processes, he shouldn't see anything from p2 which is a jail under p1. Yet, both creds will show up as "jailed".

The proper test would be to just compare u1->cr_prison to u2->cr_prison. That will work regardless of whether u1 is the base system or a higher-level jail.

That's the same check that is in prison_check(), which also checks prison_ischild() which I think should also be called. Maybe the call to prison_check() should be in if (!see_jail_proc)?

prison_check() is required in all cases, because it covers jails that can never be seen, i.e. if you're trying to see processes in a parent jail, or a jail is trying to see the base system. The reason prison_check() does the equality test is because prison_ischild() checks for a "<" kind of relationship when we want a "<=" check.

On the subject, I missed that cr_cansee() already calls prison_check() first thing, so it doesn't need to be called again in cr_canseejailproc(). The logic in cr_cannseejailproc() then boils down to (!see_jail_proc && u1->cr_prison != u2->cr_prison ? ESRCH : 0).

In D10770#223195, @jamie wrote:

prison_check() is required in all cases, because it covers jails that can never be seen, i.e. if you're trying to see processes in a parent jail, or a jail is trying to see the base system. The reason prison_check() does the equality test is because prison_ischild() checks for a "<" kind of relationship when we want a "<=" check.

On the subject, I missed that cr_cansee() already calls prison_check() first thing, so it doesn't need to be called again in cr_canseejailproc(). The logic in cr_cannseejailproc() then boils down to (!see_jail_proc && u1->cr_prison != u2->cr_prison ? ESRCH : 0).

Ok, thanks, that makes sense. One question, is this a bug in cr_canseeotheruids() and cr_canseeothergids()? Should those just be updated to call prison_check(u1, u2) where appropriate and not add a new sysctl?

No, cr_seeotheruids() and c r_seeothergids() are fine as is. Since prison_check() comes before everything else, those don't need to worry about th prison situation. You still need the new sysctl though, for the originally identified reason.

Ok, thanks, that makes sense. One question, is this a bug in cr_canseeotheruids() and cr_canseeothergids()? Should those just be updated to call prison_check(u1, u2) where appropriate and not add a new sysctl?

Thinking about it, we mayneed to do both. We need to add the sysctl for the case where you want to hide jail procs from users but not system procs, and we need to add calls to prison_check(u1, u2) in cr_canseeotheruids() and cr_canseeothergids(). Does that make sense?

In D10770#223197, @jamie wrote:

No, cr_seeotheruids() and c r_seeothergids() are fine as is. Since prison_check() comes before everything else, those don't need to worry about th prison situation. You still need the new sysctl though, for the originally identified reason.

Ah, you're right. I'll update the review.

In D10770#223197, @jamie wrote:

No, cr_seeotheruids() and c r_seeothergids() are fine as is. Since prison_check() comes before everything else, those don't need to worry about th prison situation. You still need the new sysctl though, for the originally identified reason.

Wait, I'm confused. If that's sufficient, then why am I seeing (as a non privileged user on the host) processes running in jails when I have security.bsd.see_other_gids and security.bsd.see_other_uids set to 0 (the processes happen to be the same UID as my user but aren't the same user)?

In D10770#223200, @swills wrote:

Wait, I'm confused. If that's sufficient, then why am I seeing (as a non privileged user on the host) processes running in jails when I have security.bsd.see_other_gids and security.bsd.see_other_uids set to 0 (the processes happen to be the same UID as my user but aren't the same user)?

When I mention it's sufficient, I mean in the case of not being able to (ever) see parent jails - you don't need to add another prison_check() call because it's already been done. But you still need the cr_canseejailproc() test in order not to see any other jails.

When I mention it's sufficient, I mean in the case of not being able to (ever) see parent jails - you don't need to add another prison_check() call because it's already been done. But you still need the cr_canseejailproc() test in order not to see any other jails.

Ok, that makes sense. Is it a bug in cr_canseeotheruids() and cr_canseeothergids() that they don't hide processes in jails that happen to be the same uid/gid (but aren't the same user because they're in a jail)?

In D10770#223220, @swills wrote:

Is it a bug in cr_canseeotheruids() and cr_canseeothergids() that they don't hide processes in jails that happen to be the same uid/gid (but aren't the same user because they're in a jail)?

Possibly - at least an oversight. There are situations where you may want to be able to see same-user processes in a jail, but that wouldn't be the general case.

The whole jail/user namespace thing is kind of messy. Filesystems also have this problem: do you want your users to have permission to mess with jailed users' files? Can you handle the clashes between quotas of same uid but different jail? There are similar issues for resource limits, which also have a per-uid component. The "proper" solution, using a jid/uid (or jid/gid) tuple in every case, is just to messy (especially in the filesystem case, where the media format is very difficult to change).

Generally the solution is not to allow non-system users outside the jailed environment on a system that hosts jails.

In D10770#223230, @jamie wrote:

Possibly - at least an oversight. There are situations where you may want to be able to see same-user processes in a jail, but that wouldn't be the general case.

The whole jail/user namespace thing is kind of messy. Filesystems also have this problem: do you want your users to have permission to mess with jailed users' files? Can you handle the clashes between quotas of same uid but different jail? There are similar issues for resource limits, which also have a per-uid component. The "proper" solution, using a jid/uid (or jid/gid) tuple in every case, is just to messy (especially in the filesystem case, where the media format is very difficult to change).

Generally the solution is not to allow non-system users outside the jailed environment on a system that hosts jails.

Ok, thanks, that makes sense. I guess for now the new sysctl handles my use case.

@jamie Does the updated patch look like what you had in mind?

In D10770#223306, @swills wrote:

@jamie Does the updated patch look like what you had in mind?

Yes that looks right (not that I've tested anything, but just looking).