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

[Jim Meyering]

Richard W.M. Jones wrote:
> 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);

It pains me to admit it, but a typo snuck in there.
That should be:

    sprintf (buf + strlen (buf), " %s", extra_flag);

and will be in what I push tomorrow morning.
[I missed the warning, but "make check" caught it.
 It would have better if -Werror could have been used.  Coming soon. ]