On 12/1/25 19:53, Eric W. Biederman wrote: > Roberto Sassu <[email protected]> writes: > >> On Mon, 2025-12-01 at 10:06 -0600, Eric W. Biederman wrote: >>> Roberto Sassu <[email protected]> writes: >>> >>>> + Mimi, linux-integrity (would be nice if we are in CC when linux- >>>> security-module is in CC). >>>> >>>> Apologies for not answering earlier, it seems I don't receive the >>>> emails from the linux-security-module mailing list (thanks Serge for >>>> letting me know!). >>>> >>>> I see two main effects of this patch. First, the bprm_check_security >>>> hook implementations will not see bprm->cred populated. That was a >>>> problem before we made this patch: >>>> >>>> https://patchew.org/linux/[email protected]/ >>> >>> Thanks, that is definitely needed. >>> >>> Does calling process_measurement(CREDS_CHECK) on only the final file >>> pass review? Do you know of any cases where that will break things? >> >> We intentionally changed the behavior of CREDS_CHECK to be invoked only >> for the final file. We are monitoring for bug reports, if we receive >> complains from people that the patch breaks their expectation we will >> revisit the issue. >> >> Any LSM implementing bprm_check_security looking for brpm->cred would >> be affected by recalculating the DAC credentials for the final binary. >> >>> As it stands I don't think it should be assumed that any LSM has >>> computed it's final creds until bprm_creds_from_file. Not just the >>> uid and gid. >> >> Uhm, I can be wrong, but most LSMs calculate their state change in >> bprm_creds_for_exec (git grep bprm_creds_for_exec|grep LSM_HOOK_INIT). >> >>> If the patch you posted for review works that helps sort that mess out. >> >> Well, it works because we changed the expectation :) > > I just haven't seen that code land in Linus's tree yet so I am a bit > cautious in adopting that. It is definitely needed as the behavior > of IMA as v6.18 simply does not work in general. > >>>> to work around the problem of not calculating the final DAC credentials >>>> early enough (well, we actually had to change our CREDS_CHECK hook >>>> behavior). >>>> >>>> The second, I could not check. If I remember well, unlike the >>>> capability LSM, SELinux/Apparmor/SMACK calculate the final credentials >>>> based on the first file being executed (thus the script, not the >>>> interpreter). Is this patch keeping the same behavior despite preparing >>>> the credentials when the final binary is found? >>> >>> The patch I posted was. >>> >>> My brain is still reeling from the realization that our security modules >>> have the implicit assumption that it is safe to calculate their security >>> information from shell scripts. >> >> If I'm interpreting this behavior correctly (please any LSM maintainer >> could comment on it), the intent is just to transition to a different >> security context where a different set of rules could apply (since we >> are executing a script). >> >> Imagine if for every script, the security transition is based on the >> interpreter, it would be hard to differentiate between scripts and >> associate to the respective processes different security labels. >> >>> In the first half of the 90's I remember there was lots of effort to try >>> and make setuid shell scripts and setuid perl scripts work, and the >>> final conclusion was it was a lost cause. >> >> Definitely I lack a lot of context... > > From the usenet comp.unix.faq that was probably updated in 1994: > http://www.faqs.org/faqs/unix-faq/faq/part4/section-7.html > > I have been trying to remember enough details by looking it up, but the > short version is that one of the big problems is there is a race between > the kernel doing it's thing and the shell opening the shell script. > > Clever people have been able to take advantage of that race and insert > arbitrary code in that window for the shell to execute. All you have to > do is google for how to find a reproducer if the one in the link above > is not enough. > >>> Now I look at security_bprm_creds_for_exec and security_bprm_check which >>> both have the implicit assumption that it is indeed safe to compute the >>> credentials from a shell script. >>> >>> When passing a file descriptor to execat we have >>> BINPRM_FLAGS_PATH_INACCESSIBLE and use /dev/fd/NNN as the filename >>> which reduces some of the races. >>> >>> However when just plain executing a shell script we pass the filename of >>> the shell script as a command line argument, and expect the shell to >>> open the filename again. This has been a time of check to time of use >>> race for decades, and one of the reasons we don't have setuid shell >>> scripts. >> >> Yes, it would be really nice to fix it! > > After 30 years I really don't expect that is even a reasonable request. > > I think we are solidly into "Don't do that then", and the LSM security > hooks are definitely doing that. > > There is the partial solution of passing /dev/fd instead of passing the > name of the script. I suspect that would break things. I don't > remember why that was never adopted. > > I think even with the TOCTOU race fixed there were other serious issues. > > I really think it behooves any security module people who want to use > the shell script as the basis of their security decisions to research > all of the old well known issues and describe how they don't apply. > > All I have energy for is to point out it is broken as is and to start > moving code down into bprm_creds_from_file to avoid the race. > > Right now as far as I can tell anything based upon the script itself > is worthless junk so changing that would not be breaking anything that > wasn't already broken. > >>> Yet the IMA implementation (without the above mentioned patch) assumes >>> the final creds will be calculated before security_bprm_check is called, >>> and security_bprm_creds_for_exec busily calculate the final creds. >>> >>> For some of the security modules I believe anyone can set any label they >>> want on a file and they remain secure (At which point I don't understand >>> the point of having labels on files). I don't believe that is the case >>> for selinux, or in general. >> >> A simple example for SELinux. Suppose that the parent process has type >> initrc_t, then the SELinux policy configures the following transitions >> based on the label of the first file executed (sesearch -T -s initrc_t >> -c process): >> >> type_transition initrc_t NetworkManager_dispatcher_exec_t:process >> NetworkManager_dispatcher_t; >> type_transition initrc_t NetworkManager_exec_t:process NetworkManager_t; >> type_transition initrc_t NetworkManager_initrc_exec_t:process initrc_t; >> type_transition initrc_t NetworkManager_priv_helper_exec_t:process >> NetworkManager_priv_helper_t; >> type_transition initrc_t abrt_dump_oops_exec_t:process abrt_dump_oops_t; >> type_transition initrc_t abrt_exec_t:process abrt_t; >> [...] >> >> (there are 747 rules in my system). >> >> If the transition would be based on the interpreter label, it would be >> hard to express with rules. > > Which is a problem for the people making the rules engine. Because > 30 years of experience with this problem says basing anything on the > script is already broken. > > I understand the frustration, but it requires a new way of launching > shell scripts to even begin to make it secure. > >> If the transition does not occur for any reason the parent process >> policy would still apply, but maybe it would not have the necessary >> permissions for the execution of the script. > > Yep. > >>> So just to remove the TOCTOU race the security_bprm_creds_for_exec >>> and security_bprm_check hooks need to be removed, after moving their >>> code into something like security_bprm_creds_from_file. >>> >>> Or am I missing something and even with the TOCTOU race are setuid shell >>> scripts somehow safe now? >> >> Take this with a looot of salt, if there is a TOCTOU race, the script >> will be executed with a security context that does not belong to it. >> But the transition already happened. Not sure if it is safe. > > Historically it hasn't been safe. > >> I also don't know how the TOCTOU race could be solved, but I also would >> like it to be fixed. I'm available to comment on any proposal! > > I am hoping someone who helped put these security hooks where they are > will speak up, and tell me what I am missing. > > All I have the energy for right now is to point out security policies > based upon shell scripts appear to be security policies that only > protect you from well behaved programs. >
Hmm, yes, that looks like an issue. I would have expected the security engine to look at bprm->filenanme especially in the case, when bprm->interp != bprm->filename, and check that it is not a sym-link with write-access for the current user and of course also that the bprm->file is not a regular file which is writable by the current user, if that is the case I would have expected the secuity engine to enforce non-new-privs on a SUID executable somehow. Bernd.
