From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 92079 invoked by alias); 17 Jan 2020 14:04:54 -0000 Mailing-List: contact libc-stable-help@sourceware.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: List-Archive: Sender: libc-stable-owner@sourceware.org Received: (qmail 91965 invoked by uid 89); 17 Jan 2020 14:04:53 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.3 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-18.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 spammy=sk:PTHREAD X-Spam-Status: No, score=-18.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on sourceware.org X-Spam-Level: X-HELO: us-smtp-delivery-1.mimecast.com Received: from us-smtp-2.mimecast.com (HELO us-smtp-delivery-1.mimecast.com) (205.139.110.61) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 17 Jan 2020 14:04:39 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1579269878; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=5UTzC2kGoNBipAVdtbTDpkXD1EwTT2UzsSY2kLlM5tA=; b=Sxw5K6hyYLoCuNEoIUu/wV7q9ICp8zoMF2hqc3aJGGV+6gpwgLWUGVJnbLd83KWE269Mpe j1i9f8SGyaH884xke21ubvPuP+OwBrAidw6aazskFxFe3c+WwW3J5vadJyA9m9q2a0L6kd EC9SHfochvQYQZGEDDif7tGRVLyip6o= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-388-TmzRVNCRMjKTOn6gcqFhlg-1; Fri, 17 Jan 2020 09:04:36 -0500 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CE585A0CC3 for ; Fri, 17 Jan 2020 14:04:35 +0000 (UTC) Received: from oldenburg2.str.redhat.com (ovpn-117-165.ams2.redhat.com [10.36.117.165]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 623816609E for ; Fri, 17 Jan 2020 14:04:35 +0000 (UTC) Received: by oldenburg2.str.redhat.com (Postfix, from userid 1000) id E37AC8299EE3; Fri, 17 Jan 2020 15:04:33 +0100 (CET) Date: Wed, 01 Jan 2020 00:00:00 -0000 To: libc-stable@sourceware.org Subject: [2.30 COMMITTED] login: pututxline could fail to overwrite existing entries [BZ #24902] User-Agent: Heirloom mailx 12.5 7/5/10 MIME-Version: 1.0 Message-Id: <20200117140433.E37AC8299EE3@oldenburg2.str.redhat.com> From: Florian Weimer X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-MC-Unique: TmzRVNCRMjKTOn6gcqFhlg-1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2020-01/txt/msg00011.txt.bz2 The internal_getut_r function updates the file_offset variable and therefore must always update last_entry as well. Previously, if pututxline could not upgrade the read lock to a write lock, internal_getut_r would update file_offset only, without updating last_entry, and a subsequent call would not overwrite the existing utmpx entry at file_offset, instead creating a new entry. This has been observed to cause unbounded file growth in high-load situations. This commit removes the buffer argument to internal_getut_r and updates the last_entry variable directly, along with file_offset. Initially reported and fixed by Ond=C5=99ej Lyson=C4=9Bk. Reviewed-by: Gabriel F. T. Gomes (cherry picked from commit 61d3db428176d9d0822e4e680305fe34285edff2) 2019-08-28 Florian Weimer [BZ #24902] * login/Makefile (tests): Add tst-pututxline-lockfail. (tst-pututxline-lockfail): Link with -lpthread. * login/utmp_file.c (internal_getut_r): Remove buffer argument. (__libc_getutid_r): Adjust. (__libc_pututline): Likewise. Check for file_offset =3D=3D -1. * login/tst-pututxline-lockfail.c: New file. diff --git a/NEWS b/NEWS index 8952a8a77d..b0ab63a83b 100644 --- a/NEWS +++ b/NEWS @@ -25,6 +25,7 @@ The following bugs are resolved with this release: [24880] login: Use struct flock64 in utmp [24986] alpha: new getegid, geteuid and getppid syscalls used unconditionally + [24902] login: pututxline could fail to overwrite existing entries [25189] Don't use a custom wrapper macro around __has_include [25203] libio: Disable vtable validation for pre-2.1 interposed handles [25204] Ignore LD_PREFER_MAP_32BIT_EXEC for SUID programs diff --git a/login/Makefile b/login/Makefile index f9c4264087..93a3c8edf2 100644 --- a/login/Makefile +++ b/login/Makefile @@ -43,7 +43,8 @@ endif subdir-dirs =3D programs vpath %.c programs =20 -tests :=3D tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-upd= wtmpx +tests :=3D tst-utmp tst-utmpx tst-grantpt tst-ptsname tst-getlogin tst-upd= wtmpx \ + tst-pututxline-lockfail =20 # Build the -lutil library with these extra functions. extra-libs :=3D libutil @@ -71,3 +72,5 @@ endif $(inst_libexecdir)/pt_chown: $(objpfx)pt_chown $(+force) $(make-target-directory) -$(INSTALL_PROGRAM) -m 4755 -o root $< $@ + +$(objpfx)tst-pututxline-lockfail: $(shared-thread-library) diff --git a/login/tst-pututxline-lockfail.c b/login/tst-pututxline-lockfai= l.c new file mode 100644 index 0000000000..47c25dc065 --- /dev/null +++ b/login/tst-pututxline-lockfail.c @@ -0,0 +1,176 @@ +/* Test the lock upgrade path in tst-pututxline. + Copyright (C) 2019 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public License as + published by the Free Software Foundation; either version 2.1 of the + License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + 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. */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +/* Path to the temporary utmp file. */ +static char *path; + +/* Used to synchronize the subprocesses. The barrier itself is + allocated in shared memory. */ +static pthread_barrier_t *barrier; + +/* Use pututxline to write an entry for PID. */ +static struct utmpx * +write_entry (pid_t pid) +{ + struct utmpx ut =3D + { + .ut_type =3D LOGIN_PROCESS, + .ut_id =3D "1", + .ut_user =3D "root", + .ut_pid =3D pid, + .ut_line =3D "entry", + .ut_host =3D "localhost", + }; + return pututxline (&ut); +} + +/* Create the initial entry in a subprocess, so that the utmp + subsystem in the original process is not disturbed. */ +static void +subprocess_create_entry (void *closure) +{ + TEST_COMPARE (utmpname (path), 0); + TEST_VERIFY (write_entry (101) !=3D NULL); +} + +/* Acquire an advisory read lock on PATH. */ +__attribute__ ((noreturn)) static void +subprocess_lock_file (void) +{ + int fd =3D xopen (path, O_RDONLY, 0); + + struct flock64 fl =3D + { + .l_type =3D F_RDLCK, + fl.l_whence =3D SEEK_SET, + }; + TEST_COMPARE (fcntl64 (fd, F_SETLKW, &fl), 0); + + /* Signal to the main process that the lock has been acquired. */ + xpthread_barrier_wait (barrier); + + /* Wait for the unlock request from the main process. */ + xpthread_barrier_wait (barrier); + + /* Implicitly unlock the file. */ + xclose (fd); + + /* Overwrite the existing entry. */ + TEST_COMPARE (utmpname (path), 0); + errno =3D 0; + setutxent (); + TEST_COMPARE (errno, 0); + TEST_VERIFY (write_entry (102) !=3D NULL); + errno =3D 0; + endutxent (); + TEST_COMPARE (errno, 0); + + _exit (0); +} + +static int +do_test (void) +{ + xclose (create_temp_file ("tst-pututxline-lockfail-", &path)); + + { + pthread_barrierattr_t attr; + xpthread_barrierattr_init (&attr); + xpthread_barrierattr_setpshared (&attr, PTHREAD_SCOPE_PROCESS); + barrier =3D support_shared_allocate (sizeof (*barrier)); + xpthread_barrier_init (barrier, &attr, 2); + xpthread_barrierattr_destroy (&attr); + } + + /* Write the initial entry. */ + support_isolate_in_subprocess (subprocess_create_entry, NULL); + + pid_t locker_pid =3D xfork (); + if (locker_pid =3D=3D 0) + subprocess_lock_file (); + + /* Wait for the file locking to complete. */ + xpthread_barrier_wait (barrier); + + /* Try to add another entry. This attempt will fail, with EINTR or + EAGAIN. */ + TEST_COMPARE (utmpname (path), 0); + TEST_VERIFY (write_entry (102) =3D=3D NULL); + if (errno !=3D EINTR) + TEST_COMPARE (errno, EAGAIN); + + /* Signal the subprocess to overwrite the entry. */ + xpthread_barrier_wait (barrier); + + /* Wait for write and unlock to complete. */ + { + int status; + xwaitpid (locker_pid, &status, 0); + TEST_COMPARE (status, 0); + } + + /* The file is no longer locked, so this operation will succeed. */ + TEST_VERIFY (write_entry (103) !=3D NULL); + errno =3D 0; + endutxent (); + TEST_COMPARE (errno, 0); + + /* Check that there is just one entry with the expected contents. + If pututxline becomes desynchronized internally, the entry is not + overwritten (bug 24902). */ + errno =3D 0; + setutxent (); + TEST_COMPARE (errno, 0); + struct utmpx *ut =3D getutxent (); + TEST_VERIFY_EXIT (ut !=3D NULL); + TEST_COMPARE (ut->ut_type, LOGIN_PROCESS); + TEST_COMPARE_STRING (ut->ut_id, "1"); + TEST_COMPARE_STRING (ut->ut_user, "root"); + TEST_COMPARE (ut->ut_pid, 103); + TEST_COMPARE_STRING (ut->ut_line, "entry"); + TEST_COMPARE_STRING (ut->ut_host, "localhost"); + TEST_VERIFY (getutxent () =3D=3D NULL); + errno =3D 0; + endutxent (); + TEST_COMPARE (errno, 0); + + xpthread_barrier_destroy (barrier); + support_shared_free (barrier); + free (path); + return 0; +} + +#include diff --git a/login/utmp_file.c b/login/utmp_file.c index 94753e0404..2d0548f6fa 100644 --- a/login/utmp_file.c +++ b/login/utmp_file.c @@ -185,9 +185,11 @@ __libc_getutent_r (struct utmp *buffer, struct utmp **= result) } =20 =20 +/* Search for *ID, updating last_entry and file_offset. Return 0 on + success and -1 on failure. If the locking operation failed, write + true to *LOCK_FAILED. */ static int -internal_getut_r (const struct utmp *id, struct utmp *buffer, - bool *lock_failed) +internal_getut_r (const struct utmp *id, bool *lock_failed) { int result =3D -1; =20 @@ -206,7 +208,7 @@ internal_getut_r (const struct utmp *id, struct utmp *b= uffer, while (1) { /* Read the next entry. */ - if (__read_nocancel (file_fd, buffer, sizeof (struct utmp)) + if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp)) !=3D sizeof (struct utmp)) { __set_errno (ESRCH); @@ -215,7 +217,7 @@ internal_getut_r (const struct utmp *id, struct utmp *b= uffer, } file_offset +=3D sizeof (struct utmp); =20 - if (id->ut_type =3D=3D buffer->ut_type) + if (id->ut_type =3D=3D last_entry.ut_type) break; } } @@ -227,7 +229,7 @@ internal_getut_r (const struct utmp *id, struct utmp *b= uffer, while (1) { /* Read the next entry. */ - if (__read_nocancel (file_fd, buffer, sizeof (struct utmp)) + if (__read_nocancel (file_fd, &last_entry, sizeof (struct utmp)) !=3D sizeof (struct utmp)) { __set_errno (ESRCH); @@ -236,7 +238,7 @@ internal_getut_r (const struct utmp *id, struct utmp *b= uffer, } file_offset +=3D sizeof (struct utmp); =20 - if (__utmp_equal (buffer, id)) + if (__utmp_equal (&last_entry, id)) break; } } @@ -265,7 +267,7 @@ __libc_getutid_r (const struct utmp *id, struct utmp *b= uffer, /* We don't have to distinguish whether we can lock the file or whether there is no entry. */ bool lock_failed =3D false; - if (internal_getut_r (id, &last_entry, &lock_failed) < 0) + if (internal_getut_r (id, &lock_failed) < 0) { *result =3D NULL; return -1; @@ -330,10 +332,9 @@ unlock_return: struct utmp * __libc_pututline (const struct utmp *data) { - if (!maybe_setutent ()) + if (!maybe_setutent () || file_offset =3D=3D -1l) return NULL; =20 - struct utmp buffer; struct utmp *pbuf; int found; =20 @@ -369,7 +370,7 @@ __libc_pututline (const struct utmp *data) else { bool lock_failed =3D false; - found =3D internal_getut_r (data, &buffer, &lock_failed); + found =3D internal_getut_r (data, &lock_failed); =20 if (__builtin_expect (lock_failed, false)) {