Skip to contentSkip to conversation

package/openssh: reset umask when init script exits

PATCH In review Will Eccles 3 participants 7 messages Oct 18, 2021 – Oct 21, 2021

lore
Share & tools
lore

Will Eccles

author
#0 lore raw

PATCH 1/1 package/openssh: reset umask when init script exits

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). 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 <[email protected]>

---
Backport to: 2021.02.6, 2021.08.1
(These are the releases on 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(+)
package/openssh/S50sshd +2 -0
index 22da41d1ca..94cf4c14e8 100644
@@ -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() {
Signature · 6 lines
2.33.1

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot
#1 lore raw

Hi Will,

Quoted 3 lines · Will Eccles

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

Quoted 22 lines

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 <[email protected]>

---
Backport to: 2021.02.6, 2021.08.1
(These are the releases on 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(+)
package/openssh/S50sshd +0 -0
index 22da41d1ca..94cf4c14e8 100644

package/openssh/S50sshd line 13

Quoted 8 lines
package/openssh/S50sshd +2 -0
@@ -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() {

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot

author
#2 lore raw

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)

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).

Quoted 1 line

And when you source the script, the trap doesn't even trigger at the end

of

Quoted 1 line

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

Quoted 70 lines · Arnout Vandecappelle

Hi Will,

Quoted 3 lines · Will Eccles

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

Quoted 24 lines

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 <[email protected]>

---
Backport to: 2021.02.6, 2021.08.1
(These are the releases on 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(+)
package/openssh/S50sshd +0 -0
index 22da41d1ca..94cf4c14e8 100644
@@ -6,6 +6,8 @@
  # Make sure the ssh-keygen progam exists
  [ -f /usr/bin/ssh-keygen ] || exit 0
Quoted 7 lines

package/openssh/S50sshd line 13

Quoted 6 lines
package/openssh/S50sshd +2 -0
+# Reset uname at exit
+trap "uname $(uname)" EXIT
  umask 077
 
  start() {

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot

#3 lore raw
Quoted 9 lines · Will Eccles

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

Quoted 80 lines

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).

Quoted 2 lines

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 <[email protected] <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>> > ---
> 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

Quoted 9 lines

>   [ -f /usr/bin/ssh-keygen ] || exit 0
>
> +# Reset uname at exit
> +trap "uname $(uname)" EXIT
>   umask 077
>
>   start() {
>

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot

author
#4 lore raw
Quoted 2 lines

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.

Quoted 4 lines

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

Quoted 147 lines · Arnout Vandecappelle
Quoted 3 lines · Will Eccles

Hi Arnout,

Interestingly, I have tested this in a few different ways and achieved

different

Quoted 1 line

results each time. I agree with you that the patch does not make much

sense, but

Quoted 1 line

on the system I wrote it on (or rather, the system I originally tested

the fix

Quoted 1 line

on, which was otherwise unmodified), it seemed to fix the problem. The

exact

Quoted 4 lines

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

Quoted 2 lines

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

Quoted 1 line

that I'm aware of; I was logged in for all of 1 minute to make the

change and

Quoted 2 lines

reboot)
4. Login as root again and test that the uname value was now 022 (as I

expected

Quoted 2 lines

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

Quoted 1 line

I have no idea how this patch would fix the issue, and I agree that it

makes no

Quoted 1 line

sense, but even in a small test I had done on the host system I had

achieved

Quoted 1 line

results which appeared to agree with this patch. On a new system (built

minutes

Quoted 1 line

ago), I cannot reproduce the exact same steps I took above, which is

quite

Quoted 1 line

puzzling, as nothing has changed (aside from a single device tree

overlay I

Quoted 3 lines

added, which should have no relevance here at all).

Quoted 1 line

And when you source the script, the trap doesn't even trigger at the

end of

Quoted 3 lines
Quoted 1 line

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

Quoted 1 line

even consider this. The file shouldn't ever be sourced as it doesn't end

in .sh

Quoted 3 lines

anyway.

I am willing to accept that I had some sort of gremlin on the original

system I

Quoted 1 line

tested this on. I can't seem to reproduce it on a newly-generated image

while

Quoted 1 line

following precisely the same steps I did above. However, this only

raises more

Quoted 1 line

questions for me to investigate, as I have no clue what else could have

caused

Quoted 1 line

the behavior I saw on the original system. I admit I didn't really think

too

Quoted 1 line

hard about why it would work this way, I just sort of accepted it and

moved on.

Quoted 5 lines

Will investigate further and see if I can reproduce it again.

Yours,
Will Eccles

Quoted 7 lines

On Tue, Oct 19, 2021 at 4:25 PM Arnout Vandecappelle <[email protected] <mailto:[email protected]>> 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

Quoted 1 line

> results in the root user's umask being configured incorrectly

(assuming

Quoted 5 lines

> 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

Quoted 9 lines
    code:

           case "$i" in
              *.sh)
                  # Source shell script for speed.
                  (
                      trap - INT QUIT TSTP
                      set start
                      . $i
Quoted 8 lines
                  )
                  ;;
              *)
                  # No sh extension, so fork subprocess.
                  $i start
                  ;;

       Since the script doesn't end with .sh, it will fork, so the umask

doesn't

Quoted 3 lines

"stick".

Same when you execute the script interactively: the umask isn't

inherited by

Quoted 3 lines

the parent shell.

And when you source the script, the trap doesn't even trigger at

the end of

Quoted 4 lines

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.

Quoted 8 lines

Regards,
Arnout

> This patch adds a trap to reset umask
> when the script exits. This is convenient on systems where, for

example,

Quoted 1 line

> configs such as /etc/profile may not be sourced by the root user.

It may

Quoted 1 line

> also prevent issues with other init scripts which may inherit

this umask

Quoted 4 lines

> unintentionally, leading to improper permissions elsewhere in the > system.
>
> Signed-off-by: Will Eccles <[email protected] <mailto:

Quoted 28 lines

[email protected]>>

Quoted 3 lines
     > ---
     > Backport to: 2021.02.6, 2021.08.1
     > (These are the releases on buildroot.org <http://buildroot.org>

as of

Quoted 2 lines

this writing, but as far
> as I can tell, even releases as far back as 2012 have the same

problem.)

Quoted 19 lines
     > ---
     >   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() {
     >

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot

#5 lore raw
In reply to Will Eccles

Will, All,

On 2021-10-18 16:30 -0400, Will Eccles spake thusly:

Quoted 10 lines

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). 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 <[email protected]>

Besides what Arnout said (and which I agree with), I am not so sure this patch is even technically correct... See below...

Quoted 13 lines

---
Backport to: 2021.02.6, 2021.08.1
(These are the releases on 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(+)
package/openssh/S50sshd +0 -0
index 22da41d1ca..94cf4c14e8 100644

package/openssh/S50sshd line 10

Quoted 5 lines
package/openssh/S50sshd +2 -0
@@ -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

This does not even do what you said it does. This is 'uname', not 'umask'...

Furthermore, the above code would fail anyway:

    $ uname $(uname); echo $?
    uname: extra operand ‘Linux’
    Try 'uname --help' for more information.
    1

So, if this very patch makes it work for you, then your issue is not about umask being set below...

Regards,
Yann E. MORIN.

Quoted 10 lines
package/openssh/S50sshd +0 -0
 umask 077
 
 start() {
Signature · 6 lines
2.33.1

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot
Signature · 10 lines
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot
author
#6 lore raw

Hi Yann,

Apologies, I have no idea how that typo even made it into the patch. That is supposed to say "umask $(umask)". Not only did the typo make it in there, but I haven't even noticed it until now (and the patch I applied to my own says umask, so apparently I typo'd when formalizing it). In any case, it's hardly worth correcting now, since the patch isn't useful.

Quoted 75 lines · Yann E. MORIN

Will, All,

On 2021-10-18 16:30 -0400, Will Eccles spake thusly:

Quoted 10 lines

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). 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 <[email protected]>

Besides what Arnout said (and which I agree with), I am not so sure this patch is even technically correct... See below...

Quoted 13 lines

---
Backport to: 2021.02.6, 2021.08.1
(These are the releases on 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(+)
package/openssh/S50sshd +0 -0
index 22da41d1ca..94cf4c14e8 100644

package/openssh/S50sshd line 10

Quoted 5 lines
package/openssh/S50sshd +2 -0
@@ -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

This does not even do what you said it does. This is 'uname', not 'umask'...

Furthermore, the above code would fail anyway:

    $ uname $(uname); echo $?
    uname: extra operand ‘Linux’
    Try 'uname --help' for more information.
    1

So, if this very patch makes it work for you, then your issue is not about umask being set below...

Regards,
Yann E. MORIN.

Quoted 10 lines
package/openssh/S50sshd +0 -1
 umask 077
 
 start() {
--

2.33.1

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot

--

.-----------------.--------------------.------------------.--------------------.

|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics'
conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___
     |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is
no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v
 conspiracy.  |
Quoted 2 lines

'------------------------------^-------^------------------^--------------------'

_______________________________________________
buildroot mailing list
[email protected]
https://lists.buildroot.org/mailman/listinfo/buildroot