From patchwork Wed Mar 25 15:40:22 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Schwab X-Patchwork-Id: 5807 Received: (qmail 126527 invoked by alias); 25 Mar 2015 15:40:32 -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 124859 invoked by uid 89); 25 Mar 2015 15:40:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL, BAYES_00, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx2.suse.de From: Andreas Schwab To: Florian Weimer Cc: libc-alpha@sourceware.org Subject: Re: [PATCH] CVE-2014-8121: Fix nss_files file management [BZ#18007] References: <54EB120A.1010202@redhat.com> <5506F010.1090608@redhat.com> <871tkdtg89.fsf@mid.deneb.enyo.de> <87wq25s0dh.fsf@mid.deneb.enyo.de> <87r3sdrzo4.fsf@mid.deneb.enyo.de> X-Yow: I have the power to HALT PRODUCTION on all TEENAGE SEX COMEDIES!! Date: Wed, 25 Mar 2015 16:40:22 +0100 In-Reply-To: <87r3sdrzo4.fsf@mid.deneb.enyo.de> (Florian Weimer's message of "Wed, 25 Mar 2015 14:10:03 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4.91 (gnu/linux) MIME-Version: 1.0 You need to take a global view at the bug. The sharing of global state between getXXent and getXXbyYY is broken by design. Andreas. [BZ #18007] * nis/nss_compat/compat-grp.c (internal_endgrent): Add parameter needent, call nss_endgrent only if non-zero. (_nss_compat_endgrent): Pass non-zero. (_nss_compat_getgrnam_r, _nss_compat_getgrgid_r): Pass zero. * nis/nss_compat/compat-pwd.c (internal_endpwent): Add parameter needent, call nss_endpwent only if non-zero. (_nss_compat_endpwent): Pass non-zero. (_nss_compat_getpwnam_r, _nss_compat_getpwuid_r): Pass zero. * nis/nss_compat/compat-spwd.c (internal_setspent): Add parameter needent, call nss_setspent only if non-zero. (_nss_compat_setspent): Pass non-zero. (internal_endspent): Add parameter needent, call nss_endspent only if non-zero. (_nss_compat_endspent, _nss_compat_getspent_r): Pass non-zero. (_nss_compat_getspnam_r): Pass zero. * nss/nss_files/files-XXX.c (position, last_use, keep_stream): Remove. All uses removed. (internal_setent): Remove parameter stayopen, add parameter stream. Use it instead of global variable. (CONCAT(_nss_files_set,ENTNAME)): Pass global stream. (internal_endent, internal_getent): Add parameter stream. Use it instead of global variable. (CONCAT(_nss_files_end,ENTNAME)) (CONCAT(_nss_files_get,ENTNAME_r)): Pass global stream. (_nss_files_get##name##_r): Pass local stream. Remove locking. * nss/nss_files/files-alias.c (position, last_use): Remove. All uses removed. (internal_setent, internal_endent): Add parameter stream. Use it instead of global variable. (_nss_files_setaliasent, _nss_files_endaliasent): Pass global stream. (get_next_alias): Add parameter stream. (_nss_files_getaliasent_r): Pass global stream. (_nss_files_getaliasbyname_r): Pass local stream. Remove locking. * nss/nss_files/files-hosts.c (_nss_files_gethostbyname3_r) (_nss_files_gethostbyname4_r): Pass local stream to internal_setent, internal_getent and internal_endent. Remove locking. --- nis/nss_compat/compat-grp.c | 10 ++--- nis/nss_compat/compat-pwd.c | 10 ++--- nis/nss_compat/compat-spwd.c | 18 ++++---- nss/nss_files/files-XXX.c | 105 ++++++++++--------------------------------- nss/nss_files/files-alias.c | 86 +++++++++++------------------------ nss/nss_files/files-hosts.c | 35 +++++---------- 6 files changed, 80 insertions(+), 184 deletions(-) diff --git a/nis/nss_compat/compat-grp.c b/nis/nss_compat/compat-grp.c index 5e9c527..987d914 100644 --- a/nis/nss_compat/compat-grp.c +++ b/nis/nss_compat/compat-grp.c @@ -192,9 +192,9 @@ _nss_compat_setgrent (int stayopen) static enum nss_status -internal_endgrent (ent_t *ent) +internal_endgrent (ent_t *ent, int needent) { - if (nss_endgrent) + if (needent && nss_endgrent) nss_endgrent (); if (ent->stream != NULL) @@ -222,7 +222,7 @@ _nss_compat_endgrent (void) __libc_lock_lock (lock); - result = internal_endgrent (&ext_ent); + result = internal_endgrent (&ext_ent, 1); __libc_lock_unlock (lock); @@ -532,7 +532,7 @@ _nss_compat_getgrnam_r (const char *name, struct group *grp, if (result == NSS_STATUS_SUCCESS) result = internal_getgrnam_r (name, grp, &ent, buffer, buflen, errnop); - internal_endgrent (&ent); + internal_endgrent (&ent, 0); return result; } @@ -661,7 +661,7 @@ _nss_compat_getgrgid_r (gid_t gid, struct group *grp, if (result == NSS_STATUS_SUCCESS) result = internal_getgrgid_r (gid, grp, &ent, buffer, buflen, errnop); - internal_endgrent (&ent); + internal_endgrent (&ent, 0); return result; } diff --git a/nis/nss_compat/compat-pwd.c b/nis/nss_compat/compat-pwd.c index e3e3dbb..aec5c0f 100644 --- a/nis/nss_compat/compat-pwd.c +++ b/nis/nss_compat/compat-pwd.c @@ -309,9 +309,9 @@ _nss_compat_setpwent (int stayopen) static enum nss_status -internal_endpwent (ent_t *ent) +internal_endpwent (ent_t *ent, int needent) { - if (nss_endpwent) + if (needent && nss_endpwent) nss_endpwent (); if (ent->stream != NULL) @@ -346,7 +346,7 @@ _nss_compat_endpwent (void) __libc_lock_lock (lock); - result = internal_endpwent (&ext_ent); + result = internal_endpwent (&ext_ent, 1); __libc_lock_unlock (lock); @@ -871,7 +871,7 @@ _nss_compat_getpwnam_r (const char *name, struct passwd *pwd, if (result == NSS_STATUS_SUCCESS) result = internal_getpwnam_r (name, pwd, &ent, buffer, buflen, errnop); - internal_endpwent (&ent); + internal_endpwent (&ent, 0); return result; } @@ -1110,7 +1110,7 @@ _nss_compat_getpwuid_r (uid_t uid, struct passwd *pwd, if (result == NSS_STATUS_SUCCESS) result = internal_getpwuid_r (uid, pwd, &ent, buffer, buflen, errnop); - internal_endpwent (&ent); + internal_endpwent (&ent, 0); return result; } diff --git a/nis/nss_compat/compat-spwd.c b/nis/nss_compat/compat-spwd.c index 73c2ed3..d67c145 100644 --- a/nis/nss_compat/compat-spwd.c +++ b/nis/nss_compat/compat-spwd.c @@ -169,7 +169,7 @@ copy_spwd_changes (struct spwd *dest, struct spwd *src, } static enum nss_status -internal_setspent (ent_t *ent, int stayopen) +internal_setspent (ent_t *ent, int stayopen, int needent) { enum nss_status status = NSS_STATUS_SUCCESS; @@ -239,7 +239,7 @@ internal_setspent (ent_t *ent, int stayopen) give_spwd_free (&ent->pwd); - if (status == NSS_STATUS_SUCCESS && nss_setspent) + if (needent && status == NSS_STATUS_SUCCESS && nss_setspent) ent->setent_status = nss_setspent (stayopen); return status; @@ -256,7 +256,7 @@ _nss_compat_setspent (int stayopen) if (ni == NULL) init_nss_interface (); - result = internal_setspent (&ext_ent, stayopen); + result = internal_setspent (&ext_ent, stayopen, 1); __libc_lock_unlock (lock); @@ -265,9 +265,9 @@ _nss_compat_setspent (int stayopen) static enum nss_status -internal_endspent (ent_t *ent) +internal_endspent (ent_t *ent, int needent) { - if (nss_endspent) + if (needent && nss_endspent) nss_endspent (); if (ent->stream != NULL) @@ -303,7 +303,7 @@ _nss_compat_endspent (void) __libc_lock_lock (lock); - result = internal_endspent (&ext_ent); + result = internal_endspent (&ext_ent, 1); __libc_lock_unlock (lock); @@ -658,7 +658,7 @@ _nss_compat_getspent_r (struct spwd *pwd, char *buffer, size_t buflen, init_nss_interface (); if (ext_ent.stream == NULL) - result = internal_setspent (&ext_ent, 1); + result = internal_setspent (&ext_ent, 1, 1); if (result == NSS_STATUS_SUCCESS) result = internal_getspent_r (pwd, &ext_ent, buffer, buflen, errnop); @@ -830,12 +830,12 @@ _nss_compat_getspnam_r (const char *name, struct spwd *pwd, __libc_lock_unlock (lock); - result = internal_setspent (&ent, 0); + result = internal_setspent (&ent, 0, 0); if (result == NSS_STATUS_SUCCESS) result = internal_getspnam_r (name, pwd, &ent, buffer, buflen, errnop); - internal_endspent (&ent); + internal_endspent (&ent, 0); return result; } diff --git a/nss/nss_files/files-XXX.c b/nss/nss_files/files-XXX.c index a7a45e5..aabd4ea 100644 --- a/nss/nss_files/files-XXX.c +++ b/nss/nss_files/files-XXX.c @@ -63,21 +63,18 @@ __libc_lock_define_initialized (static, lock) /* Maintenance of the shared stream open on the database file. */ static FILE *stream; -static fpos_t position; -static enum { nouse, getent, getby } last_use; -static int keep_stream; /* Open database file if not already opened. */ static enum nss_status -internal_setent (int stayopen) +internal_setent (FILE **stream) { enum nss_status status = NSS_STATUS_SUCCESS; - if (stream == NULL) + if (*stream == NULL) { - stream = fopen (DATAFILE, "rce"); + *stream = fopen (DATAFILE, "rce"); - if (stream == NULL) + if (*stream == NULL) status = errno == EAGAIN ? NSS_STATUS_TRYAGAIN : NSS_STATUS_UNAVAIL; else { @@ -90,7 +87,7 @@ internal_setent (int stayopen) int result; int flags; - result = flags = fcntl (fileno (stream), F_GETFD, 0); + result = flags = fcntl (fileno (*stream), F_GETFD, 0); if (result >= 0) { # ifdef O_CLOEXEC @@ -100,15 +97,15 @@ internal_setent (int stayopen) # endif { flags |= FD_CLOEXEC; - result = fcntl (fileno (stream), F_SETFD, flags); + result = fcntl (fileno (*stream), F_SETFD, flags); } } if (result < 0) { /* Something went wrong. Close the stream and return a failure. */ - fclose (stream); - stream = NULL; + fclose (*stream); + *stream = NULL; status = NSS_STATUS_UNAVAIL; } } @@ -116,11 +113,7 @@ internal_setent (int stayopen) } } else - rewind (stream); - - /* Remember STAYOPEN flag. */ - if (stream != NULL) - keep_stream |= stayopen; + rewind (*stream); return status; } @@ -134,16 +127,7 @@ CONCAT(_nss_files_set,ENTNAME) (int stayopen) __libc_lock_lock (lock); - status = internal_setent (stayopen); - - if (status == NSS_STATUS_SUCCESS && fgetpos (stream, &position) < 0) - { - fclose (stream); - stream = NULL; - status = NSS_STATUS_UNAVAIL; - } - - last_use = getent; + status = internal_setent (&stream); __libc_lock_unlock (lock); @@ -153,12 +137,12 @@ CONCAT(_nss_files_set,ENTNAME) (int stayopen) /* Close the database file. */ static void -internal_endent (void) +internal_endent (FILE **stream) { - if (stream != NULL) + if (*stream != NULL) { - fclose (stream); - stream = NULL; + fclose (*stream); + *stream = NULL; } } @@ -169,10 +153,7 @@ CONCAT(_nss_files_end,ENTNAME) (void) { __libc_lock_lock (lock); - internal_endent (); - - /* Reset STAYOPEN flag. */ - keep_stream = 0; + internal_endent (&stream); __libc_lock_unlock (lock); @@ -227,7 +208,7 @@ get_contents (char *linebuf, size_t len, FILE *stream) /* Parsing the database file into `struct STRUCTURE' data structures. */ static enum nss_status -internal_getent (struct STRUCTURE *result, +internal_getent (FILE *stream, struct STRUCTURE *result, char *buffer, size_t buflen, int *errnop H_ERRNO_PROTO EXTRA_ARGS_DECL) { @@ -300,45 +281,14 @@ CONCAT(_nss_files_get,ENTNAME_r) (struct STRUCTURE *result, char *buffer, { int save_errno = errno; - status = internal_setent (0); + status = internal_setent (&stream); __set_errno (save_errno); - - if (status == NSS_STATUS_SUCCESS && fgetpos (stream, &position) < 0) - { - fclose (stream); - stream = NULL; - status = NSS_STATUS_UNAVAIL; - } } if (status == NSS_STATUS_SUCCESS) - { - /* If the last use was not by the getent function we need the - position the stream. */ - if (last_use != getent) - { - if (fsetpos (stream, &position) < 0) - status = NSS_STATUS_UNAVAIL; - else - last_use = getent; - } - - if (status == NSS_STATUS_SUCCESS) - { - status = internal_getent (result, buffer, buflen, errnop - H_ERRNO_ARG EXTRA_ARGS_VALUE); - - /* Remember this position if we were successful. If the - operation failed we give the user a chance to repeat the - operation (perhaps the buffer was too small). */ - if (status == NSS_STATUS_SUCCESS) - fgetpos (stream, &position); - else - /* We must make sure we reposition the stream the next call. */ - last_use = nouse; - } - } + status = internal_getent (stream, result, buffer, buflen, errnop + H_ERRNO_ARG EXTRA_ARGS_VALUE); __libc_lock_unlock (lock); @@ -364,27 +314,20 @@ _nss_files_get##name##_r (proto, \ size_t buflen, int *errnop H_ERRNO_PROTO) \ { \ enum nss_status status; \ + FILE *stream = NULL; \ \ - __libc_lock_lock (lock); \ - \ - /* Reset file pointer to beginning or open file. */ \ - status = internal_setent (keep_stream); \ + /* Open file. */ \ + status = internal_setent (&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 \ + while ((status = internal_getent (stream, result, buffer, buflen, errnop \ H_ERRNO_ARG EXTRA_ARGS_VALUE)) \ == NSS_STATUS_SUCCESS) \ { break_if_match } \ \ - if (! keep_stream) \ - internal_endent (); \ + internal_endent (&stream); \ } \ \ - __libc_lock_unlock (lock); \ - \ return status; \ } diff --git a/nss/nss_files/files-alias.c b/nss/nss_files/files-alias.c index d5c5089..a043831 100644 --- a/nss/nss_files/files-alias.c +++ b/nss/nss_files/files-alias.c @@ -36,20 +36,18 @@ __libc_lock_define_initialized (static, lock) /* Maintenance of the shared stream open on the database file. */ static FILE *stream; -static fpos_t position; -static enum { nouse, getent, getby } last_use; static enum nss_status -internal_setent (void) +internal_setent (FILE **stream) { enum nss_status status = NSS_STATUS_SUCCESS; - if (stream == NULL) + if (*stream == NULL) { - stream = fopen ("/etc/aliases", "rce"); + *stream = fopen ("/etc/aliases", "rce"); - if (stream == NULL) + if (*stream == NULL) status = errno == EAGAIN ? NSS_STATUS_TRYAGAIN : NSS_STATUS_UNAVAIL; else { @@ -62,7 +60,7 @@ internal_setent (void) int result; int flags; - result = flags = fcntl (fileno (stream), F_GETFD, 0); + result = flags = fcntl (fileno (*stream), F_GETFD, 0); if (result >= 0) { # ifdef O_CLOEXEC @@ -72,14 +70,14 @@ internal_setent (void) # endif { flags |= FD_CLOEXEC; - result = fcntl (fileno (stream), F_SETFD, flags); + result = fcntl (fileno (*stream), F_SETFD, flags); } } if (result < 0) { /* Something went wrong. Close the stream and return a failure. */ - fclose (stream); + fclose (*stream); stream = NULL; status = NSS_STATUS_UNAVAIL; } @@ -88,7 +86,7 @@ internal_setent (void) } } else - rewind (stream); + rewind (*stream); return status; } @@ -102,16 +100,7 @@ _nss_files_setaliasent (void) __libc_lock_lock (lock); - status = internal_setent (); - - if (status == NSS_STATUS_SUCCESS && fgetpos (stream, &position) < 0) - { - fclose (stream); - stream = NULL; - status = NSS_STATUS_UNAVAIL; - } - - last_use = getent; + status = internal_setent (&stream); __libc_lock_unlock (lock); @@ -121,12 +110,12 @@ _nss_files_setaliasent (void) /* Close the database file. */ static void -internal_endent (void) +internal_endent (FILE **stream) { - if (stream != NULL) + if (*stream != NULL) { - fclose (stream); - stream = NULL; + fclose (*stream); + *stream = NULL; } } @@ -137,7 +126,7 @@ _nss_files_endaliasent (void) { __libc_lock_lock (lock); - internal_endent (); + internal_endent (&stream); __libc_lock_unlock (lock); @@ -146,7 +135,7 @@ _nss_files_endaliasent (void) /* Parsing the database file into `struct aliasent' data structures. */ static enum nss_status -get_next_alias (const char *match, struct aliasent *result, +get_next_alias (FILE *stream, const char *match, struct aliasent *result, char *buffer, size_t buflen, int *errnop) { enum nss_status status = NSS_STATUS_NOTFOUND; @@ -397,35 +386,16 @@ _nss_files_getaliasent_r (struct aliasent *result, char *buffer, size_t buflen, /* Be prepared that the set*ent function was not called before. */ if (stream == NULL) - status = internal_setent (); + status = internal_setent (&stream); if (status == NSS_STATUS_SUCCESS) { - /* If the last use was not by the getent function we need the - position the stream. */ - if (last_use != getent) - { - if (fsetpos (stream, &position) < 0) - status = NSS_STATUS_UNAVAIL; - else - last_use = getent; - } + result->alias_local = 1; - if (status == NSS_STATUS_SUCCESS) - { - result->alias_local = 1; - - /* Read lines until we get a definite result. */ - do - status = get_next_alias (NULL, result, buffer, buflen, errnop); - while (status == NSS_STATUS_RETURN); - - /* If we successfully read an entry remember this position. */ - if (status == NSS_STATUS_SUCCESS) - fgetpos (stream, &position); - else - last_use = nouse; - } + /* Read lines until we get a definite result. */ + do + status = get_next_alias (stream, NULL, result, buffer, buflen, errnop); + while (status == NSS_STATUS_RETURN); } __libc_lock_unlock (lock); @@ -440,6 +410,7 @@ _nss_files_getaliasbyname_r (const char *name, struct aliasent *result, { /* Return next entry in host file. */ enum nss_status status = NSS_STATUS_SUCCESS; + FILE *stream = NULL; if (name == NULL) { @@ -447,11 +418,8 @@ _nss_files_getaliasbyname_r (const char *name, struct aliasent *result, return NSS_STATUS_UNAVAIL; } - __libc_lock_lock (lock); - - /* Open the stream or rest it. */ - status = internal_setent (); - last_use = getby; + /* Open the stream. */ + status = internal_setent (&stream); if (status == NSS_STATUS_SUCCESS) { @@ -459,13 +427,11 @@ _nss_files_getaliasbyname_r (const char *name, struct aliasent *result, /* Read lines until we get a definite result. */ do - status = get_next_alias (name, result, buffer, buflen, errnop); + status = get_next_alias (stream, name, result, buffer, buflen, errnop); while (status == NSS_STATUS_RETURN); } - internal_endent (); - - __libc_lock_unlock (lock); + internal_endent (&stream); return status; } diff --git a/nss/nss_files/files-hosts.c b/nss/nss_files/files-hosts.c index 4c51c90..4117458 100644 --- a/nss/nss_files/files-hosts.c +++ b/nss/nss_files/files-hosts.c @@ -120,14 +120,13 @@ _nss_files_gethostbyname3_r (const char *name, int af, struct hostent *result, char *buffer, size_t buflen, int *errnop, int *herrnop, int32_t *ttlp, char **canonp) { + FILE *stream = NULL; uintptr_t pad = -(uintptr_t) buffer % __alignof__ (struct hostent_data); buffer += pad; buflen = buflen > pad ? buflen - pad : 0; - __libc_lock_lock (lock); - - /* Reset file pointer to beginning or open file. */ - enum nss_status status = internal_setent (keep_stream); + /* Open file. */ + enum nss_status status = internal_setent (&stream); if (status == NSS_STATUS_SUCCESS) { @@ -135,10 +134,7 @@ _nss_files_gethostbyname3_r (const char *name, int af, struct hostent *result, addresses to IPv6 addresses really the right thing to do? */ int flags = ((_res.options & RES_USE_INET6) ? AI_V4MAPPED : 0); - /* Tell getent function that we have repositioned the file pointer. */ - last_use = getby; - - while ((status = internal_getent (result, buffer, buflen, errnop, + while ((status = internal_getent (stream, result, buffer, buflen, errnop, herrnop, af, flags)) == NSS_STATUS_SUCCESS) { @@ -165,7 +161,7 @@ _nss_files_gethostbyname3_r (const char *name, int af, struct hostent *result, bufferend = (char *) &result->h_aliases[naliases + 1]; again: - while ((status = internal_getent (&tmp_result_buf, tmp_buffer, + while ((status = internal_getent (stream, &tmp_result_buf, tmp_buffer, tmp_buflen, errnop, herrnop, af, flags)) == NSS_STATUS_SUCCESS) @@ -341,15 +337,12 @@ _nss_files_gethostbyname3_r (const char *name, int af, struct hostent *result, free (tmp_buffer); } - if (! keep_stream) - internal_endent (); + internal_endent (&stream); } if (canonp && status == NSS_STATUS_SUCCESS) *canonp = result->h_name; - __libc_lock_unlock (lock); - return status; } @@ -378,16 +371,13 @@ _nss_files_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat, char *buffer, size_t buflen, int *errnop, int *herrnop, int32_t *ttlp) { - __libc_lock_lock (lock); + FILE *stream = NULL; - /* Reset file pointer to beginning or open file. */ - enum nss_status status = internal_setent (keep_stream); + /* Open file. */ + enum nss_status status = internal_setent (&stream); if (status == NSS_STATUS_SUCCESS) { - /* Tell getent function that we have repositioned the file pointer. */ - last_use = getby; - bool any = false; bool got_canon = false; while (1) @@ -399,7 +389,7 @@ _nss_files_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat, buflen = buflen > pad ? buflen - pad : 0; struct hostent result; - status = internal_getent (&result, buffer, buflen, errnop, + status = internal_getent (stream, &result, buffer, buflen, errnop, herrnop, AF_UNSPEC, 0); if (status != NSS_STATUS_SUCCESS) break; @@ -475,8 +465,7 @@ _nss_files_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat, status = NSS_STATUS_SUCCESS; } - if (! keep_stream) - internal_endent (); + internal_endent (&stream); } else if (status == NSS_STATUS_TRYAGAIN) { @@ -489,7 +478,5 @@ _nss_files_gethostbyname4_r (const char *name, struct gaih_addrtuple **pat, *herrnop = NO_DATA; } - __libc_lock_unlock (lock); - return status; }