Message-ID: <87ms42rq3t.fsf@email.froward.int.ebiederm.org>
Date: Mon, 01 Dec 2025 12:53:10 -0600
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Roberto Sassu <roberto.sassu@...weicloud.com>
Cc: Bernd Edlinger <bernd.edlinger@...mail.de>,  Alexander Viro
 <viro@...iv.linux.org.uk>,  Alexey Dobriyan <adobriyan@...il.com>,  Oleg
 Nesterov <oleg@...hat.com>,  Kees Cook <kees@...nel.org>,  Andy Lutomirski
 <luto@...capital.net>,  Will Drewry <wad@...omium.org>,  Christian Brauner
 <brauner@...nel.org>,  Andrew Morton <akpm@...ux-foundation.org>,  Michal
 Hocko <mhocko@...e.com>,  Serge Hallyn <serge@...lyn.com>,  James Morris
 <jamorris@...ux.microsoft.com>,  Randy Dunlap <rdunlap@...radead.org>,
  Suren Baghdasaryan <surenb@...gle.com>,  Yafang Shao
 <laoar.shao@...il.com>,  Helge Deller <deller@....de>,  Adrian Reber
 <areber@...hat.com>,  Thomas Gleixner <tglx@...utronix.de>,  Jens Axboe
 <axboe@...nel.dk>,  Alexei Starovoitov <ast@...nel.org>,
  "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
  "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
  linux-kselftest@...r.kernel.org,  linux-mm@...ck.org,
  linux-security-module@...r.kernel.org,  tiozhang
 <tiozhang@...iglobal.com>,  Luis Chamberlain <mcgrof@...nel.org>,  "Paulo
 Alcantara (SUSE)" <pc@...guebit.com>,  Sergey Senozhatsky
 <senozhatsky@...omium.org>,  Frederic Weisbecker <frederic@...nel.org>,
  YueHaibing <yuehaibing@...wei.com>,  Paul Moore <paul@...l-moore.com>,
  Aleksa Sarai <cyphar@...har.com>,  Stefan Roesch <shr@...kernel.io>,
  Chao Yu <chao@...nel.org>,  xu xin <xu.xin16@....com.cn>,  Jeff Layton
 <jlayton@...nel.org>,  Jan Kara <jack@...e.cz>,  David Hildenbrand
 <david@...hat.com>,  Dave Chinner <dchinner@...hat.com>,  Shuah Khan
 <shuah@...nel.org>,  Elena Reshetova <elena.reshetova@...el.com>,  David
 Windsor <dwindsor@...il.com>,  Mateusz Guzik <mjguzik@...il.com>,  Ard
 Biesheuvel <ardb@...nel.org>,  "Joel Fernandes (Google)"
 <joel@...lfernandes.org>,  "Matthew Wilcox (Oracle)"
 <willy@...radead.org>,  Hans Liljestrand <ishkamiel@...il.com>,  Penglei
 Jiang <superman.xpt@...il.com>,  Lorenzo Stoakes
 <lorenzo.stoakes@...cle.com>,  Adrian Ratiu <adrian.ratiu@...labora.com>,
  Ingo Molnar <mingo@...nel.org>,  "Peter Zijlstra (Intel)"
 <peterz@...radead.org>,  Cyrill Gorcunov <gorcunov@...il.com>,  Eric
 Dumazet <edumazet@...gle.com>,  zohar@...ux.ibm.com,
  linux-integrity@...r.kernel.org,  Ryan Lee <ryan.lee@...onical.com>,
  apparmor <apparmor@...ts.ubuntu.com>
Subject: Re: Are setuid shell scripts safe? (Implied by
 security_bprm_creds_for_exec)

Roberto Sassu <roberto.sassu@...weicloud.com> writes:

> On Mon, 2025-12-01 at 10:06 -0600, Eric W. Biederman wrote:
>> Roberto Sassu <roberto.sassu@...weicloud.com> 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/20251008113503.2433343-1-roberto.sassu@huaweicloud.com/
>> 
>> 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.

Eric

Powered by blists - more mailing lists