[PATCH] gshadow: Handle the parser's full buffer error code

[PATCH] gshadow: Handle the parser's full buffer error code

[David Michael]

    * gshadow/fgetsgent_r.c (__fgetsgent_r): Return ERANGE when the
    parse_line function returns its out-of-space error.
---

Hi,

The fgetgsent function isn't handling errors from parse_line.  That
means it can run out of buffer space when adding pointers to group
members and exit early without setting all members of the static result
struct.  The static result's members will remain pointing at buffer
locations from the previous line, which have been overwritten with
incompatible data, causing segfaults after it is returned normally.

This was detected due to systemd segfaulting.  See:
https://github.com/coreos/bugs/issues/1394

If you don't want to mess with your /etc/gshadow to test it, the
following program will also segfault (tested on Fedora and CoreOS).

Thanks.

David


#include <gshadow.h>
#include <stdio.h>
int main() {
  struct sgrp *entry;
  char **member;
  FILE *input = fdopen (0, "rb");
  while (entry = fgetsgent (input))
    {
      printf ("%s", entry->sg_namp);
      for (member = entry->sg_mem; *member; member++)
        printf(", %s", *member);
      printf ("\n");
    }
  fclose (input);
  return 0;
}

Feed this through stdin.  It should fit in the allocated buffer on
x86_64 and succeed if you delete the last character from the second
line.

grp0:*::root
grp1:*::somebody.a1,somebody.a2,somebody.a3,somebody.a4,somebody.a5,somebody.a6,somebody.a7,somebody.a8,somebody.a9,somebody.a10,somebody.a11,somebody.a12,somebody.a13,somebody.a14,somebody.a15,somebody.a16,somebody.a17,somebody.a18,somebody.a19,somebody.a20,somebody.a21,somebody.a22,somebody.a23,somebody.a24,somebody.a25,somebody.a26,somebody.a27,somebody.a28,somebody.a29,somebody.a30,somebody.a31,somebody.a32,somebody.a33,somebody.a34,somebody.a35,somebody.a36,somebody.a37,somebody.a38,somebody.a39,somebody.a40,somebody.a41,somebody.a42,somebody.a43,somebody.a44,somebody.a45,somebody.a46,somebody.a47,a1234
grp2:*::root

 gshadow/fgetsgent_r.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/gshadow/fgetsgent_r.c b/gshadow/fgetsgent_r.c
index b70f6fa..8c13c55 100644
--- a/gshadow/fgetsgent_r.c
+++ b/gshadow/fgetsgent_r.c
@@ -37,6 +37,7 @@ __fgetsgent_r (FILE *stream, struct sgrp *resbuf, char *buffer, size_t buflen,
 	       struct sgrp **result)
 {
   char *p;
+  int rc;
 
   _IO_flockfile (stream);
   do
@@ -64,11 +65,18 @@ __fgetsgent_r (FILE *stream, struct sgrp *resbuf, char *buffer, size_t buflen,
     } while (*p == '\0' || *p == '#' ||	/* Ignore empty and comment lines.  */
 	     /* Parse the line.  If it is invalid, loop to
 		get the next line of the file to parse.  */
-	     ! parse_line (buffer, (void *) resbuf, (void *) buffer, buflen,
-			   &errno));
+	     !(rc = parse_line (buffer, (void *) resbuf,
+				(void *) buffer, buflen, &errno)));
 
   _IO_funlockfile (stream);
 
+  if (rc < 0)
+    {
+      *result = NULL;
+      __set_errno (ERANGE);
+      return errno;
+    }
+
   *result = resbuf;
   return 0;
 }
-- 
2.7.4

Re: [PATCH] gshadow: Handle the parser's full buffer error code

[David Michael]

On Fri, Jun 24, 2016 at 5:27 PM, David Michael <fedora.dm0@gmail.com> wrote:
>     * gshadow/fgetsgent_r.c (__fgetsgent_r): Return ERANGE when the
>     parse_line function returns its out-of-space error.

Ping.  Has anyone had a chance to look into this segfault?

Thanks.

David

Re: [PATCH] gshadow: Handle the parser's full buffer error code

[Florian Weimer]

On 06/25/2016 02:27 AM, David Michael wrote:

>     * gshadow/fgetsgent_r.c (__fgetsgent_r): Return ERANGE when the
>     parse_line function returns its out-of-space error.

> The fgetgsent function isn't handling errors from parse_line.  That
> means it can run out of buffer space when adding pointers to group
> members and exit early without setting all members of the static result
> struct.  The static result's members will remain pointing at buffer
> locations from the previous line, which have been overwritten with
> incompatible data, causing segfaults after it is returned normally.

This needs a bug in Bugzilla.

Do you have a copyright assignment covering glibc on file with the FSF?

Thanks,
Florian

Re: Re: [PATCH] gshadow: Handle the parser's full buffer error code

[David Michael]

On Fri, Jul 8, 2016 at 5:02 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/25/2016 02:27 AM, David Michael wrote:
>
>>     * gshadow/fgetsgent_r.c (__fgetsgent_r): Return ERANGE when the
>>     parse_line function returns its out-of-space error.
>
>
>> The fgetgsent function isn't handling errors from parse_line.  That
>> means it can run out of buffer space when adding pointers to group
>> members and exit early without setting all members of the static result
>> struct.  The static result's members will remain pointing at buffer
>> locations from the previous line, which have been overwritten with
>> incompatible data, causing segfaults after it is returned normally.
>
>
> This needs a bug in Bugzilla.

I have filed bug #20338.[0]

> Do you have a copyright assignment covering glibc on file with the FSF?

I don't personally, but the copyright holder of this change (if it is
considered legally significant) should be CoreOS, Inc.  I would
imagine they've contributed before, but if not, I can try to find
someone to sign off on it later today.

Thanks.

David

[0] https://sourceware.org/bugzilla/show_bug.cgi?id=20338