Libguestfs - Sep 2019 - Re: [PATCH nbdkit] Ban use of stack Variable Length Arrays (VLAs).

I'm not someone who thinks VLAs are automatically bad and unlike Linux
kernel code they can sometimes be used safely in userspace.  However
for an internet exposed server there is an argument that they might
cause some kind of exploitable situation especially if the code is
compiled without other stack hardening features.  Also in highly
multithreaded code with limited stack sizes (as nbdkit is on some
platforms) allowing unbounded stack allocation can be a bad idea
because it can cause a segfault.

So this commit bans them.  Only when using --enable-gcc-warnings, but
upstream developers ought to be using that.

There were in fact only two places where VLAs were being used.  In one
of those places (plugins/sh) removing the VLA actually made the code
better.

For interesting discussion about VLAs in the kernel see:
https://lwn.net/Articles/763253/
---
 configure.ac     | 2 +-
 plugins/sh/sh.c  | 7 +++----
 server/sockets.c | 8 +++++++-
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/configure.ac b/configure.ac
index 5842c6f..d326377 100644
--- a/configure.ac
+++ b/configure.ac
@@ -90,7 +90,7 @@ AC_ARG_ENABLE([gcc-warnings],
       [gcc_warnings=no]
 )
 if test "x$gcc_warnings" = "xyes"; then
-    WARNINGS_CFLAGS="-Wall -Wshadow -Werror"
+    WARNINGS_CFLAGS="-Wall -Wshadow -Wvla -Werror"
     AC_SUBST([WARNINGS_CFLAGS])
 fi
 
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index c73b08b..acb50c4 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -74,8 +74,7 @@ sh_load (void)
 static void
 sh_unload (void)
 {
-  const size_t tmpdir_len = strlen (tmpdir);
-  char cmd[7 + tmpdir_len + 1]; /* "rm -rf " + tmpdir + \0 */
+  CLEANUP_FREE char *cmd = NULL;
 
   /* Run the unload method.  Ignore all errors. */
   if (script) {
@@ -85,8 +84,8 @@ sh_unload (void)
   }
 
   /* Delete the temporary directory.  Ignore all errors. */
-  snprintf (cmd, sizeof cmd, "rm -rf %s", tmpdir);
-  system (cmd);
+  if (asprintf (&cmd, "rm -rf %s", tmpdir) >= 0)
+    system (cmd);
 
   free (script);
 }
diff --git a/server/sockets.c b/server/sockets.c
index 26d65c6..dfaa3ea 100644
--- a/server/sockets.c
+++ b/server/sockets.c
@@ -366,10 +366,16 @@ accept_connection (int listen_sock)
 static void
 check_sockets_and_quit_fd (int *socks, size_t nr_socks)
 {
-  struct pollfd fds[nr_socks + 1];
   size_t i;
   int r;
 
+  CLEANUP_FREE struct pollfd *fds +    malloc (sizeof (struct pollfd) *
(nr_socks+1));
+  if (fds == NULL) {
+    perror ("malloc");
+    exit (EXIT_FAILURE);
+  }
+
   for (i = 0; i < nr_socks; ++i) {
     fds[i].fd = socks[i];
     fds[i].events = POLLIN;
-- 
2.23.0

On 9/5/19 6:28 AM, Richard W.M. Jones wrote:
> I'm not someone who thinks VLAs are automatically bad and unlike Linux
> kernel code they can sometimes be used safely in userspace.  However
> for an internet exposed server there is an argument that they might
> cause some kind of exploitable situation especially if the code is
> compiled without other stack hardening features.  Also in highly
> multithreaded code with limited stack sizes (as nbdkit is on some
> platforms) allowing unbounded stack allocation can be a bad idea
> because it can cause a segfault.
> 
> So this commit bans them.  Only when using --enable-gcc-warnings, but
> upstream developers ought to be using that.
> 
> There were in fact only two places where VLAs were being used.  In one
> of those places (plugins/sh) removing the VLA actually made the code
> better.
> 
> For interesting discussion about VLAs in the kernel see:
> https://lwn.net/Articles/763253/
> ---
>  configure.ac     | 2 +-
>  plugins/sh/sh.c  | 7 +++----
>  server/sockets.c | 8 +++++++-
>  3 files changed, 11 insertions(+), 6 deletions(-)
> +++ b/configure.ac
> @@ -90,7 +90,7 @@ AC_ARG_ENABLE([gcc-warnings],
>        [gcc_warnings=no]
>  )
>  if test "x$gcc_warnings" = "xyes"; then
> -    WARNINGS_CFLAGS="-Wall -Wshadow -Werror"
> +    WARNINGS_CFLAGS="-Wall -Wshadow -Wvla -Werror"
I'm guessing that both gcc and clang are okay with our current list; we
may reach the point where we need to probe at configure time on which
options we can safely use, instead of merely open-coding a list, but
we'll deal with that when it breaks the build.

> +++ b/plugins/sh/sh.c
> @@ -74,8 +74,7 @@ sh_load (void)
>  static void
>  sh_unload (void)
>  {
> -  const size_t tmpdir_len = strlen (tmpdir);
> -  char cmd[7 + tmpdir_len + 1]; /* "rm -rf " + tmpdir + \0 */
> +  CLEANUP_FREE char *cmd = NULL;
>  
>    /* Run the unload method.  Ignore all errors. */
>    if (script) {
> @@ -85,8 +84,8 @@ sh_unload (void)
>    }
>  
>    /* Delete the temporary directory.  Ignore all errors. */
> -  snprintf (cmd, sizeof cmd, "rm -rf %s", tmpdir);
> -  system (cmd);
> +  if (asprintf (&cmd, "rm -rf %s", tmpdir) >= 0)
> +    system (cmd);
Safe only because our tmpdir pattern contains no shell metacharacters
(your patch does not change that fact).  If we ever decided to honor
$TMPDIR (that is, creating $TMPDIR/nbdkitshXXXXXX instead of
/tmp/nbdkitshXXXXXX), then we'd need shell quoting here.  But doesn't
change this patch.
> +++ b/server/sockets.c
> @@ -366,10 +366,16 @@ accept_connection (int listen_sock)
>  static void
>  check_sockets_and_quit_fd (int *socks, size_t nr_socks)
>  {
> -  struct pollfd fds[nr_socks + 1];
>    size_t i;
>    int r;
>  
> +  CLEANUP_FREE struct pollfd *fds > +    malloc (sizeof (struct pollfd)
* (nr_socks+1));
This is indeed safer, but adds a malloc() in a loop.  Thankfully, the
loop of accept_incoming_connections() doesn't cycle that quickly (once
per new client), so I think it's in the noise compared to pre-allocating
the array once before starting the loop.

ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On Thu, Sep 05, 2019 at 07:57:44AM -0500, Eric Blake
wrote:
> On 9/5/19 6:28 AM, Richard W.M. Jones wrote:
> > I'm not someone who thinks VLAs are automatically bad and unlike
Linux
> > kernel code they can sometimes be used safely in userspace.  However
> > for an internet exposed server there is an argument that they might
> > cause some kind of exploitable situation especially if the code is
> > compiled without other stack hardening features.  Also in highly
> > multithreaded code with limited stack sizes (as nbdkit is on some
> > platforms) allowing unbounded stack allocation can be a bad idea
> > because it can cause a segfault.
> > 
> > So this commit bans them.  Only when using --enable-gcc-warnings, but
> > upstream developers ought to be using that.
> > 
> > There were in fact only two places where VLAs were being used.  In one
> > of those places (plugins/sh) removing the VLA actually made the code
> > better.
> > 
> > For interesting discussion about VLAs in the kernel see:
> > https://lwn.net/Articles/763253/
> > ---
> >  configure.ac     | 2 +-
> >  plugins/sh/sh.c  | 7 +++----
> >  server/sockets.c | 8 +++++++-
> >  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> > +++ b/configure.ac
> > @@ -90,7 +90,7 @@ AC_ARG_ENABLE([gcc-warnings],
> >        [gcc_warnings=no]
> >  )
> >  if test "x$gcc_warnings" = "xyes"; then
> > -    WARNINGS_CFLAGS="-Wall -Wshadow -Werror"
> > +    WARNINGS_CFLAGS="-Wall -Wshadow -Wvla -Werror"
> 
> I'm guessing that both gcc and clang are okay with our current list; we
> may reach the point where we need to probe at configure time on which
> options we can safely use, instead of merely open-coding a list, but
> we'll deal with that when it breaks the build.
With the proviso that I didn't actually test it under clang, it
does seem to have the flag:

https://clang.llvm.org/docs/DiagnosticsReference.html#wvla

[...]

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top