Message ID | 20211018203038.32453-1-will@eccles.dev |
---|---|
State | Rejected |
Headers | show |
Series | package/openssh: reset umask when init script exits | expand |
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@eccles.dev> > --- > 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(+) > > 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() { >
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). > 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@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@eccles.dev> > > --- > > 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(+) > > > > 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() { > > >
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@mind.be > <mailto:arnout@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@eccles.dev <mailto:will@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() { > > >
> 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@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@mind.be > > <mailto:arnout@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@eccles.dev <mailto: > will@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() { > > > > > >
Will, All, On 2021-10-18 16:30 -0400, Will Eccles spake thusly: > 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 <will@eccles.dev> Besides what Arnout said (and which I agree with), I am not so sure this patch is even technically correct... See below... > --- > 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(+) > > 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 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. > umask 077 > > start() { > -- > 2.33.1 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
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. On Thu, Oct 21, 2021 at 1:46 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > Will, All, > > On 2021-10-18 16:30 -0400, Will Eccles spake thusly: > > 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 <will@eccles.dev> > > Besides what Arnout said (and which I agree with), I am not so sure this > patch is even technically correct... See below... > > > --- > > 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(+) > > > > 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 > > 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. > > > umask 077 > > > > start() { > > -- > > 2.33.1 > > > > _______________________________________________ > > buildroot mailing list > > buildroot@buildroot.org > > 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. | > > '------------------------------^-------^------------------^--------------------' >
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() {
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 <will@eccles.dev> --- 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(+)