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

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(a)redhat.com&gt; > >>> > >>> --- > >>> 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(a)redhat.com&gt; > 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);