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 r...@space.bg r...@ringlet.net r...@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.
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 <r...@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 <ans...@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 <ans...@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 <ans...@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 <ans...@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 <r...@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
signature.asc
Description: Digital signature
