about summary refs log tree commit diff
diff options
context:
space:
mode:
authorJoseph Myers <joseph@codesourcery.com>2017-10-07 13:44:49 +0200
committerFlorian Weimer <fweimer@redhat.com>2017-10-07 13:44:49 +0200
commit79c6f51428a9ec977e611e609a8be6aebcb00006 (patch)
tree82831b0f090837567146e60ecf699232cdb3a631
parent7fca94796b67a8ca3730da255e64ee95a818f231 (diff)
downloadglibc-79c6f51428a9ec977e611e609a8be6aebcb00006.tar.gz
glibc-79c6f51428a9ec977e611e609a8be6aebcb00006.tar.xz
glibc-79c6f51428a9ec977e611e609a8be6aebcb00006.zip
Fix nss_nisplus build with mainline GCC (bug 20978).
glibc build with current mainline GCC fails because
nis/nss_nisplus/nisplus-alias.c contains code

  if (name != NULL)
    {
      *errnop = EINVAL;
      return NSS_STATUS_UNAVAIL;
    }

  char buf[strlen (name) + 9 + tablename_len];

producing an error about strlen being called on a pointer that is
always NULL (and a subsequent use of that pointer with a %s format in
snprintf).

As Andreas noted, the bogus conditional comes from a 1997 change:

-  if (name == NULL || strlen(name) > 8)
-    return NSS_STATUS_NOTFOUND;
-  else
+  if (name != NULL || strlen(name) <= 8)

So the intention is clearly to return an error for NULL name.

This patch duly inverts the sense of the conditional.  It fixes the
build with GCC mainline, and passes usual glibc testsuite testing for
x86_64.  However, I have not tried any actual substantive nisplus
testing, do not have an environment for such testing, and do not know
whether it is possible that strlen (name) or tablename_len might be
large so that the VLA for buf is actually a security issue.  However,
if it is a security issue, there are plenty of other similar instances
in the nisplus code (that haven't been hidden by a bogus comparison
with NULL) - and nis_table.c:__create_ib_request uses strdupa on the
string passed to nis_list, so a local fix in the caller wouldn't
suffice anyway (see bug 20987).  (Calls to strdupa and other such
macros that use alloca must be considered equally questionable
regarding stack overflow issues as direct calls to alloca and VLA
declarations.)

	[BZ #20978]
	* nis/nss_nisplus/nisplus-alias.c (_nss_nisplus_getaliasbyname_r):
	Compare name == NULL, not name != NULL.

(cherry picked from commit f88759ea9bd3c8d8fef28f123ba9767cb0e421a3)
-rw-r--r--ChangeLog6
-rw-r--r--NEWS1
-rw-r--r--nis/nss_nisplus/nisplus-alias.c2
3 files changed, 8 insertions, 1 deletions
diff --git a/ChangeLog b/ChangeLog
index e593c33b0c..79e3eb2e53 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2016-12-21  Joseph Myers  <joseph@codesourcery.com>
+
+	[BZ #20978]
+	* nis/nss_nisplus/nisplus-alias.c (_nss_nisplus_getaliasbyname_r):
+	Compare name == NULL, not name != NULL.
+
 2016-11-08  Joseph Myers  <joseph@codesourcery.com>
 
 	[BZ #20790]
diff --git a/NEWS b/NEWS
index e2935c063d..4831542023 100644
--- a/NEWS
+++ b/NEWS
@@ -24,6 +24,7 @@ Security related changes:
 The following bugs are resolved with this release:
 
   [20790] Fix rpcgen buffer overrun
+  [20978] Fix strlen on null pointer in nss_nisplus
   [21209] Ignore and remove LD_HWCAP_MASK for AT_SECURE programs
   [21289] Fix symbol redirect for fts_set
   [21386] Assertion in fork for distinct parent PID is incorrect
diff --git a/nis/nss_nisplus/nisplus-alias.c b/nis/nss_nisplus/nisplus-alias.c
index 7f698b4e6d..cb5acce01d 100644
--- a/nis/nss_nisplus/nisplus-alias.c
+++ b/nis/nss_nisplus/nisplus-alias.c
@@ -291,7 +291,7 @@ _nss_nisplus_getaliasbyname_r (const char *name, struct aliasent *alias,
 	return status;
     }
 
-  if (name != NULL)
+  if (name == NULL)
     {
       *errnop = EINVAL;
       return NSS_STATUS_UNAVAIL;