From 2f3bd411aefa9747f17740e9ab06676d51241098 Mon Sep 17 00:00:00 2001 From: Siddhesh Poyarekar Date: Thu, 27 Mar 2014 07:15:22 +0530 Subject: Fix nscd lookup for innetgr when netgroup has wildcards (BZ #16758) nscd works correctly when the request in innetgr is a wildcard, i.e. when one or more of host, user or domain parameters is NULL. However, it does not work when the the triplet in the netgroup definition has a wildcard. This is easy to reproduce for a triplet defined as follows: foonet (,foo,) Here, an innetgr call that looks like this: innetgr ("foonet", "foohost", "foo", NULL); should succeed and so should: innetgr ("foonet", NULL, "foo", "foodomain"); It does succeed with nscd disabled, but not with nscd enabled. This fix adds this additional check for all three parts of the triplet so that it gives the correct result. [BZ #16758] * nscd/netgroupcache.c (addinnetgrX): Succeed if triplet has blank values. (cherry picked from commit fbd6b5a4052316f7eb03c4617eebfaafc59dcc06) --- ChangeLog | 6 ++++++ NEWS | 6 +++--- nscd/netgroupcache.c | 10 +++++++--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index 4502ab2b78..3cb4c4d2a1 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2015-12-20 Siddhesh Poyarekar + + [BZ #16758] + * nscd/netgroupcache.c (addinnetgrX): Succeed if triplet has + blank values. + 2015-11-24 Andreas Schwab [BZ #17062] diff --git a/NEWS b/NEWS index c9cce289ed..9771c0792d 100644 --- a/NEWS +++ b/NEWS @@ -9,9 +9,9 @@ Version 2.19.1 * The following bugs are resolved with this release: - 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16878, 16882, 16885, - 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, - 17213, 17263, 17269, 17325, 17555, 18007, 18032, 18287. + 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16878, 16882, + 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, + 17153, 17213, 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 diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c index 084f74df2f..8c619eab31 100644 --- a/nscd/netgroupcache.c +++ b/nscd/netgroupcache.c @@ -562,15 +562,19 @@ addinnetgrX (struct database_dyn *db, int fd, request_header *req, { bool success = true; - if (host != NULL) + /* For the host, user and domain in each triplet, we assume success + if the value is blank because that is how the wildcard entry to + match anything is stored in the netgroup cache. */ + if (host != NULL && *triplets != '\0') success = strcmp (host, triplets) == 0; triplets = (const char *) rawmemchr (triplets, '\0') + 1; - if (success && user != NULL) + if (success && user != NULL && *triplets != '\0') success = strcmp (user, triplets) == 0; triplets = (const char *) rawmemchr (triplets, '\0') + 1; - if (success && (domain == NULL || strcmp (domain, triplets) == 0)) + if (success && (domain == NULL || *triplets == '\0' + || strcmp (domain, triplets) == 0)) { dataset->resp.result = 1; break; -- cgit 1.4.1 From 56b2cf5633f90c722b8f4ed257311b23ebed7399 Mon Sep 17 00:00:00 2001 From: Siddhesh Poyarekar Date: Thu, 27 Mar 2014 19:49:51 +0530 Subject: Return NULL for wildcard values in getnetgrent from nscd (BZ #16759) getnetgrent is supposed to return NULL for values that are wildcards in the (host, user, domain) triplet. This works correctly with nscd disabled, but with it enabled, it returns a blank ("") instead of a NULL. This is easily seen with the output of `getent netgroup foonet` for a netgroup foonet defined as follows in /etc/netgroup: foonet (,foo,) The output with nscd disabled is: foonet ( ,foo,) while with nscd enabled, it is: foonet (,foo,) The extra space with nscd disabled is due to the fact that `getent netgroup` adds it if the return value from getnetgrent is NULL for either host or user. (cherry picked from commit dd3022d75e6fb8957843d6d84257a5d8457822d5) --- ChangeLog | 4 ++++ NEWS | 6 +++--- inet/getnetgrent_r.c | 14 +++++++++++--- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/ChangeLog b/ChangeLog index 3cb4c4d2a1..896b564707 100644 --- a/ChangeLog +++ b/ChangeLog @@ -4,6 +4,10 @@ * nscd/netgroupcache.c (addinnetgrX): Succeed if triplet has blank values. + [BZ #16759] + * inet/getnetgrent_r.c (get_nonempty_val): New function. + (nscd_getnetgrent): Use it. + 2015-11-24 Andreas Schwab [BZ #17062] diff --git a/NEWS b/NEWS index 9771c0792d..6f295a201a 100644 --- a/NEWS +++ b/NEWS @@ -9,9 +9,9 @@ Version 2.19.1 * The following bugs are resolved with this release: - 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16878, 16882, - 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, - 17153, 17213, 17263, 17269, 17325, 17555, 18007, 18032, 18287. + 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16878, + 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, + 17137, 17153, 17213, 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 diff --git a/inet/getnetgrent_r.c b/inet/getnetgrent_r.c index 62cdfda9cb..f6d064dbb5 100644 --- a/inet/getnetgrent_r.c +++ b/inet/getnetgrent_r.c @@ -235,6 +235,14 @@ endnetgrent (void) } #ifdef USE_NSCD +static const char * +get_nonempty_val (const char *in) +{ + if (*in == '\0') + return NULL; + return in; +} + static enum nss_status nscd_getnetgrent (struct __netgrent *datap, char *buffer, size_t buflen, int *errnop) @@ -243,11 +251,11 @@ nscd_getnetgrent (struct __netgrent *datap, char *buffer, size_t buflen, return NSS_STATUS_UNAVAIL; datap->type = triple_val; - datap->val.triple.host = datap->cursor; + datap->val.triple.host = get_nonempty_val (datap->cursor); datap->cursor = (char *) __rawmemchr (datap->cursor, '\0') + 1; - datap->val.triple.user = datap->cursor; + datap->val.triple.user = get_nonempty_val (datap->cursor); datap->cursor = (char *) __rawmemchr (datap->cursor, '\0') + 1; - datap->val.triple.domain = datap->cursor; + datap->val.triple.domain = get_nonempty_val (datap->cursor); datap->cursor = (char *) __rawmemchr (datap->cursor, '\0') + 1; return NSS_STATUS_SUCCESS; -- cgit 1.4.1 From b963026c07a304bcfcf56ad5ee9b4f0797c7d3df Mon Sep 17 00:00:00 2001 From: Siddhesh Poyarekar Date: Thu, 27 Mar 2014 19:48:15 +0530 Subject: Avoid overlapping addresses to stpcpy calls in nscd (BZ #16760) Calls to stpcpy from nscd netgroups code will have overlapping source and destination when all three values in the returned triplet are non-NULL and in the expected (host,user,domain) order. This is seen in valgrind as: ==3181== Source and destination overlap in stpcpy(0x19973b48, 0x19973b48) ==3181== at 0x4C2F30A: stpcpy (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==3181== by 0x12567A: addgetnetgrentX (string3.h:111) ==3181== by 0x12722D: addgetnetgrent (netgroupcache.c:665) ==3181== by 0x11114C: nscd_run_worker (connections.c:1338) ==3181== by 0x4E3C102: start_thread (pthread_create.c:309) ==3181== by 0x59B81AC: clone (clone.S:111) ==3181== Fix this by using memmove instead of stpcpy. (cherry picked from commit ea7d8b95e2fcb81f68b04ed7787a3dbda023991a) --- ChangeLog | 4 ++++ NEWS | 7 ++++--- nscd/netgroupcache.c | 16 ++++++++++------ 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/ChangeLog b/ChangeLog index 896b564707..e82ba7d174 100644 --- a/ChangeLog +++ b/ChangeLog @@ -8,6 +8,10 @@ * inet/getnetgrent_r.c (get_nonempty_val): New function. (nscd_getnetgrent): Use it. + [BZ #16760] + * nscd/netgroupcache.c (addgetnetgrentX): Use memmove instead + of stpcpy. + 2015-11-24 Andreas Schwab [BZ #17062] diff --git a/NEWS b/NEWS index 6f295a201a..2972c4a5ea 100644 --- a/NEWS +++ b/NEWS @@ -9,9 +9,10 @@ Version 2.19.1 * The following bugs are resolved with this release: - 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16878, - 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, - 17137, 17153, 17213, 17263, 17269, 17325, 17555, 18007, 18032, 18287. + 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, + 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, + 17079, 17137, 17153, 17213, 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 diff --git a/nscd/netgroupcache.c b/nscd/netgroupcache.c index 8c619eab31..c61d10b170 100644 --- a/nscd/netgroupcache.c +++ b/nscd/netgroupcache.c @@ -211,6 +211,10 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, const char *nuser = data.val.triple.user; const char *ndomain = data.val.triple.domain; + size_t hostlen = strlen (nhost ?: "") + 1; + size_t userlen = strlen (nuser ?: "") + 1; + size_t domainlen = strlen (ndomain ?: "") + 1; + if (nhost == NULL || nuser == NULL || ndomain == NULL || nhost > nuser || nuser > ndomain) { @@ -228,9 +232,6 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, : last + strlen (last) + 1 - buffer); /* We have to make temporary copies. */ - size_t hostlen = strlen (nhost ?: "") + 1; - size_t userlen = strlen (nuser ?: "") + 1; - size_t domainlen = strlen (ndomain ?: "") + 1; size_t needed = hostlen + userlen + domainlen; if (buflen - req->key_len - bufused < needed) @@ -264,9 +265,12 @@ addgetnetgrentX (struct database_dyn *db, int fd, request_header *req, } char *wp = buffer + buffilled; - wp = stpcpy (wp, nhost) + 1; - wp = stpcpy (wp, nuser) + 1; - wp = stpcpy (wp, ndomain) + 1; + wp = memmove (wp, nhost ?: "", hostlen); + wp += hostlen; + wp = memmove (wp, nuser ?: "", userlen); + wp += userlen; + wp = memmove (wp, ndomain ?: "", domainlen); + wp += domainlen; buffilled = wp - buffer; ++nentries; } -- cgit 1.4.1 From e12c42df43d80e46a6576dba94d31134727d868e Mon Sep 17 00:00:00 2001 From: Paul Pluzhnikov Date: Sat, 8 Aug 2015 15:53:03 -0700 Subject: Fix BZ #17905 (cherry picked from commit 0f58539030e436449f79189b6edab17d7479796e) --- ChangeLog | 8 ++++++++ NEWS | 4 ++-- catgets/Makefile | 11 ++++++++--- catgets/catgets.c | 19 ++++++++++++------- catgets/open_catalog.c | 23 ++++++++++++++--------- catgets/tst-catgets.c | 31 +++++++++++++++++++++++++++++++ 6 files changed, 75 insertions(+), 21 deletions(-) diff --git a/ChangeLog b/ChangeLog index e82ba7d174..0770353791 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2015-08-08 Paul Pluzhnikov + + [BZ #17905] + * catgets/Makefile (tst-catgets-mem): New test. + * catgets/catgets.c (catopen): Don't use unbounded alloca. + * catgets/open_catalog.c (__open_catalog): Likewise. + * catgets/tst-catgets.c (do_bz17905): Test unbounded alloca. + 2015-12-20 Siddhesh Poyarekar [BZ #16758] diff --git a/NEWS b/NEWS index 2972c4a5ea..ad87a8d6d8 100644 --- a/NEWS +++ b/NEWS @@ -11,8 +11,8 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, - 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 18007, 18032, - 18287. + 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 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 diff --git a/catgets/Makefile b/catgets/Makefile index c95442d369..8c39e3d95f 100644 --- a/catgets/Makefile +++ b/catgets/Makefile @@ -45,14 +45,14 @@ catgets-CPPFLAGS := -DNLSPATH='"$(msgcatdir)/%L/%N:$(msgcatdir)/%L/LC_MESSAGES/% CPPFLAGS-gencat = -DNOT_IN_libc generated = de.msg test1.cat test1.h test2.cat test2.h sample.SJIS.cat \ - test-gencat.h + test-gencat.h tst-catgets.mtrace tst-catgets-mem.out generated-dirs = de -tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de +tst-catgets-ENV = NLSPATH="$(objpfx)%l/%N.cat" LANG=de MALLOC_TRACE=$(objpfx)tst-catgets.mtrace ifeq ($(run-built-tests),yes) tests: $(objpfx)de/libc.cat $(objpfx)test1.cat $(objpfx)test2.cat \ - $(objpfx)test-gencat.out + $(objpfx)test-gencat.out $(objpfx)tst-catgets-mem.out # This test just checks whether the program produces any error or not. # The result is not tested. $(objpfx)test1.cat: test1.msg $(objpfx)gencat @@ -80,4 +80,9 @@ $(objpfx)test-gencat.out: test-gencat.sh $(objpfx)test-gencat \ $(objpfx)sample.SJIS.cat: sample.SJIS $(objpfx)gencat GCONV_PATH=$(common-objpfx)iconvdata LC_ALL=C \ $(built-program-cmd) -H $(objpfx)test-gencat.h < $(word 1,$^) > $@ + +$(objpfx)tst-catgets-mem.out: $(objpfx)tst-catgets.out + $(common-objpfx)malloc/mtrace $(objpfx)tst-catgets.mtrace > $@; \ + $(evaluate-test) + endif diff --git a/catgets/catgets.c b/catgets/catgets.c index eac2827214..820c0f6ed9 100644 --- a/catgets/catgets.c +++ b/catgets/catgets.c @@ -16,7 +16,6 @@ License along with the GNU C Library; if not, see . */ -#include #include #include #include @@ -35,6 +34,7 @@ catopen (const char *cat_name, int flag) __nl_catd result; const char *env_var = NULL; const char *nlspath = NULL; + char *tmp = NULL; if (strchr (cat_name, '/') == NULL) { @@ -54,7 +54,10 @@ catopen (const char *cat_name, int flag) { /* Append the system dependent directory. */ size_t len = strlen (nlspath) + 1 + sizeof NLSPATH; - char *tmp = alloca (len); + tmp = malloc (len); + + if (__glibc_unlikely (tmp == NULL)) + return (nl_catd) -1; __stpcpy (__stpcpy (__stpcpy (tmp, nlspath), ":"), NLSPATH); nlspath = tmp; @@ -65,16 +68,18 @@ catopen (const char *cat_name, int flag) result = (__nl_catd) malloc (sizeof (*result)); if (result == NULL) - /* We cannot get enough memory. */ - return (nl_catd) -1; - - if (__open_catalog (cat_name, nlspath, env_var, result) != 0) + { + /* We cannot get enough memory. */ + result = (nl_catd) -1; + } + else if (__open_catalog (cat_name, nlspath, env_var, result) != 0) { /* Couldn't open the file. */ free ((void *) result); - return (nl_catd) -1; + result = (nl_catd) -1; } + free (tmp); return (nl_catd) result; } diff --git a/catgets/open_catalog.c b/catgets/open_catalog.c index bc44f98247..5fa4011446 100644 --- a/catgets/open_catalog.c +++ b/catgets/open_catalog.c @@ -47,6 +47,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var, size_t tab_size; const char *lastp; int result = -1; + char *buf = NULL; if (strchr (cat_name, '/') != NULL || nlspath == NULL) fd = open_not_cancel_2 (cat_name, O_RDONLY); @@ -57,23 +58,23 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var, if (__builtin_expect (bufact + (n) >= bufmax, 0)) \ { \ char *old_buf = buf; \ - bufmax += 256 + (n); \ - buf = (char *) alloca (bufmax); \ - memcpy (buf, old_buf, bufact); \ + bufmax += (bufmax < 256 + (n)) ? 256 + (n) : bufmax; \ + buf = realloc (buf, bufmax); \ + if (__glibc_unlikely (buf == NULL)) \ + { \ + free (old_buf); \ + return -1; \ + } \ } /* The RUN_NLSPATH variable contains a colon separated list of descriptions where we expect to find catalogs. We have to recognize certain % substitutions and stop when we found the first existing file. */ - char *buf; size_t bufact; - size_t bufmax; + size_t bufmax = 0; size_t len; - buf = NULL; - bufmax = 0; - fd = -1; while (*run_nlspath != '\0') { @@ -188,7 +189,10 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var, /* Avoid dealing with directories and block devices */ if (__builtin_expect (fd, 0) < 0) - return -1; + { + free (buf); + return -1; + } if (__builtin_expect (__fxstat64 (_STAT_VER, fd, &st), 0) < 0) goto close_unlock_return; @@ -325,6 +329,7 @@ __open_catalog (const char *cat_name, const char *nlspath, const char *env_var, /* Release the lock again. */ close_unlock_return: close_not_cancel_no_status (fd); + free (buf); return result; } diff --git a/catgets/tst-catgets.c b/catgets/tst-catgets.c index fdaa834949..3e1fe84fcb 100644 --- a/catgets/tst-catgets.c +++ b/catgets/tst-catgets.c @@ -1,7 +1,10 @@ +#include #include #include #include +#include #include +#include static const char *msgs[] = @@ -12,6 +15,33 @@ static const char *msgs[] = }; #define nmsgs (sizeof (msgs) / sizeof (msgs[0])) + +/* Test for unbounded alloca. */ +static int +do_bz17905 (void) +{ + char *buf; + struct rlimit rl; + nl_catd result; + + const int sz = 1024 * 1024; + + getrlimit (RLIMIT_STACK, &rl); + rl.rlim_cur = sz; + setrlimit (RLIMIT_STACK, &rl); + + buf = malloc (sz + 1); + memset (buf, 'A', sz); + buf[sz] = '\0'; + setenv ("NLSPATH", buf, 1); + + result = catopen (buf, NL_CAT_LOCALE); + assert (result == (nl_catd) -1); + + free (buf); + return 0; +} + #define ROUNDS 5 int @@ -62,5 +92,6 @@ main (void) } } + result += do_bz17905 (); return result; } -- cgit 1.4.1 From 4d558c76f318fae8da0dce07e92bf079f935da07 Mon Sep 17 00:00:00 2001 From: Paul Pluzhnikov Date: Sat, 8 Aug 2015 15:54:40 -0700 Subject: Fix trailing space. (cherry picked from commit 7565d2a862683a3c26ffb1f32351b8c5ab9f7b31) --- catgets/tst-catgets.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/catgets/tst-catgets.c b/catgets/tst-catgets.c index 3e1fe84fcb..25ef056ef2 100644 --- a/catgets/tst-catgets.c +++ b/catgets/tst-catgets.c @@ -30,7 +30,7 @@ do_bz17905 (void) rl.rlim_cur = sz; setrlimit (RLIMIT_STACK, &rl); - buf = malloc (sz + 1); + buf = malloc (sz + 1); memset (buf, 'A', sz); buf[sz] = '\0'; setenv ("NLSPATH", buf, 1); -- cgit 1.4.1 From fd6e33ebd157966fed025a8cf68f2f0835dcbf02 Mon Sep 17 00:00:00 2001 From: Paul Pluzhnikov Date: Sat, 26 Sep 2015 13:27:48 -0700 Subject: Fix BZ #18985 -- out of range data to strftime() causes a segfault (cherry picked from commit d36c75fc0d44deec29635dd239b0fbd206ca49b7) --- ChangeLog | 8 ++++++++ NEWS | 2 +- time/strftime_l.c | 20 +++++++++++++------- time/tst-strftime.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 73 insertions(+), 9 deletions(-) diff --git a/ChangeLog b/ChangeLog index 0770353791..871c722510 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2015-09-26 Paul Pluzhnikov + + [BZ #18985] + * time/strftime_l.c (a_wkday, f_wkday, a_month, f_month): Range check. + (__strftime_internal): Likewise. + * time/tst-strftime.c (do_bz18985): New test. + (do_test): Call it. + 2015-08-08 Paul Pluzhnikov [BZ #17905] diff --git a/NEWS b/NEWS index ad87a8d6d8..44fe9164cf 100644 --- a/NEWS +++ b/NEWS @@ -12,7 +12,7 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18287. + 18032, 18287, 18905. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a diff --git a/time/strftime_l.c b/time/strftime_l.c index dfb7b4c483..29c7cdd556 100644 --- a/time/strftime_l.c +++ b/time/strftime_l.c @@ -509,13 +509,17 @@ __strftime_internal (s, maxsize, format, tp, tzset_called ut_argument only a few elements. Dereference the pointers only if the format requires this. Then it is ok to fail if the pointers are invalid. */ # define a_wkday \ - ((const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(ABDAY_1) + tp->tm_wday)) + ((const CHAR_T *) (tp->tm_wday < 0 || tp->tm_wday > 6 \ + ? "?" : _NL_CURRENT (LC_TIME, NLW(ABDAY_1) + tp->tm_wday))) # define f_wkday \ - ((const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(DAY_1) + tp->tm_wday)) + ((const CHAR_T *) (tp->tm_wday < 0 || tp->tm_wday > 6 \ + ? "?" : _NL_CURRENT (LC_TIME, NLW(DAY_1) + tp->tm_wday))) # define a_month \ - ((const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(ABMON_1) + tp->tm_mon)) + ((const CHAR_T *) (tp->tm_mon < 0 || tp->tm_mon > 11 \ + ? "?" : _NL_CURRENT (LC_TIME, NLW(ABMON_1) + tp->tm_mon))) # define f_month \ - ((const CHAR_T *) _NL_CURRENT (LC_TIME, NLW(MON_1) + tp->tm_mon)) + ((const CHAR_T *) (tp->tm_mon < 0 || tp->tm_mon > 11 \ + ? "?" : _NL_CURRENT (LC_TIME, NLW(MON_1) + tp->tm_mon))) # define ampm \ ((const CHAR_T *) _NL_CURRENT (LC_TIME, tp->tm_hour > 11 \ ? NLW(PM_STR) : NLW(AM_STR))) @@ -525,8 +529,10 @@ __strftime_internal (s, maxsize, format, tp, tzset_called ut_argument # define ap_len STRLEN (ampm) #else # if !HAVE_STRFTIME -# define f_wkday (weekday_name[tp->tm_wday]) -# define f_month (month_name[tp->tm_mon]) +# define f_wkday (tp->tm_wday < 0 || tp->tm_wday > 6 \ + ? "?" : weekday_name[tp->tm_wday]) +# define f_month (tp->tm_mon < 0 || tp->tm_mon > 11 \ + ? "?" : month_name[tp->tm_mon]) # define a_wkday f_wkday # define a_month f_month # define ampm (L_("AMPM") + 2 * (tp->tm_hour > 11)) @@ -1320,7 +1326,7 @@ __strftime_internal (s, maxsize, format, tp, tzset_called ut_argument *tzset_called = true; } # endif - zone = tzname[tp->tm_isdst]; + zone = tp->tm_isdst <= 1 ? tzname[tp->tm_isdst] : "?"; } #endif if (! zone) diff --git a/time/tst-strftime.c b/time/tst-strftime.c index 374fba4262..af3ff72faf 100644 --- a/time/tst-strftime.c +++ b/time/tst-strftime.c @@ -4,6 +4,56 @@ #include +static int +do_bz18985 (void) +{ + char buf[1000]; + struct tm ttm; + int rc, ret = 0; + + memset (&ttm, 1, sizeof (ttm)); + ttm.tm_zone = NULL; /* Dereferenced directly if non-NULL. */ + rc = strftime (buf, sizeof (buf), "%a %A %b %B %c %z %Z", &ttm); + + if (rc == 66) + { + const char expected[] + = "? ? ? ? ? ? 16843009 16843009:16843009:16843009 16844909 +467836 ?"; + if (0 != strcmp (buf, expected)) + { + printf ("expected:\n %s\ngot:\n %s\n", expected, buf); + ret += 1; + } + } + else + { + printf ("expected 66, got %d\n", rc); + ret += 1; + } + + /* Check negative values as well. */ + memset (&ttm, 0xFF, sizeof (ttm)); + ttm.tm_zone = NULL; /* Dereferenced directly if non-NULL. */ + rc = strftime (buf, sizeof (buf), "%a %A %b %B %c %z %Z", &ttm); + + if (rc == 30) + { + const char expected[] = "? ? ? ? ? ? -1 -1:-1:-1 1899 "; + if (0 != strcmp (buf, expected)) + { + printf ("expected:\n %s\ngot:\n %s\n", expected, buf); + ret += 1; + } + } + else + { + printf ("expected 30, got %d\n", rc); + ret += 1; + } + + return ret; +} + static struct { const char *fmt; @@ -104,7 +154,7 @@ do_test (void) } } - return result; + return result + do_bz18985 (); } #define TEST_FUNCTION do_test () -- cgit 1.4.1 From b5cba5cff937e5336ff23380785da80cab09146c Mon Sep 17 00:00:00 2001 From: Ondřej Bílka Date: Sat, 11 Jul 2015 17:44:10 +0200 Subject: Handle overflow in __hcreate_r Hi, As in bugzilla entry there is overflow in hsearch when looking for prime number as SIZE_MAX - 1 is divisible by 5. We fix that by rejecting large inputs before looking for prime. * misc/hsearch_r.c (__hcreate_r): Handle overflow. (cherry picked from commit 2f5c1750558fe64bac361f52d6827ab1bcfe52bc) --- ChangeLog | 5 +++++ misc/hsearch_r.c | 9 ++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/ChangeLog b/ChangeLog index 871c722510..1ea2c735b7 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,8 @@ +2015-08-25 Ondřej Bílka + + [BZ #18240] + * misc/hsearch_r.c (__hcreate_r): Handle overflow. + 2015-09-26 Paul Pluzhnikov [BZ #18985] diff --git a/misc/hsearch_r.c b/misc/hsearch_r.c index 81c27d800c..4414a25cba 100644 --- a/misc/hsearch_r.c +++ b/misc/hsearch_r.c @@ -19,7 +19,7 @@ #include #include #include - +#include #include /* [Aho,Sethi,Ullman] Compilers: Principles, Techniques and Tools, 1986 @@ -73,6 +73,13 @@ hcreate_r (nel, htab) return 0; } + if (nel >= SIZE_MAX / sizeof (_ENTRY)) + { + __set_errno (ENOMEM); + return 0; + } + + /* There is still another table active. Return with error. */ if (htab->table != NULL) return 0; -- cgit 1.4.1 From 3c9e8d9477aba0f514171bb4706670052544479b Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Thu, 28 Jan 2016 13:59:11 +0100 Subject: Improve check against integer wraparound in hcreate_r [BZ #18240] (cherry picked from commit bae7c7c764413b23e61cb099ce33be4c4ee259bb) --- ChangeLog | 12 +++++++++ NEWS | 2 +- misc/Makefile | 3 ++- misc/bug18240.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ misc/hsearch_r.c | 35 +++++++++++++------------- 5 files changed, 107 insertions(+), 20 deletions(-) create mode 100644 misc/bug18240.c diff --git a/ChangeLog b/ChangeLog index 1ea2c735b7..e17bd64f3a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,15 @@ +2016-01-27 Paul Eggert + + [BZ #18240] + * misc/hsearch_r.c (isprime, __hcreate_r): Protect against + unsigned int wraparound. + +2016-01-27 Florian Weimer + + [BZ #18240] + * misc/bug18240.c: New test. + * misc/Makefile (tests): Add it. + 2015-08-25 Ondřej Bílka [BZ #18240] diff --git a/NEWS b/NEWS index 44fe9164cf..0d1952c9f4 100644 --- a/NEWS +++ b/NEWS @@ -12,7 +12,7 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18287, 18905. + 18032, 18240, 18287, 18905. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a diff --git a/misc/Makefile b/misc/Makefile index b039182fa0..ad9e92137a 100644 --- a/misc/Makefile +++ b/misc/Makefile @@ -76,7 +76,8 @@ install-lib := libg.a gpl2lgpl := error.c error.h tests := tst-dirname tst-tsearch tst-fdset tst-efgcvt tst-mntent tst-hsearch \ - tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 + tst-error1 tst-pselect tst-insremque tst-mntent2 bug-hsearch1 \ + bug18240 ifeq ($(run-built-tests),yes) tests: $(objpfx)tst-error1-mem endif diff --git a/misc/bug18240.c b/misc/bug18240.c new file mode 100644 index 0000000000..4b26865a31 --- /dev/null +++ b/misc/bug18240.c @@ -0,0 +1,75 @@ +/* Test integer wraparound in hcreate. + Copyright (C) 2016 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 +#include + +static void +test_size (size_t size) +{ + int res = hcreate (size); + if (res == 0) + { + if (errno == ENOMEM) + return; + printf ("error: hcreate (%zu): %m\n", size); + exit (1); + } + char *keys[100]; + for (int i = 0; i < 100; ++i) + { + if (asprintf (keys + i, "%d", i) < 0) + { + printf ("error: asprintf: %m\n"); + exit (1); + } + ENTRY e = { keys[i], (char *) "value" }; + if (hsearch (e, ENTER) == NULL) + { + printf ("error: hsearch (\"%s\"): %m\n", keys[i]); + exit (1); + } + } + hdestroy (); + + for (int i = 0; i < 100; ++i) + free (keys[i]); +} + +static int +do_test (void) +{ + test_size (500); + test_size (-1); + test_size (-3); + test_size (INT_MAX - 2); + test_size (INT_MAX - 1); + test_size (INT_MAX); + test_size (((unsigned) INT_MAX) + 1); + test_size (UINT_MAX - 2); + test_size (UINT_MAX - 1); + test_size (UINT_MAX); + return 0; +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/misc/hsearch_r.c b/misc/hsearch_r.c index 4414a25cba..af5521234c 100644 --- a/misc/hsearch_r.c +++ b/misc/hsearch_r.c @@ -46,15 +46,12 @@ static int isprime (unsigned int number) { /* no even number will be passed */ - unsigned int div = 3; - - while (div * div < number && number % div != 0) - div += 2; - - return number % div != 0; + for (unsigned int div = 3; div <= number / div; div += 2) + if (number % div == 0) + return 0; + return 1; } - /* Before using the hash table we must allocate memory for it. Test for an existing table are done. We allocate one element more as the found prime number says. This is done for more effective @@ -73,13 +70,6 @@ hcreate_r (nel, htab) return 0; } - if (nel >= SIZE_MAX / sizeof (_ENTRY)) - { - __set_errno (ENOMEM); - return 0; - } - - /* There is still another table active. Return with error. */ if (htab->table != NULL) return 0; @@ -88,10 +78,19 @@ hcreate_r (nel, htab) use will not work. */ if (nel < 3) nel = 3; - /* Change nel to the first prime number not smaller as nel. */ - nel |= 1; /* make odd */ - while (!isprime (nel)) - nel += 2; + + /* Change nel to the first prime number in the range [nel, UINT_MAX - 2], + The '- 2' means 'nel += 2' cannot overflow. */ + for (nel |= 1; ; nel += 2) + { + if (UINT_MAX - 2 < nel) + { + __set_errno (ENOMEM); + return 0; + } + if (isprime (nel)) + break; + } htab->size = nel; htab->filled = 0; -- cgit 1.4.1 From 78c76f7374df7f3caff43840a01247bb7d25597e Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Fri, 12 Feb 2016 12:57:40 +0100 Subject: hsearch_r: Apply VM size limit in test case (cherry picked from commit f34f146e682d8d529dcf64b3c2781bf3f2f05f6c) --- ChangeLog | 4 ++++ misc/bug18240.c | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/ChangeLog b/ChangeLog index e17bd64f3a..9907019604 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,7 @@ +2016-02-12 Florian Weimer + + * misc/bug18240.c (do_test): Set RLIMIT_AS. + 2016-01-27 Paul Eggert [BZ #18240] diff --git a/misc/bug18240.c b/misc/bug18240.c index 4b26865a31..773586ee10 100644 --- a/misc/bug18240.c +++ b/misc/bug18240.c @@ -22,6 +22,7 @@ #include #include #include +#include static void test_size (size_t size) @@ -58,6 +59,27 @@ test_size (size_t size) static int do_test (void) { + /* Limit the size of the process, so that memory allocation will + fail without impacting the entire system. */ + { + struct rlimit limit; + if (getrlimit (RLIMIT_AS, &limit) != 0) + { + printf ("getrlimit (RLIMIT_AS) failed: %m\n"); + return 1; + } + long target = 100 * 1024 * 1024; + if (limit.rlim_cur == RLIM_INFINITY || limit.rlim_cur > target) + { + limit.rlim_cur = target; + if (setrlimit (RLIMIT_AS, &limit) != 0) + { + printf ("setrlimit (RLIMIT_AS) failed: %m\n"); + return 1; + } + } + } + test_size (500); test_size (-1); test_size (-3); -- cgit 1.4.1 From a02f3e795993ae0f80242b488061b74666605625 Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Tue, 29 Mar 2016 12:57:56 +0200 Subject: CVE-2016-3075: Stack overflow in _nss_dns_getnetbyname_r [BZ #19879] The defensive copy is not needed because the name may not alias the output buffer. (cherry picked from commit 317b199b4aff8cfa27f2302ab404d2bb5032b9a4) --- ChangeLog | 7 +++++++ NEWS | 7 ++++++- resolv/nss_dns/dns-network.c | 5 +---- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9907019604..685dd909f2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2016-04-01 Florian Weimer + + [BZ #19879] + CVE-2016-3075 + * resolv/nss_dns/dns-network.c (_nss_dns_getnetbyname_r): Do not + copy name. + 2016-02-12 Florian Weimer * misc/bug18240.c (do_test): Set RLIMIT_AS. diff --git a/NEWS b/NEWS index 0d1952c9f4..d7da53f9ba 100644 --- a/NEWS +++ b/NEWS @@ -12,7 +12,7 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18240, 18287, 18905. + 18032, 18240, 18287, 18905, 19879. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a @@ -63,6 +63,11 @@ Version 2.19.1 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. + +* The getnetbyname implementation in nss_dns had a potentially unbounded + alloca call (in the form of a call to strdupa), leading to a stack + overflow (stack exhaustion) and a crash if getnetbyname is invoked + on a very long name. (CVE-2016-3075) Version 2.19 diff --git a/resolv/nss_dns/dns-network.c b/resolv/nss_dns/dns-network.c index 13ad38c5de..37de664818 100644 --- a/resolv/nss_dns/dns-network.c +++ b/resolv/nss_dns/dns-network.c @@ -118,17 +118,14 @@ _nss_dns_getnetbyname_r (const char *name, struct netent *result, } net_buffer; querybuf *orig_net_buffer; int anslen; - char *qbuf; enum nss_status status; if (__res_maybe_init (&_res, 0) == -1) return NSS_STATUS_UNAVAIL; - qbuf = strdupa (name); - net_buffer.buf = orig_net_buffer = (querybuf *) alloca (1024); - anslen = __libc_res_nsearch (&_res, qbuf, C_IN, T_PTR, net_buffer.buf->buf, + anslen = __libc_res_nsearch (&_res, name, C_IN, T_PTR, net_buffer.buf->buf, 1024, &net_buffer.ptr, NULL, NULL, NULL, NULL); if (anslen < 0) { -- cgit 1.4.1 From aa60d72514a2bc51017edeb0ccdd9904b8c2e745 Mon Sep 17 00:00:00 2001 From: Stefan Liebler Date: Thu, 28 Apr 2016 10:12:05 +0200 Subject: S/390: Fix setcontext/swapcontext which are not restoring sigmask. [BZ #18080] This patch uses sigprocmask(SIG_SETMASK) instead of SIG_BLOCK in setcontext, swapcontext. (cherry picked from commit 2e807f29595eb5b1e5d0decc6e356a3562ecc58e) --- ChangeLog | 16 ++ NEWS | 2 +- stdlib/Makefile | 10 +- stdlib/tst-setcontext2.c | 230 +++++++++++++++++++++ sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S | 2 +- sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S | 16 +- sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S | 2 +- sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S | 16 +- 8 files changed, 264 insertions(+), 30 deletions(-) create mode 100644 stdlib/tst-setcontext2.c diff --git a/ChangeLog b/ChangeLog index 685dd909f2..9b6e9f54e0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,19 @@ +2016-04-28 Stefan Liebler + + [BZ #18080] + * sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S + (__setcontext): Use SIG_SETMASK instead of SIG_BLOCK. + * sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S + (__setcontext): Likewise. + * sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S + (__swapcontext): Use SIG_SETMASK instead of SIG_BLOCK. + Call rt_sigprocmask syscall one time to set new signal mask + and retrieve the current signal mask instead of two calls. + * sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S + (__swapcontext): Likewise. + * stdlib/Makefile (tests): Add new testcase tst-setcontext2. + * stdlib/tst-setcontext2.c: New file. + 2016-04-01 Florian Weimer [BZ #19879] diff --git a/NEWS b/NEWS index d7da53f9ba..18d873e4b8 100644 --- a/NEWS +++ b/NEWS @@ -12,7 +12,7 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18240, 18287, 18905, 19879. + 18032, 18080, 18240, 18287, 18905, 19879. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a diff --git a/stdlib/Makefile b/stdlib/Makefile index 1be16eb8d0..b46c4a1fea 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -64,11 +64,11 @@ test-srcs := tst-fmtmsg tests := tst-strtol tst-strtod testmb testrand testsort testdiv \ test-canon test-canon2 tst-strtoll tst-environ \ tst-xpg-basename tst-random tst-random2 tst-bsearch \ - tst-limits tst-rand48 bug-strtod tst-setcontext \ - test-a64l tst-qsort tst-system testmb2 bug-strtod2 \ - tst-atof1 tst-atof2 tst-strtod2 tst-strtod3 tst-rand48-2 \ - tst-makecontext tst-strtod4 tst-strtod5 tst-qsort2 \ - tst-makecontext2 tst-strtod6 tst-unsetenv1 \ + tst-limits tst-rand48 bug-strtod tst-setcontext \ + tst-setcontext2 test-a64l tst-qsort tst-system testmb2 \ + bug-strtod2 tst-atof1 tst-atof2 tst-strtod2 tst-strtod3 \ + tst-rand48-2 tst-makecontext tst-strtod4 tst-strtod5 \ + tst-qsort2 tst-makecontext2 tst-strtod6 tst-unsetenv1 \ tst-makecontext3 bug-getcontext bug-fmtmsg1 \ tst-secure-getenv tst-strtod-overflow tst-strtod-round \ tst-tininess tst-strtod-underflow tst-tls-atexit diff --git a/stdlib/tst-setcontext2.c b/stdlib/tst-setcontext2.c new file mode 100644 index 0000000000..8582cc0c1c --- /dev/null +++ b/stdlib/tst-setcontext2.c @@ -0,0 +1,230 @@ +/* Testcase checks, if setcontext(), swapcontext() restores signal-mask + and if pending signals are delivered after those calls. + 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 +#include + +volatile int global; +volatile sig_atomic_t handlerCalled; + +static void +check (const char *funcName) +{ + sigset_t set; + + /* check if SIGUSR2 is unblocked after setcontext-call. */ + sigprocmask (SIG_BLOCK, NULL, &set); + + if (sigismember (&set, SIGUSR2) != 0) + { + printf ("FAIL: SIGUSR2 is blocked after %s.\n", funcName); + exit (1); + } + + if (sigismember (&set, SIGUSR1) != 1) + { + printf ("FAIL: SIGUSR1 is not blocked after %s.\n", funcName); + exit (1); + } +} + +static void +signalmask (int how, int signum) +{ + sigset_t set; + sigemptyset (&set); + sigaddset (&set, signum); + if (sigprocmask (how, &set, NULL) != 0) + { + printf ("FAIL: sigprocmaks (%d, %d, NULL): %m\n", how, signum); + exit (1); + } +} + +static void +signalpending (int signum, const char *msg) +{ + sigset_t set; + sigemptyset (&set); + if (sigpending (&set) != 0) + { + printf ("FAIL: sigpending: %m\n"); + exit (1); + } + if (sigismember (&set, SIGUSR2) != 1) + { + printf ("FAIL: Signal %d is not pending %s\n", signum, msg); + exit (1); + } +} + +static void +handler (int __attribute__ ((unused)) signum) +{ + handlerCalled ++; +} + +static int +do_test (void) +{ + ucontext_t ctx, oldctx; + struct sigaction action; + pid_t pid; + + pid = getpid (); + + /* unblock SIGUSR2 */ + signalmask (SIG_UNBLOCK, SIGUSR2); + + /* block SIGUSR1 */ + signalmask (SIG_BLOCK, SIGUSR1); + + /* register handler for SIGUSR2 */ + action.sa_flags = 0; + action.sa_handler = handler; + sigemptyset (&action.sa_mask); + sigaction (SIGUSR2, &action, NULL); + + if (getcontext (&ctx) != 0) + { + printf ("FAIL: getcontext: %m\n"); + exit (1); + } + + global++; + + if (global == 1) + { + puts ("after getcontext"); + + /* block SIGUSR2 */ + signalmask (SIG_BLOCK, SIGUSR2); + + /* send SIGUSR2 to me */ + handlerCalled = 0; + kill (pid, SIGUSR2); + + /* was SIGUSR2 handler called? */ + if (handlerCalled != 0) + { + puts ("FAIL: signal handler was called, but signal was blocked."); + exit (1); + } + + /* is SIGUSR2 pending? */ + signalpending (SIGUSR2, "before setcontext"); + + /* SIGUSR2 will be unblocked by setcontext-call. */ + if (setcontext (&ctx) != 0) + { + printf ("FAIL: setcontext: %m\n"); + exit (1); + } + } + else if (global == 2) + { + puts ("after setcontext"); + + /* check SIGUSR1/2 */ + check ("setcontext"); + + /* was SIGUSR2 handler called? */ + if (handlerCalled != 1) + { + puts ("FAIL: signal handler was not called after setcontext."); + exit (1); + } + + /* block SIGUSR2 */ + signalmask (SIG_BLOCK, SIGUSR2); + + /* send SIGUSR2 to me */ + handlerCalled = 0; + kill (pid, SIGUSR2); + + /* was SIGUSR2 handler called? */ + if (handlerCalled != 0) + { + puts ("FAIL: signal handler was called, but signal was blocked."); + exit (1); + } + + /* is SIGUSR2 pending? */ + signalpending (SIGUSR2, "before swapcontext"); + + if (swapcontext (&oldctx, &ctx) != 0) + { + printf ("FAIL: swapcontext: %m\n"); + exit (1); + } + + puts ("after returned from swapcontext"); + + if (global != 3) + { + puts ("FAIL: returned from swapcontext without ctx-context called."); + exit (1); + } + + puts ("test succeeded"); + return 0; + } + else if ( global != 3 ) + { + puts ("FAIL: 'global' not incremented three times"); + exit (1); + } + + puts ("after swapcontext"); + /* check SIGUSR1/2 */ + check ("swapcontext"); + + /* was SIGUSR2 handler called? */ + if (handlerCalled != 1) + { + puts ("FAIL: signal handler was not called after swapcontext."); + exit (1); + } + + /* check sigmask in old context of swapcontext-call */ + if (sigismember (&oldctx.uc_sigmask, SIGUSR2) != 1) + { + puts ("FAIL: SIGUSR2 is not blocked in oldctx.uc_sigmask."); + exit (1); + } + + if (sigismember (&oldctx.uc_sigmask, SIGUSR1) != 1) + { + puts ("FAIL: SIGUSR1 is not blocked in oldctx.uc_sigmaks."); + exit (1); + } + + /* change to old context, which was gathered by swapcontext() call. */ + setcontext (&oldctx); + + puts ("FAIL: returned from setcontext (&oldctx)"); + exit (1); +} + +#define TEST_FUNCTION do_test () +#include "../test-skeleton.c" diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S b/sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S index 42839e26f1..b26377398a 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S +++ b/sysdeps/unix/sysv/linux/s390/s390-32/setcontext.S @@ -34,7 +34,7 @@ ENTRY(__setcontext) lr %r1,%r2 /* rt_sigprocmask (SIG_SETMASK, &sc->sc_mask, NULL, sigsetsize). */ - la %r2,SIG_BLOCK + la %r2,SIG_SETMASK la %r3,SC_MASK(%r1) slr %r4,%r4 lhi %r5,_NSIG8 diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S b/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S index 9206aa334d..8f9cfd834d 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S +++ b/sysdeps/unix/sysv/linux/s390/s390-32/swapcontext.S @@ -24,7 +24,7 @@ /* __swapcontext (ucontext_t *oucp, const ucontext_t *ucp) Saves the machine context in oucp such that when it is activated, - it appears as if __swapcontextt() returned again, restores the + it appears as if __swapcontext() returned again, restores the machine context in ucp and thereby resumes execution in that context. @@ -39,13 +39,6 @@ ENTRY(__swapcontext) lr %r1,%r2 lr %r0,%r3 - /* sigprocmask (SIG_BLOCK, NULL, &sc->sc_mask). */ - la %r2,SIG_BLOCK - slr %r3,%r3 - la %r4,SC_MASK(%r1) - lhi %r5,_NSIG8 - svc SYS_ify(rt_sigprocmask) - /* Store fpu context. */ stfpc SC_FPC(%r1) std %f0,SC_FPRS(%r1) @@ -74,11 +67,12 @@ ENTRY(__swapcontext) /* Store general purpose registers. */ stm %r0,%r15,SC_GPRS(%r1) - /* sigprocmask (SIG_SETMASK, &sc->sc_mask, NULL). */ - la %r2,SIG_BLOCK + /* rt_sigprocmask (SIG_SETMASK, &ucp->uc_sigmask, &oucp->uc_sigmask, + sigsetsize). */ + la %r2,SIG_SETMASK lr %r5,%r0 la %r3,SC_MASK(%r5) - slr %r4,%r4 + la %r4,SC_MASK(%r1) lhi %r5,_NSIG8 svc SYS_ify(rt_sigprocmask) diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S b/sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S index 83df5ce461..1464e6a094 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S +++ b/sysdeps/unix/sysv/linux/s390/s390-64/setcontext.S @@ -34,7 +34,7 @@ ENTRY(__setcontext) lgr %r1,%r2 /* sigprocmask (SIG_SETMASK, &sc->sc_mask, NULL). */ - la %r2,SIG_BLOCK + la %r2,SIG_SETMASK la %r3,SC_MASK(%r1) slgr %r4,%r4 lghi %r5,_NSIG8 diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S b/sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S index e3e624c91b..8346fd5dd1 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S +++ b/sysdeps/unix/sysv/linux/s390/s390-64/swapcontext.S @@ -24,7 +24,7 @@ /* __swapcontext (ucontext_t *oucp, const ucontext_t *ucp) Saves the machine context in oucp such that when it is activated, - it appears as if __swapcontextt() returned again, restores the + it appears as if __swapcontext() returned again, restores the machine context in ucp and thereby resumes execution in that context. @@ -39,13 +39,6 @@ ENTRY(__swapcontext) lgr %r1,%r2 lgr %r0,%r3 - /* sigprocmask (SIG_BLOCK, NULL, &sc->sc_mask). */ - la %r2,SIG_BLOCK - slgr %r3,%r3 - la %r4,SC_MASK(%r1) - lghi %r5,_NSIG8 - svc SYS_ify(rt_sigprocmask) - /* Store fpu context. */ stfpc SC_FPC(%r1) std %f0,SC_FPRS(%r1) @@ -74,12 +67,13 @@ ENTRY(__swapcontext) /* Store general purpose registers. */ stmg %r0,%r15,SC_GPRS(%r1) - /* rt_sigprocmask (SIG_SETMASK, &sc->sc_mask, NULL, sigsetsize). */ - la %r2,SIG_BLOCK + /* rt_sigprocmask (SIG_SETMASK, &ucp->uc_sigmask, &oucp->uc_sigmask, + sigsetsize). */ + la %r2,SIG_SETMASK lgr %r5,%r0 la %r3,SC_MASK(%r5) + la %r4,SC_MASK(%r1) lghi %r5,_NSIG8 - slgr %r4,%r4 svc SYS_ify(rt_sigprocmask) /* Load fpu context. */ -- cgit 1.4.1 From 05cc5bbd3cf037daee848c11792a6471de01298d Mon Sep 17 00:00:00 2001 From: Stefan Liebler Date: Thu, 28 Apr 2016 10:21:58 +0200 Subject: S390: Fix "backtrace() returns infinitely deep stack frames with makecontext()" [BZ #18508]. On s390/s390x backtrace(buffer, size) returns the series of called functions until "makecontext_ret" and additional entries (up to "size") with "makecontext_ret". GDB-backtrace is also warning: "Backtrace stopped: previous frame identical to this frame (corrupt stack?)" To reproduce this scenario you have to setup a new context with makecontext() and activate it with setcontext(). See e.g. cf() function in testcase stdlib/tst-makecontext.c. Or see bug in libgo "Bug 66303 - runtime.Caller() returns infinitely deep stack frames on s390x " (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66303). This patch omits the cfi_startproc/cfi_endproc directives in ENTRY/END macro of __makecontext_ret. Thus no frame information is generated in .eh_frame and backtrace stops after __makecontext_ret. There is also no .eh_frame info for _start or thread_start functions. ChangeLog: [BZ #18508] * stdlib/Makefile ($(objpfx)tst-makecontext3): Depend on $(libdl). * stdlib/tst-makecontext.c (cf): Test if _Unwind_Backtrace is not called infinitely times. (backtrace_helper): New function. (trace_arg): New struct. (st1): Enlarge stack size. * sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S: (__makecontext_ret): Omit cfi_startproc and cfi_endproc. * sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S: Likewise. (cherry picked from commit 890b7a4b33d482b5c768ab47d70758b80227e9bc) --- ChangeLog | 15 ++++++++ NEWS | 2 +- stdlib/Makefile | 2 ++ stdlib/tst-makecontext.c | 41 +++++++++++++++++++++- .../sysv/linux/s390/s390-32/__makecontext_ret.S | 8 +++++ .../sysv/linux/s390/s390-64/__makecontext_ret.S | 8 +++++ 6 files changed, 74 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9b6e9f54e0..ac7f870508 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,18 @@ +2016-04-28 Stefan Liebler + + [BZ #18508] + * stdlib/Makefile ($(objpfx)tst-makecontext3): + Depend on $(libdl). + * stdlib/tst-makecontext.c (cf): Test if _Unwind_Backtrace + is not called infinitely times. + (backtrace_helper): New function. + (trace_arg): New struct. + (st1): Enlarge stack size. + * sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S: + (__makecontext_ret): Omit cfi_startproc and cfi_endproc. + * sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S: + Likewise. + 2016-04-28 Stefan Liebler [BZ #18080] diff --git a/NEWS b/NEWS index 18d873e4b8..b2a1871389 100644 --- a/NEWS +++ b/NEWS @@ -12,7 +12,7 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18080, 18240, 18287, 18905, 19879. + 18032, 18080, 18240, 18287, 18508, 18905, 19879. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a diff --git a/stdlib/Makefile b/stdlib/Makefile index b46c4a1fea..8a34b838b6 100644 --- a/stdlib/Makefile +++ b/stdlib/Makefile @@ -164,3 +164,5 @@ tst-tls-atexit-lib.so-no-z-defs = yes $(objpfx)tst-tls-atexit: $(common-objpfx)nptl/libpthread.so \ $(common-objpfx)dlfcn/libdl.so $(objpfx)tst-tls-atexit.out: $(objpfx)tst-tls-atexit-lib.so + +$(objpfx)tst-makecontext: $(libdl) diff --git a/stdlib/tst-makecontext.c b/stdlib/tst-makecontext.c index 7968a6d3dc..ef1e27a634 100644 --- a/stdlib/tst-makecontext.c +++ b/stdlib/tst-makecontext.c @@ -19,23 +19,62 @@ #include #include #include +#include +#include +#include +#include ucontext_t ucp; -char st1[8192]; +char st1[16384]; __thread int thr; int somevar = -76; long othervar = -78L; +struct trace_arg +{ + int cnt, size; +}; + +static _Unwind_Reason_Code +backtrace_helper (struct _Unwind_Context *ctx, void *a) +{ + struct trace_arg *arg = a; + if (++arg->cnt == arg->size) + return _URC_END_OF_STACK; + return _URC_NO_REASON; +} + void cf (int i) { + struct trace_arg arg = { .size = 100, .cnt = -1 }; + void *handle; + _Unwind_Reason_Code (*unwind_backtrace) (_Unwind_Trace_Fn, void *); + if (i != othervar || thr != 94) { printf ("i %d thr %d\n", i, thr); exit (1); } + /* Test if callback function of _Unwind_Backtrace is not called infinitely + times. See Bug 18508 or gcc bug "Bug 66303 - runtime.Caller() returns + infinitely deep stack frames on s390x.". + The go runtime calls backtrace_full() in + /libbacktrace/backtrace.c, which uses _Unwind_Backtrace(). */ + handle = dlopen (LIBGCC_S_SO, RTLD_LAZY); + if (handle != NULL) + { + unwind_backtrace = dlsym (handle, "_Unwind_Backtrace"); + if (unwind_backtrace != NULL) + { + unwind_backtrace (backtrace_helper, &arg); + assert (arg.cnt != -1 && arg.cnt < 100); + } + dlclose (handle); + } + /* Since uc_link below has been set to NULL, setcontext is supposed to terminate the process normally after this function returns. */ } diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S b/sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S index 83cf0d8ffa..67ea206de4 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S +++ b/sysdeps/unix/sysv/linux/s390/s390-32/__makecontext_ret.S @@ -17,6 +17,14 @@ #include +/* We do not want .eh_frame info so that __makecontext_ret stops unwinding + if backtrace was called within a context created by makecontext. (There + is also no .eh_frame info for _start or thread_start.) */ +#undef cfi_startproc +#define cfi_startproc +#undef cfi_endproc +#define cfi_endproc + ENTRY(__makecontext_ret) basr %r14,%r7 ltr %r8,%r8 /* Check whether uc_link is 0. */ diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S b/sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S index 71ecbab08e..a2bf3ca02d 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S +++ b/sysdeps/unix/sysv/linux/s390/s390-64/__makecontext_ret.S @@ -17,6 +17,14 @@ #include +/* We do not want .eh_frame info so that __makecontext_ret stops unwinding + if backtrace was called within a context created by makecontext. (There + is also no .eh_frame info for _start or thread_start.) */ +#undef cfi_startproc +#define cfi_startproc +#undef cfi_endproc +#define cfi_endproc + ENTRY(__makecontext_ret) basr %r14,%r7 ltgr %r8,%r8 /* Check whether uc_link is 0. */ -- cgit 1.4.1 From 5ae82aa4bf45cdaafeb1c25e09897eabff210de9 Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Fri, 29 Apr 2016 09:33:07 +0200 Subject: glob: Simplify the interface for the GLOB_ALTDIRFUNC callback gl_readdir Previously, application code had to set up the d_namlen member if the target supported it, involving conditional compilation. After this change, glob will use the length of the string in d_name instead of d_namlen to determine the file name length. All glibc targets provide the d_type and d_ino members, and setting them as needed for gl_readdir is straightforward. Changing the behavior with regards to d_ino is left to a future cleanup. (cherry picked from commit 137fe72eca6923a00381a3ca9f0e7672c1f85e3f) --- ChangeLog | 15 +++++++++++++++ manual/examples/mkdirent.c | 42 ++++++++++++++++++++++++++++++++++++++++++ manual/pattern.texi | 39 ++++++++++++++++++++++++++++++++++++++- posix/bug-glob2.c | 2 +- posix/glob.c | 24 +++--------------------- posix/tst-gnuglob.c | 2 +- 6 files changed, 100 insertions(+), 24 deletions(-) create mode 100644 manual/examples/mkdirent.c diff --git a/ChangeLog b/ChangeLog index ac7f870508..cb3db0379d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,18 @@ +2016-04-29 Florian Weimer + + glob: Simplify and document the interface for the GLOB_ALTDIRFUNC + callback function gl_readdir. + * posix/glob.c (NAMELEN, CONVERT_D_NAMLEN): Remove. + (CONVERT_DIRENT_DIRENT64): Use strcpy instead of memcpy. + (glob_in_dir): Remove len. Use strdup instead of malloc and + memcpy to copy the name. + * manual/pattern.texi (Calling Glob): Document requirements for + implementations of the gl_readdir callback function. + * manual/examples/mkdirent.c: New example. + * posix/bug-glob2.c (my_readdir): Set d_ino to 1 unconditionally, + per the manual guidance. + * posix/tst-gnuglob.c (my_readdir): Likewise. + 2016-04-28 Stefan Liebler [BZ #18508] diff --git a/manual/examples/mkdirent.c b/manual/examples/mkdirent.c new file mode 100644 index 0000000000..f8400f46d7 --- /dev/null +++ b/manual/examples/mkdirent.c @@ -0,0 +1,42 @@ +/* Example for creating a struct dirent object for use with glob. + Copyright (C) 2016 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or + modify it under the terms of the GNU General Public License + as published by the Free Software Foundation; either version 2 + of the License, or (at your option) any later version. + + This program 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 General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program; if not, if not, see . +*/ + +#include +#include +#include +#include +#include + +struct dirent * +mkdirent (const char *name) +{ + size_t dirent_size = offsetof (struct dirent, d_name) + 1; + size_t name_length = strlen (name); + size_t total_size = dirent_size + name_length; + if (total_size < dirent_size) + { + errno = ENOMEM; + return NULL; + } + struct dirent *result = malloc (total_size); + if (result == NULL) + return NULL; + result->d_type = DT_UNKNOWN; + result->d_ino = 1; /* Do not skip this entry. */ + memcpy (result->d_name, name, name_length + 1); + return result; +} diff --git a/manual/pattern.texi b/manual/pattern.texi index da848c340b..4cf26a7cf3 100644 --- a/manual/pattern.texi +++ b/manual/pattern.texi @@ -237,7 +237,44 @@ function used to read the contents of a directory. It is used if the @code{GLOB_ALTDIRFUNC} bit is set in the flag parameter. The type of this field is @w{@code{struct dirent *(*) (void *)}}. -This is a GNU extension. +An implementation of @code{gl_readdir} needs to initialize the following +members of the @code{struct dirent} object: + +@table @code +@item d_type +This member should be set to the file type of the entry if it is known. +Otherwise, the value @code{DT_UNKNOWN} can be used. The @code{glob} +function may use the specified file type to avoid callbacks in cases +where the file type indicates that the data is not required. + +@item d_ino +This member needs to be non-zero, otherwise @code{glob} may skip the +current entry and call the @code{gl_readdir} callback function again to +retrieve another entry. + +@item d_name +This member must be set to the name of the entry. It must be +null-terminated. +@end table + +The example below shows how to allocate a @code{struct dirent} object +containing a given name. + +@smallexample +@include mkdirent.c.texi +@end smallexample + +The @code{glob} function reads the @code{struct dirent} members listed +above and makes a copy of the file name in the @code{d_name} member +immediately after the @code{gl_readdir} callback function returns. +Future invocations of any of the callback functions may dealloacte or +reuse the buffer. It is the responsibility of the caller of the +@code{glob} function to allocate and deallocate the buffer, around the +call to @code{glob} or using the callback functions. For example, an +application could allocate the buffer in the @code{gl_readdir} callback +function, and deallocate it in the @code{gl_closedir} callback function. + +The @code{gl_readdir} member is a GNU extension. @item gl_opendir The address of an alternative implementation of the @code{opendir} diff --git a/posix/bug-glob2.c b/posix/bug-glob2.c index 8e21deb658..1b1ca5a719 100644 --- a/posix/bug-glob2.c +++ b/posix/bug-glob2.c @@ -193,7 +193,7 @@ my_readdir (void *gdir) return NULL; } - dir->d.d_ino = dir->idx; + dir->d.d_ino = 1; /* glob should not skip this entry. */ #ifdef _DIRENT_HAVE_D_TYPE dir->d.d_type = filesystem[dir->idx].type; diff --git a/posix/glob.c b/posix/glob.c index f1431088a2..05749130cc 100644 --- a/posix/glob.c +++ b/posix/glob.c @@ -57,10 +57,8 @@ #if defined HAVE_DIRENT_H || defined __GNU_LIBRARY__ # include -# define NAMLEN(dirent) strlen((dirent)->d_name) #else # define dirent direct -# define NAMLEN(dirent) (dirent)->d_namlen # ifdef HAVE_SYS_NDIR_H # include # endif @@ -76,12 +74,6 @@ #endif -/* In GNU systems, defines this macro for us. */ -#ifdef _D_NAMLEN -# undef NAMLEN -# define NAMLEN(d) _D_NAMLEN(d) -#endif - /* When used in the GNU libc the symbol _DIRENT_HAVE_D_TYPE is available if the `d_type' member for `struct dirent' is available. HAVE_STRUCT_DIRENT_D_TYPE plays the same role in GNULIB. */ @@ -105,12 +97,6 @@ /* If the system has the `struct dirent64' type we use it internally. */ #if defined _LIBC && !defined COMPILE_GLOB64 -# if defined HAVE_DIRENT_H || defined __GNU_LIBRARY__ -# define CONVERT_D_NAMLEN(d64, d32) -# else -# define CONVERT_D_NAMLEN(d64, d32) \ - (d64)->d_namlen = (d32)->d_namlen; -# endif # if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__ # define CONVERT_D_INO(d64, d32) @@ -127,8 +113,7 @@ # endif # define CONVERT_DIRENT_DIRENT64(d64, d32) \ - memcpy ((d64)->d_name, (d32)->d_name, NAMLEN (d32) + 1); \ - CONVERT_D_NAMLEN (d64, d32) \ + strcpy ((d64)->d_name, (d32)->d_name); \ CONVERT_D_INO (d64, d32) \ CONVERT_D_TYPE (d64, d32) #endif @@ -1562,7 +1547,6 @@ glob_in_dir (const char *pattern, const char *directory, int flags, while (1) { const char *name; - size_t len; #if defined _LIBC && !defined COMPILE_GLOB64 struct dirent64 *d; union @@ -1630,12 +1614,10 @@ glob_in_dir (const char *pattern, const char *directory, int flags, names = newnames; cur = 0; } - len = NAMLEN (d); - names->name[cur] = (char *) malloc (len + 1); + names->name[cur] = strdup (d->d_name); if (names->name[cur] == NULL) goto memory_error; - *((char *) mempcpy (names->name[cur++], name, len)) - = '\0'; + ++cur; ++nfound; } } diff --git a/posix/tst-gnuglob.c b/posix/tst-gnuglob.c index 1c72357de3..48c7527be8 100644 --- a/posix/tst-gnuglob.c +++ b/posix/tst-gnuglob.c @@ -211,7 +211,7 @@ my_readdir (void *gdir) return NULL; } - dir->d.d_ino = dir->idx; + dir->d.d_ino = 1; /* glob should not skip this entry. */ #ifdef _DIRENT_HAVE_D_TYPE dir->d.d_type = filesystem[dir->idx].type; -- cgit 1.4.1 From e97fb84811238c627f93e5e703a11eb841601947 Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Wed, 4 May 2016 12:09:35 +0200 Subject: CVE-2016-1234: glob: Do not copy d_name field of struct dirent [BZ #19779] Instead, we store the data we need from the return value of readdir in an object of the new type struct readdir_result. This type is independent of the layout of struct dirent. (cherry picked from commit 5171f3079f2cc53e0548fc4967361f4d1ce9d7ea) --- ChangeLog | 21 ++++ NEWS | 6 +- posix/bug-glob2.c | 14 ++- posix/glob.c | 223 +++++++++++++++++++--------------- sysdeps/unix/sysv/linux/i386/glob64.c | 22 ++++ 5 files changed, 185 insertions(+), 101 deletions(-) diff --git a/ChangeLog b/ChangeLog index cb3db0379d..c55bff53ba 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,24 @@ +2016-05-04 Florian Weimer + + [BZ #19779] + CVE-2016-1234 + Avoid copying names of directory entries. + * posix/glob.c (DIRENT_MUST_BE, DIRENT_MIGHT_BE_SYMLINK) + (DIRENT_MIGHT_BE_DIR, CONVERT_D_INO, CONVERT_D_TYPE) + (CONVERT_DIRENT_DIRENT64, REAL_DIR_ENTRY): Remove macros. + (struct readdir_result): New type. + (D_TYPE_TO_RESULT, D_INO_TO_RESULT, READDIR_RESULT_INITIALIZER) + (GL_READDIR): New macros. + (readdir_result_might_be_symlink, readdir_result_might_be_dir) + (convert_dirent, convert_dirent64): New functions. + (glob_in_dir): Use struct readdir_result. Call convert_dirent or + convert_dirent64. Adjust references to the readdir result. + * sysdeps/unix/sysv/linux/i386/glob64.c: + (convert_dirent, GL_READDIR): Redefine for second file inclusion. + * posix/bug-glob2.c (LONG_NAME): Define. + (filesystem): Add LONG_NAME. + (my_DIR): Increase the size of room_for_dirent. + 2016-04-29 Florian Weimer glob: Simplify and document the interface for the GLOB_ALTDIRFUNC diff --git a/NEWS b/NEWS index b2a1871389..00a8add0f3 100644 --- a/NEWS +++ b/NEWS @@ -12,7 +12,7 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18080, 18240, 18287, 18508, 18905, 19879. + 18032, 18080, 18240, 18287, 18508, 18905, 19779, 19879. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a @@ -68,6 +68,10 @@ Version 2.19.1 alloca call (in the form of a call to strdupa), leading to a stack overflow (stack exhaustion) and a crash if getnetbyname is invoked on a very long name. (CVE-2016-3075) + +* The glob function suffered from a stack-based buffer overflow when it was + called with the GLOB_ALTDIRFUNC flag and encountered a long file name. + Reported by Alexander Cherepanov. (CVE-2016-1234) Version 2.19 diff --git a/posix/bug-glob2.c b/posix/bug-glob2.c index 1b1ca5a719..3f9c620299 100644 --- a/posix/bug-glob2.c +++ b/posix/bug-glob2.c @@ -40,6 +40,17 @@ # define PRINTF(fmt, args...) #endif +#define LONG_NAME \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" \ + "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" static struct { @@ -58,6 +69,7 @@ static struct { ".", 3, DT_DIR, 0755 }, { "..", 3, DT_DIR, 0755 }, { "a", 3, DT_REG, 0644 }, + { LONG_NAME, 3, DT_REG, 0644 }, { "unreadable", 2, DT_DIR, 0111 }, { ".", 3, DT_DIR, 0111 }, { "..", 3, DT_DIR, 0755 }, @@ -75,7 +87,7 @@ typedef struct int level; int idx; struct dirent d; - char room_for_dirent[NAME_MAX]; + char room_for_dirent[sizeof (LONG_NAME)]; } my_DIR; diff --git a/posix/glob.c b/posix/glob.c index 05749130cc..ae3b8b770f 100644 --- a/posix/glob.c +++ b/posix/glob.c @@ -24,7 +24,9 @@ #include #include #include +#include #include +#include /* Outcomment the following line for production quality code. */ /* #define NDEBUG 1 */ @@ -73,69 +75,8 @@ # endif /* HAVE_VMSDIR_H */ #endif - -/* When used in the GNU libc the symbol _DIRENT_HAVE_D_TYPE is available - if the `d_type' member for `struct dirent' is available. - HAVE_STRUCT_DIRENT_D_TYPE plays the same role in GNULIB. */ -#if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE -/* True if the directory entry D must be of type T. */ -# define DIRENT_MUST_BE(d, t) ((d)->d_type == (t)) - -/* True if the directory entry D might be a symbolic link. */ -# define DIRENT_MIGHT_BE_SYMLINK(d) \ - ((d)->d_type == DT_UNKNOWN || (d)->d_type == DT_LNK) - -/* True if the directory entry D might be a directory. */ -# define DIRENT_MIGHT_BE_DIR(d) \ - ((d)->d_type == DT_DIR || DIRENT_MIGHT_BE_SYMLINK (d)) - -#else /* !HAVE_D_TYPE */ -# define DIRENT_MUST_BE(d, t) false -# define DIRENT_MIGHT_BE_SYMLINK(d) true -# define DIRENT_MIGHT_BE_DIR(d) true -#endif /* HAVE_D_TYPE */ - -/* If the system has the `struct dirent64' type we use it internally. */ -#if defined _LIBC && !defined COMPILE_GLOB64 - -# if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__ -# define CONVERT_D_INO(d64, d32) -# else -# define CONVERT_D_INO(d64, d32) \ - (d64)->d_ino = (d32)->d_ino; -# endif - -# ifdef _DIRENT_HAVE_D_TYPE -# define CONVERT_D_TYPE(d64, d32) \ - (d64)->d_type = (d32)->d_type; -# else -# define CONVERT_D_TYPE(d64, d32) -# endif - -# define CONVERT_DIRENT_DIRENT64(d64, d32) \ - strcpy ((d64)->d_name, (d32)->d_name); \ - CONVERT_D_INO (d64, d32) \ - CONVERT_D_TYPE (d64, d32) -#endif - - -#if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__ -/* Posix does not require that the d_ino field be present, and some - systems do not provide it. */ -# define REAL_DIR_ENTRY(dp) 1 -#else -# define REAL_DIR_ENTRY(dp) (dp->d_ino != 0) -#endif /* POSIX */ - #include #include - -/* NAME_MAX is usually defined in or . */ -#include -#ifndef NAME_MAX -# define NAME_MAX (sizeof (((struct dirent *) 0)->d_name)) -#endif - #include #ifdef _LIBC @@ -180,8 +121,111 @@ static const char *next_brace_sub (const char *begin, int flags) __THROWNL; +/* A representation of a directory entry which does not depend on the + layout of struct dirent, or the size of ino_t. */ +struct readdir_result +{ + const char *name; +# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE + uint8_t type; +# endif + bool skip_entry; +}; + +# if defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE +/* Initializer based on the d_type member of struct dirent. */ +# define D_TYPE_TO_RESULT(source) (source)->d_type, + +/* True if the directory entry D might be a symbolic link. */ +static bool +readdir_result_might_be_symlink (struct readdir_result d) +{ + return d.type == DT_UNKNOWN || d.type == DT_LNK; +} + +/* True if the directory entry D might be a directory. */ +static bool +readdir_result_might_be_dir (struct readdir_result d) +{ + return d.type == DT_DIR || readdir_result_might_be_symlink (d); +} +# else /* defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE */ +# define D_TYPE_TO_RESULT(source) + +/* If we do not have type information, symbolic links and directories + are always a possibility. */ + +static bool +readdir_result_might_be_symlink (struct readdir_result d) +{ + return true; +} + +static bool +readdir_result_might_be_dir (struct readdir_result d) +{ + return true; +} + +# endif /* defined _DIRENT_HAVE_D_TYPE || defined HAVE_STRUCT_DIRENT_D_TYPE */ + +# if (defined POSIX || defined WINDOWS32) && !defined __GNU_LIBRARY__ +/* Initializer for skip_entry. POSIX does not require that the d_ino + field be present, and some systems do not provide it. */ +# define D_INO_TO_RESULT(source) false, +# else +# define D_INO_TO_RESULT(source) (source)->d_ino == 0, +# endif + +/* Construct an initializer for a struct readdir_result object from a + struct dirent *. No copy of the name is made. */ +#define READDIR_RESULT_INITIALIZER(source) \ + { \ + source->d_name, \ + D_TYPE_TO_RESULT (source) \ + D_INO_TO_RESULT (source) \ + } + #endif /* !defined _LIBC || !defined GLOB_ONLY_P */ +/* Call gl_readdir on STREAM. This macro can be overridden to reduce + type safety if an old interface version needs to be supported. */ +#ifndef GL_READDIR +# define GL_READDIR(pglob, stream) ((pglob)->gl_readdir (stream)) +#endif + +/* Extract name and type from directory entry. No copy of the name is + made. If SOURCE is NULL, result name is NULL. Keep in sync with + convert_dirent64 below. */ +static struct readdir_result +convert_dirent (const struct dirent *source) +{ + if (source == NULL) + { + struct readdir_result result = { NULL, }; + return result; + } + struct readdir_result result = READDIR_RESULT_INITIALIZER (source); + return result; +} + +#ifndef COMPILE_GLOB64 +/* Like convert_dirent, but works on struct dirent64 instead. Keep in + sync with convert_dirent above. */ +static struct readdir_result +convert_dirent64 (const struct dirent64 *source) +{ + if (source == NULL) + { + struct readdir_result result = { NULL, }; + return result; + } + struct readdir_result result = READDIR_RESULT_INITIALIZER (source); + return result; +} +#endif + + #ifndef attribute_hidden # define attribute_hidden #endif @@ -1546,55 +1590,36 @@ glob_in_dir (const char *pattern, const char *directory, int flags, while (1) { - const char *name; -#if defined _LIBC && !defined COMPILE_GLOB64 - struct dirent64 *d; - union - { - struct dirent64 d64; - char room [offsetof (struct dirent64, d_name[0]) - + NAME_MAX + 1]; - } - d64buf; - - if (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)) - { - struct dirent *d32 = (*pglob->gl_readdir) (stream); - if (d32 != NULL) - { - CONVERT_DIRENT_DIRENT64 (&d64buf.d64, d32); - d = &d64buf.d64; - } - else - d = NULL; - } - else - d = __readdir64 (stream); + struct readdir_result d; + { + if (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0)) + d = convert_dirent (GL_READDIR (pglob, stream)); + else + { +#ifdef COMPILE_GLOB64 + d = convert_dirent (__readdir (stream)); #else - struct dirent *d = (__builtin_expect (flags & GLOB_ALTDIRFUNC, 0) - ? ((struct dirent *) - (*pglob->gl_readdir) (stream)) - : __readdir (stream)); + d = convert_dirent64 (__readdir64 (stream)); #endif - if (d == NULL) + } + } + if (d.name == NULL) break; - if (! REAL_DIR_ENTRY (d)) + if (d.skip_entry) continue; /* If we shall match only directories use the information provided by the dirent call if possible. */ - if ((flags & GLOB_ONLYDIR) && !DIRENT_MIGHT_BE_DIR (d)) + if ((flags & GLOB_ONLYDIR) && !readdir_result_might_be_dir (d)) continue; - name = d->d_name; - - if (fnmatch (pattern, name, fnm_flags) == 0) + if (fnmatch (pattern, d.name, fnm_flags) == 0) { /* If the file we found is a symlink we have to make sure the target file exists. */ - if (!DIRENT_MIGHT_BE_SYMLINK (d) - || link_exists_p (dfd, directory, dirlen, name, pglob, - flags)) + if (!readdir_result_might_be_symlink (d) + || link_exists_p (dfd, directory, dirlen, d.name, + pglob, flags)) { if (cur == names->count) { @@ -1614,7 +1639,7 @@ glob_in_dir (const char *pattern, const char *directory, int flags, names = newnames; cur = 0; } - names->name[cur] = strdup (d->d_name); + names->name[cur] = strdup (d.name); if (names->name[cur] == NULL) goto memory_error; ++cur; diff --git a/sysdeps/unix/sysv/linux/i386/glob64.c b/sysdeps/unix/sysv/linux/i386/glob64.c index b4fcd1a73c..802c957d6c 100644 --- a/sysdeps/unix/sysv/linux/i386/glob64.c +++ b/sysdeps/unix/sysv/linux/i386/glob64.c @@ -1,3 +1,21 @@ +/* Two glob variants with 64-bit support, for dirent64 and __olddirent64. + Copyright (C) 1998-2016 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 @@ -38,11 +56,15 @@ int __old_glob64 (const char *__pattern, int __flags, #undef dirent #define dirent __old_dirent64 +#undef GL_READDIR +# define GL_READDIR(pglob, stream) \ + ((struct __old_dirent64 *) (pglob)->gl_readdir (stream)) #undef __readdir #define __readdir(dirp) __old_readdir64 (dirp) #undef glob #define glob(pattern, flags, errfunc, pglob) \ __old_glob64 (pattern, flags, errfunc, pglob) +#define convert_dirent __old_convert_dirent #define glob_in_dir __old_glob_in_dir #define GLOB_ATTRIBUTE attribute_compat_text_section -- cgit 1.4.1 From 762aafec34478bcef01a16acf1959732ab8bb2b6 Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Fri, 29 Apr 2016 10:35:34 +0200 Subject: CVE-2016-3706: getaddrinfo: stack overflow in hostent conversion [BZ #20010] When converting a struct hostent response to struct gaih_addrtuple, the gethosts macro (which is called from gaih_inet) used alloca, without malloc fallback for large responses. This commit changes this code to use calloc unconditionally. This commit also consolidated a second hostent-to-gaih_addrtuple conversion loop (in gaih_inet) to use the new conversion function. (cherry picked from commit 4ab2ab03d4351914ee53248dc5aef4a8c88ff8b9) --- ChangeLog | 10 ++++ NEWS | 7 ++- sysdeps/posix/getaddrinfo.c | 130 +++++++++++++++++++++++--------------------- 3 files changed, 85 insertions(+), 62 deletions(-) diff --git a/ChangeLog b/ChangeLog index c55bff53ba..671f2bfa8e 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2016-04-29 Florian Weimer + + [BZ #20010] + CVE-2016-3706 + * sysdeps/posix/getaddrinfo.c + (convert_hostent_to_gaih_addrtuple): New function. + (gethosts): Call convert_hostent_to_gaih_addrtuple. + (gaih_inet): Use convert_hostent_to_gaih_addrtuple to convert + AF_INET data. + 2016-05-04 Florian Weimer [BZ #19779] diff --git a/NEWS b/NEWS index 00a8add0f3..5304a054d2 100644 --- a/NEWS +++ b/NEWS @@ -12,7 +12,7 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18080, 18240, 18287, 18508, 18905, 19779, 19879. + 18032, 18080, 18240, 18287, 18508, 18905, 19779, 19879, 20010. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a @@ -72,6 +72,11 @@ Version 2.19.1 * The glob function suffered from a stack-based buffer overflow when it was called with the GLOB_ALTDIRFUNC flag and encountered a long file name. Reported by Alexander Cherepanov. (CVE-2016-1234) + +* Previously, getaddrinfo copied large amounts of address data to the stack, + even after the fix for CVE-2013-4458 has been applied, potentially + resulting in a stack overflow. getaddrinfo now uses a heap allocation + instead. Reported by Michael Petlan. (CVE-2016-3706) Version 2.19 diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c index d2283bcd4a..df6ce8b13e 100644 --- a/sysdeps/posix/getaddrinfo.c +++ b/sysdeps/posix/getaddrinfo.c @@ -168,9 +168,58 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp, return 0; } +/* Convert struct hostent to a list of struct gaih_addrtuple objects. + h_name is not copied, and the struct hostent object must not be + deallocated prematurely. *RESULT must be NULL or a pointer to an + object allocated using malloc, which is freed. */ +static bool +convert_hostent_to_gaih_addrtuple (const struct addrinfo *req, + int family, + struct hostent *h, + struct gaih_addrtuple **result) +{ + free (*result); + *result = NULL; + + /* Count the number of addresses in h->h_addr_list. */ + size_t count = 0; + for (char **p = h->h_addr_list; *p != NULL; ++p) + ++count; + + /* Report no data if no addresses are available, or if the incoming + address size is larger than what we can store. */ + if (count == 0 || h->h_length > sizeof (((struct gaih_addrtuple) {}).addr)) + return true; + + struct gaih_addrtuple *array = calloc (count, sizeof (*array)); + if (array == NULL) + return false; + + for (size_t i = 0; i < count; ++i) + { + if (family == AF_INET && req->ai_family == AF_INET6) + { + /* Perform address mapping. */ + array[i].family = AF_INET6; + memcpy(array[i].addr + 3, h->h_addr_list[i], sizeof (uint32_t)); + array[i].addr[2] = htonl (0xffff); + } + else + { + array[i].family = family; + memcpy (array[i].addr, h->h_addr_list[i], h->h_length); + } + array[i].next = array + i + 1; + } + array[0].name = h->h_name; + array[count - 1].next = NULL; + + *result = array; + return true; +} + #define gethosts(_family, _type) \ { \ - int i; \ int herrno; \ struct hostent th; \ struct hostent *h; \ @@ -219,36 +268,23 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp, } \ else if (h != NULL) \ { \ - for (i = 0; h->h_addr_list[i]; i++) \ + /* Make sure that addrmem can be freed. */ \ + if (!malloc_addrmem) \ + addrmem = NULL; \ + if (!convert_hostent_to_gaih_addrtuple (req, _family,h, &addrmem)) \ { \ - if (*pat == NULL) \ - { \ - *pat = __alloca (sizeof (struct gaih_addrtuple)); \ - (*pat)->scopeid = 0; \ - } \ - uint32_t *addr = (*pat)->addr; \ - (*pat)->next = NULL; \ - (*pat)->name = i == 0 ? strdupa (h->h_name) : NULL; \ - if (_family == AF_INET && req->ai_family == AF_INET6) \ - { \ - (*pat)->family = AF_INET6; \ - addr[3] = *(uint32_t *) h->h_addr_list[i]; \ - addr[2] = htonl (0xffff); \ - addr[1] = 0; \ - addr[0] = 0; \ - } \ - else \ - { \ - (*pat)->family = _family; \ - memcpy (addr, h->h_addr_list[i], sizeof(_type)); \ - } \ - pat = &((*pat)->next); \ + _res.options |= old_res_options & RES_USE_INET6; \ + result = -EAI_SYSTEM; \ + goto free_and_return; \ } \ + *pat = addrmem; \ + /* The conversion uses malloc unconditionally. */ \ + malloc_addrmem = true; \ \ if (localcanon != NULL && canon == NULL) \ canon = strdupa (localcanon); \ \ - if (_family == AF_INET6 && i > 0) \ + if (_family == AF_INET6 && *pat != NULL) \ got_ipv6 = true; \ } \ } @@ -612,44 +648,16 @@ gaih_inet (const char *name, const struct gaih_service *service, { if (h != NULL) { - int i; - /* We found data, count the number of addresses. */ - for (i = 0; h->h_addr_list[i]; ++i) - ; - if (i > 0 && *pat != NULL) - --i; - - if (__libc_use_alloca (alloca_used - + i * sizeof (struct gaih_addrtuple))) - addrmem = alloca_account (i * sizeof (struct gaih_addrtuple), - alloca_used); - else - { - addrmem = malloc (i - * sizeof (struct gaih_addrtuple)); - if (addrmem == NULL) - { - result = -EAI_MEMORY; - goto free_and_return; - } - malloc_addrmem = true; - } - - /* Now convert it into the list. */ - struct gaih_addrtuple *addrfree = addrmem; - for (i = 0; h->h_addr_list[i]; ++i) + /* We found data, convert it. */ + if (!convert_hostent_to_gaih_addrtuple + (req, AF_INET, h, &addrmem)) { - if (*pat == NULL) - { - *pat = addrfree++; - (*pat)->scopeid = 0; - } - (*pat)->next = NULL; - (*pat)->family = AF_INET; - memcpy ((*pat)->addr, h->h_addr_list[i], - h->h_length); - pat = &((*pat)->next); + result = -EAI_MEMORY; + goto free_and_return; } + *pat = addrmem; + /* The conversion uses malloc unconditionally. */ + malloc_addrmem = true; } } else -- cgit 1.4.1 From c08e8bd0ef1d16d0139dbc80a976e2cbf2517f02 Mon Sep 17 00:00:00 2001 From: Carlos O'Donell Date: Tue, 16 Feb 2016 21:26:37 -0500 Subject: CVE-2015-7547: getaddrinfo() stack-based buffer overflow (Bug 18665). * A stack-based buffer overflow was found in libresolv when invoked from libnss_dns, allowing specially crafted DNS responses to seize control of execution flow in the DNS client. The buffer overflow occurs in the functions send_dg (send datagram) and send_vc (send TCP) for the NSS module libnss_dns.so.2 when calling getaddrinfo with AF_UNSPEC family. The use of AF_UNSPEC triggers the low-level resolver code to send out two parallel queries for A and AAAA. A mismanagement of the buffers used for those queries could result in the response of a query writing beyond the alloca allocated buffer created by _nss_dns_gethostbyname4_r. Buffer management is simplified to remove the overflow. Thanks to the Google Security Team and Red Hat for reporting the security impact of this issue, and Robert Holiday of Ciena for reporting the related bug 18665. (CVE-2015-7547) See also: https://sourceware.org/ml/libc-alpha/2016-02/msg00416.html https://sourceware.org/ml/libc-alpha/2016-02/msg00418.html (cherry picked from commit e9db92d3acfe1822d56d11abcea5bfc4c41cf6ca) --- ChangeLog | 15 +++ NEWS | 16 ++- resolv/nss_dns/dns-host.c | 111 +++++++++++++++++++- resolv/res_query.c | 3 + resolv/res_send.c | 257 +++++++++++++++++++++++++++++++++++----------- 5 files changed, 339 insertions(+), 63 deletions(-) diff --git a/ChangeLog b/ChangeLog index 671f2bfa8e..df32997787 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,18 @@ +2016-02-15 Carlos O'Donell + + [BZ #18665] + * resolv/nss_dns/dns-host.c (gaih_getanswer_slice): Always set + *herrno_p. + (gaih_getanswer): Document functional behviour. Return tryagain + if any result is tryagain. + * resolv/res_query.c (__libc_res_nsearch): Set buffer size to zero + when freed. + * resolv/res_send.c: Add copyright text. + (__libc_res_nsend): Document that MAXPACKET is expected. + (send_vc): Document. Remove buffer reuse. + (send_dg): Document. Remove buffer reuse. Set *thisanssizp to set the + size of the buffer. Add Dprint for truncated UDP buffer. + 2016-04-29 Florian Weimer [BZ #20010] diff --git a/NEWS b/NEWS index 5304a054d2..8e00fe9a0f 100644 --- a/NEWS +++ b/NEWS @@ -12,7 +12,7 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18080, 18240, 18287, 18508, 18905, 19779, 19879, 20010. + 18032, 18080, 18240, 18287, 18508, 18665, 18905, 19779, 19879, 20010. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a @@ -77,6 +77,20 @@ Version 2.19.1 even after the fix for CVE-2013-4458 has been applied, potentially resulting in a stack overflow. getaddrinfo now uses a heap allocation instead. Reported by Michael Petlan. (CVE-2016-3706) + +* A stack-based buffer overflow was found in libresolv when invoked from + libnss_dns, allowing specially crafted DNS responses to seize control + of execution flow in the DNS client. The buffer overflow occurs in + the functions send_dg (send datagram) and send_vc (send TCP) for the + NSS module libnss_dns.so.2 when calling getaddrinfo with AF_UNSPEC + family. The use of AF_UNSPEC triggers the low-level resolver code to + send out two parallel queries for A and AAAA. A mismanagement of the + buffers used for those queries could result in the response of a query + writing beyond the alloca allocated buffer created by + _nss_dns_gethostbyname4_r. Buffer management is simplified to remove + the overflow. Thanks to the Google Security Team and Red Hat for + reporting the security impact of this issue, and Robert Holiday of + Ciena for reporting the related bug 18665. (CVE-2015-7547) Version 2.19 diff --git a/resolv/nss_dns/dns-host.c b/resolv/nss_dns/dns-host.c index f36d28bd70..63029d998e 100644 --- a/resolv/nss_dns/dns-host.c +++ b/resolv/nss_dns/dns-host.c @@ -1052,7 +1052,10 @@ gaih_getanswer_slice (const querybuf *answer, int anslen, const char *qname, int h_namelen = 0; if (ancount == 0) - return NSS_STATUS_NOTFOUND; + { + *h_errnop = HOST_NOT_FOUND; + return NSS_STATUS_NOTFOUND; + } while (ancount-- > 0 && cp < end_of_message && had_error == 0) { @@ -1229,7 +1232,14 @@ gaih_getanswer_slice (const querybuf *answer, int anslen, const char *qname, /* Special case here: if the resolver sent a result but it only contains a CNAME while we are looking for a T_A or T_AAAA record, we fail with NOTFOUND instead of TRYAGAIN. */ - return canon == NULL ? NSS_STATUS_TRYAGAIN : NSS_STATUS_NOTFOUND; + if (canon != NULL) + { + *h_errnop = HOST_NOT_FOUND; + return NSS_STATUS_NOTFOUND; + } + + *h_errnop = NETDB_INTERNAL; + return NSS_STATUS_TRYAGAIN; } @@ -1243,11 +1253,101 @@ gaih_getanswer (const querybuf *answer1, int anslen1, const querybuf *answer2, enum nss_status status = NSS_STATUS_NOTFOUND; + /* Combining the NSS status of two distinct queries requires some + compromise and attention to symmetry (A or AAAA queries can be + returned in any order). What follows is a breakdown of how this + code is expected to work and why. We discuss only SUCCESS, + TRYAGAIN, NOTFOUND and UNAVAIL, since they are the only returns + that apply (though RETURN and MERGE exist). We make a distinction + between TRYAGAIN (recoverable) and TRYAGAIN' (not-recoverable). + A recoverable TRYAGAIN is almost always due to buffer size issues + and returns ERANGE in errno and the caller is expected to retry + with a larger buffer. + + Lastly, you may be tempted to make significant changes to the + conditions in this code to bring about symmetry between responses. + Please don't change anything without due consideration for + expected application behaviour. Some of the synthesized responses + aren't very well thought out and sometimes appear to imply that + IPv4 responses are always answer 1, and IPv6 responses are always + answer 2, but that's not true (see the implementation of send_dg + and send_vc to see response can arrive in any order, particularly + for UDP). However, we expect it holds roughly enough of the time + that this code works, but certainly needs to be fixed to make this + a more robust implementation. + + ---------------------------------------------- + | Answer 1 Status / | Synthesized | Reason | + | Answer 2 Status | Status | | + |--------------------------------------------| + | SUCCESS/SUCCESS | SUCCESS | [1] | + | SUCCESS/TRYAGAIN | TRYAGAIN | [5] | + | SUCCESS/TRYAGAIN' | SUCCESS | [1] | + | SUCCESS/NOTFOUND | SUCCESS | [1] | + | SUCCESS/UNAVAIL | SUCCESS | [1] | + | TRYAGAIN/SUCCESS | TRYAGAIN | [2] | + | TRYAGAIN/TRYAGAIN | TRYAGAIN | [2] | + | TRYAGAIN/TRYAGAIN' | TRYAGAIN | [2] | + | TRYAGAIN/NOTFOUND | TRYAGAIN | [2] | + | TRYAGAIN/UNAVAIL | TRYAGAIN | [2] | + | TRYAGAIN'/SUCCESS | SUCCESS | [3] | + | TRYAGAIN'/TRYAGAIN | TRYAGAIN | [3] | + | TRYAGAIN'/TRYAGAIN' | TRYAGAIN' | [3] | + | TRYAGAIN'/NOTFOUND | TRYAGAIN' | [3] | + | TRYAGAIN'/UNAVAIL | UNAVAIL | [3] | + | NOTFOUND/SUCCESS | SUCCESS | [3] | + | NOTFOUND/TRYAGAIN | TRYAGAIN | [3] | + | NOTFOUND/TRYAGAIN' | TRYAGAIN' | [3] | + | NOTFOUND/NOTFOUND | NOTFOUND | [3] | + | NOTFOUND/UNAVAIL | UNAVAIL | [3] | + | UNAVAIL/SUCCESS | UNAVAIL | [4] | + | UNAVAIL/TRYAGAIN | UNAVAIL | [4] | + | UNAVAIL/TRYAGAIN' | UNAVAIL | [4] | + | UNAVAIL/NOTFOUND | UNAVAIL | [4] | + | UNAVAIL/UNAVAIL | UNAVAIL | [4] | + ---------------------------------------------- + + [1] If the first response is a success we return success. + This ignores the state of the second answer and in fact + incorrectly sets errno and h_errno to that of the second + answer. However because the response is a success we ignore + *errnop and *h_errnop (though that means you touched errno on + success). We are being conservative here and returning the + likely IPv4 response in the first answer as a success. + + [2] If the first response is a recoverable TRYAGAIN we return + that instead of looking at the second response. The + expectation here is that we have failed to get an IPv4 response + and should retry both queries. + + [3] If the first response was not a SUCCESS and the second + response is not NOTFOUND (had a SUCCESS, need to TRYAGAIN, + or failed entirely e.g. TRYAGAIN' and UNAVAIL) then use the + result from the second response, otherwise the first responses + status is used. Again we have some odd side-effects when the + second response is NOTFOUND because we overwrite *errnop and + *h_errnop that means that a first answer of NOTFOUND might see + its *errnop and *h_errnop values altered. Whether it matters + in practice that a first response NOTFOUND has the wrong + *errnop and *h_errnop is undecided. + + [4] If the first response is UNAVAIL we return that instead of + looking at the second response. The expectation here is that + it will have failed similarly e.g. configuration failure. + + [5] Testing this code is complicated by the fact that truncated + second response buffers might be returned as SUCCESS if the + first answer is a SUCCESS. To fix this we add symmetry to + TRYAGAIN with the second response. If the second response + is a recoverable error we now return TRYAGIN even if the first + response was SUCCESS. */ + if (anslen1 > 0) status = gaih_getanswer_slice(answer1, anslen1, qname, &pat, &buffer, &buflen, errnop, h_errnop, ttlp, &first); + if ((status == NSS_STATUS_SUCCESS || status == NSS_STATUS_NOTFOUND || (status == NSS_STATUS_TRYAGAIN /* We want to look at the second answer in case of an @@ -1263,8 +1363,15 @@ gaih_getanswer (const querybuf *answer1, int anslen1, const querybuf *answer2, &pat, &buffer, &buflen, errnop, h_errnop, ttlp, &first); + /* Use the second response status in some cases. */ if (status != NSS_STATUS_SUCCESS && status2 != NSS_STATUS_NOTFOUND) status = status2; + /* Do not return a truncated second response (unless it was + unavoidable e.g. unrecoverable TRYAGAIN). */ + if (status == NSS_STATUS_SUCCESS + && (status2 == NSS_STATUS_TRYAGAIN + && *errnop == ERANGE && *h_errnop != NO_RECOVERY)) + status = NSS_STATUS_TRYAGAIN; } return status; diff --git a/resolv/res_query.c b/resolv/res_query.c index c5c34026ef..6f5280df8e 100644 --- a/resolv/res_query.c +++ b/resolv/res_query.c @@ -394,6 +394,7 @@ __libc_res_nsearch(res_state statp, { free (*answerp2); *answerp2 = NULL; + *nanswerp2 = 0; *answerp2_malloced = 0; } } @@ -433,6 +434,7 @@ __libc_res_nsearch(res_state statp, { free (*answerp2); *answerp2 = NULL; + *nanswerp2 = 0; *answerp2_malloced = 0; } @@ -506,6 +508,7 @@ __libc_res_nsearch(res_state statp, { free (*answerp2); *answerp2 = NULL; + *nanswerp2 = 0; *answerp2_malloced = 0; } if (saved_herrno != -1) diff --git a/resolv/res_send.c b/resolv/res_send.c index 416da8777e..e80bb7a336 100644 --- a/resolv/res_send.c +++ b/resolv/res_send.c @@ -1,3 +1,20 @@ +/* Copyright (C) 2016 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 + . */ + /* * Copyright (c) 1985, 1989, 1993 * The Regents of the University of California. All rights reserved. @@ -360,6 +377,8 @@ __libc_res_nsend(res_state statp, const u_char *buf, int buflen, #ifdef USE_HOOKS if (__builtin_expect (statp->qhook || statp->rhook, 0)) { if (anssiz < MAXPACKET && ansp) { + /* Always allocate MAXPACKET, callers expect + this specific size. */ u_char *buf = malloc (MAXPACKET); if (buf == NULL) return (-1); @@ -653,6 +672,77 @@ libresolv_hidden_def (res_nsend) /* Private */ +/* The send_vc function is responsible for sending a DNS query over TCP + to the nameserver numbered NS from the res_state STATP i.e. + EXT(statp).nssocks[ns]. The function supports sending both IPv4 and + IPv6 queries at the same serially on the same socket. + + Please note that for TCP there is no way to disable sending both + queries, unlike UDP, which honours RES_SNGLKUP and RES_SNGLKUPREOP + and sends the queries serially and waits for the result after each + sent query. This implemetnation should be corrected to honour these + options. + + Please also note that for TCP we send both queries over the same + socket one after another. This technically violates best practice + since the server is allowed to read the first query, respond, and + then close the socket (to service another client). If the server + does this, then the remaining second query in the socket data buffer + will cause the server to send the client an RST which will arrive + asynchronously and the client's OS will likely tear down the socket + receive buffer resulting in a potentially short read and lost + response data. This will force the client to retry the query again, + and this process may repeat until all servers and connection resets + are exhausted and then the query will fail. It's not known if this + happens with any frequency in real DNS server implementations. This + implementation should be corrected to use two sockets by default for + parallel queries. + + The query stored in BUF of BUFLEN length is sent first followed by + the query stored in BUF2 of BUFLEN2 length. Queries are sent + serially on the same socket. + + Answers to the query are stored firstly in *ANSP up to a max of + *ANSSIZP bytes. If more than *ANSSIZP bytes are needed and ANSCP + is non-NULL (to indicate that modifying the answer buffer is allowed) + then malloc is used to allocate a new response buffer and ANSCP and + ANSP will both point to the new buffer. If more than *ANSSIZP bytes + are needed but ANSCP is NULL, then as much of the response as + possible is read into the buffer, but the results will be truncated. + When truncation happens because of a small answer buffer the DNS + packets header feild TC will bet set to 1, indicating a truncated + message and the rest of the socket data will be read and discarded. + + Answers to the query are stored secondly in *ANSP2 up to a max of + *ANSSIZP2 bytes, with the actual response length stored in + *RESPLEN2. If more than *ANSSIZP bytes are needed and ANSP2 + is non-NULL (required for a second query) then malloc is used to + allocate a new response buffer, *ANSSIZP2 is set to the new buffer + size and *ANSP2_MALLOCED is set to 1. + + The ANSP2_MALLOCED argument will eventually be removed as the + change in buffer pointer can be used to detect the buffer has + changed and that the caller should use free on the new buffer. + + Note that the answers may arrive in any order from the server and + therefore the first and second answer buffers may not correspond to + the first and second queries. + + It is not supported to call this function with a non-NULL ANSP2 + but a NULL ANSCP. Put another way, you can call send_vc with a + single unmodifiable buffer or two modifiable buffers, but no other + combination is supported. + + It is the caller's responsibility to free the malloc allocated + buffers by detecting that the pointers have changed from their + original values i.e. *ANSCP or *ANSP2 has changed. + + If errors are encountered then *TERRNO is set to an appropriate + errno value and a zero result is returned for a recoverable error, + and a less-than zero result is returned for a non-recoverable error. + + If no errors are encountered then *TERRNO is left unmodified and + a the length of the first response in bytes is returned. */ static int send_vc(res_state statp, const u_char *buf, int buflen, const u_char *buf2, int buflen2, @@ -662,11 +752,7 @@ send_vc(res_state statp, { const HEADER *hp = (HEADER *) buf; const HEADER *hp2 = (HEADER *) buf2; - u_char *ans = *ansp; - int orig_anssizp = *anssizp; - // XXX REMOVE - // int anssiz = *anssizp; - HEADER *anhp = (HEADER *) ans; + HEADER *anhp = (HEADER *) *ansp; struct sockaddr_in6 *nsap = EXT(statp).nsaddrs[ns]; int truncating, connreset, resplen, n; struct iovec iov[4]; @@ -742,6 +828,8 @@ send_vc(res_state statp, * Receive length & response */ int recvresp1 = 0; + /* Skip the second response if there is no second query. + To do that we mark the second response as received. */ int recvresp2 = buf2 == NULL; uint16_t rlen16; read_len: @@ -778,33 +866,14 @@ send_vc(res_state statp, u_char **thisansp; int *thisresplenp; if ((recvresp1 | recvresp2) == 0 || buf2 == NULL) { + /* We have not received any responses + yet or we only have one response to + receive. */ thisanssizp = anssizp; thisansp = anscp ?: ansp; assert (anscp != NULL || ansp2 == NULL); thisresplenp = &resplen; } else { - if (*anssizp != MAXPACKET) { - /* No buffer allocated for the first - reply. We can try to use the rest - of the user-provided buffer. */ -#ifdef _STRING_ARCH_unaligned - *anssizp2 = orig_anssizp - resplen; - *ansp2 = *ansp + resplen; -#else - int aligned_resplen - = ((resplen + __alignof__ (HEADER) - 1) - & ~(__alignof__ (HEADER) - 1)); - *anssizp2 = orig_anssizp - aligned_resplen; - *ansp2 = *ansp + aligned_resplen; -#endif - } else { - /* The first reply did not fit into the - user-provided buffer. Maybe the second - answer will. */ - *anssizp2 = orig_anssizp; - *ansp2 = *ansp; - } - thisanssizp = anssizp2; thisansp = ansp2; thisresplenp = resplen2; @@ -812,10 +881,14 @@ send_vc(res_state statp, anhp = (HEADER *) *thisansp; *thisresplenp = rlen; - if (rlen > *thisanssizp) { - /* Yes, we test ANSCP here. If we have two buffers - both will be allocatable. */ - if (__builtin_expect (anscp != NULL, 1)) { + /* Is the answer buffer too small? */ + if (*thisanssizp < rlen) { + /* If the current buffer is non-NULL and it's not + pointing at the static user-supplied buffer then + we can reallocate it. */ + if (thisansp != NULL && thisansp != ansp) { + /* Always allocate MAXPACKET, callers expect + this specific size. */ u_char *newp = malloc (MAXPACKET); if (newp == NULL) { *terrno = ENOMEM; @@ -827,6 +900,9 @@ send_vc(res_state statp, if (thisansp == ansp2) *ansp2_malloced = 1; anhp = (HEADER *) newp; + /* A uint16_t can't be larger than MAXPACKET + thus it's safe to allocate MAXPACKET but + read RLEN bytes instead. */ len = rlen; } else { Dprint(statp->options & RES_DEBUG, @@ -990,6 +1066,66 @@ reopen (res_state statp, int *terrno, int ns) return 1; } +/* The send_dg function is responsible for sending a DNS query over UDP + to the nameserver numbered NS from the res_state STATP i.e. + EXT(statp).nssocks[ns]. The function supports IPv4 and IPv6 queries + along with the ability to send the query in parallel for both stacks + (default) or serially (RES_SINGLKUP). It also supports serial lookup + with a close and reopen of the socket used to talk to the server + (RES_SNGLKUPREOP) to work around broken name servers. + + The query stored in BUF of BUFLEN length is sent first followed by + the query stored in BUF2 of BUFLEN2 length. Queries are sent + in parallel (default) or serially (RES_SINGLKUP or RES_SNGLKUPREOP). + + Answers to the query are stored firstly in *ANSP up to a max of + *ANSSIZP bytes. If more than *ANSSIZP bytes are needed and ANSCP + is non-NULL (to indicate that modifying the answer buffer is allowed) + then malloc is used to allocate a new response buffer and ANSCP and + ANSP will both point to the new buffer. If more than *ANSSIZP bytes + are needed but ANSCP is NULL, then as much of the response as + possible is read into the buffer, but the results will be truncated. + When truncation happens because of a small answer buffer the DNS + packets header feild TC will bet set to 1, indicating a truncated + message, while the rest of the UDP packet is discarded. + + Answers to the query are stored secondly in *ANSP2 up to a max of + *ANSSIZP2 bytes, with the actual response length stored in + *RESPLEN2. If more than *ANSSIZP bytes are needed and ANSP2 + is non-NULL (required for a second query) then malloc is used to + allocate a new response buffer, *ANSSIZP2 is set to the new buffer + size and *ANSP2_MALLOCED is set to 1. + + The ANSP2_MALLOCED argument will eventually be removed as the + change in buffer pointer can be used to detect the buffer has + changed and that the caller should use free on the new buffer. + + Note that the answers may arrive in any order from the server and + therefore the first and second answer buffers may not correspond to + the first and second queries. + + It is not supported to call this function with a non-NULL ANSP2 + but a NULL ANSCP. Put another way, you can call send_vc with a + single unmodifiable buffer or two modifiable buffers, but no other + combination is supported. + + It is the caller's responsibility to free the malloc allocated + buffers by detecting that the pointers have changed from their + original values i.e. *ANSCP or *ANSP2 has changed. + + If an answer is truncated because of UDP datagram DNS limits then + *V_CIRCUIT is set to 1 and the return value non-zero to indicate to + the caller to retry with TCP. The value *GOTSOMEWHERE is set to 1 + if any progress was made reading a response from the nameserver and + is used by the caller to distinguish between ECONNREFUSED and + ETIMEDOUT (the latter if *GOTSOMEWHERE is 1). + + If errors are encountered then *TERRNO is set to an appropriate + errno value and a zero result is returned for a recoverable error, + and a less-than zero result is returned for a non-recoverable error. + + If no errors are encountered then *TERRNO is left unmodified and + a the length of the first response in bytes is returned. */ static int send_dg(res_state statp, const u_char *buf, int buflen, const u_char *buf2, int buflen2, @@ -999,8 +1135,6 @@ send_dg(res_state statp, { const HEADER *hp = (HEADER *) buf; const HEADER *hp2 = (HEADER *) buf2; - u_char *ans = *ansp; - int orig_anssizp = *anssizp; struct timespec now, timeout, finish; struct pollfd pfd[1]; int ptimeout; @@ -1033,6 +1167,8 @@ send_dg(res_state statp, int need_recompute = 0; int nwritten = 0; int recvresp1 = 0; + /* Skip the second response if there is no second query. + To do that we mark the second response as received. */ int recvresp2 = buf2 == NULL; pfd[0].fd = EXT(statp).nssocks[ns]; pfd[0].events = POLLOUT; @@ -1196,55 +1332,56 @@ send_dg(res_state statp, int *thisresplenp; if ((recvresp1 | recvresp2) == 0 || buf2 == NULL) { + /* We have not received any responses + yet or we only have one response to + receive. */ thisanssizp = anssizp; thisansp = anscp ?: ansp; assert (anscp != NULL || ansp2 == NULL); thisresplenp = &resplen; } else { - if (*anssizp != MAXPACKET) { - /* No buffer allocated for the first - reply. We can try to use the rest - of the user-provided buffer. */ -#ifdef _STRING_ARCH_unaligned - *anssizp2 = orig_anssizp - resplen; - *ansp2 = *ansp + resplen; -#else - int aligned_resplen - = ((resplen + __alignof__ (HEADER) - 1) - & ~(__alignof__ (HEADER) - 1)); - *anssizp2 = orig_anssizp - aligned_resplen; - *ansp2 = *ansp + aligned_resplen; -#endif - } else { - /* The first reply did not fit into the - user-provided buffer. Maybe the second - answer will. */ - *anssizp2 = orig_anssizp; - *ansp2 = *ansp; - } - thisanssizp = anssizp2; thisansp = ansp2; thisresplenp = resplen2; } if (*thisanssizp < MAXPACKET - /* Yes, we test ANSCP here. If we have two buffers - both will be allocatable. */ - && anscp + /* If the current buffer is non-NULL and it's not + pointing at the static user-supplied buffer then + we can reallocate it. */ + && (thisansp != NULL && thisansp != ansp) #ifdef FIONREAD + /* Is the size too small? */ && (ioctl (pfd[0].fd, FIONREAD, thisresplenp) < 0 || *thisanssizp < *thisresplenp) #endif ) { + /* Always allocate MAXPACKET, callers expect + this specific size. */ u_char *newp = malloc (MAXPACKET); if (newp != NULL) { - *anssizp = MAXPACKET; - *thisansp = ans = newp; + *thisanssizp = MAXPACKET; + *thisansp = newp; if (thisansp == ansp2) *ansp2_malloced = 1; } } + /* We could end up with truncation if anscp was NULL + (not allowed to change caller's buffer) and the + response buffer size is too small. This isn't a + reliable way to detect truncation because the ioctl + may be an inaccurate report of the UDP message size. + Therefore we use this only to issue debug output. + To do truncation accurately with UDP we need + MSG_TRUNC which is only available on Linux. We + can abstract out the Linux-specific feature in the + future to detect truncation. */ + if (__glibc_unlikely (*thisanssizp < *thisresplenp)) { + Dprint(statp->options & RES_DEBUG, + (stdout, ";; response may be truncated (UDP)\n") + ); + } + HEADER *anhp = (HEADER *) *thisansp; socklen_t fromlen = sizeof(struct sockaddr_in6); assert (sizeof(from) <= fromlen); -- cgit 1.4.1 From 10d268070a8aa9a878668e7f060e92ed668de146 Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Fri, 25 Mar 2016 11:49:51 +0100 Subject: resolv: Always set *resplen2 out parameter in send_dg [BZ #19791] Since commit 44d20bca52ace85850012b0ead37b360e3ecd96e (Implement second fallback mode for DNS requests), there is a code path which returns early, before *resplen2 is initialized. This happens if the name server address is immediately recognized as invalid (because of lack of protocol support, or if it is a broadcast address such 255.255.255.255, or another invalid address). If this happens and *resplen2 was non-zero (which is the case if a previous query resulted in a failure), __libc_res_nquery would reuse an existing second answer buffer. This answer has been previously identified as unusable (for example, it could be an NXDOMAIN response). Due to the presence of a second answer, no name server switching will occur. The result is a name resolution failure, although a successful resolution would have been possible if name servers have been switched and queries had proceeded along the search path. The above paragraph still simplifies the situation. Before glibc 2.23, if the second answer needed malloc, the stub resolver would still attempt to reuse the second answer, but this is not possible because __libc_res_nsearch has freed it, after the unsuccessful call to __libc_res_nquerydomain, and set the buffer pointer to NULL. This eventually leads to an assertion failure in __libc_res_nquery: /* Make sure both hp and hp2 are defined */ assert((hp != NULL) && (hp2 != NULL)); If assertions are disabled, the consequence is a NULL pointer dereference on the next line. Starting with glibc 2.23, as a result of commit e9db92d3acfe1822d56d11abcea5bfc4c41cf6ca (CVE-2015-7547: getaddrinfo() stack-based buffer overflow (Bug 18665)), the second answer is always allocated with malloc. This means that the assertion failure happens with small responses as well because there is no buffer to reuse, as soon as there is a name resolution failure which triggers a search for an answer along the search path. This commit addresses the issue by ensuring that *resplen2 is initialized before the send_dg function returns. This commit also addresses a bug where an invalid second reply is incorrectly returned as a valid to the caller. (cherry picked from commit b66d837bb5398795c6b0f651bd5a5d66091d8577) --- ChangeLog | 9 ++++++++ NEWS | 3 ++- resolv/res_send.c | 63 +++++++++++++++++++++++++++++++++++-------------------- 3 files changed, 51 insertions(+), 24 deletions(-) diff --git a/ChangeLog b/ChangeLog index df32997787..f9a9e335e6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,12 @@ +2016-03-25 Florian Weimer + + [BZ #19791] + * resolv/res_send.c (close_and_return_error): New function. + (send_dg): Initialize *resplen2 after reopen failure. Call + close_and_return_error for error returns. On error paths without + __res_iclose, initialze *resplen2 explicitly. Update comment for + successful return. + 2016-02-15 Carlos O'Donell [BZ #18665] diff --git a/NEWS b/NEWS index 8e00fe9a0f..d14f9edc6f 100644 --- a/NEWS +++ b/NEWS @@ -12,7 +12,8 @@ Version 2.19.1 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18080, 18240, 18287, 18508, 18665, 18905, 19779, 19879, 20010. + 18032, 18080, 18240, 18287, 18508, 18665, 18905, 19779, 19791, 19879, + 20010. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a diff --git a/resolv/res_send.c b/resolv/res_send.c index e80bb7a336..11d0bbd1a2 100644 --- a/resolv/res_send.c +++ b/resolv/res_send.c @@ -672,6 +672,18 @@ libresolv_hidden_def (res_nsend) /* Private */ +/* Close the resolver structure, assign zero to *RESPLEN2 if RESPLEN2 + is not NULL, and return zero. */ +static int +__attribute__ ((warn_unused_result)) +close_and_return_error (res_state statp, int *resplen2) +{ + __res_iclose(statp, false); + if (resplen2 != NULL) + *resplen2 = 0; + return 0; +} + /* The send_vc function is responsible for sending a DNS query over TCP to the nameserver numbered NS from the res_state STATP i.e. EXT(statp).nssocks[ns]. The function supports sending both IPv4 and @@ -1159,7 +1171,11 @@ send_dg(res_state statp, retry_reopen: retval = reopen (statp, terrno, ns); if (retval <= 0) - return retval; + { + if (resplen2 != NULL) + *resplen2 = 0; + return retval; + } retry: evNowTime(&now); evConsTime(&timeout, seconds, 0); @@ -1172,8 +1188,6 @@ send_dg(res_state statp, int recvresp2 = buf2 == NULL; pfd[0].fd = EXT(statp).nssocks[ns]; pfd[0].events = POLLOUT; - if (resplen2 != NULL) - *resplen2 = 0; wait: if (need_recompute) { recompute_resend: @@ -1181,9 +1195,7 @@ send_dg(res_state statp, if (evCmpTime(finish, now) <= 0) { poll_err_out: Perror(statp, stderr, "poll", errno); - err_out: - __res_iclose(statp, false); - return (0); + return close_and_return_error (statp, resplen2); } evSubTime(&timeout, &finish, &now); need_recompute = 0; @@ -1230,7 +1242,9 @@ send_dg(res_state statp, } *gotsomewhere = 1; - return (0); + if (resplen2 != NULL) + *resplen2 = 0; + return 0; } if (n < 0) { if (errno == EINTR) @@ -1298,7 +1312,7 @@ send_dg(res_state statp, fail_sendmmsg: Perror(statp, stderr, "sendmmsg", errno); - goto err_out; + return close_and_return_error (statp, resplen2); } } else @@ -1316,7 +1330,7 @@ send_dg(res_state statp, if (errno == EINTR || errno == EAGAIN) goto recompute_resend; Perror(statp, stderr, "send", errno); - goto err_out; + return close_and_return_error (statp, resplen2); } just_one: if (nwritten != 0 || buf2 == NULL || single_request) @@ -1394,7 +1408,7 @@ send_dg(res_state statp, goto wait; } Perror(statp, stderr, "recvfrom", errno); - goto err_out; + return close_and_return_error (statp, resplen2); } *gotsomewhere = 1; if (__builtin_expect (*thisresplenp < HFIXEDSZ, 0)) { @@ -1405,7 +1419,7 @@ send_dg(res_state statp, (stdout, ";; undersized: %d\n", *thisresplenp)); *terrno = EMSGSIZE; - goto err_out; + return close_and_return_error (statp, resplen2); } if ((recvresp1 || hp->id != anhp->id) && (recvresp2 || hp2->id != anhp->id)) { @@ -1454,7 +1468,7 @@ send_dg(res_state statp, ? *thisanssizp : *thisresplenp); /* record the error */ statp->_flags |= RES_F_EDNS0ERR; - goto err_out; + return close_and_return_error (statp, resplen2); } #endif if (!(statp->options & RES_INSECURE2) @@ -1506,10 +1520,10 @@ send_dg(res_state statp, } next_ns: - __res_iclose(statp, false); /* don't retry if called from dig */ if (!statp->pfcode) - return (0); + return close_and_return_error (statp, resplen2); + __res_iclose(statp, false); } if (anhp->rcode == NOERROR && anhp->ancount == 0 && anhp->aa == 0 && anhp->ra == 0 && anhp->arcount == 0) { @@ -1531,6 +1545,8 @@ send_dg(res_state statp, __res_iclose(statp, false); // XXX if we have received one reply we could // XXX use it and not repeat it over TCP... + if (resplen2 != NULL) + *resplen2 = 0; return (1); } /* Mark which reply we received. */ @@ -1546,21 +1562,22 @@ send_dg(res_state statp, __res_iclose (statp, false); retval = reopen (statp, terrno, ns); if (retval <= 0) - return retval; + { + if (resplen2 != NULL) + *resplen2 = 0; + return retval; + } pfd[0].fd = EXT(statp).nssocks[ns]; } } goto wait; } - /* - * All is well, or the error is fatal. Signal that the - * next nameserver ought not be tried. - */ + /* All is well. We have received both responses (if + two responses were requested). */ return (resplen); - } else if (pfd[0].revents & (POLLERR | POLLHUP | POLLNVAL)) { - /* Something went wrong. We can stop trying. */ - goto err_out; - } + } else if (pfd[0].revents & (POLLERR | POLLHUP | POLLNVAL)) + /* Something went wrong. We can stop trying. */ + return close_and_return_error (statp, resplen2); else { /* poll should not have returned > 0 in this case. */ abort (); -- cgit 1.4.1 From ce92632d1297d032e5781cfa077e300f5c167471 Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Mon, 23 May 2016 20:18:34 +0200 Subject: CVE-2016-4429: sunrpc: Do not use alloca in clntudp_call [BZ #20112] The call is technically in a loop, and under certain circumstances (which are quite difficult to reproduce in a test case), alloca can be invoked repeatedly during a single call to clntudp_call. As a result, the available stack space can be exhausted (even though individual alloca sizes are bounded implicitly by what can fit into a UDP packet, as a side effect of the earlier successful send operation). (cherry picked from commit bc779a1a5b3035133024b21e2f339fe4219fb11c) --- ChangeLog | 7 +++++++ NEWS | 6 +++++- sunrpc/clnt_udp.c | 10 +++++++++- 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index f9a9e335e6..fb2d7ff399 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2016-05-23 Florian Weimer + + CVE-2016-4429 + [BZ #20112] + * sunrpc/clnt_udp.c (clntudp_call): Use malloc/free for the error + payload. + 2016-03-25 Florian Weimer [BZ #19791] diff --git a/NEWS b/NEWS index d14f9edc6f..937c618a0b 100644 --- a/NEWS +++ b/NEWS @@ -13,7 +13,7 @@ Version 2.19.1 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, 18032, 18080, 18240, 18287, 18508, 18665, 18905, 19779, 19791, 19879, - 20010. + 20010, 20112. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a @@ -92,6 +92,10 @@ Version 2.19.1 the overflow. Thanks to the Google Security Team and Red Hat for reporting the security impact of this issue, and Robert Holiday of Ciena for reporting the related bug 18665. (CVE-2015-7547) + +* The Sun RPC UDP client could exhaust all available stack space when + flooded with crafted ICMP and UDP messages. Reported by Aldy Hernandez' + alloca plugin for GCC. (CVE-2016-4429) Version 2.19 diff --git a/sunrpc/clnt_udp.c b/sunrpc/clnt_udp.c index 1b6a20b826..81d5637cd7 100644 --- a/sunrpc/clnt_udp.c +++ b/sunrpc/clnt_udp.c @@ -420,9 +420,15 @@ send_again: struct sock_extended_err *e; struct sockaddr_in err_addr; struct iovec iov; - char *cbuf = (char *) alloca (outlen + 256); + char *cbuf = malloc (outlen + 256); int ret; + if (cbuf == NULL) + { + cu->cu_error.re_errno = errno; + return (cu->cu_error.re_status = RPC_CANTRECV); + } + iov.iov_base = cbuf + 256; iov.iov_len = outlen; msg.msg_name = (void *) &err_addr; @@ -447,10 +453,12 @@ send_again: cmsg = CMSG_NXTHDR (&msg, cmsg)) if (cmsg->cmsg_level == SOL_IP && cmsg->cmsg_type == IP_RECVERR) { + free (cbuf); e = (struct sock_extended_err *) CMSG_DATA(cmsg); cu->cu_error.re_errno = e->ee_errno; return (cu->cu_error.re_status = RPC_CANTRECV); } + free (cbuf); } #endif do -- cgit 1.4.1 From 1a43fd3240c587b403240cf316d241f91ce50d8f Mon Sep 17 00:00:00 2001 From: Leonhard Holz Date: Tue, 13 Jan 2015 11:33:56 +0530 Subject: Fix memory handling in strxfrm_l [BZ #16009] [Modified from the original email by Siddhesh Poyarekar] This patch solves bug #16009 by implementing an additional path in strxfrm that does not depend on caching the weight and rule indices. In detail the following changed: * The old main loop was factored out of strxfrm_l into the function do_xfrm_cached to be able to alternativly use the non-caching version do_xfrm. * strxfrm_l allocates a a fixed size array on the stack. If this is not sufficiant to store the weight and rule indices, the non-caching path is taken. As the cache size is not dependent on the input there can be no problems with integer overflows or stack allocations greater than __MAX_ALLOCA_CUTOFF. Note that malloc-ing is not possible because the definition of strxfrm does not allow an oom errorhandling. * The uncached path determines the weight and rule index for every char and for every pass again. * Passing all the locale data array by array resulted in very long parameter lists, so I introduced a structure that holds them. * Checking for zero src string has been moved a bit upwards, it is before the locale data initialization now. * To verify that the non-caching path works correct I added a test run to localedata/sort-test.sh & localedata/xfrm-test.c where all strings are patched up with spaces so that they are too large for the caching path. (cherry picked from commit 0f9e585480edcdf1e30dc3d79e24b84aeee516fa) Conflicts: NEWS string/strxfrm_l.c --- ChangeLog | 16 ++ NEWS | 10 +- localedata/sort-test.sh | 6 + localedata/xfrm-test.c | 52 ++++- string/strxfrm_l.c | 499 ++++++++++++++++++++++++++++++++++++++---------- 5 files changed, 476 insertions(+), 107 deletions(-) diff --git a/ChangeLog b/ChangeLog index fb2d7ff399..53e173f5f0 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,19 @@ +2016-07-11 Leonhard Holz + + [BZ #16009] + * string/strxfrm_l.c (STRXFRM): Allocate fixed size cache for + weights and rules. Use do_xfrm_cached if data fits in cache, + do_xfrm otherwise. Moved former main loop to... + * (do_xfrm_cached): New function. + * (do_xfrm): Non-caching version of do_xfrm_cached. Uses + find_idx, find_position and stack_push. + * (find_idx): New function. + * (find_position): Likewise. + * localedata/sort-test.sh: Added test run for do_xfrm. + * localedata/xfrm-test.c (main): Added command line option + -nocache to run the test with strings that are too large for + the STRXFRM cache. + 2016-05-23 Florian Weimer CVE-2016-4429 diff --git a/NEWS b/NEWS index 937c618a0b..2a8f8f4788 100644 --- a/NEWS +++ b/NEWS @@ -9,11 +9,11 @@ Version 2.19.1 * The following bugs are resolved with this release: - 15946, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, - 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, - 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, 18007, - 18032, 18080, 18240, 18287, 18508, 18665, 18905, 19779, 19791, 19879, - 20010, 20112. + 15946, 16009, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, + 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, + 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, + 18007, 18032, 18080, 18240, 18287, 18508, 18665, 18905, 19779, 19791, + 19879, 20010, 20112. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a diff --git a/localedata/sort-test.sh b/localedata/sort-test.sh index 8a7ca89688..b01a78ea14 100644 --- a/localedata/sort-test.sh +++ b/localedata/sort-test.sh @@ -49,11 +49,17 @@ for l in $lang; do ${common_objpfx}localedata/xfrm-test $id < $cns.in \ > ${common_objpfx}localedata/$cns.xout || here=1 cmp -s $cns.in ${common_objpfx}localedata/$cns.xout || here=1 + LOCPATH=${common_objpfx}localedata GCONV_PATH=${common_objpfx}/iconvdata \ + LC_ALL=$l ${test_program_prefix} \ + ${common_objpfx}localedata/xfrm-test $id -nocache < $cns.in \ + > ${common_objpfx}localedata/$cns.nocache.xout || here=1 + cmp -s $cns.in ${common_objpfx}localedata/$cns.nocache.xout || here=1 if test $here -eq 0; then echo "$l xfrm-test OK" else echo "$l xfrm-test FAIL" diff -u $cns.in ${common_objpfx}localedata/$cns.xout | sed 's/^/ /' + diff -u $cns.in ${common_objpfx}localedata/$cns.nocache.xout | sed 's/^/ /' status=1 fi done diff --git a/localedata/xfrm-test.c b/localedata/xfrm-test.c index d2aba7d26e..5cf29f60c9 100644 --- a/localedata/xfrm-test.c +++ b/localedata/xfrm-test.c @@ -23,7 +23,10 @@ #include #include #include +#include +/* Keep in sync with string/strxfrm_l.c. */ +#define SMALL_STR_SIZE 4095 struct lines { @@ -37,6 +40,7 @@ int main (int argc, char *argv[]) { int result = 0; + bool nocache = false; size_t nstrings, nstrings_max; struct lines *strings; char *line = NULL; @@ -44,7 +48,18 @@ main (int argc, char *argv[]) size_t n; if (argc < 2) - error (1, 0, "usage: %s ", argv[0]); + error (1, 0, "usage: %s [-nocache]", argv[0]); + + if (argc == 3) + { + if (strcmp (argv[2], "-nocache") == 0) + nocache = true; + else + { + printf ("Unknown option %s!\n", argv[2]); + exit (1); + } + } setlocale (LC_ALL, ""); @@ -59,9 +74,9 @@ main (int argc, char *argv[]) while (1) { - char saved, *newp; - int needed; - int l; + char saved, *word, *newp; + size_t l, line_len, needed; + if (getline (&line, &len, stdin) < 0) break; @@ -83,10 +98,35 @@ main (int argc, char *argv[]) saved = line[l]; line[l] = '\0'; - needed = strxfrm (NULL, line, 0); + + if (nocache) + { + line_len = strlen (line); + word = malloc (line_len + SMALL_STR_SIZE + 1); + if (word == NULL) + { + printf ("malloc failed: %m\n"); + exit (1); + } + memset (word, ' ', SMALL_STR_SIZE); + memcpy (word + SMALL_STR_SIZE, line, line_len); + word[line_len + SMALL_STR_SIZE] = '\0'; + } + else + word = line; + + needed = strxfrm (NULL, word, 0); newp = malloc (needed + 1); - strxfrm (newp, line, needed + 1); + if (newp == NULL) + { + printf ("malloc failed: %m\n"); + exit (1); + } + strxfrm (newp, word, needed + 1); strings[nstrings].xfrm = newp; + + if (nocache) + free (word); line[l] = saved; ++nstrings; } diff --git a/string/strxfrm_l.c b/string/strxfrm_l.c index 04b9338f05..e496550f92 100644 --- a/string/strxfrm_l.c +++ b/string/strxfrm_l.c @@ -40,8 +40,23 @@ #define CONCAT(a,b) CONCAT1(a,b) #define CONCAT1(a,b) a##b +/* Maximum string size that is calculated with cached indices. Right now this + is an arbitrary value open to optimizations. SMALL_STR_SIZE * 4 has to be + lower than __MAX_ALLOCA_CUTOFF. Keep localedata/xfrm-test.c in sync. */ +#define SMALL_STR_SIZE 4095 + #include "../locale/localeinfo.h" +/* Group locale data for shorter parameter lists. */ +typedef struct +{ + uint_fast32_t nrules; + unsigned char *rulesets; + USTRING_TYPE *weights; + int32_t *table; + USTRING_TYPE *extra; + int32_t *indirect; +} locale_data_t; #ifndef WIDE_CHAR_VERSION @@ -80,115 +95,330 @@ utf8_encode (char *buf, int val) } #endif - -size_t -STRXFRM (STRING_TYPE *dest, const STRING_TYPE *src, size_t n, __locale_t l) +/* Find next weight and rule index. Inlined since called for every char. */ +static __always_inline size_t +find_idx (const USTRING_TYPE **us, int32_t *weight_idx, + unsigned char *rule_idx, const locale_data_t *l_data, const int pass) { - struct __locale_data *current = l->__locales[LC_COLLATE]; - uint_fast32_t nrules = current->values[_NL_ITEM_INDEX (_NL_COLLATE_NRULES)].word; - /* We don't assign the following values right away since it might be - unnecessary in case there are no rules. */ - const unsigned char *rulesets; - const int32_t *table; - const USTRING_TYPE *weights; - const USTRING_TYPE *extra; - const int32_t *indirect; - uint_fast32_t pass; - size_t needed; - size_t last_needed; - const USTRING_TYPE *usrc; - size_t srclen = STRLEN (src); - int32_t *idxarr; - unsigned char *rulearr; - size_t idxmax; - size_t idxcnt; - int use_malloc; + /* Prepare variables required by findidx(). */ + int32_t *table = l_data->table; + int32_t *indirect = l_data->indirect; + USTRING_TYPE *extra = l_data->extra; #include WEIGHT_H + int32_t tmp = findidx (us, -1); + *rule_idx = tmp >> 24; + int32_t idx = tmp & 0xffffff; + size_t len = l_data->weights[idx++]; - if (nrules == 0) + /* Skip over indices of previous levels. */ + for (int i = 0; i < pass; i++) { - if (n != 0) - STPNCPY (dest, src, MIN (srclen + 1, n)); - - return srclen; + idx += len; + len = l_data->weights[idx++]; } - rulesets = (const unsigned char *) - current->values[_NL_ITEM_INDEX (_NL_COLLATE_RULESETS)].string; - table = (const int32_t *) - current->values[_NL_ITEM_INDEX (CONCAT(_NL_COLLATE_TABLE,SUFFIX))].string; - weights = (const USTRING_TYPE *) - current->values[_NL_ITEM_INDEX (CONCAT(_NL_COLLATE_WEIGHT,SUFFIX))].string; - extra = (const USTRING_TYPE *) - current->values[_NL_ITEM_INDEX (CONCAT(_NL_COLLATE_EXTRA,SUFFIX))].string; - indirect = (const int32_t *) - current->values[_NL_ITEM_INDEX (CONCAT(_NL_COLLATE_INDIRECT,SUFFIX))].string; - use_malloc = 0; + *weight_idx = idx; + return len; +} - assert (((uintptr_t) table) % __alignof__ (table[0]) == 0); - assert (((uintptr_t) weights) % __alignof__ (weights[0]) == 0); - assert (((uintptr_t) extra) % __alignof__ (extra[0]) == 0); - assert (((uintptr_t) indirect) % __alignof__ (indirect[0]) == 0); +static int +find_position (const USTRING_TYPE *us, const locale_data_t *l_data, + const int pass) +{ + int32_t weight_idx; + unsigned char rule_idx; + const USTRING_TYPE *usrc = us; - /* Handle an empty string as a special case. */ - if (srclen == 0) - { - if (n != 0) - *dest = L('\0'); - return 0; - } + find_idx (&usrc, &weight_idx, &rule_idx, l_data, pass); + return l_data->rulesets[rule_idx * l_data->nrules + pass] & sort_position; +} - /* We need the elements of the string as unsigned values since they - are used as indeces. */ - usrc = (const USTRING_TYPE *) src; - - /* Perform the first pass over the string and while doing this find - and store the weights for each character. Since we want this to - be as fast as possible we are using `alloca' to store the temporary - values. But since there is no limit on the length of the string - we have to use `malloc' if the string is too long. We should be - very conservative here. */ - if (! __libc_use_alloca ((srclen + 1) * (sizeof (int32_t) + 1))) - { - idxarr = (int32_t *) malloc ((srclen + 1) * (sizeof (int32_t) + 1)); - rulearr = (unsigned char *) &idxarr[srclen]; - - if (idxarr == NULL) - /* No memory. Well, go with the stack then. - - XXX Once this implementation is stable we will handle this - differently. Instead of precomputing the indeces we will - do this in time. This means, though, that this happens for - every pass again. */ - goto try_stack; - use_malloc = 1; - } - else +/* Do the transformation. */ +static size_t +do_xfrm (const USTRING_TYPE *usrc, STRING_TYPE *dest, size_t n, + const locale_data_t *l_data) +{ + int32_t weight_idx; + unsigned char rule_idx; + uint_fast32_t pass; + size_t needed = 0; + size_t last_needed; + + /* Now the passes over the weights. */ + for (pass = 0; pass < l_data->nrules; ++pass) { - try_stack: - idxarr = (int32_t *) alloca (srclen * sizeof (int32_t)); - rulearr = (unsigned char *) alloca (srclen + 1); + size_t backw_len = 0; + last_needed = needed; + const USTRING_TYPE *cur = usrc; + const USTRING_TYPE *backw_start = NULL; + + /* We assume that if a rule has defined `position' in one section + this is true for all of them. */ + int position = find_position (cur, l_data, pass); + + if (position == 0) + { + while (*cur != L('\0')) + { + const USTRING_TYPE *pos = cur; + size_t len = find_idx (&cur, &weight_idx, &rule_idx, l_data, + pass); + int rule = l_data->rulesets[rule_idx * l_data->nrules + pass]; + + if ((rule & sort_forward) != 0) + { + /* Handle the pushed backward sequence. */ + if (backw_start != NULL) + { + for (size_t i = backw_len; i > 0; ) + { + int32_t weight_idx; + unsigned char rule_idx; + size_t len = find_idx (&backw_start, &weight_idx, + &rule_idx, l_data, pass); + if (needed + i < n) + for (size_t j = len; j > 0; j--) + dest[needed + i - j] = + l_data->weights[weight_idx++]; + + i -= len; + } + + needed += backw_len; + backw_start = NULL; + backw_len = 0; + } + + /* Now handle the forward element. */ + if (needed + len < n) + while (len-- > 0) + dest[needed++] = l_data->weights[weight_idx++]; + else + /* No more characters fit into the buffer. */ + needed += len; + } + else + { + /* Remember start of the backward sequence & track length. */ + if (backw_start == NULL) + backw_start = pos; + backw_len += len; + } + } + + + /* Handle the pushed backward sequence. */ + if (backw_start != NULL) + { + for (size_t i = backw_len; i > 0; ) + { + size_t len = find_idx (&backw_start, &weight_idx, &rule_idx, + l_data, pass); + if (needed + i < n) + for (size_t j = len; j > 0; j--) + dest[needed + i - j] = + l_data->weights[weight_idx++]; + + i -= len; + } + + needed += backw_len; + } + } + else + { + int val = 1; +#ifndef WIDE_CHAR_VERSION + char buf[7]; + size_t buflen; +#endif + size_t i; + + while (*cur != L('\0')) + { + const USTRING_TYPE *pos = cur; + size_t len = find_idx (&cur, &weight_idx, &rule_idx, l_data, + pass); + int rule = l_data->rulesets[rule_idx * l_data->nrules + pass]; + + if ((rule & sort_forward) != 0) + { + /* Handle the pushed backward sequence. */ + if (backw_start != NULL) + { + for (size_t p = backw_len; p > 0; p--) + { + size_t len; + int32_t weight_idx; + unsigned char rule_idx; + const USTRING_TYPE *backw_cur = backw_start; + + /* To prevent a warning init the used vars. */ + len = find_idx (&backw_cur, &weight_idx, + &rule_idx, l_data, pass); + + for (i = 1; i < p; i++) + len = find_idx (&backw_cur, &weight_idx, + &rule_idx, l_data, pass); + + if (len != 0) + { +#ifdef WIDE_CHAR_VERSION + if (needed + 1 + len < n) + { + dest[needed] = val; + for (i = 0; i < len; ++i) + dest[needed + 1 + i] = + l_data->weights[weight_idx + i]; + } + needed += 1 + len; +#else + buflen = utf8_encode (buf, val); + if (needed + buflen + len < n) + { + for (i = 0; i < buflen; ++i) + dest[needed + i] = buf[i]; + for (i = 0; i < len; ++i) + dest[needed + buflen + i] = + l_data->weights[weight_idx + i]; + } + needed += buflen + len; +#endif + val = 1; + } + else + ++val; + } + + backw_start = NULL; + backw_len = 0; + } + + /* Now handle the forward element. */ + if (len != 0) + { +#ifdef WIDE_CHAR_VERSION + if (needed + 1 + len < n) + { + dest[needed] = val; + for (i = 0; i < len; ++i) + dest[needed + 1 + i] = + l_data->weights[weight_idx + i]; + } + needed += 1 + len; +#else + buflen = utf8_encode (buf, val); + if (needed + buflen + len < n) + { + for (i = 0; i < buflen; ++i) + dest[needed + i] = buf[i]; + for (i = 0; i < len; ++i) + dest[needed + buflen + i] = + l_data->weights[weight_idx + i]; + } + needed += buflen + len; +#endif + val = 1; + } + else + ++val; + } + else + { + /* Remember start of the backward sequence & track length. */ + if (backw_start == NULL) + backw_start = pos; + backw_len++; + } + } + + /* Handle the pushed backward sequence. */ + if (backw_start != NULL) + { + for (size_t p = backw_len; p > 0; p--) + { + size_t len; + int32_t weight_idx; + unsigned char rule_idx; + const USTRING_TYPE *backw_cur = backw_start; + + /* To prevent a warning init the used vars. */ + len = find_idx (&backw_cur, &weight_idx, + &rule_idx, l_data, pass); + + for (i = 1; i < p; i++) + len = find_idx (&backw_cur, &weight_idx, + &rule_idx, l_data, pass); + + if (len != 0) + { +#ifdef WIDE_CHAR_VERSION + if (needed + 1 + len < n) + { + dest[needed] = val; + for (i = 0; i < len; ++i) + dest[needed + 1 + i] = + l_data->weights[weight_idx + i]; + } + needed += 1 + len; +#else + buflen = utf8_encode (buf, val); + if (needed + buflen + len < n) + { + for (i = 0; i < buflen; ++i) + dest[needed + i] = buf[i]; + for (i = 0; i < len; ++i) + dest[needed + buflen + i] = + l_data->weights[weight_idx + i]; + } + needed += buflen + len; +#endif + val = 1; + } + else + ++val; + } + } + } + + /* Finally store the byte to separate the passes or terminate + the string. */ + if (needed < n) + dest[needed] = pass + 1 < l_data->nrules ? L('\1') : L('\0'); + ++needed; } - idxmax = 0; - do + /* This is a little optimization: many collation specifications have + a `position' rule at the end and if no non-ignored character + is found the last \1 byte is immediately followed by a \0 byte + signalling this. We can avoid the \1 byte(s). */ + if (needed > 2 && needed == last_needed + 1) { - int32_t tmp = findidx (&usrc, -1); - rulearr[idxmax] = tmp >> 24; - idxarr[idxmax] = tmp & 0xffffff; - - ++idxmax; + /* Remove the \1 byte. */ + if (--needed <= n) + dest[needed - 1] = L('\0'); } - while (*usrc != L('\0')); - /* This element is only read, the value never used but to determine - another value which then is ignored. */ - rulearr[idxmax] = '\0'; + /* Return the number of bytes/words we need, but don't count the NUL + byte/word at the end. */ + return needed - 1; +} + +/* Do the transformation using weight-index and rule cache. */ +static size_t +do_xfrm_cached (STRING_TYPE *dest, size_t n, const locale_data_t *l_data, + size_t idxmax, int32_t *idxarr, const unsigned char *rulearr) +{ + uint_fast32_t nrules = l_data->nrules; + unsigned char *rulesets = l_data->rulesets; + USTRING_TYPE *weights = l_data->weights; + uint_fast32_t pass; + size_t needed = 0; + size_t last_needed; + size_t idxcnt; - /* Now the passes over the weights. We now use the indeces we found - before. */ - needed = 0; + /* Now the passes over the weights. */ for (pass = 0; pass < nrules; ++pass) { size_t backw_stop = ~0ul; @@ -434,14 +664,91 @@ STRXFRM (STRING_TYPE *dest, const STRING_TYPE *src, size_t n, __locale_t l) dest[needed - 1] = L('\0'); } - /* Free the memory if needed. */ - if (use_malloc) - free (idxarr); - /* Return the number of bytes/words we need, but don't count the NUL byte/word at the end. */ return needed - 1; } + +size_t +STRXFRM (STRING_TYPE *dest, const STRING_TYPE *src, size_t n, __locale_t l) +{ + locale_data_t l_data; + struct __locale_data *current = l->__locales[LC_COLLATE]; + l_data.nrules = current->values[_NL_ITEM_INDEX (_NL_COLLATE_NRULES)].word; + + /* Handle byte comparison case. */ + if (l_data.nrules == 0) + { + size_t srclen = STRLEN (src); + + if (n != 0) + STPNCPY (dest, src, MIN (srclen + 1, n)); + + return srclen; + } + + /* Handle an empty string, code hereafter relies on strlen (src) > 0. */ + if (*src == L('\0')) + { + if (n != 0) + *dest = L('\0'); + return 0; + } + + /* Get the locale data. */ + l_data.rulesets = (unsigned char *) + current->values[_NL_ITEM_INDEX (_NL_COLLATE_RULESETS)].string; + l_data.table = (int32_t *) + current->values[_NL_ITEM_INDEX (CONCAT(_NL_COLLATE_TABLE,SUFFIX))].string; + l_data.weights = (USTRING_TYPE *) + current->values[_NL_ITEM_INDEX (CONCAT(_NL_COLLATE_WEIGHT,SUFFIX))].string; + l_data.extra = (USTRING_TYPE *) + current->values[_NL_ITEM_INDEX (CONCAT(_NL_COLLATE_EXTRA,SUFFIX))].string; + l_data.indirect = (int32_t *) + current->values[_NL_ITEM_INDEX (CONCAT(_NL_COLLATE_INDIRECT,SUFFIX))].string; + + assert (((uintptr_t) l_data.table) % __alignof__ (l_data.table[0]) == 0); + assert (((uintptr_t) l_data.weights) % __alignof__ (l_data.weights[0]) == 0); + assert (((uintptr_t) l_data.extra) % __alignof__ (l_data.extra[0]) == 0); + assert (((uintptr_t) l_data.indirect) % __alignof__ (l_data.indirect[0]) == 0); + + /* We need the elements of the string as unsigned values since they + are used as indeces. */ + const USTRING_TYPE *usrc = (const USTRING_TYPE *) src; + + /* Allocate cache for small strings on the stack and fill it with weight and + rule indices. If the cache size is not sufficient, continue with the + uncached xfrm version. */ + size_t idxmax = 0; + const USTRING_TYPE *cur = usrc; + int32_t *idxarr = alloca (SMALL_STR_SIZE * sizeof (int32_t)); + unsigned char *rulearr = alloca (SMALL_STR_SIZE + 1); + /* Prepare variables required by findidx(). */ + int32_t *table = l_data.table; + int32_t *indirect = l_data.indirect; + USTRING_TYPE *extra = l_data.extra; +#include WEIGHT_H + + do + { + int32_t tmp = findidx (&cur, -1); + rulearr[idxmax] = tmp >> 24; + idxarr[idxmax] = tmp & 0xffffff; + + ++idxmax; + } + while (*cur != L('\0') && idxmax < SMALL_STR_SIZE); + + /* This element is only read, the value never used but to determine + another value which then is ignored. */ + rulearr[idxmax] = '\0'; + + /* Do the transformation. */ + if (*cur == L('\0')) + return do_xfrm_cached (dest, n, &l_data, idxmax, idxarr, rulearr); + else + return do_xfrm (usrc, dest, n, &l_data); +} libc_hidden_def (STRXFRM) #ifndef WIDE_CHAR_VERSION -- cgit 1.4.1 From dea992adae5ff1194d7e49b698424eba741df62a Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Thu, 15 Oct 2015 09:23:07 +0200 Subject: Always enable pointer guard [BZ #18928] Honoring the LD_POINTER_GUARD environment variable in AT_SECURE mode has security implications. This commit enables pointer guard unconditionally, and the environment variable is now ignored. [BZ #18928] * sysdeps/generic/ldsodefs.h (struct rtld_global_ro): Remove _dl_pointer_guard member. * elf/rtld.c (_rtld_global_ro): Remove _dl_pointer_guard initializer. (security_init): Always set up pointer guard. (process_envvars): Do not process LD_POINTER_GUARD. (cherry picked from commit a014cecd82b71b70a6a843e250e06b541ad524f7) Conflicts: NEWS --- ChangeLog | 10 ++++++++++ NEWS | 4 ++-- elf/rtld.c | 15 ++++----------- sysdeps/generic/ldsodefs.h | 3 --- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/ChangeLog b/ChangeLog index 53e173f5f0..f0bd736694 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2016-07-11 Florian Weimer + + [BZ #18928] + * sysdeps/generic/ldsodefs.h (struct rtld_global_ro): Remove + _dl_pointer_guard member. + * elf/rtld.c (_rtld_global_ro): Remove _dl_pointer_guard + initializer. + (security_init): Always set up pointer guard. + (process_envvars): Do not process LD_POINTER_GUARD. + 2016-07-11 Leonhard Holz [BZ #16009] diff --git a/NEWS b/NEWS index 2a8f8f4788..9bd31e4ab4 100644 --- a/NEWS +++ b/NEWS @@ -12,8 +12,8 @@ Version 2.19.1 15946, 16009, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, - 18007, 18032, 18080, 18240, 18287, 18508, 18665, 18905, 19779, 19791, - 19879, 20010, 20112. + 18007, 18032, 18080, 18240, 18287, 18508, 18665, 18905, 18928, 19779, + 19791, 19879, 20010, 20112. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a diff --git a/elf/rtld.c b/elf/rtld.c index 6dcbabc284..375c47d72c 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -162,7 +162,6 @@ struct rtld_global_ro _rtld_global_ro attribute_relro = ._dl_hwcap_mask = HWCAP_IMPORTANT, ._dl_lazy = 1, ._dl_fpu_control = _FPU_DEFAULT, - ._dl_pointer_guard = 1, ._dl_pagesize = EXEC_PAGESIZE, ._dl_inhibit_cache = 0, @@ -857,15 +856,12 @@ security_init (void) #endif /* Set up the pointer guard as well, if necessary. */ - if (GLRO(dl_pointer_guard)) - { - uintptr_t pointer_chk_guard = _dl_setup_pointer_guard (_dl_random, - stack_chk_guard); + uintptr_t pointer_chk_guard + = _dl_setup_pointer_guard (_dl_random, stack_chk_guard); #ifdef THREAD_SET_POINTER_GUARD - THREAD_SET_POINTER_GUARD (pointer_chk_guard); + THREAD_SET_POINTER_GUARD (pointer_chk_guard); #endif - __pointer_chk_guard_local = pointer_chk_guard; - } + __pointer_chk_guard_local = pointer_chk_guard; /* We do not need the _dl_random value anymore. The less information we leave behind, the better, so clear the @@ -2605,9 +2601,6 @@ process_envvars (enum mode *modep) GLRO(dl_use_load_bias) = envline[14] == '1' ? -1 : 0; break; } - - if (memcmp (envline, "POINTER_GUARD", 13) == 0) - GLRO(dl_pointer_guard) = envline[14] != '0'; break; case 14: diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h index ffeb093887..9d767b64d2 100644 --- a/sysdeps/generic/ldsodefs.h +++ b/sysdeps/generic/ldsodefs.h @@ -590,9 +590,6 @@ struct rtld_global_ro /* List of auditing interfaces. */ struct audit_ifaces *_dl_audit; unsigned int _dl_naudit; - - /* 0 if internal pointer values should not be guarded, 1 if they should. */ - EXTERN int _dl_pointer_guard; }; # define __rtld_global_attribute__ # ifdef IS_IN_rtld -- cgit 1.4.1 From 66986dec455c2011085a04b72a5bd55d9f9c7d1c Mon Sep 17 00:00:00 2001 From: Florian Weimer Date: Tue, 6 Oct 2015 13:12:36 +0200 Subject: Harden tls_dtor_list with pointer mangling [BZ #19018] (cherry picked from commit f586e1328681b400078c995a0bb6ad301ef73549) Conflicts: NEWS stdlib/cxa_thread_atexit_impl.c --- ChangeLog | 7 +++++++ NEWS | 4 ++-- stdlib/cxa_thread_atexit_impl.c | 12 ++++++++++-- 3 files changed, 19 insertions(+), 4 deletions(-) diff --git a/ChangeLog b/ChangeLog index f0bd736694..5d3bc8f7d2 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +2016-07-11 Florian Weimer + + [BZ #19018] + * stdlib/cxa_thread_atexit_impl.c (__cxa_thread_atexit_impl): + Mangle function pointer before storing it. + (__call_tls_dtors): Demangle function pointer before calling it. + 2016-07-11 Florian Weimer [BZ #18928] diff --git a/NEWS b/NEWS index 9bd31e4ab4..41481cd33e 100644 --- a/NEWS +++ b/NEWS @@ -12,8 +12,8 @@ Version 2.19.1 15946, 16009, 16545, 16574, 16623, 16657, 16695, 16743, 16758, 16759, 16760, 16878, 16882, 16885, 16916, 16932, 16943, 16958, 17048, 17062, 17069, 17079, 17137, 17153, 17213, 17263, 17269, 17325, 17555, 17905, - 18007, 18032, 18080, 18240, 18287, 18508, 18665, 18905, 18928, 19779, - 19791, 19879, 20010, 20112. + 18007, 18032, 18080, 18240, 18287, 18508, 18665, 18905, 18928, 19018, + 19779, 19791, 19879, 20010, 20112. * A buffer overflow in gethostbyname_r and related functions performing DNS requests has been fixed. If the NSS functions were called with a diff --git a/stdlib/cxa_thread_atexit_impl.c b/stdlib/cxa_thread_atexit_impl.c index d2f88d3ed8..6030e5fc6f 100644 --- a/stdlib/cxa_thread_atexit_impl.c +++ b/stdlib/cxa_thread_atexit_impl.c @@ -42,6 +42,10 @@ static __thread struct link_map *lm_cache; int __cxa_thread_atexit_impl (dtor_func func, void *obj, void *dso_symbol) { +#ifdef PTR_MANGLE + PTR_MANGLE (func); +#endif + /* Prepend. */ struct dtor_list *new = calloc (1, sizeof (struct dtor_list)); new->func = func; @@ -83,9 +87,13 @@ __call_tls_dtors (void) while (tls_dtor_list) { struct dtor_list *cur = tls_dtor_list; - tls_dtor_list = tls_dtor_list->next; + dtor_func func = cur->func; +#ifdef PTR_DEMANGLE + PTR_DEMANGLE (func); +#endif - cur->func (cur->obj); + tls_dtor_list = tls_dtor_list->next; + func (cur->obj); __rtld_lock_lock_recursive (GL(dl_load_lock)); -- cgit 1.4.1