Joey Hess <[EMAIL PROTECTED]> writes:
> Timo Lilja wrote:
> > Procmeter3 segfaults immediately on startup if /proc/mounts has long
> > lines.
> >
> > The function Initialise() in modules/df.c has few buffer overflows
> > when there are long lines in /proc/mounts: sscanf() is not called with
> > a proper size arguments on one occasion.
There are three instances of sscanf using "%s" without a length
specifier in the ProcMeter modules. Two are in df.c and are fixed in
this patch and the other is in acpi.c.
-------------------- acpi.c.diff --------------------
--- acpi.c 2006/03/11 16:37:00 1.11
+++ acpi.c 2006/12/19 19:25:04
@@ -131,7 +131,7 @@
static char ret[256];
if ((ptr = strstr(buf, key))) {
- if (sscanf(ptr + strlen(key), "%s", ret) == 1)
+ if (sscanf(ptr + strlen(key), "%255s", ret) == 1)
return ret;
}
return NULL;
-------------------- acpi.c.diff --------------------
Otherwise fgets is used everywhere which leaves open the problem of
insufficient buffer space to hold a whole line reading to odd
behaviour but closes the possibility of buffer overflow.
> > The line[] buffer size is currently 128 which is a bit too small. The
> > attached patch fixes both the sscanf() format string and the size of
> > the line[] buffer by extending it to 256 characters.
>
> Sizing the agruments to sscanf and losing the segfault is definitly a
> good thing. Raising the static buffer size seems to just be papering
> over the problem of there being a static buffer in the first place, as
> far as I know there's nothing special about 256 bytes that makes it big
> enough in all cases.
>
> I wonder if perhaps there's a potential for this to be a security hole,
> although at the moment I can't think of any setups that let an untrusted
> user put arbitrary data in /proc/mounts ..
I think that the only security hole would be in the buffer overflows
which are now fixed. But the data they are reading has been sanitised
to some extent (possibly none I suppose) by the kernel.
> > It might be a good idea to add some functionality to the GUI rendering
> > of the disk information as well. At the moment, only the initial part
> > of the mount name is displayed to the user.
>
> Maybe Andrew can do that..
The GUI rendering has a fixed length string for display. This is done
because an arbitrary width doesn't make sense for displaying in a
small applet like ProcMeter. If you have the window wide enough to
display a 64 character mount point then it takes up rather a lot of
screen space.
You should be able to set your own label for any displayed item by
using the configuration file. The following is the syntax that you
need, you will need to replace the name "foobar1" with whatever name
ProcMeter gives the item and "foobar2" with whatever you want
displayed.
-------------------- .procmeterrc --------------------
[DiskUsage.DF_Used_/foobar1]
label = foobar2
-------------------- .procmeterrc --------------------
Thanks for the patch, it will be in the next version.
--
Andrew.
----------------------------------------------------------------------
Andrew M. Bishop [EMAIL PROTECTED]
http://www.gedanken.demon.co.uk/
ProcMeter homepage: http://www.gedanken.demon.co.uk/procmeter3/
--
To UNSUBSCRIBE, email to [EMAIL PROTECTED]
with a subject of "unsubscribe". Trouble? Contact [EMAIL PROTECTED]
