[PATCH 00/12] pull: race fixes

[PATCH 00/12] pull: race fixes

[Sami Kerola]

Hello,

I had a look of all chmod(3), chown(3), and stat(3) calls in executables if
there are file operation, preceeded by open(3) in unsafe manner.  The
changes in this pull request attempsts to correct these TACTOU cases.

The pull request has also few improvements, such as checking block device
correct way when ever opening them to write in them and replacements stat(3)
with access(3) when applicable.


----------------------------------------------------------------
The following changes since commit 4419ffb9eff5801fdbd385a4a6199b3877f802ad:
  lib/sysfs: make sysfs_get_devname()/blkid_devno_to_wholedisk() more robust (2015-10-30 11:08:32 +0100)
are available in the git repository at:
  git://github.com/kerolasa/lelux-utiliteetit.git races
for you to fetch changes up to 09acbb45083e37f112f3d0558f4875f94db457d4:
  fdformat: fix block device open race (2015-10-31 19:00:09 +0000)
----------------------------------------------------------------

Sami Kerola (12):
  login: fix stat(2) race
  swapon: fix stat(3) and open(3) race
  fsck: retire stat(3) when access(3) does better job
  agetty: fix chown(3), chmod(3) and open(3) race
  script, hwclock: check file exist with access(3) rather than stat(3)
  lib/fileutils: add is_same_inode() check
  include/blkdev: add open_blkdev_or_file() function
  mkfs.bfs: fix block device open race
  mkfs.minix: fix block device open race
  mkswap: fix block device open race
  mesg: fix multiple races
  fdformat: fix block device open race

 disk-utils/fdformat.c   | 11 ++++-------
 disk-utils/fsck.c       |  3 +--
 disk-utils/mkfs.bfs.c   |  2 +-
 disk-utils/mkfs.minix.c |  9 +++------
 disk-utils/mkswap.c     | 25 ++++++++-----------------
 include/blkdev.h        |  3 +++
 include/fileutils.h     | 14 ++++++++++++++
 lib/blkdev.c            | 27 +++++++++++++++++++++++++++
 login-utils/login.c     |  6 ++----
 sys-utils/hwclock.c     |  5 +----
 sys-utils/swapon.c      | 14 +++++++-------
 term-utils/agetty.c     | 10 +++++-----
 term-utils/mesg.c       | 25 +++++++++++++++++--------
 term-utils/script.c     |  3 +--
 14 files changed, 94 insertions(+), 63 deletions(-)

-- 
2.6.2

[PATCH 01/12] login: fix stat(2) race

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 login-utils/login.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/login-utils/login.c b/login-utils/login.c
index 0e45de7..b708467 100644
--- a/login-utils/login.c
+++ b/login-utils/login.c
@@ -256,13 +256,11 @@ static void motd(void)
 		struct stat st;
 		int fd;
 
-		if (stat(motdfile, &st) || !st.st_size)
-			continue;
 		fd = open(motdfile, O_RDONLY, 0);
 		if (fd < 0)
 			continue;
-
-		sendfile(fileno(stdout), fd, NULL, st.st_size);
+		if (!fstat(fd, &st) && st.st_size)
+			sendfile(fileno(stdout), fd, NULL, st.st_size);
 		close(fd);
 	}
 
-- 
2.6.2

[PATCH 02/12] swapon: fix stat(3) and open(3) race

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/swapon.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/sys-utils/swapon.c b/sys-utils/swapon.c
index 32a3107..c4d1544 100644
--- a/sys-utils/swapon.c
+++ b/sys-utils/swapon.c
@@ -475,7 +475,13 @@ static int swapon_checks(const char *special)
 	unsigned long long devsize = 0;
 	int permMask;
 
-	if (stat(special, &st) < 0) {
+	fd = open(special, O_RDONLY);
+	if (fd == -1) {
+		warn(_("cannot open %s"), special);
+		goto err;
+	}
+
+	if (fstat(fd, &st) < 0) {
 		warn(_("stat of %s failed"), special);
 		goto err;
 	}
@@ -500,12 +506,6 @@ static int swapon_checks(const char *special)
 		devsize = st.st_size;
 	}
 
-	fd = open(special, O_RDONLY);
-	if (fd == -1) {
-		warn(_("cannot open %s"), special);
-		goto err;
-	}
-
 	if (S_ISBLK(st.st_mode) && blkdev_get_size(fd, &devsize)) {
 		warnx(_("%s: get size failed"), special);
 		goto err;
-- 
2.6.2

[PATCH 03/12] fsck: retire stat(3) when access(3) does better job

[Sami Kerola]

The stat(3) and access(3) are in this case almost interchangeable, so choose
the lightweight function with additional advantage checking the file is
executable.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/fsck.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/disk-utils/fsck.c b/disk-utils/fsck.c
index ff504aa..5be2b44 100644
--- a/disk-utils/fsck.c
+++ b/disk-utils/fsck.c
@@ -547,14 +547,13 @@ static char *find_fsck(const char *type)
 	const char *tpl;
 	static char prog[256];
 	char *p = xstrdup(fsck_path);
-	struct stat st;
 
 	/* Are we looking for a program or just a type? */
 	tpl = (strncmp(type, "fsck.", 5) ? "%s/fsck.%s" : "%s/%s");
 
 	for(s = strtok(p, ":"); s; s = strtok(NULL, ":")) {
 		sprintf(prog, tpl, s, type);
-		if (stat(prog, &st) == 0)
+		if (access(prog, X_OK) == 0)
 			break;
 	}
 	free(p);
-- 
2.6.2

[PATCH 04/12] agetty: fix chown(3), chmod(3) and open(3) race

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/agetty.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/term-utils/agetty.c b/term-utils/agetty.c
index 0237b79..a9c8785 100644
--- a/term-utils/agetty.c
+++ b/term-utils/agetty.c
@@ -1004,23 +1004,23 @@ static void open_tty(char *tty, struct termios *tp, struct options *op)
 		     (int)sizeof(buf)) || (len < 0))
 			log_err(_("/dev/%s: cannot open as standard input: %m"), tty);
 
+		/* Open the tty as standard input. */
+		if ((fd = open(buf, O_RDWR|O_NOCTTY|O_NONBLOCK, 0)) < 0)
+			log_err(_("/dev/%s: cannot open as standard input: %m"), tty);
+
 		/*
 		 * There is always a race between this reset and the call to
 		 * vhangup() that s.o. can use to get access to your tty.
 		 * Linux login(1) will change tty permissions. Use root owner and group
 		 * with permission -rw------- for the period between getty and login.
 		 */
-		if (chown(buf, 0, gid) || chmod(buf, (gid ? 0620 : 0600))) {
+		if (fchown(fd, 0, gid) || fchmod(fd, (gid ? 0620 : 0600))) {
 			if (errno == EROFS)
 				log_warn("%s: %m", buf);
 			else
 				log_err("%s: %m", buf);
 		}
 
-		/* Open the tty as standard input. */
-		if ((fd = open(buf, O_RDWR|O_NOCTTY|O_NONBLOCK, 0)) < 0)
-			log_err(_("/dev/%s: cannot open as standard input: %m"), tty);
-
 		/* Sanity checks... */
 		if (fstat(fd, &st) < 0)
 			log_err("%s: %m", buf);
-- 
2.6.2

[PATCH 05/12] script, hwclock: check file exist with access(3) rather than stat(3)

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 sys-utils/hwclock.c | 5 +----
 term-utils/script.c | 3 +--
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
index 4b201d7..0d11061 100644
--- a/sys-utils/hwclock.c
+++ b/sys-utils/hwclock.c
@@ -258,15 +258,12 @@ hw_clock_is_utc(const bool utc, const bool local_opt,
 static int read_adjtime(struct adjtime *adjtime_p)
 {
 	FILE *adjfile;
-	int rc;			/* local return code */
-	struct stat statbuf;	/* We don't even use the contents of this. */
 	char line1[81];		/* String: first line of adjtime file */
 	char line2[81];		/* String: second line of adjtime file */
 	char line3[81];		/* String: third line of adjtime file */
 	long timeval;
 
-	rc = stat(adj_file_name, &statbuf);
-	if (rc < 0 && errno == ENOENT) {
+	if (access(adj_file_name, R_OK) != 0) {
 		/* He doesn't have a adjtime file, so we'll use defaults. */
 		adjtime_p->drift_factor = 0;
 		adjtime_p->last_adj_time = 0;
diff --git a/term-utils/script.c b/term-utils/script.c
index b6227c0..e247c83 100644
--- a/term-utils/script.c
+++ b/term-utils/script.c
@@ -609,7 +609,6 @@ static void getmaster(struct script_control *ctl)
 	}
 #else
 	char *pty, *bank, *cp;
-	struct stat stb;
 
 	ctl->isterm = isatty(STDIN_FILENO);
 
@@ -617,7 +616,7 @@ static void getmaster(struct script_control *ctl)
 	for (bank = "pqrs"; *bank; bank++) {
 		ctl->line[strlen("/dev/pty")] = *bank;
 		*pty = '0';
-		if (stat(ctl->line, &stb) < 0)
+		if (access(ctl->line, F_OK) != 0)
 			break;
 		for (cp = "0123456789abcdef"; *cp; cp++) {
 			*pty = *cp;
-- 
2.6.2

[PATCH 06/12] lib/fileutils: add is_same_inode() check

[Sami Kerola]

Check if a file path and descriptor are indeed the same file.  This function
is needed for TACTOU avoidance.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/fileutils.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/fileutils.h b/include/fileutils.h
index be6d1f7..55df1a7 100644
--- a/include/fileutils.h
+++ b/include/fileutils.h
@@ -4,6 +4,7 @@
 #include <stdio.h>
 #include <fcntl.h>
 #include <unistd.h>
+#include <sys/stat.h>
 
 #include "c.h"
 
@@ -25,6 +26,19 @@ static inline FILE *xfmkstemp(char **tmpname, const char *dir, const char *prefi
 	return ret;
 }
 
+static inline int is_same_inode(const int fd, const char *path)
+{
+	struct stat f, p;
+
+	if (fstat(fd, &f) < 0)
+		return 0;
+	if (stat(path, &p) < 0)
+		return 0;
+	if (f.st_dev != p.st_dev || f.st_ino != p.st_ino)
+		return 0;
+	return 1;
+}
+
 extern int dup_fd_cloexec(int oldfd, int lowfd);
 extern int get_fd_tabsize(void);
 
-- 
2.6.2

[PATCH 07/12] include/blkdev: add open_blkdev_or_file() function

[Sami Kerola]

Purpose of this function is to open a path that is potentially pointing to a
block device or file without races, and checking ensuring block device is
not busy when path points to such.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 include/blkdev.h |  3 +++
 lib/blkdev.c     | 27 +++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/blkdev.h b/include/blkdev.h
index c994795..fde2dbf 100644
--- a/include/blkdev.h
+++ b/include/blkdev.h
@@ -97,6 +97,9 @@ struct hd_geometry {
 /* are we working with block device? */
 int is_blkdev(int fd);
 
+/* open block device or file */
+int open_blkdev_or_file(const char *name, const int oflag);
+
 /* Determine size in bytes */
 off_t blkdev_find_size (int fd);
 
diff --git a/lib/blkdev.c b/lib/blkdev.c
index 5f49582..da8d835 100644
--- a/lib/blkdev.c
+++ b/lib/blkdev.c
@@ -25,9 +25,15 @@
 # include <sys/disk.h>
 #endif
 
+#ifndef EBADFD
+# define EBADFD 77		/* File descriptor in bad state */
+#endif
+
 #include "blkdev.h"
 #include "c.h"
 #include "linux_version.h"
+#include "fileutils.h"
+#include "nls.h"
 
 static long
 blkdev_valid_offset (int fd, off_t offset) {
@@ -254,6 +260,27 @@ int blkdev_is_misaligned(int fd)
 #endif
 }
 
+int open_blkdev_or_file(const char *name, const int oflag)
+{
+	int fd;
+	struct stat ftype;
+
+	if (stat(name, &ftype) < 0)
+		return -1;
+	if (S_ISBLK(ftype.st_mode)) {
+		fd = open(name, oflag | O_EXCL);
+	} else
+		fd = open(name, oflag);
+	if (-1 < fd && !is_same_inode(fd, name)) {
+		close(fd);
+		errno = EBADFD;
+		return -1;
+	}
+	if (-1 < fd && S_ISBLK(ftype.st_mode) && blkdev_is_misaligned(fd))
+		warnx(_("warning: %s is misaligned"), name);
+	return fd;
+}
+
 int blkdev_is_cdrom(int fd)
 {
 #ifdef CDROM_GET_CAPABILITY
-- 
2.6.2

[PATCH 08/12] mkfs.bfs: fix block device open race

[Sami Kerola]

This fix makes mkfs.bfs also to detect if block device is busy, and warn if
the device is misaligned.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/mkfs.bfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/disk-utils/mkfs.bfs.c b/disk-utils/mkfs.bfs.c
index 7057194..139130e 100644
--- a/disk-utils/mkfs.bfs.c
+++ b/disk-utils/mkfs.bfs.c
@@ -187,7 +187,7 @@ int main(int argc, char **argv)
 	if (!S_ISBLK(statbuf.st_mode))
 		errx(EXIT_FAILURE, _("%s is not a block special device"), device);
 
-	fd = open(device, O_RDWR | O_EXCL);
+	fd = open_blkdev_or_file(device, O_RDWR);
 	if (fd < 0)
 		err(EXIT_FAILURE, _("cannot open %s"), device);
 
-- 
2.6.2

[PATCH 09/12] mkfs.minix: fix block device open race

[Sami Kerola]

This fix makes mkfs.minix also to warn if block is misaligned.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/mkfs.minix.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/disk-utils/mkfs.minix.c b/disk-utils/mkfs.minix.c
index f2a442e..5c94fbf 100644
--- a/disk-utils/mkfs.minix.c
+++ b/disk-utils/mkfs.minix.c
@@ -819,14 +819,11 @@ int main(int argc, char ** argv)
 	if (is_mounted(ctl.device_name))
 		errx(MKFS_EX_ERROR, _("%s is mounted; will not make a filesystem here!"),
 			ctl.device_name);
-	if (stat(ctl.device_name, &statbuf) < 0)
-		err(MKFS_EX_ERROR, _("stat of %s failed"), ctl.device_name);
-	if (S_ISBLK(statbuf.st_mode))
-		ctl.device_fd = open(ctl.device_name, O_RDWR | O_EXCL);
-	else
-		ctl.device_fd = open(ctl.device_name, O_RDWR);
+	ctl.device_fd = open_blkdev_or_file(ctl.device_name, O_RDWR);
 	if (ctl.device_fd < 0)
 		err(MKFS_EX_ERROR, _("cannot open %s"), ctl.device_name);
+	if (fstat(ctl.device_fd, &statbuf) < 0)
+		err(MKFS_EX_ERROR, _("stat of %s failed"), ctl.device_name);
 	determine_device_blocks(&ctl, &statbuf);
 	setup_tables(&ctl);
 	if (ctl.check_blocks)
-- 
2.6.2

[PATCH 10/12] mkswap: fix block device open race

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/mkswap.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/disk-utils/mkswap.c b/disk-utils/mkswap.c
index c7166f7..02639ce 100644
--- a/disk-utils/mkswap.c
+++ b/disk-utils/mkswap.c
@@ -238,25 +238,16 @@ static void open_device(struct mkswap_control *ctl)
 	assert(ctl);
 	assert(ctl->devname);
 
-	if (stat(ctl->devname, &ctl->devstat) < 0)
-		err(EXIT_FAILURE, _("stat of %s failed"), ctl->devname);
-
-	if (S_ISBLK(ctl->devstat.st_mode))
-		ctl->fd = open(ctl->devname, O_RDWR | O_EXCL);
-	else {
-		if (ctl->check) {
-			ctl->check = 0;
-			warnx(_("warning: checking bad blocks from swap file is not supported: %s"),
-				ctl->devname);
-		}
-		ctl->fd = open(ctl->devname, O_RDWR);
-	}
+	ctl->fd = open_blkdev_or_file(ctl->devname, O_RDWR);
 	if (ctl->fd < 0)
 		err(EXIT_FAILURE, _("cannot open %s"), ctl->devname);
-
-	if (S_ISBLK(ctl->devstat.st_mode))
-		if (blkdev_is_misaligned(ctl->fd))
-			warnx(_("warning: %s is misaligned"), ctl->devname);
+	if (fstat(ctl->fd, &ctl->devstat) < 0)
+		err(EXIT_FAILURE, _("stat of %s failed"), ctl->devname);
+	if (ctl->check && S_ISREG(ctl->devstat.st_mode)) {
+		ctl->check = 0;
+		warnx(_("warning: checking bad blocks from swap file is not supported: %s"),
+		       ctl->devname);
+	}
 }
 
 static void wipe_device(struct mkswap_control *ctl)
-- 
2.6.2

[PATCH 11/12] mesg: fix multiple races

[Sami Kerola]

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 term-utils/mesg.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/term-utils/mesg.c b/term-utils/mesg.c
index 77d89c7..f959e1f 100644
--- a/term-utils/mesg.c
+++ b/term-utils/mesg.c
@@ -59,6 +59,7 @@
 #include "nls.h"
 #include "c.h"
 #include "rpmatch.h"
+#include "fileutils.h"
 
 /* exit codes */
 
@@ -90,7 +91,7 @@ int main(int argc, char *argv[])
 {
 	struct stat sb;
 	char *tty;
-	int ch, verbose = FALSE;
+	int ch, fd, verbose = FALSE, ret;
 
 	static const struct option longopts[] = {
 		{ "verbose",    no_argument,       0, 'v' },
@@ -123,11 +124,15 @@ int main(int argc, char *argv[])
 
 	if ((tty = ttyname(STDERR_FILENO)) == NULL)
 		err(MESG_EXIT_FAILURE, _("ttyname failed"));
-
-	if (stat(tty, &sb) < 0)
+	if ((fd = open(tty, O_RDONLY)) < 0)
+		err(MESG_EXIT_FAILURE, _("cannot open %s"), tty);
+	if (!is_same_inode(fd, tty))
+		err(MESG_EXIT_FAILURE, _("TOCTOU exploitation detected"));
+	if (fstat(fd, &sb))
 		err(MESG_EXIT_FAILURE, _("stat of %s failed"), tty);
 
 	if (!*argv) {
+		close(fd);
 		if (sb.st_mode & (S_IWGRP | S_IWOTH)) {
 			puts(_("is y"));
 			return IS_ALLOWED;
@@ -139,24 +144,28 @@ int main(int argc, char *argv[])
 	switch (rpmatch(argv[0])) {
 	case RPMATCH_YES:
 #ifdef USE_TTY_GROUP
-		if (chmod(tty, sb.st_mode | S_IWGRP) < 0)
+		if (fchmod(fd, sb.st_mode | S_IWGRP) < 0)
 #else
-		if (chmod(tty, sb.st_mode | S_IWGRP | S_IWOTH) < 0)
+		if (fchmod(fd, sb.st_mode | S_IWGRP | S_IWOTH) < 0)
 #endif
 			err(MESG_EXIT_FAILURE, _("change %s mode failed"), tty);
 		if (verbose)
 			puts(_("write access to your terminal is allowed"));
-		return IS_ALLOWED;
+		ret = IS_ALLOWED;
+		break;
 	case RPMATCH_NO:
-		if (chmod(tty, sb.st_mode & ~(S_IWGRP|S_IWOTH)) < 0)
+		if (fchmod(fd, sb.st_mode & ~(S_IWGRP|S_IWOTH)) < 0)
 			 err(MESG_EXIT_FAILURE, _("change %s mode failed"), tty);
 		if (verbose)
 			puts(_("write access to your terminal is denied"));
-		return IS_NOT_ALLOWED;
+		ret = IS_NOT_ALLOWED;
+		break;
 	case RPMATCH_INVALID:
 		warnx(_("invalid argument: %s"), argv[0]);
 		usage(stderr);
         default:
                 abort();
 	}
+	close(fd);
+	return ret;
 }
-- 
2.6.2

[PATCH 12/12] fdformat: fix block device open race

[Sami Kerola]

This fix makes fdformat also to detect if block device is busy, and warn if
the device is misaligned.

Signed-off-by: Sami Kerola <kerolasa@iki.fi>
---
 disk-utils/fdformat.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/disk-utils/fdformat.c b/disk-utils/fdformat.c
index e7d43da..086ad55 100644
--- a/disk-utils/fdformat.c
+++ b/disk-utils/fdformat.c
@@ -216,17 +216,14 @@ int main(int argc, char **argv)
 
 	if (argc < 1)
 		usage(stderr);
-	if (stat(argv[0], &st) < 0)
+	ctrl = open_blkdev_or_file(argv[0], O_RDWR);
+	if (ctrl < 0)
+		err(EXIT_FAILURE, _("cannot open %s"), argv[0]);
+	if (fstat(ctrl, &st) < 0)
 		err(EXIT_FAILURE, _("stat of %s failed"), argv[0]);
 	if (!S_ISBLK(st.st_mode))
 		/* do not test major - perhaps this was an USB floppy */
 		errx(EXIT_FAILURE, _("%s: not a block device"), argv[0]);
-	if (access(argv[0], W_OK) < 0)
-		err(EXIT_FAILURE, _("cannot access file %s"), argv[0]);
-
-	ctrl = open(argv[0], O_RDWR);
-	if (ctrl < 0)
-		err(EXIT_FAILURE, _("cannot open %s"), argv[0]);
 	if (ioctl(ctrl, FDGETPRM, (long) &param) < 0)
 		err(EXIT_FAILURE, _("could not determine current format type"));
 
-- 
2.6.2

Re: [PATCH 05/12] script, hwclock: check file exist with access(3) rather than stat(3)

[Karel Zak]

On Sat, Oct 31, 2015 at 07:21:17PM +0000, Sami Kerola wrote:
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  sys-utils/hwclock.c | 5 +----
>  term-utils/script.c | 3 +--
>  2 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/sys-utils/hwclock.c b/sys-utils/hwclock.c
> index 4b201d7..0d11061 100644
> --- a/sys-utils/hwclock.c
> +++ b/sys-utils/hwclock.c
> @@ -258,15 +258,12 @@ hw_clock_is_utc(const bool utc, const bool local_opt,
>  static int read_adjtime(struct adjtime *adjtime_p)
>  {
>  	FILE *adjfile;
> -	int rc;			/* local return code */
> -	struct stat statbuf;	/* We don't even use the contents of this. */
>  	char line1[81];		/* String: first line of adjtime file */
>  	char line2[81];		/* String: second line of adjtime file */
>  	char line3[81];		/* String: third line of adjtime file */
>  	long timeval;
>  
> -	rc = stat(adj_file_name, &statbuf);
> -	if (rc < 0 && errno == ENOENT) {
> +	if (access(adj_file_name, R_OK) != 0) {

Not sure, but I remember reports from people who use suid hwclock.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 08/12] mkfs.bfs: fix block device open race

[Karel Zak]

On Sat, Oct 31, 2015 at 07:21:20PM +0000, Sami Kerola wrote:
> This fix makes mkfs.bfs also to detect if block device is busy, and warn if
> the device is misaligned.
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  disk-utils/mkfs.bfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/disk-utils/mkfs.bfs.c b/disk-utils/mkfs.bfs.c
> index 7057194..139130e 100644
> --- a/disk-utils/mkfs.bfs.c
> +++ b/disk-utils/mkfs.bfs.c
> @@ -187,7 +187,7 @@ int main(int argc, char **argv)
>  	if (!S_ISBLK(statbuf.st_mode))
>  		errx(EXIT_FAILURE, _("%s is not a block special device"), device);
>  
> -	fd = open(device, O_RDWR | O_EXCL);
> +	fd = open_blkdev_or_file(device, O_RDWR);

 It seems code assumes block devices only, why we need _or_file()
 here? It would be enough to check statbuf and fstat(fd) for inode
 number if you want to avoid races.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 07/12] include/blkdev: add open_blkdev_or_file() function

[Karel Zak]

On Sat, Oct 31, 2015 at 07:21:19PM +0000, Sami Kerola wrote:
> +int open_blkdev_or_file(const char *name, const int oflag)
> +{
> +	int fd;
> +	struct stat ftype;
> +
> +	if (stat(name, &ftype) < 0)
> +		return -1;
> +	if (S_ISBLK(ftype.st_mode)) {
> +		fd = open(name, oflag | O_EXCL);
> +	} else
> +		fd = open(name, oflag);
> +	if (-1 < fd && !is_same_inode(fd, name)) {
> +		close(fd);
> +		errno = EBADFD;
> +		return -1;
> +	}
> +	if (-1 < fd && S_ISBLK(ftype.st_mode) && blkdev_is_misaligned(fd))
> +		warnx(_("warning: %s is misaligned"), name);
> +	return fd;
> +}

 Hmmm... this abstraction introduces extra stat() calls. Would be
 enough to use is_same_inode() directly in the code to reuse struct
 stat?
    
    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 06/12] lib/fileutils: add is_same_inode() check

[Karel Zak]

On Sat, Oct 31, 2015 at 07:21:18PM +0000, Sami Kerola wrote:
> Check if a file path and descriptor are indeed the same file.  This function
> is needed for TACTOU avoidance.
> 
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  include/fileutils.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/fileutils.h b/include/fileutils.h
> index be6d1f7..55df1a7 100644
> --- a/include/fileutils.h
> +++ b/include/fileutils.h
> @@ -4,6 +4,7 @@
>  #include <stdio.h>
>  #include <fcntl.h>
>  #include <unistd.h>
> +#include <sys/stat.h>
>  
>  #include "c.h"
>  
> @@ -25,6 +26,19 @@ static inline FILE *xfmkstemp(char **tmpname, const char *dir, const char *prefi
>  	return ret;
>  }
>  
> +static inline int is_same_inode(const int fd, const char *path)
> +{
> +	struct stat f, p;
> +
> +	if (fstat(fd, &f) < 0)
> +		return 0;
> +	if (stat(path, &p) < 0)
> +		return 0;
> +	if (f.st_dev != p.st_dev || f.st_ino != p.st_ino)
> +		return 0;
> +	return 1;
> +}

Maybe don't call fstat() here and use 

    static inline int is_same_inode(struct stat st, const char *path)

than you reduce stat() calls on many places...

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 00/12] pull: race fixes

[Karel Zak]

On Sat, Oct 31, 2015 at 07:21:12PM +0000, Sami Kerola wrote:
>   login: fix stat(2) race
>   swapon: fix stat(3) and open(3) race
>   fsck: retire stat(3) when access(3) does better job
>   agetty: fix chown(3), chmod(3) and open(3) race
>   script, hwclock: check file exist with access(3) rather than stat(3)

 Applied, thanks.

>   lib/fileutils: add is_same_inode() check
>   include/blkdev: add open_blkdev_or_file() function
>   mkfs.bfs: fix block device open race
>   mkfs.minix: fix block device open race
>   mkswap: fix block device open race
>   mesg: fix multiple races
>   fdformat: fix block device open race

 Here I don't like the pattern

  open_blkdev_or_file()
  fstat()

 IMHO it would be enough to use is_same_inode() to avoid race.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 04/12] agetty: fix chown(3), chmod(3) and open(3) race

[Karel Zak]

On Sat, Oct 31, 2015 at 07:21:16PM +0000, Sami Kerola wrote:
> Signed-off-by: Sami Kerola <kerolasa@iki.fi>
> ---
>  term-utils/agetty.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/term-utils/agetty.c b/term-utils/agetty.c
> index 0237b79..a9c8785 100644
> --- a/term-utils/agetty.c
> +++ b/term-utils/agetty.c
> @@ -1004,23 +1004,23 @@ static void open_tty(char *tty, struct termios *tp, struct options *op)
>  		     (int)sizeof(buf)) || (len < 0))
>  			log_err(_("/dev/%s: cannot open as standard input: %m"), tty);
>  
> +		/* Open the tty as standard input. */
> +		if ((fd = open(buf, O_RDWR|O_NOCTTY|O_NONBLOCK, 0)) < 0)
> +			log_err(_("/dev/%s: cannot open as standard input: %m"), tty);
> +
>  		/*
>  		 * There is always a race between this reset and the call to
>  		 * vhangup() that s.o. can use to get access to your tty.
>  		 * Linux login(1) will change tty permissions. Use root owner and group
>  		 * with permission -rw------- for the period between getty and login.
>  		 */
> -		if (chown(buf, 0, gid) || chmod(buf, (gid ? 0620 : 0600))) {
> +		if (fchown(fd, 0, gid) || fchmod(fd, (gid ? 0620 : 0600))) {

Note that this is only a cosmetic change, because we re-open() the tty
later (and it's not dangerous due to vhangup & friends in login(1)).

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 08/12] mkfs.bfs: fix block device open race

[Sami Kerola]

On 9 November 2015 at 08:48, Karel Zak <kzak@redhat.com> wrote:
> On Sat, Oct 31, 2015 at 07:21:20PM +0000, Sami Kerola wrote:
>> This fix makes mkfs.bfs also to detect if block device is busy, and warn if
>> the device is misaligned.
>
>  It seems code assumes block devices only, why we need _or_file()
>  here?

Assumption file systems is only created to a block device does not
seem right. The following changes that.

https://github.com/kerolasa/lelux-utiliteetit/commit/98b4f74e1f8f5735e29b6ac37cf4d3dcd77be701

> It would be enough to check statbuf and fstat(fd) for inode
>  number if you want to avoid races.

Maybe yes to avoid races, but the open_blkdev_or_file() does couple
things one always wants when creating file systems, or doing similar
operations. 1) use open(3) flag O_EXCL when opening block device to
ensure nothing else has it open already 2) check for TOCTOU race 3)
warn if device is misaligned.

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 07/12] include/blkdev: add open_blkdev_or_file() function

[Sami Kerola]

On 9 November 2015 at 08:51, Karel Zak <kzak@redhat.com> wrote:
>  Hmmm... this abstraction introduces extra stat() calls. Would be
>  enough to use is_same_inode() directly in the code to reuse struct
>  stat?

I agree, previous version of open_blkdev_or_file() did too many stat()
calls. Reconsidered version takes stat structure as pointer. I found
all occurrences where this function was needed having to do stat() any
away, so this seems like the best way.

https://github.com/kerolasa/lelux-utiliteetit/commit/8fe1e638fa81d88e1e405f6221c7a592284c9b23

-- 
Sami Kerola
http://www.iki.fi/kerolasa/

Re: [PATCH 00/12] pull: race fixes

[Karel Zak]

On Sat, Oct 31, 2015 at 07:21:12PM +0000, Sami Kerola wrote:
>   git://github.com/kerolasa/lelux-utiliteetit.git races

 Merged, thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com