[Libguestfs] Re: [PATCH libguestfs] sfdisk: guard against buffer overflow

[Richard W.M. Jones]

On Wed, Aug 12, 2009 at 09:19:37PM +0200, Jim Meyering wrote:
> Jim Meyering wrote:
> > Richard W.M. Jones wrote:
> >> On Wed, Aug 12, 2009 at 06:52:40PM +0200, Jim Meyering wrote:
> >>> From: Jim Meyering <meyering at redhat.com>
> >>>
> >>> ---
> >>>  daemon/daemon.h |    2 +-
> >>>  daemon/sfdisk.c |    2 +-
> >>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/daemon/daemon.h b/daemon/daemon.h
> >>> index 166f3bf..bebc86f 100644
> >>> --- a/daemon/daemon.h
> >>> +++ b/daemon/daemon.h
> >>> @@ -174,7 +174,7 @@ extern void reply (xdrproc_t xdrp, char *ret);
> >>>  #define NEED_ROOT_OR_IS_DEVICE(path,errcode) \
> >>>    do {									\
> >>>      if (strncmp ((path), "/dev/", 5) == 0)				\
> >>> -      IS_DEVICE ((path),(errcode));					\
> >>> +      RESOLVE_DEVICE ((path), return errcode);				\
> >>>      else {								\
> >>>        NEED_ROOT ((errcode));						\
> >>>        ABS_PATH ((path),(errcode));					\
> >>> diff --git a/daemon/sfdisk.c b/daemon/sfdisk.c
> >>> index 693e89a..cf62f51 100644
> >>> --- a/daemon/sfdisk.c
> >>> +++ b/daemon/sfdisk.c
> >>> @@ -53,7 +53,7 @@ sfdisk (char *device, int n, int cyls, int heads, int sectors,
> >>>    if (extra_flag)
> >>>      sprintf (buf + strlen (buf), " %s", extra_flag);
> >>>
> >>> -  /* Safe because of IS_DEVICE above: */
> >>> +  /* Safe because of RESOLVES_DEVICE above: */
> >
> > This stray "S" is a typo I'd started to fix when I realized that the
> > IS_DEVICE (renamed to RESOLVE_DEVICE) is now, in the final patch, gone.
> >
> > I'll rebase -i to fix my typo before committing.
> > I'll also write a separate commit to add a check for
> > buffer overflow.
> >
> >>>    sprintf (buf + strlen (buf), " %s", device);
> 
> Here's the promised patch
> 
> >From 8aca97fc1e7c0513c2781e3443c51b88876aaa6c Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering at redhat.com>
> Date: Wed, 12 Aug 2009 21:16:30 +0200
> Subject: [PATCH libguestfs] sfdisk: guard against buffer overflow
> 
> * daemon/sfdisk.c (sfdisk): Don't let outrageous "extra_flag"
> or "device" strings overflow a fixed-size buffer.
> ---
>  daemon/sfdisk.c |   19 ++++++++++++++++---
>  1 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/daemon/sfdisk.c b/daemon/sfdisk.c
> index 1ec0c85..8ef46e5 100644
> --- a/daemon/sfdisk.c
> +++ b/daemon/sfdisk.c
> @@ -48,10 +48,23 @@ sfdisk (const char *device, int n, int cyls, int heads, int sectors,
>      sprintf (buf + strlen (buf), " -H %d", heads);
>    if (sectors)
>      sprintf (buf + strlen (buf), " -S %d", sectors);
> -  if (extra_flag)
> -    sprintf (buf + strlen (buf), " %s", extra_flag);
> 
> -  /* Safe because of RESOLVE_DEVICE above: */
> +  /* The above are all guaranteed to fit in the fixed-size buffer.
> +     However, extra_flag and device have no restrictions,
> +     so we must check.  */
> +
> +  if (extra_flag) {
> +    if (strlen (buf) + 1 + strlen (extra_flag) >= sizeof buf) {
> +      reply_with_error ("internal buffer overflow: sfdisk extra_flag too long");
> +      return -1;
> +    }
> +    snprintf (buf + strlen (buf), " %s", extra_flag);
> +  }
> +
> +  if (strlen (buf) + 1 + strlen (device) >= sizeof buf) {
> +    reply_with_error ("internal buffer overflow: sfdisk device name too long");
> +    return -1;
> +  }
>    sprintf (buf + strlen (buf), " %s", device);
> 
>    if (verbose)
> --
> 1.6.4.337.g5420e

Yes that looks fine to me, ACK.

Rich.

-- 
Richard Jones, Emerging Technologies, Red Hat  http://et.redhat.com/~rjones
Read my programming blog: http://rwmj.wordpress.com
Fedora now supports 75 OCaml packages (the OPEN alternative to F#)
http://cocan.org/getting_started_with_ocaml_on_red_hat_and_fedora