Linus Torvalds <linus@xxxxxxxxxxxx> writes: > On Wed, Feb 23, 2022 at 5:24 PM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote: >> >> Question: Running a suid program today charges the activity of that >> program to the user who ran that program, not to the user the program >> runs as. Does anyone see a problem with charging the user the program >> runs as? > > So I think that there's actually two independent issues with limits > when you have situations like this where the actual user might be > ambiguous. > > - the "who to charge" question > > - the "how do we *check* the limit" question > > and honestly, I think that when it comes to suid binaries, the first > question is fundamentally ambiguous, because it almost certainly > depends on the user. > > Which to me implies that there probably isn't an answer that is always > right, and that what you should look at is that second option. > > So I would actually suggest that the "execute a suid binary" should > charge the real user, but *because* it is suid, it should then not > check the limit (or, perhaps, should check the hard limit?). > > You have to charge somebody, but at that point it's a bit ambiguous > whether it should be allowed. > > Exactly so that if you're over a process limit (or something similar - > think "too many files open" or whatever because you screwed up and > opened everything) you could still log in as yourself (ssh/login > charges some admin thing, which probably has high limits or is > unlimited), and hopefully get shell access, and then be able to "exec > sudo" to actually get admin access that should be disabled from the > network. > > The above is just one (traditional) example of a fork/open bomb case > where a user isn't really able to no longer function as himself, but > wants to fix things (maybe the user has another terminal open, but > then he can hopefully use a shell-buiiltin 'kill' instead). > > And I'm not saying it's "the thing that needs to work". I'm more > making up an example. > > So I'm only saying that the above actually has two examples to the two > sides of the coin: "login" lowering privileges to a user that may be > over some limit - and succeeding despite that - and 'suid' succeeding > despite the original user perhaps being over-committed. > > So it's intended exactly as an example of "picking the new or the old > user would be wrong in either case if you check limits at the > transition point". > > Hmm? That doesn't really clarify anything for me. We have two checks one in fork and one in exec and you seem to be talking about the check in exec. The check I have problems with for a suid executable is the check in fork. If the new process is accounted to the previous user and we use the permissions of the effective user for checking it that does not make sense to me. If we can sort out that the check in fork. I think I have clarity about the other cases. The check in exec while clumsy and needing cleaning up seems to make sense to me. We have a transition that starts with fork and ends with exec and has operations like setuid in between. If something like setuid() is called before exec we check in exec. The case the check in exec is aimed at supporting are processes spawned from a parent that have a different user (than the parent) and will never call fork again. Those processes would be fundamentally immune to RLIMIT_NPROC if we don't check somewhere besides fork. There is existing code in apache to use RLIMIT_NPROC this way. For your login case I have no problems with it in principle. In practice I think you have to login as root to deal with a fork bomb that hits RLIMIT_NPROC and does not die gracefully. What I don't see about your login example is how it is practically different from the apache cgi script case, that the code has supported for 20 years, and that would be a regression if stopped supporting. If we want to stop supporting that case we can just remove all of the RLIMIT_NPROC tests everywhere except for fork, a nice cleanup. That still leaves me with mismatched effective vs real uid checks in fork when the effective and real uids don't match. Which means testing for root with "capable(CAP_SYS_ADMIN)" does not work. Which today is make the code a bit of a challenge to understand and work with. Eric
