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

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