diff mbox series

[RFC] login: Rewrite utmp/wtmp locking mechanism (BZ #24492)

Message ID 20200807191302.3129249-1-adhemerval.zanella@linaro.org
State New
Headers show
Series [RFC] login: Rewrite utmp/wtmp locking mechanism (BZ #24492) | expand

Commit Message

Adhemerval Zanella Aug. 7, 2020, 7:13 p.m. UTC
The utmp locking mechanism locks the utmp database file (either the
default UTMP_FILE/WTMP_FILE or the one passed on utmpname).  For default
UTMP_FILE/WTMP_FILE, the file is usually readable by non-root users and
thus it can be locked as well.

The current implementation mitigates by using an alarm with a non
configurable TIMEOUT to force the lock to fail if the timer expiries.
Although it makes the putut{x}line/updwtmp{x} to not block indefinitely,
it allows an unprivileged process to deny updates on the databases.

This patch fixes it by locking a different file and only on symbols that
actually update the databases (putut{x}line/updwtmp{x}).  The lock file
path is constructed based on the selected database:

  - for default ones (UTMP_FILE/WTMP_FILE) the UTMP_FILE ".lock" and
    WTMP_FILE ".lock" file is used.

  - Otherwise the lock file is constructed by appending the '.lock'
    suffix on the file name set by utmpname.

Also, the lock is not create for default databases since the '/var/lock'
usually has permissive access and any process could create them.
Rather, it is expected the lock files to be created with more
restrictive permission so unprivileged processes can't lock it.

The reads are done without locking, which theoretically means that a
read could see a partially-updated record.  I don't see a easy way to
avoid it while allowing the utmp databases to be accessible to all users
and without making the utmpx access API to use a broker to access
the database.  In any case, I think this issue is less of an issue than
missing updates from priviledege processes.

This patch is based on top of the utmp/utmp 64-bit support [1]

Checked on x86_64-linux-gnu and i686-linux-gnu.

[1] https://sourceware.org/pipermail/libc-alpha/2020-August/116850.html
---
 login/tst-pututxline-lockfail.c               |  40 +++-
 .../tst-utmp-default.script                   |   6 +
 login/tst-utmp32.root/tst-utmp32.script       |   4 +
 login/utmp_file.c                             | 182 ++++++++----------
 4 files changed, 128 insertions(+), 104 deletions(-)
diff mbox series

Patch

diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
index 3d4b1d2d6f..8d05c55496 100644
--- a/login/tst-pututxline-lockfail.c
+++ b/login/tst-pututxline-lockfail.c
@@ -1,4 +1,4 @@ 
-/* Test the lock upgrade path in tst-pututxline.
+/* Test the lock path in putut{x}line.
    Copyright (C) 2019-2020 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -16,20 +16,25 @@ 
    License along with the GNU C Library; see the file COPYING.LIB.  If
    not, see <https://www.gnu.org/licenses/>.  */
 
-/* pututxline upgrades the read lock on the file to a write lock.
-   This test verifies that if the lock upgrade fails, the utmp
-   subsystem remains in a consistent state, so that pututxline can be
-   called again.  */
+/* putut{x}line and updwtmp{x} do not lock the utmp{x} file, but rather a
+   specific file based on the database path.  For non default files
+   set by utmpname is is the '/var/lock/`basename $path`.lock.
+   This test verifies that if the lock fails, the utmp subsystem remains
+   in a consistent state, so that putut{x}line can be called again.  */
 
 #include <errno.h>
 #include <fcntl.h>
 #include <stdlib.h>
+#include <stdio.h>
+#include <limits.h>
+#include <libgen.h>
 #include <support/check.h>
 #include <support/namespace.h>
 #include <support/support.h>
 #include <support/temp_file.h>
 #include <support/xthread.h>
 #include <support/xunistd.h>
+#include <support/xsignal.h>
 #include <unistd.h>
 #include <utmp.h>
 #include <utmpx.h>
@@ -66,11 +71,15 @@  subprocess_create_entry (void *closure)
   TEST_VERIFY (write_entry (101) != NULL);
 }
 
-/* Acquire an advisory read lock on PATH.  */
+/* Acquire an advisory read lock file for on PATH.  */
 __attribute__ ((noreturn)) static void
 subprocess_lock_file (void)
 {
-  int fd = xopen (path, O_RDONLY, 0);
+  char lockpath[PATH_MAX];
+  snprintf (lockpath, sizeof (lockpath), "%s.lock", path);
+
+  int fd = xopen (lockpath, O_RDONLY | O_CREAT, 0644);
+  add_temp_file (lockpath);
 
   struct flock64 fl =
     {
@@ -101,6 +110,9 @@  subprocess_lock_file (void)
   _exit (0);
 }
 
+/* Do-nothing handler for locking timeout.  */
+static void timeout_handler (int signum) {};
+
 static int
 do_test (void)
 {
@@ -125,6 +137,20 @@  do_test (void)
   /* Wait for the file locking to complete.  */
   xpthread_barrier_wait (barrier);
 
+  {
+    /* The 'subprocess_lock_file' creates and locks the file created by
+       utmpname pututxline, the SIGARLM forces the utmp lock to return
+       and the function call to fail.  */
+
+    struct sigaction action;
+    action.sa_handler = timeout_handler;
+    sigemptyset (&action.sa_mask);
+    action.sa_flags = 0;
+    xsigaction (SIGALRM, &action, NULL);
+
+    alarm (5);
+  }
+
   /* Try to add another entry.  This attempt will fail, with EINTR or
      EAGAIN.  */
   TEST_COMPARE (utmpname (path), 0);
diff --git a/login/tst-utmp-default.root/tst-utmp-default.script b/login/tst-utmp-default.root/tst-utmp-default.script
index 26ef984f5f..4707326de9 100644
--- a/login/tst-utmp-default.root/tst-utmp-default.script
+++ b/login/tst-utmp-default.root/tst-utmp-default.script
@@ -5,6 +5,12 @@  touch  0664 /var/run/wtmp.v2
 # Same for the old files as well
 touch  0664 /var/run/utmp
 touch  0664 /var/run/wtmp
+# For default UTMP_FILE we need to create the lock files
+mkdirp 0755 /var/lock
+touch  0664 /var/lock/utmp.v2.lock
+touch  0664 /var/lock/wtmp.v2.lock
+touch  0664 /var/lock/utmp.lock
+touch  0664 /var/lock/wtmp.lock
 
 # Must run localedef as root to write into default paths.
 su
diff --git a/login/tst-utmp32.root/tst-utmp32.script b/login/tst-utmp32.root/tst-utmp32.script
index 4aadc63335..5385cdac41 100644
--- a/login/tst-utmp32.root/tst-utmp32.script
+++ b/login/tst-utmp32.root/tst-utmp32.script
@@ -2,6 +2,10 @@ 
 mkdirp 0755 /var/run
 touch  0664 /var/run/utmp.v2
 touch  0664 /var/run/wtmp.v2
+# For default UTMP_FILE we need to create the lock files
+mkdirp 0755 /var/lock
+touch  0664 /var/lock/utmp.v2.lock
+touch  0664 /var/lock/wtmp.v2.lock
 
 # Must run localedef as root to write into default paths.
 su
diff --git a/login/utmp_file.c b/login/utmp_file.c
index f3f14a744d..8f41229ab0 100644
--- a/login/utmp_file.c
+++ b/login/utmp_file.c
@@ -20,6 +20,7 @@ 
 #include <fcntl.h>
 #include <stdbool.h>
 #include <string.h>
+#include <stdio.h>
 #include <sys/param.h>
 #include <not-cancel.h>
 
@@ -121,62 +122,63 @@  matches_last_entry (enum operation_mode_t mode, short int type,
     }
 }
 
-/* Locking timeout.  */
-#ifndef TIMEOUT
-# define TIMEOUT 10
-#endif
+/* Construct a lock file base on FILE depending of DEFAULT_DB: if true
+   the lock is constructed on /var/lock; otherwise is used the FILE
+   path itself.  The lock file is also created if it does not exist if
+   DEFAULT_DB is false.  */
+static int
+lock_write_file (const char *file, bool default_db)
+{
+  char path[PATH_MAX];
+  if (default_db)
+    __snprintf (path, sizeof (path), "/var/lock/%s.lock", __basename (file));
+  else
+    __snprintf (path, sizeof (path), "%s.lock", file);
 
-/* Do-nothing handler for locking timeout.  */
-static void timeout_handler (int signum) {};
+  int flags = O_RDWR | O_LARGEFILE | O_CLOEXEC;
+  mode_t mode = 0644;
 
+  /* The errno need to reset if 'create_file' is set and the O_CREAT does not
+     fail.  */
+  int saved_errno = errno;
+  int fd = __open_nocancel (path, flags, mode);
+  if (fd == -1 && errno == ENOENT && !default_db)
+    fd = __open_nocancel (path, flags | O_CREAT, mode);
+  if (fd == -1)
+    return -1;
+  __set_errno (saved_errno);
 
-/* try_file_lock (LOCKING, FD, TYPE) returns true if the locking
-   operation failed and recovery needs to be performed.
+  struct flock64 fl =
+    {
+     .l_type = F_WRLCK,
+     .l_whence = SEEK_SET,
+    };
 
-   file_unlock (FD) removes the lock (which must have been
-   successfully acquired). */
+  if (__fcntl64_nocancel (fd, F_SETLKW, &fl) == -1)
+    {
+      __close_nocancel_nostatus (fd);
+      return -1;
+    }
+  return fd;
+}
 
-static bool
-try_file_lock (int fd, int type)
-{
-  /* Cancel any existing alarm.  */
-  int old_timeout = alarm (0);
-
-  /* Establish signal handler.  */
-  struct sigaction old_action;
-  struct sigaction action;
-  action.sa_handler = timeout_handler;
-  __sigemptyset (&action.sa_mask);
-  action.sa_flags = 0;
-  __sigaction (SIGALRM, &action, &old_action);
-
-  alarm (TIMEOUT);
-
-  /* Try to get the lock.  */
- struct flock64 fl =
-   {
-    .l_type = type,
-    .l_whence = SEEK_SET,
-   };
-
- bool status = __fcntl64_nocancel (fd, F_SETLKW, &fl) < 0;
- int saved_errno = errno;
-
- /* Reset the signal handler and alarm.  We must reset the alarm
-    before resetting the handler so our alarm does not generate a
-    spurious SIGALRM seen by the user.  However, we cannot just set
-    the user's old alarm before restoring the handler, because then
-    it's possible our handler could catch the user alarm's SIGARLM and
-    then the user would never see the signal he expected.  */
-  alarm (0);
-  __sigaction (SIGALRM, &old_action, NULL);
-  if (old_timeout != 0)
-    alarm (old_timeout);
+static void
+unlock_write_file (const char *file, int lockfd, bool default_db)
+{
+  __close_nocancel_nostatus (lockfd);
 
-  __set_errno (saved_errno);
-  return status;
+  char path[PATH_MAX];
+  __snprintf (path, sizeof (path), "/var/lock/%s.lock", basename (file));
+  if (! default_db)
+    {
+      /* Ignore error for the case the file does not exist.  */
+      int saved_errno = errno;
+      __unlink (path);
+      __set_errno (saved_errno);
+    }
 }
 
+
 static void
 file_unlock (int fd)
 {
@@ -251,8 +253,7 @@  internal_getutent_r (enum operation_mode_t mode, void *buffer)
 {
   int saved_errno = errno;
 
-  if (!maybe_setutent (mode)
-      || try_file_lock (file_fd, F_RDLCK))
+  if (!maybe_setutent (mode))
     return -1;
 
   ssize_t nbytes = read_last_entry (mode);
@@ -304,9 +305,6 @@  static bool
 internal_getut_r (enum operation_mode_t mode, short int type, const char *id,
 		  const char *line)
 {
-  if (try_file_lock (file_fd, F_RDLCK))
-    return false;
-
   bool r = internal_getut_nolock (mode, type, id, line);
   file_unlock (file_fd);
   return r;
@@ -335,8 +333,7 @@  static int
 internal_getutline_r (enum operation_mode_t mode, const char *line,
 		      void *buffer)
 {
-  if (!maybe_setutent (mode)
-      || try_file_lock (file_fd, F_RDLCK))
+  if (!maybe_setutent (mode))
     return -1;
 
   while (1)
@@ -375,13 +372,13 @@  internal_pututline (enum operation_mode_t mode, short int type,
   if (!maybe_setutent (mode))
     return false;
 
+  const char *file_name = mode == UTMP_TIME64
+    ? __libc_utmp_file_name
+    : utmp_file_name_time32 (__libc_utmp_file_name);
+
   if (! file_writable)
     {
       /* We must make the file descriptor writable before going on.  */
-      const char *file_name = mode == UTMP_TIME64
-	? __libc_utmp_file_name
-	: utmp_file_name_time32 (__libc_utmp_file_name);
-
       int new_fd = __open_nocancel
 	(file_name, O_RDWR | O_LARGEFILE | O_CLOEXEC);
       if (new_fd == -1)
@@ -396,34 +393,25 @@  internal_pututline (enum operation_mode_t mode, short int type,
       file_writable = true;
     }
 
-  /* Exclude other writers before validating the cache.  */
-  if (try_file_lock (file_fd, F_WRLCK))
+  /* To avoid DOS when accessing the utmp{x} database for update, the lock
+     file should be accessible only by previleged users (BZ #24492).  For non
+     default utmp{x} database the function tries to create the lock file.  */
+  bool default_db = __libc_utmpname_mode == UTMPNAME_TIME64
+		   || __libc_utmpname_mode == UTMPNAME_TIME32;
+  int lockfd = lock_write_file (file_name, default_db);
+  if (lockfd == -1)
     return false;
 
   /* Find the correct place to insert the data.  */
   const size_t utmp_size = last_entry_size (mode);
-  bool found = false;
-  if (matches_last_entry (mode, type, id, line))
-    {
-      /* Read back the entry under the write lock.  */
-      file_offset -= utmp_size;
-      ssize_t nbytes = read_last_entry (mode);
-      if (nbytes < 0)
-	{
-	  file_unlock (file_fd);
-	  return false;
-	}
-
-      if (nbytes == 0)
-	/* End of file reached.  */
-	found = false;
-      else
-	found = matches_last_entry (mode, type, id, line);
-    }
+  bool ret = false;
 
-  if (!found)
-    /* Search forward for the entry.  */
-    found = internal_getut_nolock (mode, type, id, line);
+  if (matches_last_entry (mode, type, id, line))
+    /* Read back the entry under the write lock.  */
+    file_offset -= utmp_size;
+  bool found = internal_getut_nolock (mode, type, id, line);
+  if (!found && errno != ESRCH)
+    goto internal_pututline_out;
 
   off64_t write_offset;
   if (!found)
@@ -444,31 +432,29 @@  internal_pututline (enum operation_mode_t mode, short int type,
   ssize_t nbytes;
   if (__lseek64 (file_fd, write_offset, SEEK_SET) < 0
       || (nbytes = __write_nocancel (file_fd, data, utmp_size)) < 0)
-    {
-      /* There is no need to recover the file position because all
-	 reads use pread64, and any future write is preceded by
-	 another seek.  */
-      file_unlock (file_fd);
-      return false;
-    }
+    /* There is no need to recover the file position because all reads use
+       pread64, and any future write is preceded by another seek.  */
+    goto internal_pututline_out;
 
   if (nbytes != utmp_size)
     {
       /* If we appended a new record this is only partially written.
 	 Remove it.  */
       if (!found)
-	(void) __ftruncate64 (file_fd, write_offset);
-      file_unlock (file_fd);
+	__ftruncate64 (file_fd, write_offset);
       /* Assume that the write failure was due to missing disk
 	 space.  */
       __set_errno (ENOSPC);
-      return false;
+      goto internal_pututline_out;
     }
 
-  file_unlock (file_fd);
   file_offset = write_offset + utmp_size;
+  ret = true;
 
-  return true;
+internal_pututline_out:
+  /* Release the write lock.  */
+  unlock_write_file (file_name, lockfd, default_db);
+  return ret;
 }
 
 static int
@@ -484,7 +470,10 @@  internal_updwtmp (enum operation_mode_t mode, const char *file,
   if (fd < 0)
     return -1;
 
-  if (try_file_lock (fd, F_WRLCK))
+  bool default_db = strcmp (file, _PATH_UTMP) == 0
+		    || strcmp (file, _PATH_UTMP_BASE) == 0;
+  int lockfd = lock_write_file (file, default_db);
+  if (lockfd == -1)
     {
       __close_nocancel_nostatus (fd);
       return -1;
@@ -514,9 +503,8 @@  internal_updwtmp (enum operation_mode_t mode, const char *file,
   result = 0;
 
 unlock_return:
-  file_unlock (fd);
-
   /* Close WTMP file.  */
+  unlock_write_file (file, lockfd, default_db);
   __close_nocancel_nostatus (fd);
 
   return result;