Fix 'security.bsd.see_jail_proc' by using cr_bsd_visible()

[olce]

Summary

As implemented, this security policy would only prevent seeing processes in
sub-jails, but would not prevent sending signals to, changing priority of or
debugging processes in these, enabling attacks where unprivileged users could
tamper with random processes in sub-jails in particular circumstances (conflated
UIDs) despite the policy being in force.

PR: 272092

[zlei]

sys/kern/kern_prot.c

1809

Sockets are not processes, so I suspect this change is wrong.

sys/netinet/in_prot.c

70

inpcbs are network related objects, they are not processes, so I suspect this change is wrong.

[olce]

I don't think so, no. I mean, yes, inpcbs are network objects for sure, but that's not the point. The goal of the policy is to hide objects (and subjects) that are not in the same jail. I don't see any reason why sockets would be exempt from that policy. On the contrary, it would be a security leak, since processes in a parent jail could see sockets of sub-jails.

I guess you've been misguided by the policy name: see_jail_proc? It is misnamed in the sense that it really extends to much more than just processes (naming is hard, indeed ;-)), and it wouldn't make much sense to restrict it to them. And its creator, @swills, was aware of that in particular concerning sockets, as you can see from the following comment which he wrote:

* 'see_jail_proc' determines whether or not visibility of processes and                                                                                                                                                                                                                                                                                  
* sockets with credentials holding different jail ids is possible using a                                                                                                                                                                                                                                                                                
* variety of system MIBs.

Again, I see no reason why sockets should be treated differently. Do you see some?

[olce]

Rename cr_bsd_visibility() to cr_bsd_visible().

[mhorne]

I definitely agree with the motivation and direction of the change at large. It is difficult to estimate the total scope of these changes, since you are changing several functions, each with several callers. Looks like the behaviour of the kern.proc.* sysctls is unchanged, since they were already checking see_jail_proc by calling p_cansee().

I do not know what type of coverage our existing test suite has of these policies, but it is probably little if not none. Still, it might be a useful experiment to run the test suite before and after the whole series of changes and check the test results for regressions. With the following settings applied: security.bsd.see_other_uids=1, security.bsd.see_other_gids=1, security.bsd.see_jail_proc=0.

I spot two places in the documentation that might need to be tweaked after these changes (but maybe not):

• The DISABLING PTRACE section of ptrace(2)
• The description of security.bsd.see_jail_proc in security(7)

sys/kern/kern_prot.c

1809

Sockets inherit the credentials of the process that creates them. I think it is right to enforce the same visibility policy for socket and inpcb objects. Otherwise, what would be the meaning of giving them credentials?

[olce]

Ok, I'll look at the test suite and see if I see something relevant. And anyway, run it before and after the whole changes.

I spot two places in the documentation that might need to be tweaked after these changes (but maybe not):

• The DISABLING PTRACE section of ptrace(2)
• The description of security.bsd.see_jail_proc in security(7)

Ok, I'll check that as well.

[olce]

In D40628#935069, @mhorne wrote:

I definitely agree with the motivation and direction of the change at large. It is difficult to estimate the total scope of these changes, since you are changing several functions, each with several callers. Looks like the behaviour of the kern.proc.* sysctls is unchanged, since they were already checking see_jail_proc by calling p_cansee().

I followed the rule of thumb that, if some function calls cr_seeotheruids(), then it should perform the other policy checks as well, so should call cr_bsd_visible() instead.

The behavior of cr_cansee(), and hence that of p_cansee(), is unchanged. That of p_canwait() neither (the code that is updated is under #ifdef 0). By contrast, behavior is directly changed for cr_cansignal(), p_cansched(), p_candebug(), cr_canseesocket() and cr_canseeinpcb().

I've checked the impacts more thoroughly. cr_cansignal() impacts p_cansignal() which, as you would expect, controls who one can signal. p_cansched() is used to check whether some thread can act on some thread/process, with "act" being changing the cpuset, or "protect" (see protect(1)), or renice or change priority or scheduling class (AFAIK, this list is exhaustive). p_candebug() is used when tracing/debugging processes, as well as for some procctl(2) requests. cr_canseesocket() is called before obtaining credentials on sockets (and some OFED driver when listing SDP connections). cr_canseeinpcb() is similar for everything IP-related.

So nothing unexpected or that we would want to rule out I'd say.

[olce]

In D40628#935069, @mhorne wrote:

I spot two places in the documentation that might need to be tweaked after these changes (but maybe not):

• The DISABLING PTRACE section of ptrace(2)
• The description of security.bsd.see_jail_proc in security(7)

Indeed, they needed updates. I found an additional one: sysctl(8).

See the new revisions at end of stack (D41108, D41109 and D41113).