Hi,
Lloyd wrote on Fri, Jan 03, 2025 at 06:38:05AM +0000:
> Sebastien Marie wrote:
>> I would recommand to run neofetch (or others fortune(6)-like program)
>> under interactive shell only.
> While this is good advice, security(8) cannot guarantee that .profile
> will not generate output to stdout or stderr, and the code makes this
> assumption, so the output should be suppressed regardless.
The problem only occurs when the initialization file starts a background
process that prints to standard output, which is even more crazy.
Normal output generated by the initialization file occurs before
the "echo ENV", so it won't prevent security(8) from finding ENV at
the end of the output.
Then again, even though printing from the background is crazy, the
totally misleading error message "Failed to find ENV" is not good,
and for that reason, i think i should commit the patch appended
below.
The patch is quite straightforward. It does not cause a significant
complication because the code already uses shell parsing -
necessarily, because "." is a builtin command, so execve(2) is out.
I do *not* want to suppress STDERR because that does not interfere
with ENV and PATH detection logic. If shell initialization files
print to STDERR, that's so undesirable that whining to the admin
about it makes sense to me. Maybe it wouldn't be worth an explicit
test because it isn't a typical misconfiguration and will hardly
cause a security risk, but if we get the check for free like in
this case, why not...
> This is followed when evaluating .login and .cshrc in check_csh()
> but omitted in the checks for .profile for Korn shell.
> I believe it is an oversight.
OK, i just checked my mail archive and it turned out to be
intentional rather than an oversight.
On March 24, 2011, while writing these three check functions,
Andrew Fresh (afresh1@) mentioned that he didn't find it easy
to redirect STDERR from these subshells.
On March 25, 2011, i replied:
Oh just leave STDERR alone, if somebody is printing to STDERR
from .profile, again, he gets what he deserves.
To which Andrew replied:
The problem is the eval `tset... in /root/.login complains at me. A lot.
So on March 26, 2011, i wrote:
Ah, I see, it relies on the path, which may not be set in the
artificial environment set up by security(8).
For now, i'm suggesting to just ditch STDERR while spawning shells,
which is the same the old script did.
Maybe we can return to it later and do something better.
That "do something better" did indeed happen later, specifically,
three years later. In rev. 1.30 (June 26, 2014), i finally got
rid of the STDERR trashing at the higher level and instead
added the ">& /dev/null" to check_csh(). Not adding anything
to the check_sh() and check_ksh() functions was intentional
because STDERR trouble had only been observed with csh + tset,
not with ksh.
Maybe we could also investigate whether things have changed in that
decade so that we can now make check_csh() more strict, too,
and change ">& /dev/null" to just "> /dev/null" there.
But that's a separate question and not for this patch.
Any OKs for the following patch?
Ingo
Index: security
===================================================================
RCS file: /cvs/src/libexec/security/security,v
diff -u -p -r1.45 security
--- security 10 Jan 2025 10:16:48 -0000 1.45
+++ security 10 Jan 2025 13:53:12 -0000
@@ -256,7 +256,8 @@ sub check_sh {
$umaskset ||= check_umask($filename);
nag !(open my $fh, '-|', qw(/bin/sh -c),
- ". $filename; echo ENV=\$ENV; echo PATH=\$PATH"),
+ ". $filename > /dev/null; " .
+ "echo ENV=\$ENV; echo PATH=\$PATH"),
"cannot spawn /bin/sh: $!"
and next;
my @output = <$fh>;
@@ -290,7 +291,7 @@ sub check_ksh {
check_umask($filename);
nag !(open my $fh, '-|', qw(/bin/ksh -c),
- ". $filename; echo PATH=\$PATH"),
+ ". $filename > /dev/null; echo PATH=\$PATH"),
"cannot spawn /bin/ksh: $!"
and next;
my @output = <$fh>;
