[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]