[Libguestfs] [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>
>>
>> ---
>> 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>
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