[Buildroot] [PATCH] package/openssh: reset umask when init script exits

[Will Eccles]

> IIRC, it fork()s but doesn't exec(), instead directly
> jumping to the interpreter.

Huh, that's interesting. Never looked much into busybox utilities before,
save for a few specific ones.

> That doesn't actually explain the behaviour you observed, because it
> should still not propagate the umask to the parent process... And
> it definitely doesn't explain how that behaviour could suddenly
> disappear...

I agree, and I am still looking into this. No results yet. I am ready to
accept that the system I found this on was somehow broken and this was a
fluke. It's just strange to me that I saw success with this before at all,
now that I think about it.

Yours,
Will


On Wed, Oct 20, 2021 at 8:08 AM Arnout Vandecappelle <arnout at mind.be> wrote:

>
>
> On 19/10/2021 23:42, Will Eccles wrote:
> > Hi Arnout,
> >
> > Interestingly, I have tested this in a few different ways and achieved
> different
> > results each time. I agree with you that the patch does not make much
> sense, but
> > on the system I wrote it on (or rather, the system I originally tested
> the fix
> > on, which was otherwise unmodified), it seemed to fix the problem. The
> exact
> > steps I followed to test this were:
> >
> > 1. Build the system image (of course)
> > 2. Login as root and test that the uname value was 077 (no config files
> were
> > sourced by the shell, as none were installed on the system)
> > 3. Apply this patch to the S50sshd file and reboot (nothing else was
> changed
> > that I'm aware of; I was logged in for all of 1 minute to make the
> change and
> > reboot)
> > 4. Login as root again and test that the uname value was now 022 (as I
> expected
> > it to be)
> >
>
>   Hm, this reminds me: I believe that busybox ash does something smart to
> execute shell scripts. IIRC, it fork()s but doesn't exec(), instead
> directly
> jumping to the interpreter.
>
>   That doesn't actually explain the behaviour you observed, because it
> should
> still not propagate the umask to the parent process... And it definitely
> doesn't
> explain how that behaviour could suddenly disappear...
>
>   Regards,
>   Arnout
>
>
> > I have no idea how this patch would fix the issue, and I agree that it
> makes no
> > sense, but even in a small test I had done on the host system I had
> achieved
> > results which appeared to agree with this patch. On a new system (built
> minutes
> > ago), I cannot reproduce the exact same steps I took above, which is
> quite
> > puzzling, as nothing has changed (aside from a single device tree
> overlay I
> > added, which should have no relevance here at all).
> >
> >  > And when you source the script, the trap doesn't even trigger at the
> end of
> >  > the script, so this patch doesn't actually reset the umask.
> >
> > I wrote the script under the assumption that it was never sourced, so I
> didn't
> > even consider this. The file shouldn't ever be sourced as it doesn't end
> in .sh
> > anyway.
> >
> > I am willing to accept that I had some sort of gremlin on the original
> system I
> > tested this on. I can't seem to reproduce it on a newly-generated image
> while
> > following precisely the same steps I did above. However, this only
> raises more
> > questions for me to investigate, as I have no clue what else could have
> caused
> > the behavior I saw on the original system. I admit I didn't really think
> too
> > hard about why it would work this way, I just sort of accepted it and
> moved on.
> > Will investigate further and see if I can reproduce it again.
> >
> > Yours,
> > Will Eccles
> >
> > On Tue, Oct 19, 2021 at 4:25 PM Arnout Vandecappelle <arnout at mind.be
> > <mailto:arnout at mind.be>> wrote:
> >
> >        Hi Will,
> >
> >     On 18/10/2021 22:30, Will Eccles wrote:
> >      > S50sshd updates umask to 077, but does not reset it when it
> exits. This
> >      > results in the root user's umask being configured incorrectly
> (assuming
> >      > a default of 022 or otherwise).
> >
> >        Can you explain in which context this happens?
> >
> >        Normally this script is executed by /etc/init.d/rcS, which
> contains this
> >     code:
> >
> >            case "$i" in
> >               *.sh)
> >                   # Source shell script for speed.
> >                   (
> >                       trap - INT QUIT TSTP
> >                       set start
> >                       . $i
> >                   )
> >                   ;;
> >               *)
> >                   # No sh extension, so fork subprocess.
> >                   $i start
> >                   ;;
> >
> >        Since the script doesn't end with .sh, it will fork, so the umask
> doesn't
> >     "stick".
> >
> >        Same when you execute the script interactively: the umask isn't
> inherited by
> >     the parent shell.
> >
> >        And when you source the script, the trap doesn't even trigger at
> the end of
> >     the script, so this patch doesn't actually reset the umask.
> >
> >
> >        So I don't understand how it's possible that this patch fixes
> your problem.
> >
> >
> >        Regards,
> >        Arnout
> >
> >
> >      > This patch adds a trap to reset umask
> >      > when the script exits. This is convenient on systems where, for
> example,
> >      > configs such as /etc/profile may not be sourced by the root user.
> It may
> >      > also prevent issues with other init scripts which may inherit
> this umask
> >      > unintentionally, leading to improper permissions elsewhere in the
> >      > system.
> >      >
> >      > Signed-off-by: Will Eccles <will at eccles.dev <mailto:
> will at eccles.dev>>
> >      > ---
> >      > Backport to: 2021.02.6, 2021.08.1
> >      > (These are the releases on buildroot.org <http://buildroot.org>
> as of
> >     this writing, but as far
> >      > as I can tell, even releases as far back as 2012 have the same
> problem.)
> >      > ---
> >      >   package/openssh/S50sshd | 2 ++
> >      >   1 file changed, 2 insertions(+)
> >      >
> >      > diff --git a/package/openssh/S50sshd b/package/openssh/S50sshd
> >      > index 22da41d1ca..94cf4c14e8 100644
> >      > --- a/package/openssh/S50sshd
> >      > +++ b/package/openssh/S50sshd
> >      > @@ -6,6 +6,8 @@
> >      >   # Make sure the ssh-keygen progam exists
> >      >   [ -f /usr/bin/ssh-keygen ] || exit 0
> >      >
> >      > +# Reset uname at exit
> >      > +trap "uname $(uname)" EXIT
> >      >   umask 077
> >      >
> >      >   start() {
> >      >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.buildroot.org/pipermail/buildroot/attachments/20211020/4a2bc34d/attachment.html>