Hi Florian, hi Aurelien,
On Thu, Aug 6, 2020 at 12:52 PM Florian Weimer <[email protected]> wrote: > > * Aurelien Jarno: > > > On 2020-08-06 06:08, Jinpu Wang wrote: > >> Hi Florian, > >> > >> On Wed, Aug 5, 2020 at 6:44 PM Florian Weimer <[email protected]> wrote: > >> > > >> > * Jinpu Wang: > >> > > >> > > Dear Maintainer: > >> > > > >> > > Sorry, add some missing information below: > >> > > > >> > > After update to Buster, the systemd-sysusers are segfaulting every > >> > > time. > >> > > After search around, I found following bugreport in glibc > >> > > https://sourceware.org/legacy-ml/libc-alpha/2016-06/msg01015.html > >> > > > >> > > I backported to the fix to 2.28-10, it fixed the problem. > >> > > > >> > > glibc upstream have a different fix for it in 2.32, see > >> > > https://sourceware.org/bugzilla/show_bug.cgi?id=20338 > >> > > > >> > > I think it's still easier to backport the fix in msg01015.html to 2.28 > >> > > version, > >> > > patch attached in the initial report. > >> > > >> > The patch from 2016 is incomplete because it does not seek back to the > >> > original file position, so the next call of fgetsgent_r skips over the > >> > entry that could not be fully parsed. > >> Thanks for quick response, can you provide a minimum bugfix, which > >> can be easily backported to old version like 2.28? > > > > I think we do not want to diverge from the upstream fix, even if it is a > > bit more work to backport. We first need to fix it in bullseye/sid and > > then we can try to get this in the next buster stable release. > > I can backport it to upstream release branches, all the way to version > 2.28. Would that help? > > I plan to add local copies of the new functions, so that the > GLIBC_PRIVATE ABI remains unchanged. I did a backport of your fixes from glibc 2.32 to Buster 2.28, as patch attached. I tested with systemd-sysusers, it no longer SEGV. Can you please review it? > > But I have other commitments, so that may not happen until > September-ish. > > >> as you also make the bug 20338 as a security hole. > > > > It is marked as "security-", so it is *not* considered as a security > > issue (as the content of this file is trusted). > > That's right. Thanks! Jinpu
From 2461138a872c868d14522553cab8a34994ebdc0e Mon Sep 17 00:00:00 2001 From: Jack Wang <[email protected]> Date: Tue, 4 Aug 2020 15:05:27 +0200 Subject: [PATCH 1/3] gshadow: Handle the parser's full buffer error code The fgetgsent function isn't handling errors from parse_line. That means it can run out of buffer space when adding pointers to group members and exit early without setting all members of the static result struct. The static result's members will remain pointing at buffer locations from the previous line, which have been overwritten with incompatible data, causing segfaults after it is returned normally. https://sourceware.org/legacy-ml/libc-alpha/2016-06/msg01015.html https://sourceware.org/bugzilla/show_bug.cgi?id=20338 add local copies of the new functions, so that the GLIBC_PRIVATE ABI remains unchanged, as suggested by Florian Weimer <[email protected]> Signed-off-by: Jack Wang <[email protected]> --- gshadow/fgetsgent_r.c | 183 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 149 insertions(+), 34 deletions(-) diff --git a/gshadow/fgetsgent_r.c b/gshadow/fgetsgent_r.c index 13a5b181cb9b..7cd4c795ff51 100644 --- a/gshadow/fgetsgent_r.c +++ b/gshadow/fgetsgent_r.c @@ -19,6 +19,8 @@ #include <errno.h> #include <gshadow.h> #include <stdio.h> +#include <string.h> +#include <assert.h> /* Define a line parsing function using the common code used in the nss_files module. */ @@ -30,46 +32,159 @@ struct sgent_data {}; #include <nss/nss_files/files-parse.c> +/* Set the error indicator on FP. */ +static inline void +fseterr_unlocked (FILE *fp) +{ + fp->_flags |= _IO_ERR_SEEN; +} -/* Read one shadow entry from the given stream. */ -int -__fgetsgent_r (FILE *stream, struct sgrp *resbuf, char *buffer, size_t buflen, - struct sgrp **result) +static int +__nss_readline_seek (FILE *fp, off64_t offset) { - char *p; + if (offset < 0 /* __ftello64 failed. */ + || __fseeko64 (fp, offset, SEEK_SET) < 0) + { + /* Without seeking support, it is not possible to + re-read the same line, so this is a hard failure. */ + fseterr_unlocked (fp); + __set_errno (ESPIPE); + return ESPIPE; + } + else + { + __set_errno (ERANGE); + return ERANGE; + } +} - _IO_flockfile (stream); - do +static int +__nss_readline (FILE *fp, char *buf, size_t len, off64_t *poffset) +{ + /* We need space for at least one character, the line terminator, + and the NUL byte. */ + if (len < 3) { - buffer[buflen - 1] = '\xff'; - p = fgets_unlocked (buffer, buflen, stream); - if (p == NULL && feof_unlocked (stream)) - { - _IO_funlockfile (stream); - *result = NULL; - __set_errno (ENOENT); - return errno; - } - if (p == NULL || buffer[buflen - 1] != '\xff') - { - _IO_funlockfile (stream); - *result = NULL; - __set_errno (ERANGE); - return errno; - } - - /* Skip leading blanks. */ + *poffset = -1; + __set_errno (ERANGE); + return ERANGE; + } + + while (true) + { + /* Keep original offset for retries. */ + *poffset = __ftello64 (fp); + + buf[len - 1] = '\xff'; /* Marker to recognize truncation. */ + if (fgets_unlocked (buf, len, fp) == NULL) + { + if (feof_unlocked (fp)) + { + __set_errno (ENOENT); + return ENOENT; + } + else + { + /* Any other error. Do not return ERANGE in this case + because the caller would retry. */ + if (errno == ERANGE) + __set_errno (EINVAL); + return errno; + } + } + else if (buf[len - 1] != '\xff') + /* The buffer is too small. Arrange for re-reading the same + line on the next call. */ + return __nss_readline_seek (fp, *poffset); + + /* fgets_unlocked succeeded. */ + + /* Remove leading whitespace. */ + char *p = buf; while (isspace (*p)) - ++p; - } while (*p == '\0' || *p == '#' || /* Ignore empty and comment lines. */ - /* Parse the line. If it is invalid, loop to - get the next line of the file to parse. */ - ! parse_line (buffer, (void *) resbuf, (void *) buffer, buflen, - &errno)); + ++p; + if (*p == '\0' || *p == '#') + /* Skip empty lines and comments. */ + continue; + if (p != buf) + memmove (buf, p, strlen (p)); - _IO_funlockfile (stream); + /* Return line to the caller. */ + return 0; + } +} + + +static int +__nss_parse_line_result (FILE *fp, off64_t offset, int parse_line_result) +{ + assert (parse_line_result >= -1 && parse_line_result <= 1); - *result = resbuf; - return 0; + switch (__builtin_expect (parse_line_result, 1)) + { + case 1: + /* Sucess. */ + return 0; + case 0: + /* Parse error. */ + __set_errno (EINVAL); + return EINVAL; + case -1: + /* Out of buffer space. */ + return __nss_readline_seek (fp, offset); + + default: + __builtin_unreachable (); + } +} +typedef int nss_files_parse_line (char *line, struct STRUCTURE *result, + struct parser_data *data, + size_t datalen, int *errnop); +static int +__nss_fgetent_r (FILE *fp, void *result, char *buffer, size_t buffer_length, + nss_files_parse_line parser) +{ + int ret; + + _IO_flockfile (fp); + + while (true) + { + off64_t original_offset; + ret = __nss_readline (fp, buffer, buffer_length, &original_offset); + if (ret == 0) + { + /* Parse the line into *RESULT. */ + ret = parser (buffer, result, + (struct parser_data *) buffer, buffer_length, &errno); + + /* Translate the result code from the parser into an errno + value. Also seeks back to the start of the line if + necessary. */ + ret = __nss_parse_line_result (fp, original_offset, ret); + + if (ret == EINVAL) + /* Skip over malformed lines. */ + continue; + } + break; + } + + _IO_funlockfile (fp); + + return ret; +} + +/* Read one shadow entry from the given stream. */ +int +__fgetsgent_r (FILE *stream, struct sgrp *resbuf, char *buffer, size_t buflen, + struct sgrp **result) +{ + int ret = __nss_fgetent_r (stream, resbuf, buffer, buflen, parse_line); + if (ret == 0) + *result = resbuf; + else + *result = NULL; + return ret; } weak_alias (__fgetsgent_r, fgetsgent_r) -- 2.25.1
