diff mbox

gshadow: Handle the parser's full buffer error code

Message ID 87oa6qyux5.fsf@gmail.com
State New
Headers show

Commit Message

David Michael June 25, 2016, 12:27 a.m. UTC
* 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(-)

Comments

David Michael July 8, 2016, 12:22 a.m. UTC | #1
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
Florian Weimer July 8, 2016, 11:22 a.m. UTC | #2
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
David Michael July 8, 2016, 2:51 p.m. UTC | #3
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
diff mbox

Patch

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;
 }