public inbox for glibc-cvs@sourceware.org
help / color / mirror / Atom feed
* [glibc/azanella/y2038] login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
@ 2021-02-23 12:37 Adhemerval Zanella
0 siblings, 0 replies; 7+ messages in thread
From: Adhemerval Zanella @ 2021-02-23 12:37 UTC (permalink / raw)
To: glibc-cvs
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=ed757527e89866dd5f0ae79502049bc077c33a28
commit ed757527e89866dd5f0ae79502049bc077c33a28
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date: Fri Aug 7 14:53:15 2020 -0300
login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
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
Diff:
---
login/tst-pututxline-lockfail.c | 40 ++++-
.../tst-utmp-default.root/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 --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
index 75498947b7..1cd41526c9 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-2021 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 ee1fe51b43..62dafbdf1f 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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* [glibc/azanella/y2038] login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
@ 2021-03-04 17:37 Adhemerval Zanella
0 siblings, 0 replies; 7+ messages in thread
From: Adhemerval Zanella @ 2021-03-04 17:37 UTC (permalink / raw)
To: glibc-cvs
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=01bbd25d93197db623f8bc9d5b07e6a3eef565a6
commit 01bbd25d93197db623f8bc9d5b07e6a3eef565a6
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date: Fri Aug 7 14:53:15 2020 -0300
login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
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
Diff:
---
login/tst-pututxline-lockfail.c | 44 ++++-
.../tst-utmp-default.root/tst-utmp-default.script | 6 +
login/tst-utmp32.root/tst-utmp32.script | 4 +
login/utmp_file.c | 186 ++++++++++-----------
4 files changed, 136 insertions(+), 104 deletions(-)
diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
index 75498947b7..5c9d850552 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-2021 Free Software Foundation, Inc.
This file is part of the GNU C Library.
@@ -16,24 +16,33 @@
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>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Path to the temporary utmp file. */
static char *path;
@@ -66,11 +75,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 +114,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 +141,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 ee1fe51b43..382189a901 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>
@@ -28,6 +29,10 @@
#include <utmp-compat.h>
#include <shlib-compat.h>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Descriptor for the file and position. */
static int file_fd = -1;
@@ -121,62 +126,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 +257,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 +309,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 +337,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 +376,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 +397,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 +436,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 +474,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 +507,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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* [glibc/azanella/y2038] login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
@ 2021-03-04 11:29 Adhemerval Zanella
0 siblings, 0 replies; 7+ messages in thread
From: Adhemerval Zanella @ 2021-03-04 11:29 UTC (permalink / raw)
To: glibc-cvs
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=01bbd25d93197db623f8bc9d5b07e6a3eef565a6
commit 01bbd25d93197db623f8bc9d5b07e6a3eef565a6
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date: Fri Aug 7 14:53:15 2020 -0300
login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
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
Diff:
---
login/tst-pututxline-lockfail.c | 44 ++++-
.../tst-utmp-default.root/tst-utmp-default.script | 6 +
login/tst-utmp32.root/tst-utmp32.script | 4 +
login/utmp_file.c | 186 ++++++++++-----------
4 files changed, 136 insertions(+), 104 deletions(-)
diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
index 75498947b7..5c9d850552 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-2021 Free Software Foundation, Inc.
This file is part of the GNU C Library.
@@ -16,24 +16,33 @@
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>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Path to the temporary utmp file. */
static char *path;
@@ -66,11 +75,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 +114,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 +141,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 ee1fe51b43..382189a901 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>
@@ -28,6 +29,10 @@
#include <utmp-compat.h>
#include <shlib-compat.h>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Descriptor for the file and position. */
static int file_fd = -1;
@@ -121,62 +126,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 +257,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 +309,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 +337,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 +376,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 +397,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 +436,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 +474,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 +507,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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* [glibc/azanella/y2038] login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
@ 2021-03-02 12:31 Adhemerval Zanella
0 siblings, 0 replies; 7+ messages in thread
From: Adhemerval Zanella @ 2021-03-02 12:31 UTC (permalink / raw)
To: glibc-cvs
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=5161abe9caa76e0e8a30321073efd2710317a9b6
commit 5161abe9caa76e0e8a30321073efd2710317a9b6
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date: Fri Aug 7 14:53:15 2020 -0300
login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
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
Diff:
---
login/tst-pututxline-lockfail.c | 44 ++++-
.../tst-utmp-default.root/tst-utmp-default.script | 6 +
login/tst-utmp32.root/tst-utmp32.script | 4 +
login/utmp_file.c | 186 ++++++++++-----------
4 files changed, 136 insertions(+), 104 deletions(-)
diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
index 75498947b7..5c9d850552 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-2021 Free Software Foundation, Inc.
This file is part of the GNU C Library.
@@ -16,24 +16,33 @@
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>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Path to the temporary utmp file. */
static char *path;
@@ -66,11 +75,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 +114,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 +141,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 ee1fe51b43..382189a901 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>
@@ -28,6 +29,10 @@
#include <utmp-compat.h>
#include <shlib-compat.h>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Descriptor for the file and position. */
static int file_fd = -1;
@@ -121,62 +126,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 +257,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 +309,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 +337,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 +376,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 +397,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 +436,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 +474,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 +507,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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* [glibc/azanella/y2038] login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
@ 2021-03-01 17:36 Adhemerval Zanella
0 siblings, 0 replies; 7+ messages in thread
From: Adhemerval Zanella @ 2021-03-01 17:36 UTC (permalink / raw)
To: glibc-cvs
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=c708d8de76352ec5eb1384b525ca9553bf736d87
commit c708d8de76352ec5eb1384b525ca9553bf736d87
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date: Fri Aug 7 14:53:15 2020 -0300
login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
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
Diff:
---
login/tst-pututxline-lockfail.c | 44 ++++-
.../tst-utmp-default.root/tst-utmp-default.script | 6 +
login/tst-utmp32.root/tst-utmp32.script | 4 +
login/utmp_file.c | 186 ++++++++++-----------
4 files changed, 136 insertions(+), 104 deletions(-)
diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
index 75498947b7..5c9d850552 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-2021 Free Software Foundation, Inc.
This file is part of the GNU C Library.
@@ -16,24 +16,33 @@
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>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Path to the temporary utmp file. */
static char *path;
@@ -66,11 +75,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 +114,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 +141,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 ee1fe51b43..382189a901 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>
@@ -28,6 +29,10 @@
#include <utmp-compat.h>
#include <shlib-compat.h>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Descriptor for the file and position. */
static int file_fd = -1;
@@ -121,62 +126,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 +257,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 +309,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 +337,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 +376,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 +397,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 +436,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 +474,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 +507,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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* [glibc/azanella/y2038] login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
@ 2021-02-26 20:41 Adhemerval Zanella
0 siblings, 0 replies; 7+ messages in thread
From: Adhemerval Zanella @ 2021-02-26 20:41 UTC (permalink / raw)
To: glibc-cvs
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=b71d2fe923b68de420b216a1b7ba2444478d809f
commit b71d2fe923b68de420b216a1b7ba2444478d809f
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date: Fri Aug 7 14:53:15 2020 -0300
login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
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
Diff:
---
login/tst-pututxline-lockfail.c | 44 ++++-
.../tst-utmp-default.root/tst-utmp-default.script | 6 +
login/tst-utmp32.root/tst-utmp32.script | 4 +
login/utmp_file.c | 186 ++++++++++-----------
4 files changed, 136 insertions(+), 104 deletions(-)
diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
index 75498947b7..5c9d850552 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-2021 Free Software Foundation, Inc.
This file is part of the GNU C Library.
@@ -16,24 +16,33 @@
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>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Path to the temporary utmp file. */
static char *path;
@@ -66,11 +75,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 +114,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 +141,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 ee1fe51b43..382189a901 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>
@@ -28,6 +29,10 @@
#include <utmp-compat.h>
#include <shlib-compat.h>
+#ifndef PATH_MAX
+# define PATH_MAX 1024
+#endif
+
/* Descriptor for the file and position. */
static int file_fd = -1;
@@ -121,62 +126,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 +257,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 +309,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 +337,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 +376,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 +397,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 +436,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 +474,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 +507,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;
^ permalink raw reply [flat|nested] 7+ messages in thread
* [glibc/azanella/y2038] login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
@ 2021-02-23 20:39 Adhemerval Zanella
0 siblings, 0 replies; 7+ messages in thread
From: Adhemerval Zanella @ 2021-02-23 20:39 UTC (permalink / raw)
To: glibc-cvs
https://sourceware.org/git/gitweb.cgi?p=glibc.git;h=301bfc9c800ad8bea975d7e77200698d63b2d468
commit 301bfc9c800ad8bea975d7e77200698d63b2d468
Author: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Date: Fri Aug 7 14:53:15 2020 -0300
login: Rewrite utmp/wtmp locking mechanism (BZ #24492)
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
Diff:
---
login/tst-pututxline-lockfail.c | 40 ++++-
.../tst-utmp-default.root/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 --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c
index 75498947b7..1cd41526c9 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-2021 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 ee1fe51b43..62dafbdf1f 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;
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-04 17:37 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-23 12:37 [glibc/azanella/y2038] login: Rewrite utmp/wtmp locking mechanism (BZ #24492) Adhemerval Zanella
2021-02-23 20:39 Adhemerval Zanella
2021-02-26 20:41 Adhemerval Zanella
2021-03-01 17:36 Adhemerval Zanella
2021-03-02 12:31 Adhemerval Zanella
2021-03-04 11:29 Adhemerval Zanella
2021-03-04 17:37 Adhemerval Zanella
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).