[PATCH] nisplus: Correct pwent parsing issue and resulting compilation error

[PATCH] nisplus: Correct pwent parsing issue and resulting compilation error

[Maciej W. Rozycki]

Copy and null-terminate NIS+ password file UID and GID entries whose 
length is non-zero rather than zero, fixing a bug and a compilation 
issue causing an error with GCC 8:

nss_nisplus/nisplus-parser.c: In function '_nss_nisplus_parse_pwent':
nss_nisplus/nisplus-parser.c:90:7: error: 'strncpy' destination unchanged after copying no bytes [-Werror=stringop-truncation]
       strncpy (first_unused, numstr, len);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
nss_nisplus/nisplus-parser.c:106:7: error: 'strncpy' destination unchanged after copying no bytes [-Werror=stringop-truncation]
       strncpy (first_unused, numstr, len);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

introduced with commit ac05397075f6:

commit ac05397075f621cfdbe1db527c96167a58b6d18e
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Sun Apr 30 07:01:26 2006 +0000

	* nis/nss_nisplus/nisplus-parser.c: Minor optimizations and
	cleanups.  Avoid copying data if it can be used in the old place.

(no mailing list reference available).  Obviously regardless of the 
recently added compiler diagnostics causing a build error this code has 
been long non-functional, so I guess NIS+ servers have been supplying 
strings that have already been null-terminated.  Which in turn made it 
unnecessary to make a null-terminated copy, masking this bug.

	* nis/nss_nisplus/nisplus-parser.c (_nss_nisplus_parse_pwent):
	Copy and null-terminate entries whose length is non-zero rather 
	than zero.
---
Hi,

 No issues seen in `mips-linux-gnu' o32 testing.  OK to apply?

 Also this may have to be backported and security implications evaluated.

  Maciej
---
 nis/nss_nisplus/nisplus-parser.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

glibc-nisplus-parse-pwent-len.diff
Index: glibc/nis/nss_nisplus/nisplus-parser.c
===================================================================
--- glibc.orig/nis/nss_nisplus/nisplus-parser.c	2018-02-14 14:57:00.000000000 +0000
+++ glibc/nis/nss_nisplus/nisplus-parser.c	2018-06-16 21:47:29.071633078 +0100
@@ -82,7 +82,7 @@ _nss_nisplus_parse_pwent (nis_result *re
 
   char *numstr = NISOBJVAL (2, obj);
   len = NISOBJLEN (2, obj);
-  if (len == 0 && numstr[len - 1] != '\0')
+  if (len != 0 && numstr[len - 1] != '\0')
     {
       if (len >= room_left)
 	goto no_more_room;
@@ -98,7 +98,7 @@ _nss_nisplus_parse_pwent (nis_result *re
 
   numstr = NISOBJVAL (3, obj);
   len = NISOBJLEN (3, obj);
-  if (len == 0 && numstr[len - 1] != '\0')
+  if (len != 0 && numstr[len - 1] != '\0')
     {
       if (len >= room_left)
 	goto no_more_room;

Re: [PATCH] nisplus: Correct pwent parsing issue and resulting compilation error

[Joseph Myers]

On Mon, 18 Jun 2018, Maciej W. Rozycki wrote:

> Copy and null-terminate NIS+ password file UID and GID entries whose 
> length is non-zero rather than zero, fixing a bug and a compilation 
> issue causing an error with GCC 8:

Note: (a) the warnings are bug 23266, (b) we now have alternative 
candidate patches, as there's 
<https://sourceware.org/ml/libc-alpha/2018-04/msg00209.html> as well, (c) 
as I said in <https://sourceware.org/ml/libc-alpha/2018-03/msg00532.html> 
it would be a very good idea to add a --enable-obsolete-rpc 
--enable-obsolete-nsl configuration to build-many-glibcs.py so issues 
building such a configuration with new GCC get detected and fixed quicker.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] nisplus: Correct pwent parsing issue and resulting compilation error

[Andreas Schwab]

On Jun 18 2018, "Maciej W. Rozycki" <macro@mips.com> wrote:

> Index: glibc/nis/nss_nisplus/nisplus-parser.c
> ===================================================================
> --- glibc.orig/nis/nss_nisplus/nisplus-parser.c	2018-02-14 14:57:00.000000000 +0000
> +++ glibc/nis/nss_nisplus/nisplus-parser.c	2018-06-16 21:47:29.071633078 +0100
> @@ -82,7 +82,7 @@ _nss_nisplus_parse_pwent (nis_result *re
>  
>    char *numstr = NISOBJVAL (2, obj);
>    len = NISOBJLEN (2, obj);
> -  if (len == 0 && numstr[len - 1] != '\0')
> +  if (len != 0 && numstr[len - 1] != '\0')
>      {
>        if (len >= room_left)
>  	goto no_more_room;
> @@ -98,7 +98,7 @@ _nss_nisplus_parse_pwent (nis_result *re
>  
>    numstr = NISOBJVAL (3, obj);
>    len = NISOBJLEN (3, obj);
> -  if (len == 0 && numstr[len - 1] != '\0')
> +  if (len != 0 && numstr[len - 1] != '\0')
>      {
>        if (len >= room_left)
>  	goto no_more_room;
>

I don't think this is correct.  If len == 0 then numstr[0] is undefined.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] nisplus: Correct pwent parsing issue and resulting compilation error

[Maciej W. Rozycki]

On Mon, 18 Jun 2018, Joseph Myers wrote:

> > Copy and null-terminate NIS+ password file UID and GID entries whose 
> > length is non-zero rather than zero, fixing a bug and a compilation 
> > issue causing an error with GCC 8:
> 
> Note: (a) the warnings are bug 23266, (b) we now have alternative 
> candidate patches, as there's 
> <https://sourceware.org/ml/libc-alpha/2018-04/msg00209.html> as well, (c) 

 Umm, I didn't notice `_nss_nisplus_parse_grent' suffers from the same 
problem and neither GCC bothered here for some reason.

> as I said in <https://sourceware.org/ml/libc-alpha/2018-03/msg00532.html> 
> it would be a very good idea to add a --enable-obsolete-rpc 
> --enable-obsolete-nsl configuration to build-many-glibcs.py so issues 
> building such a configuration with new GCC get detected and fixed quicker.

 Hmm, but why it's been 2+ months and such an obvious fix hasn't been 
reviewed yet?  Insufficient pinging?

  Maciej

Re: [PATCH] nisplus: Correct pwent parsing issue and resulting compilation error

[Joseph Myers]

On Mon, 18 Jun 2018, Maciej W. Rozycki wrote:

> > as I said in <https://sourceware.org/ml/libc-alpha/2018-03/msg00532.html> 
> > it would be a very good idea to add a --enable-obsolete-rpc 
> > --enable-obsolete-nsl configuration to build-many-glibcs.py so issues 
> > building such a configuration with new GCC get detected and fixed quicker.
> 
>  Hmm, but why it's been 2+ months and such an obvious fix hasn't been 
> reviewed yet?  Insufficient pinging?

Only pinged once as far as I can see.  Quite likely most developers aren't 
using those configure options and so aren't affected by the issue.

-- 
Joseph S. Myers
joseph@codesourcery.com

[PATCH v2] nisplus: Correct pwent parsing issue and resulting compilation error [BZ #23266]

[Maciej W. Rozycki]

Copy and null-terminate NIS+ password file UID and GID entries whose 
length is non-zero and are not terminated in addition to empty ones, 
fixing a bug and a compilation issue causing an error with GCC 8:

nss_nisplus/nisplus-parser.c: In function '_nss_nisplus_parse_pwent':
nss_nisplus/nisplus-parser.c:90:7: error: 'strncpy' destination unchanged after copying no bytes [-Werror=stringop-truncation]
       strncpy (first_unused, numstr, len);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
nss_nisplus/nisplus-parser.c:106:7: error: 'strncpy' destination unchanged after copying no bytes [-Werror=stringop-truncation]
       strncpy (first_unused, numstr, len);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

introduced with commit ac05397075f6:

commit ac05397075f621cfdbe1db527c96167a58b6d18e
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Sun Apr 30 07:01:26 2006 +0000

	* nis/nss_nisplus/nisplus-parser.c: Minor optimizations and
	cleanups.  Avoid copying data if it can be used in the old place.

(no mailing list reference available).  Obviously regardless of the 
recently added compiler diagnostics causing a build error this code has 
been long non-functional, so I guess NIS+ servers have been supplying 
strings that are non-empty and have already been null-terminated.  
Which in turn made it unnecessary to make a null-terminated copy, 
masking this bug.

	[BZ #23266]
	* nis/nss_nisplus/nisplus-parser.c (_nss_nisplus_parse_pwent):
	Copy and null-terminate entries that are not terminated in 
	addition to empty ones.
---
On Mon, 18 Jun 2018, Andreas Schwab wrote:

> I don't think this is correct.  If len == 0 then numstr[0] is undefined.

 Right, given that this is legacy code, let's take the path of least 
resistance and copy the approach from `_nss_nisplus_parse_grent', which is 
actually correct and makes sure that numstr[0] == '\0' if len == 0.

 OK for this version?

  Maciej
---
 nis/nss_nisplus/nisplus-parser.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

glibc-nisplus-parse-pwent-len.diff
Index: glibc/nis/nss_nisplus/nisplus-parser.c
===================================================================
--- glibc.orig/nis/nss_nisplus/nisplus-parser.c	2018-06-18 16:54:13.077894266 +0100
+++ glibc/nis/nss_nisplus/nisplus-parser.c	2018-06-18 16:55:04.831413510 +0100
@@ -82,7 +82,7 @@ _nss_nisplus_parse_pwent (nis_result *re
 
   char *numstr = NISOBJVAL (2, obj);
   len = NISOBJLEN (2, obj);
-  if (len == 0 && numstr[len - 1] != '\0')
+  if (len == 0 || numstr[len - 1] != '\0')
     {
       if (len >= room_left)
 	goto no_more_room;
@@ -98,7 +98,7 @@ _nss_nisplus_parse_pwent (nis_result *re
 
   numstr = NISOBJVAL (3, obj);
   len = NISOBJLEN (3, obj);
-  if (len == 0 && numstr[len - 1] != '\0')
+  if (len == 0 || numstr[len - 1] != '\0')
     {
       if (len >= room_left)
 	goto no_more_room;

Re: [PATCH v2] nisplus: Correct pwent parsing issue and resulting compilation error [BZ #23266]

[DJ Delorie]

"Maciej W. Rozycki" <macro@mips.com> writes:
> -  if (len == 0 && numstr[len - 1] != '\0')
> +  if (len == 0 || numstr[len - 1] != '\0')

I think this part of the patch(es) is good, but a second part might be
needed to avoid future problems[*]:

+    if (len > 0)
       strncpy (first_unused, numstr, len);

It looks like that clause handles two cases (if not, ignore the rest of
this email ;)...

1. if we don't have a valid entry, create a zero-length temporary
   entry.

2. If the entry is too long to be nul-terminated (or otherwise isn't
   nul-terminated), create a temporary nul-terminated one.

The strncpy is only needed for the second case.  Since we already know
the length, and are going to nul-terminate it ourselves anyway, a memcpy
would work just as well (but still need the above if-check), unless the
nis server is allowed to return an entry with an embedded nul, in which
case strncpy might prevent data leakage from whatever's after the nul.
I would consider that a different kind of bug, though, and we're calling
strtoul on the result anyway.

However, for the first case, we're always producing a string that causes
the following "if (numstr[0] == '\0')" test to be true, so the only real
purpose of going through all that code for zero-length strings is to
make sure the buffer has room for the trailing nul :-P  (and we need the
test for other cases anyway).

I don't think that's worth changing the code for, though.  Maybe a
comment clarifying why it's the way it is, if we care that much ;-)


[*] GCC's analyzer is getting, if not better, at least more agressive.
    I expect the trend to continue.

Re: [PATCH v2] nisplus: Correct pwent parsing issue and resulting compilation error [BZ #23266]

[Maciej W. Rozycki]

On Mon, 18 Jun 2018, DJ Delorie wrote:

> "Maciej W. Rozycki" <macro@mips.com> writes:
> > -  if (len == 0 && numstr[len - 1] != '\0')
> > +  if (len == 0 || numstr[len - 1] != '\0')
> 
> I think this part of the patch(es) is good, but a second part might be
> needed to avoid future problems[*]:
> 
> +    if (len > 0)
>        strncpy (first_unused, numstr, len);

 Do you expect GCC to start complaining here sometime even if it cannot 
statically prove that `len' can sometimes be zero, which seems the case 
here?

 It would have to always complain then if it found `strncpy' called with 
the the length argument variable.  And I think that while calling 
`strncpy' with constant zero length might be silly (though not invalid!), 
it certainly is fine to call it with a variable length argument that is 
sometimes zero, especially if the likelihood of it being zero (or more 
generally, just the context of the call) does not justify avoiding the 
call in that case.  This also seems to be the case here, being the 
unlikely error path.

> It looks like that clause handles two cases (if not, ignore the rest of
> this email ;)...
> 
> 1. if we don't have a valid entry, create a zero-length temporary
>    entry.
> 
> 2. If the entry is too long to be nul-terminated (or otherwise isn't
>    nul-terminated), create a temporary nul-terminated one.
> 
> The strncpy is only needed for the second case.  Since we already know
> the length, and are going to nul-terminate it ourselves anyway, a memcpy
> would work just as well (but still need the above if-check), unless the
> nis server is allowed to return an entry with an embedded nul, in which
> case strncpy might prevent data leakage from whatever's after the nul.
> I would consider that a different kind of bug, though, and we're calling
> strtoul on the result anyway.
> 
> However, for the first case, we're always producing a string that causes
> the following "if (numstr[0] == '\0')" test to be true, so the only real
> purpose of going through all that code for zero-length strings is to
> make sure the buffer has room for the trailing nul :-P  (and we need the
> test for other cases anyway).

 Yes, and I wouldn't have written it like this if I were to create this 
piece of code from scratch, however...

> I don't think that's worth changing the code for, though.  Maybe a
> comment clarifying why it's the way it is, if we care that much ;-)

... as I noted in the discussion, this is legacy code, so I chose the path 
of least resistance by reusing correct code already present elsewhere 
(that we don't need to touch), because uniformity has a value too.  We 
could optimise it, rewrite for clarity, etc. across all the instances.  
But as you write, is it worth it?

 I'm not sure even if it's worth adding a comment here: if unsure, then 
run `git blame' and examine the relevant commit description.

  Maciej

Re: [PATCH v2] nisplus: Correct pwent parsing issue and resulting compilation error [BZ #23266]

[DJ Delorie]

"Maciej W. Rozycki" <macro@mips.com> writes:
>> +    if (len > 0)
>>        strncpy (first_unused, numstr, len);
>
>  Do you expect GCC to start complaining here sometime even if it cannot 
> statically prove that `len' can sometimes be zero, which seems the case 
> here?

I wouldn't expect gcc to complain here, no, but... some of the things
gcc *does* complain about have surprised me in the past.  So this is
just my paranoia asserting itself :-)

(and function calls used to be expensive, so my old optimizing tricks
are speaking up ;)

> But as you write, is it worth it?

I doubt it.  Just being thorough.

[PING][PATCH v2] nisplus: Correct pwent parsing issue and resulting compilation error [BZ #23266]

[Maciej W. Rozycki]

On Mon, 18 Jun 2018, Maciej W. Rozycki wrote:

>  Right, given that this is legacy code, let's take the path of least 
> resistance and copy the approach from `_nss_nisplus_parse_grent', which is 
> actually correct and makes sure that numstr[0] == '\0' if len == 0.
> 
>  OK for this version?

 Ping for:

<https://patchwork.sourceware.org/patch/27908/>

Previous discussion:

<https://patchwork.sourceware.org/patch/27906/>

  Maciej

Re: [PING][PATCH v2] nisplus: Correct pwent parsing issue and resulting compilation error [BZ #23266]

[DJ Delorie]

"Maciej W. Rozycki" <macro@mips.com> writes:
>>  Right, given that this is legacy code, let's take the path of least 
>> resistance and copy the approach from `_nss_nisplus_parse_grent', which is 
>> actually correct and makes sure that numstr[0] == '\0' if len == 0.
>> 
>>  OK for this version?
>
>  Ping for:
>
> <https://patchwork.sourceware.org/patch/27908/>
>
> Previous discussion:
>
> <https://patchwork.sourceware.org/patch/27906/>

LGTM

[committed v3] nisplus: Correct pwent parsing issue and resulting build error [BZ #23266]

[Maciej W. Rozycki]

Copy and null-terminate NIS+ password file UID and GID entries whose
length is non-zero and are not terminated, in addition to empty ones,
fixing a bug and a compilation issue causing an error with GCC 8:

nss_nisplus/nisplus-parser.c: In function '_nss_nisplus_parse_pwent':
nss_nisplus/nisplus-parser.c:90:7: error: 'strncpy' destination unchanged after copying no bytes [-Werror=stringop-truncation]
       strncpy (first_unused, numstr, len);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
nss_nisplus/nisplus-parser.c:106:7: error: 'strncpy' destination unchanged after copying no bytes [-Werror=stringop-truncation]
       strncpy (first_unused, numstr, len);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

introduced with commit ac05397075f6:

commit ac05397075f621cfdbe1db527c96167a58b6d18e
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Sun Apr 30 07:01:26 2006 +0000

	* nis/nss_nisplus/nisplus-parser.c: Minor optimizations and
	cleanups.  Avoid copying data if it can be used in the old place.

(no mailing list reference available).  Obviously regardless of the
recently added compiler diagnostics causing a build error this code has
been long non-functional, so I guess NIS+ servers have been supplying
strings that are non-empty and have already been null-terminated.
Which in turn made it unnecessary to make a null-terminated copy,
masking this bug.

	[BZ #23266]
	* nis/nss_nisplus/nisplus-parser.c (_nss_nisplus_parse_pwent):
	Copy and null-terminate entries that are not terminated, in
	addition to empty ones.
---
On Mon, 25 Jun 2018, DJ Delorie wrote:

> >  Ping for:
> >
> > <https://patchwork.sourceware.org/patch/27908/>
> >
> > Previous discussion:
> >
> > <https://patchwork.sourceware.org/patch/27906/>
> 
> LGTM

 Thanks, applied now, with two minor updates:

* s/compilation/build/ in the heading so that it fits in 79 columns with 
  GIT's indentation applied,

* a comma added after "are not terminated" in the description and 
  ChangeLog, for clarity,

which I have decided that qualify as trivial and therefore not requiring 
further consensus.

 Here's the actual version committed, for future reference.

  Maciej

---
 ChangeLog                        |    7 +++++++
 nis/nss_nisplus/nisplus-parser.c |    4 ++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/nis/nss_nisplus/nisplus-parser.c b/nis/nss_nisplus/nisplus-parser.c
index 8dc021e..d2b0633 100644
--- a/nis/nss_nisplus/nisplus-parser.c
+++ b/nis/nss_nisplus/nisplus-parser.c
@@ -82,7 +82,7 @@ _nss_nisplus_parse_pwent (nis_result *result, struct passwd *pw,
 
   char *numstr = NISOBJVAL (2, obj);
   len = NISOBJLEN (2, obj);
-  if (len == 0 && numstr[len - 1] != '\0')
+  if (len == 0 || numstr[len - 1] != '\0')
     {
       if (len >= room_left)
 	goto no_more_room;
@@ -98,7 +98,7 @@ _nss_nisplus_parse_pwent (nis_result *result, struct passwd *pw,
 
   numstr = NISOBJVAL (3, obj);
   len = NISOBJLEN (3, obj);
-  if (len == 0 && numstr[len - 1] != '\0')
+  if (len == 0 || numstr[len - 1] != '\0')
     {
       if (len >= room_left)
 	goto no_more_room;
-- 
1.7.2.5

Re: [committed v3] nisplus: Correct pwent parsing issue and resulting build error [BZ #23266]

[Joseph Myers]

As usual when committing a fix for a bug, please mark the bug as RESOLVED 
/ FIXED in Bugzilla with target milestone set to 2.28.

-- 
Joseph S. Myers
joseph@codesourcery.com