From patchwork Mon Oct 19 11:22:18 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Aurelien Jarno X-Patchwork-Id: 9224 Received: (qmail 52477 invoked by alias); 19 Oct 2015 11:22:28 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 52465 invoked by uid 89); 19 Oct 2015 11:22:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: hall.aurel32.net From: Aurelien Jarno To: libc-alpha@sourceware.org Cc: Florian Weimer Subject: [COMMITTED 2.19] CVE-2014-8121: Do not close NSS files database during iteration [BZ #18007] Date: Mon, 19 Oct 2015 13:22:18 +0200 Message-Id: <1445253738-6087-1-git-send-email-aurelien@aurel32.net> MIME-Version: 1.0 From: Florian Weimer Robin Hack discovered Samba would enter an infinite loop processing certain quota-related requests. We eventually tracked this down to a glibc issue. Running a (simplified) test case under strace shows that /etc/passwd is continuously opened and closed: … open("/etc/passwd", O_RDONLY|O_CLOEXEC) = 3 lseek(3, 0, SEEK_CUR) = 0 read(3, "root:x:0:0:root:/root:/bin/bash\n"..., 4096) = 2717 lseek(3, 2717, SEEK_SET) = 2717 close(3) = 0 open("/etc/passwd", O_RDONLY|O_CLOEXEC) = 3 lseek(3, 0, SEEK_CUR) = 0 lseek(3, 0, SEEK_SET) = 0 read(3, "root:x:0:0:root:/root:/bin/bash\n"..., 4096) = 2717 lseek(3, 2717, SEEK_SET) = 2717 close(3) = 0 open("/etc/passwd", O_RDONLY|O_CLOEXEC) = 3 lseek(3, 0, SEEK_CUR) = 0 … The lookup function implementation in nss/nss_files/files-XXX.c:DB_LOOKUP has code to prevent that. It is supposed skip closing the input file if it was already open. /* Reset file pointer to beginning or open file. */ \ status = internal_setent (keep_stream); \ \ if (status == NSS_STATUS_SUCCESS) \ { \ /* Tell getent function that we have repositioned the file pointer. */ \ last_use = getby; \ \ while ((status = internal_getent (result, buffer, buflen, errnop \ H_ERRNO_ARG EXTRA_ARGS_VALUE)) \ == NSS_STATUS_SUCCESS) \ { break_if_match } \ \ if (! keep_stream) \ internal_endent (); \ } \ keep_stream is initialized from the stayopen flag in internal_setent. internal_setent is called from the set*ent implementation as: status = internal_setent (stayopen); However, for non-host database, this flag is always 0, per the STAYOPEN magic in nss/getXXent_r.c. Thus, the fix is this: - status = internal_setent (stayopen); + status = internal_setent (1); This is not a behavioral change even for the hosts database (where the application can specify the stayopen flag) because with a call to sethostent(0), the file handle is still not closed in the implementation of gethostent. (cherry picked from commit 03d2730b44cc2236318fd978afa2651753666c55) Conflicts: ChangeLog NEWS --- ChangeLog | 8 ++++ NEWS | 7 ++- nss/Makefile | 2 +- nss/nss_files/files-XXX.c | 2 +- nss/tst-nss-getpwent.c | 118 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 134 insertions(+), 3 deletions(-) create mode 100644 nss/tst-nss-getpwent.c diff --git a/ChangeLog b/ChangeLog index 8d621b2..a7207b1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2015-04-29 Florian Weimer + + [BZ #18007] + * nss/nss_files/files-XXX.c (CONCAT): Always enable stayopen. + (CVE-2014-8121) + * nss/tst-nss-getpwent.c: New file. + * nss/Makefile (tests): Add new test. + 2015-02-22 Paul Pluzhnikov [BZ #17269] diff --git a/NEWS b/NEWS index 9e7316b..e00543f 100644 --- a/NEWS +++ b/NEWS @@ -11,7 +11,7 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17069, 17079, 17137, 17153, 17213, - 17263, 17269, 17325, 17555, 18032, 18287. + 17263, 17269, 17325, 17555, 18007, 18032, 18287. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a @@ -57,6 +57,11 @@ Version 2.19.1 IBM937, IBM939, IBM1364 could result in an out-of-bounds array read, resulting a denial-of-service security vulnerability in applications which use functions related to iconv. (CVE-2014-6040) + +* CVE-2014-8121 The NSS files backend would reset the file pointer used by + the get*ent functions if any of the query functions for the same database + are used during the iteration, causing a denial-of-service condition in + some applications. Version 2.19 diff --git a/nss/Makefile b/nss/Makefile index c8880c0..3f9d2d0 100644 --- a/nss/Makefile +++ b/nss/Makefile @@ -37,7 +37,7 @@ install-bin := getent makedb makedb-modules = xmalloc hash-string extra-objs += $(makedb-modules:=.o) -tests = test-netdb tst-nss-test1 test-digits-dots +tests = test-netdb tst-nss-test1 test-digits-dots tst-nss-getpwent xtests = bug-erange include ../Makeconfig diff --git a/nss/nss_files/files-XXX.c b/nss/nss_files/files-XXX.c index d4cd95e..3b90f7e 100644 --- a/nss/nss_files/files-XXX.c +++ b/nss/nss_files/files-XXX.c @@ -134,7 +134,7 @@ CONCAT(_nss_files_set,ENTNAME) (int stayopen) __libc_lock_lock (lock); - status = internal_setent (stayopen); + status = internal_setent (1); if (status == NSS_STATUS_SUCCESS && fgetpos (stream, &position) < 0) { diff --git a/nss/tst-nss-getpwent.c b/nss/tst-nss-getpwent.c new file mode 100644 index 0000000..f2e8abc --- /dev/null +++ b/nss/tst-nss-getpwent.c @@ -0,0 +1,118 @@ +/* Copyright (C) 2015 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; if not, see + . */ + +#include +#include +#include +#include +#include + +int +do_test (void) +{ + /* Count the number of entries in the password database, and fetch + data from the first and last entries. */ + size_t count = 0; + struct passwd * pw; + char *first_name = NULL; + uid_t first_uid = 0; + char *last_name = NULL; + uid_t last_uid = 0; + setpwent (); + while ((pw = getpwent ()) != NULL) + { + if (first_name == NULL) + { + first_name = strdup (pw->pw_name); + if (first_name == NULL) + { + printf ("strdup: %m\n"); + return 1; + } + first_uid = pw->pw_uid; + } + + free (last_name); + last_name = strdup (pw->pw_name); + if (last_name == NULL) + { + printf ("strdup: %m\n"); + return 1; + } + last_uid = pw->pw_uid; + ++count; + } + endpwent (); + + if (count == 0) + { + printf ("No entries in the password database.\n"); + return 0; + } + + /* Try again, this time interleaving with name-based and UID-based + lookup operations. The counts do not match if the interleaved + lookups affected the enumeration. */ + size_t new_count = 0; + setpwent (); + while ((pw = getpwent ()) != NULL) + { + if (new_count == count) + { + printf ("Additional entry in the password database.\n"); + return 1; + } + ++new_count; + struct passwd *pw2 = getpwnam (first_name); + if (pw2 == NULL) + { + printf ("getpwnam (%s) failed: %m\n", first_name); + return 1; + } + pw2 = getpwnam (last_name); + if (pw2 == NULL) + { + printf ("getpwnam (%s) failed: %m\n", last_name); + return 1; + } + pw2 = getpwuid (first_uid); + if (pw2 == NULL) + { + printf ("getpwuid (%llu) failed: %m\n", + (unsigned long long) first_uid); + return 1; + } + pw2 = getpwuid (last_uid); + if (pw2 == NULL) + { + printf ("getpwuid (%llu) failed: %m\n", + (unsigned long long) last_uid); + return 1; + } + } + endpwent (); + if (new_count < count) + { + printf ("Missing entry in the password database.\n"); + return 1; + } + + return 0; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c"