Subject: xdiskusage: stack smash may cause segfault
Package: xdiskusage
Version: 1.48-10
Severity: normal
Tags: upstream patch
*** Please type your report below this line ***
I noticed that xdiskusage-1.48 could segfault given a deep hierarchy,
and tracked it down to a stack-smashing bug. Three fixed-length buffers
on the stack may be overrun, each by one "long", and the value that is
written beyond the end of one of those buffers is somewhat under control
of the "attacker" (anyone who constructs a hierarchy that xdiskusage is
used to view). The other two overruns write pointer values.
However, even if you know precisely how xdiskusage is being invoked,
it may be tricky to design a hierarchy that causes more than a segfault.
========================================================================
For an absolute minimal change, this does the job, but is ugly,
inefficient and fragile -- who wants to waste space on hierarchies
512 levels deep? Plus, depending on the existing 1024-byte maximum
line length limit is fragile. If that is ever fixed (there is a
FIXME comment) to remove or raise the limit, it would revive the bug
we're trying to fix here.
>From 2140cc66b09f2447ef7e7a89256e04c02bb18b2e Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Wed, 16 Feb 2011 18:16:11 +0100
Subject: [PATCH] MAXDEPTH: increase limit to 512
Otherwise, a long line like this:
12 a/a/a/a/a/a/a/.../a
with more than 100 components, would cause xdiskusage
to write user-controllable data beyond the end of the "totals" buffer,
and, less so (pointers), beyond the end of the "lastnode" buffer.
---
xdiskusage.C | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/xdiskusage.C b/xdiskusage.C
index 6a3ee77..f501528 100644
--- a/xdiskusage.C
+++ b/xdiskusage.C
@@ -170,7 +170,7 @@ struct Node {
int window_w = 600;
int window_h = 480;
int ncols = 5;
-#define MAXDEPTH 100
+#define MAXDEPTH 512 // enough for now, considering we chop lines at 1024
bytes
class OutputWindow : public Fl_Window {
void draw();
--
1.7.4.1.16.g759e8
========================================================================
Here's the patch I prefer:
>From c8a820f75e7f8a7012f53bc1c22435cc3f2a7407 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Wed, 16 Feb 2011 18:16:11 +0100
Subject: [PATCH 2/2] avoid buffer overrun for directory of depth 100 or more
Otherwise, a line like this:
12 a/a/a/a/a/a/a/.../a
with more than 100 components, would cause xdiskusage
to write user-controllable data beyond the end of the "totals" buffer,
and, less so (pointers), beyond the end of the "lastnode" and "parts"
buffers.
---
xdiskusage.C | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/xdiskusage.C b/xdiskusage.C
index 6a3ee77..f572623 100644
--- a/xdiskusage.C
+++ b/xdiskusage.C
@@ -463,9 +463,9 @@ OutputWindow* OutputWindow::make(const char* path, Disk*
disk) {
root->ordinal = 0;
ordinal = 0;
- Node* lastnode[MAXDEPTH];
+ Node* lastnode[MAXDEPTH+1];
ulong runningtotal;
- ulong totals[MAXDEPTH];
+ ulong totals[MAXDEPTH+1];
lastnode[0] = root;
runningtotal = 0;
totals[0] = 0;
@@ -519,7 +519,7 @@ OutputWindow* OutputWindow::make(const char* path, Disk*
disk) {
// split the path into parts:
int newdepth = 0;
- const char* parts[MAXDEPTH];
+ const char* parts[MAXDEPTH+1];
if (*p == '/') {
if (!root->name) root->name = strdup("/");
p++;
--
1.7.4.1.16.g759e8
========================================================================
If you want a little added insurance, you can add a couple assertions.
The patch below depends on the one just above, but if you adjust the
expressions, they can also apply to the original and serve as a good way
to demonstrate that there is indeed a problem. Especially since while I
do see a segfault pretty consistently on Fedora 14 (built from sources +
cast-patch), I was unable to evoke a segfault on debian unstable using
xdiskusage-1.48-10 .
Note that without the 1-element-larger "parts" array,
you'd have to adjust this loop not to modify parts[MAXDEPTH]:
for (newdepth = 0; newdepth < MAXDEPTH && *p;) {
parts[++newdepth] = p++;
>From af87585dfe2c632d9985484b56e730b5915b5f2c Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Fri, 18 Feb 2011 21:02:19 +0100
Subject: [PATCH] assert
---
xdiskusage.C | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/xdiskusage.C b/xdiskusage.C
index f572623..21ed026 100644
--- a/xdiskusage.C
+++ b/xdiskusage.C
@@ -42,6 +42,7 @@ const char* copyright =
#include <errno.h>
#include <unistd.h>
#include <sys/stat.h>
+#include <assert.h>
#include "panels.H"
#include <FL/fl_draw.H>
@@ -533,6 +534,7 @@ OutputWindow* OutputWindow::make(const char* path, Disk*
disk) {
// find out how many of the fields match:
int match = 0;
for (; match < newdepth && match < currentdepth; match++) {
+ assert (match < MAXDEPTH);
if (strcmp(parts[match+1],lastnode[match+1]->name)) break;
}
@@ -548,6 +550,7 @@ OutputWindow* OutputWindow::make(const char* path, Disk*
disk) {
} else {
Node* p = lastnode[match];
for (int j = match+1; j <= newdepth; j++) {
+ assert (j <= MAXDEPTH);
totals[j] = runningtotal;
p = newnode(parts[j], 0, p, lastnode[j]);
}
--
1.7.4.1.16.g759e8
-- System Information:
Debian Release: wheezy/sid
APT prefers unstable
APT policy: (500, 'unstable')
Architecture: amd64 (x86_64)
Kernel: Linux 2.6.32-5-amd64 (SMP w/1 CPU core)
Locale: LANG=C, LC_CTYPE=C (charmap=ANSI_X3.4-1968) (ignored: LC_ALL set to C)
Shell: /bin/sh linked to /bin/dash
Versions of packages xdiskusage depends on:
ii libc6 2.11.2-11 Embedded GNU C Library: Shared lib
ii libfltk1.1 1.1.10-4 Fast Light Toolkit - shared librar
ii libgcc1 1:4.4.5-12 GCC support library
ii libstdc++6 4.4.5-12 The GNU Standard C++ Library v3
xdiskusage recommends no packages.
xdiskusage suggests no packages.
-- no debconf information
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
