$ENV handling depends on defined(linux), why??

$ENV handling depends on defined(linux), why??

[Denys Vlasenko]

I was looking at this bit:

        if (
#ifndef linux
                getuid() == geteuid() && getgid() == getegid() &&
#endif
                iflag
        ) {
                if ((shinit = lookupvar("ENV")) != NULL && *shinit != '\0') {
                        read_profile(shinit);
                }
        }

thinking "condition order is wrong, if !iflag, calling getuid()
is pointless, we waste 4 syscalls" but then I noticed
"#ifndef linux". So, the inefficiency is not biting me, a linux user...

...but wait. (1) this check says "if we are setuid and run by non-root,
do not source $ENV". Who in their right mind would have a *setuid*
shell executable on any system where security matters?
IOW: this code is pointless anyway even for non-linux users.
And
(2) If there is some sort of standard language somewhere which says
this logic has to exist, then why we don't do this on linux?

git history shows it was there in initial import.

I propose to delete entire #ifndef/#endif block. It's likely wrong.

Alternatively, move "iflag" above "getuid() == geteuid()" checks.

Re: $ENV handling depends on defined(linux), why??

[Harald van Dijk]

On 13/10/2021 10:39, Denys Vlasenko wrote:
>                      Who in their right mind would have a *setuid*
> shell executable on any system where security matters?

I suspect this was originally not for the benefit of setuid shell 
executables, but setuid shell scripts. Linux does not support those, so 
the check is considered unnecessary on Linux.

However, actually, doing something along those lines is useful even on 
Linux when setuid applications can be tricked to launch shell processes 
in insecure ways. bash implements "privileged mode" which drops shell 
privileges except when the setuid application specifically requests 
keeping them, which I would like to see in dash as well.  Tavis Ormandy 
started a thread on that in 2013 with a patch, 
https://www.mail-archive.com/dash@vger.kernel.org/msg00788.html, it 
would be good to get something along those lines in.

Cheers,
Harald van Dijk

Re: $ENV handling depends on defined(linux), why??

[Denys Vlasenko]

On Wed, Oct 13, 2021 at 11:56 AM Harald van Dijk <harald@gigawatt.nl> wrote:
> On 13/10/2021 10:39, Denys Vlasenko wrote:
> >                      Who in their right mind would have a *setuid*
> > shell executable on any system where security matters?
>
> I suspect this was originally not for the benefit of setuid shell
> executables, but setuid shell scripts. Linux does not support those, so
> the check is considered unnecessary on Linux.
>
> However, actually, doing something along those lines is useful even on
> Linux when setuid applications can be tricked to launch shell processes
> in insecure ways.

Not sourcing $ENV is nowhere near enough to ploug this hole,
so doing it is still pointless.

> bash implements "privileged mode" which drops shell
> privileges except when the setuid application specifically requests
> keeping them

*That* would solve the problem. The code I talk about does not
solve that problem.

I propose to delete entire #ifndef/#endif block.

Re: $ENV handling depends on defined(linux), why??

[Harald van Dijk]

On 13/10/2021 20:17, Denys Vlasenko wrote:
> On Wed, Oct 13, 2021 at 11:56 AM Harald van Dijk <harald@gigawatt.nl> wrote:
>> On 13/10/2021 10:39, Denys Vlasenko wrote:
>>>                       Who in their right mind would have a *setuid*
>>> shell executable on any system where security matters?
>>
>> I suspect this was originally not for the benefit of setuid shell
>> executables, but setuid shell scripts. Linux does not support those, so
>> the check is considered unnecessary on Linux.
>>
>> However, actually, doing something along those lines is useful even on
>> Linux when setuid applications can be tricked to launch shell processes
>> in insecure ways.
> 
> Not sourcing $ENV is nowhere near enough to ploug this hole,

Agreed.

> so doing it is still pointless.

If someone were proposing to do this now, then I would agree. But the 
fact that this has been in forever makes me personally think there's 
nothing gained by changing it now to something we'd already know will 
need changing again later: on Linux the only effect of the change would 
be to cause conflicts for distros that already picked up the privmode 
patches years ago.

For better or worse, what dash implements now, except for the #ifndef 
linux, is specified by POSIX, by the way: "ENV shall be ignored if the 
user's real and effective user IDs or real and effective group IDs are 
different." That'd actually be an argument in favour of the opposite 
direction: removing only the #ifndef/#endif to make sure this check is 
performed on all operating systems. But as that's less secure than what 
bash does, I'd still favour following bash.

Cheers,
Harald van Dijk