[systemd-devel] [PATCH] cryptsetup: Do not warn If the key is /dev/*random

[Lennart Poettering]

On Tue, 03.02.15 14:18, Josh Triplett (josh at joshtriplett.org) wrote:

> On Mon, Feb 02, 2015 at 04:42:21PM +0100, Lennart Poettering wrote:
> > On Mon, 02.02.15 12:06, Cristian Rodríguez (crrodriguez at opensuse.org) wrote:
> > 
> > > Using /dev/urandom as a key is valid for swap, do not
> > > warn if this devices are world readable.
> > > ---
> > >  src/cryptsetup/cryptsetup.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
> > > index e6b37ac..38930ae 100644
> > > --- a/src/cryptsetup/cryptsetup.c
> > > +++ b/src/cryptsetup/cryptsetup.c
> > > @@ -624,8 +624,10 @@ int main(int argc, char *argv[]) {
> > >  
> > >                          /* Ideally we'd do this on the open fd, but since this is just a
> > >                           * warning it's OK to do this in two steps. */
> > > -                        if (stat(key_file, &st) >= 0 && (st.st_mode & 0005))
> > > -                                log_warning("Key file %s is world-readable. This is not a good idea!", key_file);
> > > +                        if (stat(key_file, &st) >= 0 && (st.st_mode & 0005)) {
> > > +                                if(!STR_IN_SET(key_file, "/dev/urandom", "/dev/random", "/dev/hw_random"))
> > > +                                    log_warning("Key file %s is world-readable. This is not a good idea!", key_file);
> > > +                        }
> > 
> > I'd prefer if we'd change the check instead to only apply to
> > S_ISREG() files. This way we wouldn't have to list all RNG device
> > nodes.
> 
> With the exception of /dev/*random, you don't want a world-readable
> device used for a key either.  Some people have setups that use a USB
> device (e.g. /dev/sd* or /dev/disk/by-*/*) as a keyfile, and in that
> case, the file should *not* be world-readable.

Strictly speaking that is true. But this is really just a safety net
(and only a warning) for files people create, where they forgot to set
the right perms. However, device nodes are not created by people, but
by devtmpfs and adjusted by udev, and hence are very unlikely to be
incorrect. I mean, there's little point to protect systemd-cryptsetup
from systemd-udevd, if you follow what I mean. 

I really prefer not having to include the whitelist of devices here...

Lennart

-- 
Lennart Poettering, Red Hat