Module Name:    src
Committed By:   riastradh
Date:           Wed May  6 18:49:26 UTC 2020

Modified Files:
        src/etc/rc.d: random_seed
        src/sbin/rndctl: rndctl.8 rndctl.c

Log Message:
Tweak logic to decide whether a medium is safe for an rndseed.

- Teach rndctl to load the seed, but treat it as zero entropy, if the
  medium is read-only or if the update fails.

- Teach rndctl to accept `-i' flag instructing it to ignore the
  entropy estimate in the seed.

- Teach /etc/rc.d/random_seed to:
  (a) assume nonlocal file systems are unsafe, and use -i, but
  (b) assume / is safe, even if it is nonlocal.
  If the medium is nonwritable, leave it to rndctl to detect that.
  (Could use statvfs and check for ST_LOCAL in rndctl, I guess, but I
  already implemented it this way.)

Treating nonlocal / as safe is a compromise: it's up to the operator
to secure the network for (e.g.) nfs mounts, but that's true whether
we're talking entropy or not -- if the adversary has access to the
network that you've mounted / from, they can do a lot more damage
anyway; this reduces warning fatigue for diskless systems, e.g. test
racks.


To generate a diff of this commit:
cvs rdiff -u -r1.9 -r1.10 src/etc/rc.d/random_seed
cvs rdiff -u -r1.23 -r1.24 src/sbin/rndctl/rndctl.8
cvs rdiff -u -r1.33 -r1.34 src/sbin/rndctl/rndctl.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.

Modified files:

Index: src/etc/rc.d/random_seed
diff -u src/etc/rc.d/random_seed:1.9 src/etc/rc.d/random_seed:1.10
--- src/etc/rc.d/random_seed:1.9	Fri May  1 15:52:38 2020
+++ src/etc/rc.d/random_seed	Wed May  6 18:49:26 2020
@@ -1,6 +1,6 @@
 #!/bin/sh
 #
-# $NetBSD: random_seed,v 1.9 2020/05/01 15:52:38 riastradh Exp $
+# $NetBSD: random_seed,v 1.10 2020/05/06 18:49:26 riastradh Exp $
 #
 
 # PROVIDE: random_seed
@@ -29,43 +29,40 @@ message()
 	echo "${name}: ${random_file}: $@" 1>&2
 }
 
-getfstype() {
-	df -G "$1" | while read line; do
-		set -- $line
-		if [ "$2" = "fstype" ]; then
-			echo "$1"
-			return
-		fi
-	done
-}
-
 fs_safe()
 {
-	#
-	# Enforce that the file's on a local file system.
-	# Include only the types we can actually write.
-	#
-	fstype="$(getfstype "$1")"
-	case "${fstype}" in
-	ffs|lfs|ext2fs|msdos|v7fs|zfs)
-		return 0
+	# Consider the root file system safe always.
+	df -P "$1" | (while read dev total used avail cap mountpoint; do
+		case $mountpoint in
+		'Mounted on')	continue;;
+		/)		exit 0;;
+		*)		exit 1;;
+		esac
+	done) && return 0
+
+	# Otherwise, consider local file systems safe and non-local
+	# file systems unsafe.
+	case $(df -l "$1") in
+	*Warning:*)
+		return 1
 		;;
 	*)
-		message "Bad file system type ${fstype}"
-		return 1
+		return 0
 		;;
 	esac
 }
 
 random_load()
 {
+	local flags=
+
 	if [ ! -f "${random_file}" ]; then
 		message "Not present"
 		return
 	fi
 
 	if ! fs_safe "$(dirname "${random_file}")"; then
-		return 1
+		flags=-i
 	fi
 
 	set -- $(ls -ldn "${random_file}")
@@ -75,15 +72,15 @@ random_load()
 	# The file must be owned by root,
 	if [ "$st_uid" != "0" ]; then
 		message "Bad owner ${st_uid}"
-		return 1
+		flags=-i
 	fi
 	# and root read/write only.
 	if [ "$st_mode" != "-rw-------" ]; then
 		message "Bad mode ${st_mode}"
-		return 1
+		flags=-i
 	fi
 
-	if rndctl -L "${random_file}"; then
+	if rndctl $flags -L "${random_file}"; then
 		echo "Loaded entropy from ${random_file}."
 	fi
 }
@@ -93,11 +90,6 @@ random_save()
 	oum="$(umask)"
 	umask 077
 
-	if ! fs_safe "$(dirname "${random_file}")"; then
-		umask "${oum}"
-		return 1
-	fi
-
 	if rndctl -S "${random_file}"; then
 		echo "Saved entropy to ${random_file}."
 	fi

Index: src/sbin/rndctl/rndctl.8
diff -u src/sbin/rndctl/rndctl.8:1.23 src/sbin/rndctl/rndctl.8:1.24
--- src/sbin/rndctl/rndctl.8:1.23	Fri Dec  6 14:43:18 2019
+++ src/sbin/rndctl/rndctl.8	Wed May  6 18:49:26 2020
@@ -1,4 +1,4 @@
-.\"	$NetBSD: rndctl.8,v 1.23 2019/12/06 14:43:18 riastradh Exp $
+.\"	$NetBSD: rndctl.8,v 1.24 2020/05/06 18:49:26 riastradh Exp $
 .\"
 .\" Copyright (c) 1997 Michael Graff
 .\" All rights reserved.
@@ -76,6 +76,16 @@ but no entropy is assumed to be present.
 .It Fl e
 Enable entropy estimation using the collected timing information
 for the given device name or device type.
+.It Fl i
+With the
+.Fl L
+option to load a seed from a file, ignore any estimate in the file of
+the entropy of the seed.
+This still loads the data into the kernel, but won't unblock
+.Pa /dev/random
+even if the file claims to have adequate entropy.
+This is useful if the file is on a medium, such as an NFS share, that
+the operator does not know to be secret.
 .It Fl L
 Load saved entropy from file
 .Ar save-file

Index: src/sbin/rndctl/rndctl.c
diff -u src/sbin/rndctl/rndctl.c:1.33 src/sbin/rndctl/rndctl.c:1.34
--- src/sbin/rndctl/rndctl.c:1.33	Thu Apr 30 03:27:15 2020
+++ src/sbin/rndctl/rndctl.c	Wed May  6 18:49:26 2020
@@ -1,4 +1,4 @@
-/*	$NetBSD: rndctl.c,v 1.33 2020/04/30 03:27:15 riastradh Exp $	*/
+/*	$NetBSD: rndctl.c,v 1.34 2020/05/06 18:49:26 riastradh Exp $	*/
 
 /*-
  * Copyright (c) 1997 Michael Graff.
@@ -31,7 +31,7 @@
 
 #include <sys/cdefs.h>
 #ifndef lint
-__RCSID("$NetBSD: rndctl.c,v 1.33 2020/04/30 03:27:15 riastradh Exp $");
+__RCSID("$NetBSD: rndctl.c,v 1.34 2020/05/06 18:49:26 riastradh Exp $");
 #endif
 
 #include <sys/param.h>
@@ -78,6 +78,7 @@ static char * strflags(u_int32_t);
 static void do_list(int, u_int32_t, char *);
 static void do_stats(void);
 
+static int iflag;
 static int vflag;
 
 static void
@@ -88,7 +89,8 @@ usage(void)
 	    getprogname());
 	fprintf(stderr, "       %s [-lsv] [-d devname | -t devtype]\n",
 	    getprogname());
-	fprintf(stderr, "	%s -[L|S] save-file\n", getprogname());
+	fprintf(stderr, "	%s [-i] -L save-file\n", getprogname());
+	fprintf(stderr, "	%s -S save-file\n", getprogname());
 	exit(1);
 }
 
@@ -126,8 +128,8 @@ find_name(u_int32_t type)
 	return ("???");
 }
 
-static void
-do_save(const char *filename, const void *extra, size_t nextra,
+static int
+update_seed(const char *filename, const void *extra, size_t nextra,
     uint32_t extraentropy)
 {
 	char tmp[PATH_MAX];
@@ -143,23 +145,30 @@ do_save(const char *filename, const void
 	memset(&rs, 0, sizeof rs);
 
 	/* Format the temporary file name.  */
-	if (snprintf(tmp, sizeof tmp, "%s.tmp", filename) >= PATH_MAX)
-		errx(1, "path too long");
+	if (snprintf(tmp, sizeof tmp, "%s.tmp", filename) >= PATH_MAX) {
+		warnx("path too long");
+		return -1;
+	}
 
 	/* Open /dev/urandom.  */
-	if ((fd = open(_PATH_URANDOM, O_RDONLY)) == -1)
-		err(1, "device open");
+	if ((fd = open(_PATH_URANDOM, O_RDONLY)) == -1) {
+		warn("device open");
+		return -1;
+	}
 
 	/* Find how much entropy is in the pool.  */
-	if (ioctl(fd, RNDGETENTCNT, &systementropy) == -1)
-		err(1, "ioctl(RNDGETENTCNT)");
+	if (ioctl(fd, RNDGETENTCNT, &systementropy) == -1) {
+		warn("ioctl(RNDGETENTCNT)");
+		systementropy = 0;
+	}
 
 	/* Read some data from /dev/urandom.  */
 	if ((size_t)(nread = read(fd, buf, sizeof buf)) != sizeof buf) {
 		if (nread == -1)
-			err(1, "read");
+			warn("read");
 		else
-			errx(1, "truncated read");
+			warnx("truncated read");
+		return -1;
 	}
 
 	/* Close /dev/urandom; we're done with it.  */
@@ -212,30 +221,47 @@ do_save(const char *filename, const void
 	 * begin with in which case we're hosed either way, or we've
 	 * just revealed some output which is not a problem.
 	 */
-	if ((fd = open(tmp, O_CREAT|O_TRUNC|O_WRONLY, 0600)) == -1)
-		err(1, "open seed file to save");
+	if ((fd = open(tmp, O_CREAT|O_TRUNC|O_WRONLY, 0600)) == -1) {
+		warn("open seed file to save");
+		return -1;
+	}
 	if ((size_t)(nwrit = write(fd, &rs, sizeof rs)) != sizeof rs) {
 		int error = errno;
 		if (unlink(tmp) == -1)
 			warn("unlink");
 		if (nwrit == -1)
-			errc(1, error, "write");
+			warnc(error, "write");
 		else
-			errx(1, "truncated write");
+			warnx("truncated write");
+		return -1;
 	}
 	explicit_memset(&rs, 0, sizeof rs); /* paranoia */
 	if (fsync_range(fd, FDATASYNC|FDISKSYNC, 0, 0) == -1) {
 		int error = errno;
 		if (unlink(tmp) == -1)
 			warn("unlink");
-		errc(1, error, "fsync_range");
+		warnc(error, "fsync_range");
+		return -1;
 	}
 	if (close(fd) == -1)
 		warn("close");
 
 	/* Rename it over the original file to commit.  */
-	if (rename(tmp, filename) == -1)
-		err(1, "rename");
+	if (rename(tmp, filename) == -1) {
+		warn("rename");
+		return -1;
+	}
+
+	/* Success!  */
+	return 0;
+}
+
+static void
+do_save(const char *filename)
+{
+
+	if (update_seed(filename, NULL, 0, 0) == -1)
+		exit(1);
 }
 
 static void
@@ -248,42 +274,52 @@ do_load(const char *filename)
 	ssize_t nread, nwrit;
 	SHA1_CTX s;
 	uint8_t digest[SHA1_DIGEST_LENGTH];
+	int ro = 0, fail = 0;
+	int error;
 
 	/*
 	 * The order of operations is important here:
 	 *
 	 * 1. Load the old seed.
-	 * 2. Feed the old seed into the kernel.
-	 * 3. Generate and write a new seed.
-	 * 4. Erase the old seed.
+	 * 2. Generate and write a new seed.
+	 *    => If writing the new seed fails, assume the medium was
+	 *       read-only and we might be reading the same thing on
+	 *       multiple boots, so treat the entropy estimate as zero.
+	 * 3. Feed the old seed into the kernel.
+	 * 4. Erase the old seed if we can.
 	 *
-	 * This follows the procedure in
+	 * We used to follow the similar procedure in
 	 *
 	 *	Niels Ferguson, Bruce Schneier, and Tadayoshi Kohno,
 	 *	_Cryptography Engineering_, Wiley, 2010, Sec. 9.6.2
-	 *	`Update Seed File'.
+	 *	`Update Seed File',
 	 *
-	 * There is a race condition: If another process generates a
-	 * key from /dev/urandom after step (2) but before step (3),
-	 * and if the machine crashes before step (3), an adversary who
-	 * can read the disk after the crash can probably guess the
-	 * complete state of the entropy pool and thereby predict the
-	 * key.
+	 * which is different mainly in that it does step (3) before
+	 * step (2).  We don't need to feed the old seed into the
+	 * kernel first, because we hash the old seed together with
+	 * /dev/urandom output.
 	 *
-	 * There's not much we can do here without some kind of
-	 * systemwide lock on /dev/urandom and without introducing an
-	 * opportunity for a crash to wipe out the entropy altogether.
-	 * To avoid this race, you should ensure that any key
-	 * generation happens _after_ `rndctl -L' has completed.
+	 * Writing the new seed to disk before feeding the old seed to
+	 * the kernel lets us notice if the disk is read-only, and if
+	 * so, zero the estimate of entropy in case we're getting the
+	 * same seed each time we boot.
 	 */
 
 	/* Format the temporary file name.  */
 	if (snprintf(tmp, sizeof tmp, "%s.tmp", filename) >= PATH_MAX)
 		errx(1, "path too long");
 
-	/* 1. Load the old seed.  */
-	if ((fd_seed = open(filename, O_RDWR)) == -1)
-		err(1, "open seed file to load");
+	/*
+	 * 1. Load the old seed.
+	 */
+	if ((fd_seed = open(filename, O_RDWR)) == -1) {
+		error = errno;
+		if ((error != EPERM && error != EROFS) ||
+		    (fd_seed = open(filename, O_RDONLY)) == -1)
+			err(1, "open seed file to load");
+		warnc(error, "seed file is nonwritable");
+		ro = 1;
+	}
 	if ((size_t)(nread = read(fd_seed, &rs, sizeof rs)) != sizeof rs) {
 		if (nread == -1)
 			err(1, "read seed");
@@ -318,46 +354,61 @@ do_load(const char *filename)
 			rs.entropy = 0;
 	}
 
-	/* Format the ioctl request.  */
+	/*
+	 * If the user asked or if the medium can't be updated, zero
+	 * the entropy estimate.
+	 */
+	if (iflag || ro)
+		rs.entropy = 0;
+
+	/*
+	 * 2. Generate and write a new seed.  If anything goes wrong,
+	 * assume the medium is read-only and treat it as zero entropy.
+	 */
+	if (update_seed(filename, rs.data, sizeof(rs.data), rs.entropy) == -1)
+		rs.entropy = 0;
+
+	/* Decide whether to fail at the end if there's no entropy.  */
+	fail = (rs.entropy == 0);
+
+	/*
+	 * 3. Feed the old seed into the kernel.
+	 */
 	rd.len = MIN(sizeof(rd.data), sizeof(rs.data));
 	rd.entropy = rs.entropy;
 	memcpy(rd.data, rs.data, rd.len);
 	explicit_memset(&rs, 0, sizeof rs); /* paranoia */
-
-	/* 2. Feed the old seed into the kernel.  */
 	if ((fd_random = open(_PATH_URANDOM, O_WRONLY)) == -1)
 		err(1, "open /dev/urandom");
 	if (ioctl(fd_random, RNDADDDATA, &rd) == -1)
 		err(1, "RNDADDDATA");
+	explicit_memset(&rd, 0, sizeof rd); /* paranoia */
 	if (close(fd_random) == -1)
 		warn("close /dev/urandom");
 	fd_random = -1;		/* paranoia */
 
 	/*
-	 * 3. Generate and write a new seed.  Note that we hash the old
-	 * seed together with whatever /dev/urandom returns in do_save.
-	 * Why?  After RNDADDDATA, the input may not be distributed
-	 * immediately to /dev/urandom.
-	 */
-	do_save(filename, rd.data, rd.len, rd.entropy);
-	explicit_memset(&rd, 0, sizeof rd); /* paranoia */
-
-	/*
 	 * 4. Erase the old seed.  Only effective if we're on a
 	 * fixed-address file system like ffs -- doesn't help to erase
 	 * the data on lfs, but doesn't hurt either.  No need to unlink
-	 * because do_save will have already overwritten it.
+	 * because do_save will have already renamed over it.
 	 */
-	memset(&rs, 0, sizeof rs);
-	if ((size_t)(nwrit = pwrite(fd_seed, &rs, sizeof rs, 0)) !=
-	    sizeof rs) {
-		if (nwrit == -1)
-			err(1, "overwrite old seed");
-		else
-			errx(1, "truncated overwrite");
+	if (!ro) {
+		memset(&rs, 0, sizeof rs);
+		if ((size_t)(nwrit = pwrite(fd_seed, &rs, sizeof rs, 0)) !=
+		    sizeof rs) {
+			if (nwrit == -1)
+				err(1, "overwrite old seed");
+			else
+				errx(1, "truncated overwrite");
+		}
+		if (fsync_range(fd_seed, FDATASYNC|FDISKSYNC, 0, 0) == -1)
+			err(1, "fsync_range");
 	}
-	if (fsync_range(fd_seed, FDATASYNC|FDISKSYNC, 0, 0) == -1)
-		err(1, "fsync_range");
+
+	/* Fail noisily if there's no entropy.  */
+	if (fail)
+		errx(1, "no entropy");
 }
 
 static void
@@ -535,7 +586,7 @@ main(int argc, char **argv)
 	sflag = 0;
 	type = 0xff;
 
-	while ((ch = getopt(argc, argv, "CES:L:celt:d:sv")) != -1) {
+	while ((ch = getopt(argc, argv, "CES:L:celit:d:sv")) != -1) {
 		switch (ch) {
 		case 'C':
 			rctl.flags |= RND_FLAG_NO_COLLECT;
@@ -569,6 +620,9 @@ main(int argc, char **argv)
 			rctl.mask |= RND_FLAG_NO_ESTIMATE;
 			mflag++;
 			break;
+		case 'i':
+			iflag = 1;
+			break;
 		case 'l':
 			lflag++;
 			break;
@@ -608,10 +662,16 @@ main(int argc, char **argv)
 		usage();
 
 	/*
+	 * -i makes sense only with -L.
+	 */
+	if (iflag && cmd != 'L')
+		usage();
+
+	/*
 	 * Save.
 	 */
 	if (cmd == 'S') {
-		do_save(filename, NULL, 0, 0);
+		do_save(filename);
 		exit(0);
 	}
 

Reply via email to