Bug#609096: Buffer overflow in xdigger with long argv[0]
Peter Pentchev
roam at ringlet.net
Sat Jan 8 23:16:14 UTC 2011
package xdigger
tag 609096 + pending
thanks
On Thu, Jan 06, 2011 at 04:47:16PM +1100, Silvio Cesare wrote:
> Package: xdigger
> Version: 1.0.10-13
> Severity: important
> Tags: security
>
> There is a buffer overflow in xdigger.
>
> xdigger_1.0.10/xdigger.c
> strcpy(progname, argv[0]);
>
> I confirmed execv* with a long argv[0] crashes xdigger.
>
> Some other cases in the sound module with copying and strcating pargv/argv
> might be worth looking at also. I have not investigated further. Nor have I
> investigated exploitability.
>
> xdigger is SGID games.
Hi,
Thanks for reporting this! I've fixed this overflow, along with a whole
lot of other unchecked string accesses, in the Debian Games Team's
Subversion repository; the fix will be present in the 1.0.10-13+lenny1
version when it is uploaded.
And here's the question for the Release Team - may I prepare an upload to
stable-proposed-updates with the attached debdiff? According to Moritz
Muehlenhoff's message to debian-games-devel at
http://lists.debian.org/debian-devel-games/2011/01/msg00006.html there will
be no Debian Security Advisory for this particular change; still, it might
be good to fix it, even if it is not too severe. Of course, with Squeeze's
deep freeze, there's no rush right now, IMHO :)
Again, thanks for Silvio Cesare for reporting this, to the Debian Release
Team for everything they're doing, and to Ansgar Burchardt for the helpful
hints and advice in the past couple of days! Keep up the great work, all
of you!
G'luck,
Peter
--
Peter Pentchev roam at space.bg roam at ringlet.net roam at FreeBSD.org
PGP key: http://people.FreeBSD.org/~roam/roam.key.asc
Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553
This would easier understand fewer had omitted.
-------------- next part --------------
diffstat for xdigger_1.0.10-13 xdigger_1.0.10-13+lenny1
debian/README.source | 17 +
debian/patches/buffers | 239 +++++++++++++++++
debian/source/format | 1
xdigger-1.0.10/debian/changelog | 12
xdigger-1.0.10/debian/control | 2
xdigger-1.0.10/debian/patches/config | 5
xdigger-1.0.10/debian/patches/dont-create-highscore | 5
xdigger-1.0.10/debian/patches/escape-hyphen-in-manpage | 5
xdigger-1.0.10/debian/patches/series | 1
xdigger-1.0.10/debian/patches/start-level-on-move | 5
xdigger-1.0.10/debian/rules | 5
xdigger-1.0.10/debian/xdigger.desktop | 2
12 files changed, 295 insertions(+), 4 deletions(-)
diff -u xdigger-1.0.10/debian/changelog xdigger-1.0.10/debian/changelog
--- xdigger-1.0.10/debian/changelog
+++ xdigger-1.0.10/debian/changelog
@@ -1,3 +1,15 @@
+xdigger (1.0.10-13+lenny1) stable-security; urgency=low
+
+ * Team upload.
+ * Add the buffers patch to guard against lots of buffer overflows,
+ including the one reported in the BTS. Closes: #609096
+ * Add DEP 3 descriptive headers to the rest of the patches.
+ * Add misc:Depends to the binary package.
+ * Use the quilt patch/unpatch targets in a bit more robust way and
+ add a README.source file describing the use of quilt.
+
+ -- Peter Pentchev <roam at ringlet.net> Fri, 07 Jan 2011 16:44:34 +0200
+
xdigger (1.0.10-13) unstable; urgency=low
* Add /var/games dir, used in postinst (Closes: #496340)
diff -u xdigger-1.0.10/debian/xdigger.desktop xdigger-1.0.10/debian/xdigger.desktop
--- xdigger-1.0.10/debian/xdigger.desktop
+++ xdigger-1.0.10/debian/xdigger.desktop
@@ -6,3 +6,3 @@
Icon=xdigger
-Categories=Game;
+Categories=Game;ArcadeGame;
Terminal=false
diff -u xdigger-1.0.10/debian/rules xdigger-1.0.10/debian/rules
--- xdigger-1.0.10/debian/rules
+++ xdigger-1.0.10/debian/rules
@@ -14,7 +14,7 @@
export DEB_BUILD_GNU_TYPE ?= $(shell dpkg-architecture -qDEB_BUILD_GNU_TYPE)
configure: configure-stamp
-configure-stamp: patch
+configure-stamp: $(QUILT_STAMPFN)
dh_testdir
xmkmf -a
@@ -30,13 +30,14 @@
touch build-stamp
-clean: configure clean-patched unpatch
+clean: configure clean-patched
clean-patched:
dh_testdir
dh_testroot
rm -f build-stamp configure-stamp
$(MAKE) distclean
+ $(MAKE) -f debian/rules unpatch
dh_clean
diff -u xdigger-1.0.10/debian/control xdigger-1.0.10/debian/control
--- xdigger-1.0.10/debian/control
+++ xdigger-1.0.10/debian/control
@@ -11,7 +11,7 @@
Package: xdigger
Architecture: any
-Depends: ${shlibs:Depends}
+Depends: ${misc:Depends}, ${shlibs:Depends}
Description: arcade diamonds digging game for X11
An arcade game in which you are a little (digger-)man and
have to collect diamonds. It is similar to the well-known
diff -u xdigger-1.0.10/debian/patches/series xdigger-1.0.10/debian/patches/series
--- xdigger-1.0.10/debian/patches/series
+++ xdigger-1.0.10/debian/patches/series
@@ -4,0 +5 @@
+buffers
diff -u xdigger-1.0.10/debian/patches/start-level-on-move xdigger-1.0.10/debian/patches/start-level-on-move
--- xdigger-1.0.10/debian/patches/start-level-on-move
+++ xdigger-1.0.10/debian/patches/start-level-on-move
@@ -1,3 +1,8 @@
+Description: Start playing as soon as a movement key is pressed
+Forwarded: no
+Author: Ansgar Burchardt <ansgar at debian.org>
+Last-Update: 2008-02-02
+
Index: xdigger-1.0.10/runlevels.c
===================================================================
--- xdigger-1.0.10.orig/runlevels.c 2008-02-02 19:10:18.000000000 +0100
diff -u xdigger-1.0.10/debian/patches/escape-hyphen-in-manpage xdigger-1.0.10/debian/patches/escape-hyphen-in-manpage
--- xdigger-1.0.10/debian/patches/escape-hyphen-in-manpage
+++ xdigger-1.0.10/debian/patches/escape-hyphen-in-manpage
@@ -1,3 +1,8 @@
+Description: Escape hyphens in the manual page
+Forwarded: no
+Author: Ansgar Burchardt <ansgar at debian.org>
+Last-Update: 2008-02-02
+
Index: xdigger-1.0.10/xdigger.man
===================================================================
--- xdigger-1.0.10.orig/xdigger.man 2008-02-02 19:23:04.000000000 +0100
diff -u xdigger-1.0.10/debian/patches/dont-create-highscore xdigger-1.0.10/debian/patches/dont-create-highscore
--- xdigger-1.0.10/debian/patches/dont-create-highscore
+++ xdigger-1.0.10/debian/patches/dont-create-highscore
@@ -1,3 +1,8 @@
+Description: Do not include the highscore file in the Debian package
+Forwarded: no
+Author: Ansgar Burchardt <ansgar at debian.org>
+Last-Update: 2008-02-02
+
Index: xdigger-1.0.10/Imakefile
===================================================================
--- xdigger-1.0.10.orig/Imakefile 2008-02-02 18:44:59.000000000 +0100
diff -u xdigger-1.0.10/debian/patches/config xdigger-1.0.10/debian/patches/config
--- xdigger-1.0.10/debian/patches/config
+++ xdigger-1.0.10/debian/patches/config
@@ -1,3 +1,8 @@
+Description: Use the Debian paths and file locations.
+Forwarded: no
+Author: Ansgar Burchardt <ansgar at debian.org>
+Last-Update: 2008-02-02
+
Index: xdigger-1.0.10/Imakefile
===================================================================
--- xdigger-1.0.10.orig/Imakefile 2008-02-02 18:35:23.000000000 +0100
only in patch2:
unchanged:
--- xdigger-1.0.10.orig/debian/README.source
+++ xdigger-1.0.10/debian/README.source
@@ -0,0 +1,17 @@
+The xdigger package uses quilt to maintain local changes to
+the xdigger distribution. The Debian-specific patches are maintained
+in the debian/patches/ directory.
+
+To apply all the patches, preparing the source for building, use:
+ debian/rules patch
+
+To revert the patches, preparing to build a source package, use:
+ debian/rules unpatch
+
+You do not need to manually execute these targets when building
+the package; they are part of the debian/rules target chain.
+
+For more information on the quilt integration with Debian packages,
+as well as editing, adding or removing patches, please see
+the quilt documentation; in recent versions of the Debian package of
+quilt, start at the /usr/share/doc/quilt/README.source file.
only in patch2:
unchanged:
--- xdigger-1.0.10.orig/debian/patches/buffers
+++ xdigger-1.0.10/debian/patches/buffers
@@ -0,0 +1,239 @@
+Description: Guard against buffer overflows... somewhat.
+ Use snprintf() and strncpy() instead of strcpy(), strcat() and sprintf()
+ to guard against lots of buffer overflows, including the one reported in
+ the BTS.
+ There are still a couple of writes beyond the end of the argv[] array
+ that will need a complete rewrite of xdigger to clean up.
+Bug-Debian: http://bugs.debian.org/609096
+Author: Peter Pentchev <roam at ringlet.net>
+Last-Update: 2011-01-07
+
+--- a/highscore.c
++++ b/highscore.c
+@@ -53,12 +53,13 @@
+ strcpy(highscore[i].name, "");
+ }
+
+- strcat(strcpy(filename, XDIGGER_HISCORE_DIR), "/xdigger.hiscore");
++ snprintf(filename, sizeof(filename), "%s/xdigger.hiscore",
++ XDIGGER_HISCORE_DIR);
+ if ((filehandle = fopen(filename, "r")) == NULL)
+ {
+ XBell(display, -50);
+ fprintf(stderr, "%s: can't read %s\n", progname, filename);
+- strcpy(filename, progname); strcat(filename, ".hiscore");
++ snprintf(filename, sizeof(filename), "%s.hiscore", progname);
+ fprintf(stderr, "%s: try %s ... ", progname, filename);
+ if ((filehandle = fopen(filename, "r")) == NULL)
+ /* fprintf(stderr, "can't read %s\n", filename); */
+@@ -87,12 +88,13 @@
+ FILE *filehandle;
+ int n = 0;
+
+- strcat(strcpy(filename, XDIGGER_HISCORE_DIR), "/xdigger.hiscore");
++ snprintf(filename, sizeof(filename), "%s/xdigger.hiscore",
++ XDIGGER_HISCORE_DIR);
+ if ((filehandle = fopen(filename, "w")) == NULL)
+ {
+ XBell(display, -50);
+ fprintf(stderr, "%s: can't write %s\n", progname, filename);
+- strcpy(filename, progname); strcat(filename, ".hiscore");
++ snprintf(filename, sizeof(filename), "%s.hiscore", progname);
+ fprintf(stderr, "try %s ... ", filename);
+ if ((filehandle = fopen(filename, "w")) == NULL)
+ /* fprintf(stderr, "can't write %s\n", filename); */
+@@ -128,10 +130,10 @@
+ char name[257], *c;
+
+ who = getpwuid(getuid());
+- strncpy(name, who->pw_gecos, 256);
++ snprintf(name, sizeof(name), "%s", who->pw_gecos);
+ c = strchr(name, ',') ;
+ if (c != NULL) *c = '\0';
+- strncpy(dest, name, n);
++ snprintf(dest, n, "%s", name);
+ } /* GetUserName(char *dest, size_t n) */
+
+ int InsertScore(int score, char *name)
+@@ -146,10 +148,11 @@
+ for (j=19; j>i; j--)
+ {
+ highscore[j].score = highscore[j-1].score;
+- strcpy(highscore[j].name, highscore[j-1].name);
++ snprintf(highscore[j].name, sizeof(highscore[j].name),
++ highscore[j-1].name);
+ }
+ highscore[i].score = score;
+- strncpy(highscore[i].name, name, NAMELENGH);
++ snprintf(highscore[i].name, sizeof(highscore[i].name), name);
+ break;
+ }
+ return(erg);
+@@ -168,7 +171,7 @@
+
+ for (i=0; i<20; i++)
+ {
+- sprintf(entry, "%.6d %s", highscore[i].score, highscore[i].name);
++ snprintf(entry, sizeof(entry), "%.6d %s", highscore[i].score, highscore[i].name);
+ WriteTextStr(entry, 10, 7+i, kcf_gelb, kcb_tuerkis);
+ }
+ } /* InitHighScoreText() */
+@@ -238,7 +241,7 @@
+ if ((strlen(nameinput) < 20) && (strlen(buffer) == 1) &&
+ (0x20 <= buffer[0]) && (y>=0))
+ {
+- strcat(nameinput, buffer);
++ strncat(nameinput, buffer, NAMELENGH - strlen(nameinput));
+ WriteTextStr(nameinput, 18, inpy, kcf_gelb, kcb_tuerkis);
+ WriteTextStr("\177", 18 + strlen(nameinput), inpy,
+ kcf_gelb, kcb_tuerkis);
+--- a/runlevels.c
++++ b/runlevels.c
+@@ -57,11 +57,11 @@
+ {
+ char slevel[3], scmdln[7];
+
+- sprintf(slevel, "%d", akt_level_number);
++ snprintf(slevel, sizeof(slevel), "%d", akt_level_number);
+ if (cheat)
+- strcat(strcat(strcpy(scmdln, " (C"), slevel), ")");
++ snprintf(scmdln, sizeof(scmdln), " (C%s)", slevel);
+ else
+- strcat(strcat(strcpy(scmdln, " (L"), slevel), ")");
++ snprintf(scmdln, sizeof(scmdln), " (L%s)", slevel);
+ strcpy(LastArgv, scmdln);
+ } /* ChangePS() */
+
+@@ -325,7 +325,7 @@
+ {
+ char slefttime[7];
+
+- sprintf(slefttime, "%.6d", lefttime);
++ snprintf(slefttime, sizeof(slefttime), "%.6d", lefttime);
+ if ((lefttime < 1000) && ((lefttime % 4) <= 1) && (lefttime != 0))
+ strcpy(slefttime, " ");
+ WriteTextStr(slefttime, 18, vertvar, kcf_weiss, kcb_rot);
+@@ -335,7 +335,7 @@
+ {
+ char snumber_diamonds[3];
+
+- sprintf(snumber_diamonds, "%.2d", number_diamonds);
++ snprintf(snumber_diamonds, sizeof(snumber_diamonds), "%.2d", number_diamonds);
+ WriteTextStr(snumber_diamonds, 36, vertvar, kcf_weiss, kcb_rot);
+ } /* Restore_Diamonds() */
+
+@@ -343,7 +343,7 @@
+ {
+ char sscore[7];
+
+- sprintf(sscore, "%.6d", score);
++ snprintf(sscore, sizeof(sscore), "%.6d", score);
+ WriteTextStr(sscore, 18, 1+vertvar, kcf_weiss, kcb_rot);
+ } /* Restore_Score() */
+
+@@ -351,7 +351,7 @@
+ {
+ char scollected_diamonds[3];
+
+- sprintf(scollected_diamonds, "%.2d", collected_diamonds);
++ snprintf(scollected_diamonds, sizeof(scollected_diamonds), "%.2d", collected_diamonds);
+ WriteTextStr(scollected_diamonds, 36, 1+vertvar, kcf_weiss, kcb_rot);
+ } /* Restore_Collected_Diamonds() */
+
+@@ -359,10 +359,10 @@
+ {
+ char croom[41], clives[41], slevel_number[3], slives[20];
+
+- sprintf(slevel_number, "%.2d", akt_level_number);
+- sprintf(slives, "%.2d", lives);
+- strcat(strcpy(croom, " ROOM: "), slevel_number);
+- strcat(strcpy(clives, " LIVES: "), slives);
++ snprintf(slevel_number, sizeof(slevel_number), "%.2d", akt_level_number);
++ snprintf(slives, sizeof(slives), "%.2d", lives);
++ snprintf(croom, sizeof(croom), " ROOM: %s", slevel_number);
++ snprintf(clives, sizeof(clives), " LIVES: %s", slives);
+
+ if (!vert240)
+ WriteTextStr("\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135\135", 0, 0, kcf_tuerkis, kcb_blau);
+@@ -1368,8 +1368,8 @@
+ }
+ if ((keysym == XK_9) || (keysym == XK_d))
+ {
+- if (keysym == XK_9) strcat(scheat, "9");
+- if (keysym == XK_d) strcat(scheat, "d");
++ if (keysym == XK_9) strncat(scheat, "9", sizeof(scheat) - strlen(scheat) - 1);
++ if (keysym == XK_d) strncat(scheat, "d", sizeof(scheat) - strlen(scheat) - 1);
+ if (strcmp(scheat, "99d") == 0)
+ {
+ XBell(display, 0);
+--- a/sound.c
++++ b/sound.c
+@@ -351,13 +351,13 @@
+ /*struct hostent localhost_ent, xhost_ent;*/
+
+ gethostname(localhost, sizeof(localhost));
+- strcpy(xhost, DisplayString(display));
++ snprintf(xhost, sizeof(xhost), "%s", DisplayString(display));
+ c = strchr(xhost, ':');
+ if (c) *c = 0; else xhost[0] = 0;
+ if (strlen(xhost) == 0) return(True);
+
+- strcpy(localhost, gethostbyname(localhost)->h_name);
+- strcpy(xhost, gethostbyname(xhost)->h_name);
++ snprintf(localhost, sizeof(localhost), gethostbyname(localhost)->h_name);
++ snprintf(xhost, sizeof(xhost), gethostbyname(xhost)->h_name);
+ if (debug)
+ fprintf(stderr, "%s: localhost=%s\n xhost=%s\n",
+ progname, localhost, xhost);
+@@ -496,13 +496,14 @@
+ switch (ton_typ)
+ {
+ case TON_DIAMANT:
+- strcat(name, "/diamond.au");
++ snprintf(name, sizeof(name), "%s/diamond.au", XDIGGER_LIB_DIR);
+ break;
+ case TON_SCHRITT:
+- strcat(name, "/step.au");
++ snprintf(name, sizeof(name), "%s/step.au", XDIGGER_LIB_DIR);
++ strncat(name, "/step.au");
+ break;
+ case TON_STEINE:
+- strcat(name, "/stone.au");
++ snprintf(name, sizeof(name), "%s/stone.au", XDIGGER_LIB_DIR);
+ break;
+ }
+
+@@ -510,7 +511,7 @@
+ /* if (rplay_display(name) < 0) */
+ if (Play_RPlay_Sound(name, 200) < 0)
+ {
+- sprintf(error, "%s: (rplay) ", progname);
++ snprintf(error, sizeof(error), "%s: (rplay) ", progname);
+ rplay_perror(error);
+ fprintf(stderr, "%s: disable rplay-sound.\n", progname);
+ sound_device = SD_NONE;
+--- a/xdigger.c
++++ b/xdigger.c
+@@ -176,11 +176,11 @@
+ if (level_filename == NULL)
+ {
+ level_filename = malloc(256);
+- strcat(strcpy(level_filename, XDIGGER_LIB_DIR), "/xdigger.level");
++ snprintf(level_filename, 256, "%s/xdigger.level", XDIGGER_LIB_DIR);
+ if ((f = fopen(level_filename, "r")) == NULL)
+ {
+ fprintf(stderr, "%s: can't open %s\n", progname, level_filename);
+- strcpy(level_filename, progname); strcat(level_filename, ".level");
++ snprintf(level_filename, 256, "%s.level", progname);
+ fprintf(stderr, "%s: try %s... ", progname, level_filename);
+ if ((f = fopen(level_filename, "r")) == NULL)
+ {
+@@ -362,7 +362,7 @@
+
+ pargc = argc;
+ pargv = argv;
+- strcpy(progname, argv[0]);
++ snprintf(progname, sizeof(progname), "%s", argv[0]);
+ LastArgv = argv[argc - 1] + strlen(argv[argc - 1]);
+
+ for (i = 1; i < argc; i++)
only in patch2:
unchanged:
--- xdigger-1.0.10.orig/debian/source/format
+++ xdigger-1.0.10/debian/source/format
@@ -0,0 +1 @@
+1.0
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-games-devel/attachments/20110109/ee13ccb8/attachment-0001.pgp>
More information about the Pkg-games-devel
mailing list