From patchwork Fri Aug 7 19:13:02 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 40233 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 13A9E388C026; Fri, 7 Aug 2020 19:13:13 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 13A9E388C026 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1596827593; bh=PwWF6rGtGpcgMG6bZJATjzhEG4oMxrOs93Sp/zBAZ6M=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=LTX8PscQQ1UmWygOd5KLYXBw+1J5Y6wgR0DbfCnn8W4L6Xz5RvffKYTuxKjQF3KHG Bn9ZXns9Lo66+X3CjmzLzqpAqi94V3itf3JdfsD0AMzozgS7rm8GPZ5bzNlpkOhTpX u/NOtx1xJcySojDbQjbiO3hHR+UKlkDDgEMj+iFo= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail-qt1-x841.google.com (mail-qt1-x841.google.com [IPv6:2607:f8b0:4864:20::841]) by sourceware.org (Postfix) with ESMTPS id C4012388C01C for ; Fri, 7 Aug 2020 19:13:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org C4012388C01C Received: by mail-qt1-x841.google.com with SMTP id o22so2078305qtt.13 for ; Fri, 07 Aug 2020 12:13:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=PwWF6rGtGpcgMG6bZJATjzhEG4oMxrOs93Sp/zBAZ6M=; b=TLHgzD1Wohmh9eSTVf1dJacBk7EW00NIzeHhdzWDuuMjlbyJg5SY3jsqo7LkI6Wc9s B5lILq33XAWvjG7eX1iigtSzb35vdL6zhLSPbmEhjIaO/A45eAEhYPGHqOZpf7DL43bM QTnJW9y15oj9ujOpZvMMy//P4lNFTwJKA3hnd3gdtysAehC5HhmuFRDP3G4ZmGXQEj7J V9MVZtmIUcr8l+to/TkXxfY7SFB/ydM8FMnOk7NfgsmYeA8+JFXKKI4Q/rBdayibR5Li WnnzF9Z/np09nLt689p4rf0rSg27I0Xyg6Le0ZCRTZlbkA9n9gRUQHDihbrZnUE8Tisu q/kg== X-Gm-Message-State: AOAM530zPPKMHA1ZHGvIOTQyC/FDknljlUg0+qGOVmDYPslx986veV+l zrM99z96wveCcwH51kCiIdo+fA6JQwAoXw== X-Google-Smtp-Source: ABdhPJxaQlnchxOMX2jUhSCXs+B5tINyAzOPVjFe/AGoowtKHlqSjqZ2NmBHRR980KcWBIyn6gDwsg== X-Received: by 2002:ac8:7a8d:: with SMTP id x13mr15796923qtr.192.1596827587567; Fri, 07 Aug 2020 12:13:07 -0700 (PDT) Received: from localhost.localdomain ([177.194.48.209]) by smtp.googlemail.com with ESMTPSA id z24sm6965638qki.57.2020.08.07.12.13.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 07 Aug 2020 12:13:07 -0700 (PDT) To: libc-alpha@sourceware.org Subject: [RFC] login: Rewrite utmp/wtmp locking mechanism (BZ #24492) Date: Fri, 7 Aug 2020 16:13:02 -0300 Message-Id: <20200807191302.3129249-1-adhemerval.zanella@linaro.org> X-Mailer: git-send-email 2.25.1 MIME-Version: 1.0 X-Spam-Status: No, score=-13.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Adhemerval Zanella via Libc-alpha From: Adhemerval Zanella Reply-To: Adhemerval Zanella Cc: Florian Weimer Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" The utmp locking mechanism locks the utmp database file (either the default UTMP_FILE/WTMP_FILE or the one passed on utmpname). For default UTMP_FILE/WTMP_FILE, the file is usually readable by non-root users and thus it can be locked as well. The current implementation mitigates by using an alarm with a non configurable TIMEOUT to force the lock to fail if the timer expiries. Although it makes the putut{x}line/updwtmp{x} to not block indefinitely, it allows an unprivileged process to deny updates on the databases. This patch fixes it by locking a different file and only on symbols that actually update the databases (putut{x}line/updwtmp{x}). The lock file path is constructed based on the selected database: - for default ones (UTMP_FILE/WTMP_FILE) the UTMP_FILE ".lock" and WTMP_FILE ".lock" file is used. - Otherwise the lock file is constructed by appending the '.lock' suffix on the file name set by utmpname. Also, the lock is not create for default databases since the '/var/lock' usually has permissive access and any process could create them. Rather, it is expected the lock files to be created with more restrictive permission so unprivileged processes can't lock it. The reads are done without locking, which theoretically means that a read could see a partially-updated record. I don't see a easy way to avoid it while allowing the utmp databases to be accessible to all users and without making the utmpx access API to use a broker to access the database. In any case, I think this issue is less of an issue than missing updates from priviledege processes. This patch is based on top of the utmp/utmp 64-bit support [1] Checked on x86_64-linux-gnu and i686-linux-gnu. [1] https://sourceware.org/pipermail/libc-alpha/2020-August/116850.html --- login/tst-pututxline-lockfail.c | 40 +++- .../tst-utmp-default.script | 6 + login/tst-utmp32.root/tst-utmp32.script | 4 + login/utmp_file.c | 182 ++++++++---------- 4 files changed, 128 insertions(+), 104 deletions(-) diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfail.c index 3d4b1d2d6f..8d05c55496 100644 --- a/login/tst-pututxline-lockfail.c +++ b/login/tst-pututxline-lockfail.c @@ -1,4 +1,4 @@ -/* Test the lock upgrade path in tst-pututxline. +/* Test the lock path in putut{x}line. Copyright (C) 2019-2020 Free Software Foundation, Inc. This file is part of the GNU C Library. @@ -16,20 +16,25 @@ License along with the GNU C Library; see the file COPYING.LIB. If not, see . */ -/* 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 #include #include +#include +#include +#include #include #include #include #include #include #include +#include #include #include #include @@ -66,11 +71,15 @@ subprocess_create_entry (void *closure) TEST_VERIFY (write_entry (101) != NULL); } -/* Acquire an advisory read lock on PATH. */ +/* Acquire an advisory read lock file for on PATH. */ __attribute__ ((noreturn)) static void subprocess_lock_file (void) { - int fd = xopen (path, O_RDONLY, 0); + char lockpath[PATH_MAX]; + snprintf (lockpath, sizeof (lockpath), "%s.lock", path); + + int fd = xopen (lockpath, O_RDONLY | O_CREAT, 0644); + add_temp_file (lockpath); struct flock64 fl = { @@ -101,6 +110,9 @@ subprocess_lock_file (void) _exit (0); } +/* Do-nothing handler for locking timeout. */ +static void timeout_handler (int signum) {}; + static int do_test (void) { @@ -125,6 +137,20 @@ do_test (void) /* Wait for the file locking to complete. */ xpthread_barrier_wait (barrier); + { + /* The 'subprocess_lock_file' creates and locks the file created by + utmpname pututxline, the SIGARLM forces the utmp lock to return + and the function call to fail. */ + + struct sigaction action; + action.sa_handler = timeout_handler; + sigemptyset (&action.sa_mask); + action.sa_flags = 0; + xsigaction (SIGALRM, &action, NULL); + + alarm (5); + } + /* Try to add another entry. This attempt will fail, with EINTR or EAGAIN. */ TEST_COMPARE (utmpname (path), 0); diff --git a/login/tst-utmp-default.root/tst-utmp-default.script b/login/tst-utmp-default.root/tst-utmp-default.script index 26ef984f5f..4707326de9 100644 --- a/login/tst-utmp-default.root/tst-utmp-default.script +++ b/login/tst-utmp-default.root/tst-utmp-default.script @@ -5,6 +5,12 @@ touch 0664 /var/run/wtmp.v2 # Same for the old files as well touch 0664 /var/run/utmp touch 0664 /var/run/wtmp +# For default UTMP_FILE we need to create the lock files +mkdirp 0755 /var/lock +touch 0664 /var/lock/utmp.v2.lock +touch 0664 /var/lock/wtmp.v2.lock +touch 0664 /var/lock/utmp.lock +touch 0664 /var/lock/wtmp.lock # Must run localedef as root to write into default paths. su diff --git a/login/tst-utmp32.root/tst-utmp32.script b/login/tst-utmp32.root/tst-utmp32.script index 4aadc63335..5385cdac41 100644 --- a/login/tst-utmp32.root/tst-utmp32.script +++ b/login/tst-utmp32.root/tst-utmp32.script @@ -2,6 +2,10 @@ mkdirp 0755 /var/run touch 0664 /var/run/utmp.v2 touch 0664 /var/run/wtmp.v2 +# For default UTMP_FILE we need to create the lock files +mkdirp 0755 /var/lock +touch 0664 /var/lock/utmp.v2.lock +touch 0664 /var/lock/wtmp.v2.lock # Must run localedef as root to write into default paths. su diff --git a/login/utmp_file.c b/login/utmp_file.c index f3f14a744d..8f41229ab0 100644 --- a/login/utmp_file.c +++ b/login/utmp_file.c @@ -20,6 +20,7 @@ #include #include #include +#include #include #include @@ -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;