Re: Fix /etc/rc.d/random umask handling (/entropy permissions)

[Jilles Tjoelker]

On Mon, Jan 23, 2017 at 10:52:21AM -0800, Simon J. Gerraty wrote:
> Jilles Tjoelker  wrote:
> > Index: etc/rc.d/random
> > ===
> > --- etc/rc.d/random (revision 311446)
> > +++ etc/rc.d/random (working copy)
> > @@ -20,12 +20,14 @@
> >  
> >  save_dev_random()
> >  {
> > +   oumask=`umask`

> why not simply use a sub-shell to tighten umask

> (umask 077; what-ever)

With our /bin/sh, the save-restore method saves a fork. A command
substitution with a single umask command does not fork, while a subshell
containing umask and something else does.

The effect is fairly minor, but good performance is often the product of
many small optimizations.

-- 
Jilles Tjoelker
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: Fix /etc/rc.d/random umask handling (/entropy permissions)

[Simon J. Gerraty]

Jilles Tjoelker  wrote:
> Index: etc/rc.d/random
> ===
> --- etc/rc.d/random   (revision 311446)
> +++ etc/rc.d/random   (working copy)
> @@ -20,12 +20,14 @@
>  
>  save_dev_random()
>  {
> + oumask=`umask`

why not simply use a sub-shell to tighten umask

(umask 077; what-ever)

___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: Fix /etc/rc.d/random umask handling (/entropy permissions)

[Jilles Tjoelker]

On Sun, Jan 22, 2017 at 01:22:07AM +, Lu Tung-Pin wrote:
> On 2017-01-21 22:01, Jilles Tjoelker wrote:
> > [Adding Cc: Dag-Erling Smørgrav who committed r273957 which seems to
> > have introduced this]
> > On Sat, Jan 21, 2017 at 01:21:42AM +, Lu Tung-Pin wrote:
> >> A 2014 change broke the umask handling in /etc/rc.d/random,
> >> leaving /entropy with ug+r permissions. Quick fix attached,

> Edit: go+r permissions.

> > Switching the umask here will avoid incorrect permissions on
> > /entropy on new installations, but will not fix existing systems. A
> > chmod command may be useful here.

> Note that random_start() first removes /entropy via feed_dev_random().
> There's also a removal in random_stop(). Provided that a removal occurs,
> the chmod won't be necessary on machines with an existing go+r /entropy.

Right, /entropy is deleted after being read so the chmod is not needed.

> I'm wondering, though: Would it be better to replace all the umask
> fiddling with simple chmods? Every other rc.d script uses chmod if it
> needs to set tighter permissions. When umask is used (dmesg, mountd,
> syslogd), it's with a relaxed 022 setting.

The umask ensures the file is created with the correct permissions so
there is no race window where an unprivileged process can open the file.
A permissions change has no existing opens.

-- 
Jilles Tjoelker
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: Fix /etc/rc.d/random umask handling (/entropy permissions)

[Lu Tung-Pin]

On 2017-01-21 22:01, Jilles Tjoelker wrote:

[Adding Cc: Dag-Erling Smørgrav who committed r273957 which seems to
have introduced this]
On Sat, Jan 21, 2017 at 01:21:42AM +, Lu Tung-Pin wrote:

A 2014 change broke the umask handling in /etc/rc.d/random,
leaving /entropy with ug+r permissions. Quick fix attached,



Edit: go+r permissions.

Switching the umask here will avoid incorrect permissions on /entropy 
on

new installations, but will not fix existing systems. A chmod command
may be useful here.


Note that random_start() first removes /entropy via feed_dev_random().
There's also a removal in random_stop(). Provided that a removal occurs,
the chmod won't be necessary on machines with an existing go+r /entropy.

I'm wondering, though: Would it be better to replace all the umask
fiddling with simple chmods? Every other rc.d script uses chmod if it
needs to set tighter permissions. When umask is used (dmesg, mountd,
syslogd), it's with a relaxed 022 setting.
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"

Re: Fix /etc/rc.d/random umask handling (/entropy permissions)

[Jilles Tjoelker]

[Adding Cc: Dag-Erling Smørgrav who committed r273957 which seems to
have introduced this]
On Sat, Jan 21, 2017 at 01:21:42AM +, Lu Tung-Pin wrote:
> A 2014 change broke the umask handling in /etc/rc.d/random,
> leaving /entropy with ug+r permissions. Quick fix attached,
> mirroring random_stop() behavior.

> (Incidentally, /usr/libexec/save-entropy is still fine for
> /var/db/entropy/*, as is /etc/rc.d/random for the new
> /boot/entropy.)

> --- /etc/rc.d/random.old  2017-01-21 11:48:30.975009000 +1100
> +++ /etc/rc.d/random  2017-01-19 18:04:34.224632000 +1100
> @@ -20,12 +20,15 @@
>  
>  save_dev_random()
>  {
> + oumask=`umask`
> + umask 077
>   for f ; do
>   if :>>"$f" ; then
>   debug "saving entropy to $f"
>   dd if=/dev/random of="$f" bs=4096 count=1 2>/dev/null
>   fi
>   done
> + umask ${oumask}
>  }
>  
>  feed_dev_random()

Switching the umask here will avoid incorrect permissions on /entropy on
new installations, but will not fix existing systems. A chmod command
may be useful here.

On another note,  if :>>"$f"  is bogus. Since : is a special builtin, a
redirection error causes the shell to abort the script. The conditional
seems to have been added to show error messages when the entropy file
cannot be written without showing dd's statistics. I think this can be
done more easily using dd's status=none parameter.

My revised patch is below:

Index: etc/rc.d/random
===
--- etc/rc.d/random (revision 311446)
+++ etc/rc.d/random (working copy)
@@ -20,12 +20,14 @@
 
 save_dev_random()
 {
+   oumask=`umask`
+   umask 077
for f ; do
-   if :>>"$f" ; then
-   debug "saving entropy to $f"
-   dd if=/dev/random of="$f" bs=4096 count=1 2>/dev/null
-   fi
+   debug "saving entropy to $f"
+   dd if=/dev/random of="$f" bs=4096 count=1 status=none &&
+   chmod 600 "$f"
done
+   umask ${oumask}
 }
 
 feed_dev_random()

-- 
Jilles Tjoelker
___
freebsd-current@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "freebsd-current-unsubscr...@freebsd.org"