[PATCH] Buffer over-run fix for getusershell(3)

[PATCH] Buffer over-run fix for getusershell(3)

[David Stacey]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

This is the first patch resulting from the Coverity Scan analysis of the 
Cygwin source code. The patch fixes Coverity ID 59932. Note that we 
don't have that many bugs in the Cygwin source code - that's just an ID 
that Coverity assigned to this issue. The patch is only a single line, 
so it falls into our definition of 'trivial'.

getusershell(3) returns the next line from the '/etc/shells' file [1]. 
This contains a path to an executable, so it makes sense for 'buf' to 
contain PATH_MAX characters.

Now, the definition of PATH_MAX is the maximum length of the path, 
including the null terminator [2]. So the for() loop should copy 
PATH_MAX-1 characters, in order to allow for the null terminator.

However, by copying PATH_MAX characters, there is a possible buffer 
over-run when the null terminator is applied. The patch (attached) 
corrects this.

Change Log:
2014-05-18  David Stacey  <drstacey@tiscali.co.uk>

         * winsup/cygwin/syscalls.cc(getusershell) :
         Fixed theoretical buffer overrun of 'buf' that would occur if
         /etc/shells contained a line longer than 4095 characters.

Cheers,

Dave.

[1] http://linux.die.net/man/3/getusershell
[2] http://pubs.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html


[-- Attachment #2: getusershell_buffer_overrun.patch --]
[-- Type: text/plain, Size: 568 bytes --]

--- cygwin-orig-src/winsup/cygwin/syscalls.cc	2014-05-14 12:29:12.000000000 +0100
+++ cygwin-src/winsup/cygwin/syscalls.cc	2014-05-18 19:43:18.355953300 +0100
@@ -4179,7 +4179,7 @@
   /* Get each non-whitespace character as part of the shell path as long as
      it fits in buf. */
   for (buf_idx = 0;
-       ch != EOF && !isspace (ch) && buf_idx < PATH_MAX;
+       ch != EOF && !isspace (ch) && buf_idx < (PATH_MAX - 1);
        buf_idx++, ch = getc (shell_fp))
     buf[buf_idx] = ch;
   /* Skip any trailing non-whitespace character not fitting in buf.  If the

Re: [PATCH] Buffer over-run fix for getusershell(3)

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1555 bytes --]

On May 18 20:12, David Stacey wrote:
> This is the first patch resulting from the Coverity Scan analysis of the
> Cygwin source code. The patch fixes Coverity ID 59932. Note that we don't
> have that many bugs in the Cygwin source code - that's just an ID that
> Coverity assigned to this issue. The patch is only a single line, so it
> falls into our definition of 'trivial'.
> 
> getusershell(3) returns the next line from the '/etc/shells' file [1]. This
> contains a path to an executable, so it makes sense for 'buf' to contain
> PATH_MAX characters.
> 
> Now, the definition of PATH_MAX is the maximum length of the path, including
> the null terminator [2]. So the for() loop should copy PATH_MAX-1
> characters, in order to allow for the null terminator.
> 
> However, by copying PATH_MAX characters, there is a possible buffer over-run
> when the null terminator is applied. The patch (attached) corrects this.
> 
> Change Log:
> 2014-05-18  David Stacey  <...>
> 
>         * winsup/cygwin/syscalls.cc(getusershell) :
>         Fixed theoretical buffer overrun of 'buf' that would occur if
>         /etc/shells contained a line longer than 4095 characters.

Thanks, patch applied.  Just your ChangeLog needs a bit of work.  The
Cygwin dir has its own ChangeLog file so the path should be relative to
that:

	* syscalls.cc (getusershell): ...


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]