Bug#609096: Buffer overflow in xdigger with long argv[0]
Peter Pentchev
roam at ringlet.net
Sun Jan 16 18:38:43 UTC 2011
On Thu, Jan 13, 2011 at 10:27:11PM +0000, Adam D. Barratt wrote:
> On Thu, 2011-01-13 at 12:18 +0200, Peter Pentchev wrote:
> > On Wed, Jan 12, 2011 at 09:10:53PM +0000, Adam D. Barratt wrote:
> > > This change looked a little odd:
> [...]
> > > + case TON_SCHRITT:
> > > +- strcat(name, "/step.au");
> > > ++ snprintf(name, sizeof(name), "%s/step.au", XDIGGER_LIB_DIR);
> > > ++ strncat(name, "/step.au");
> > > + break;
> >
> > Oops! The strncat() should not be there, I'll prepare a new upload.
> >
> > > + case TON_STEINE:
> > > +- strcat(name, "/stone.au");
> > > ++ snprintf(name, sizeof(name), "%s/stone.au", XDIGGER_LIB_DIR);
> > > + break;
> > >
> > > Why have the filenames changed from foo.au to XDIGGER_LIB_DIR/foo.au?
> >
> > They haven't changed :) A couple of lines above that, the "name" variable
> > is initialized to XDIGGER_LIB_DIR, so the strcat() that was there just
> > added foo.au to it. The snprintf() does both.
>
> Ah, I see.
>
> > I've corrected the patch to remove the strncat() that I'd put there before
> > deciding to change it to snprintf() :)
> [...]
> > > Have you verified whether the addition of ${misc:Depends} makes any
> > > practical difference to the generated binary packages, rather than
> > > simply quietening lintian?
> >
> > Actually, it does not make any difference; I'll remove it.
>
> Thanks.
Here it is.
> > > Were the update to xdigger.desktop and the addition of
> > > debian/source/format intentional?
> >
> > Well, the update to xdigger.desktop was done in a sweeping change by
> > Paul Wise (pabs) two and a half years ago; I don't know why he didn't
> > mention it in the changelog. That was before xdigger was removed from
> > unstable and testing, and before there were any thoughts of preparing
> > a Lenny-only upload.
> >
> > Should I document it in the changelog, or revert it from the Subversion
> > repository?
>
> One or the other. :-)
I documented it, since the change did seem kind of useful.
> > > If so, why aren't they mentioned in
> > > the changelog? fwiw, given that the default source format is not going
> > > to change in lenny, the source/format change is at best a no-op.
> >
> > As to the default source format, I initially tried to convert it to
> > 3.0 (quilt), but then Ansgar Burchardt kindly reminded me that you would
> > not really allow this as a stable update :) So I reverted the 3.0 changes
> > and placed 1.0 as the source format name; I could remove it if you'd like,
> > no problem, and quite understandable.
>
> Ansgar was correct. :-) It's technically a no-op; I'm not going to
> complain (too) loudly if you leave it in.
Okay, removed.
Here's the new debdiff; thanks for your time!
G'luck,
Peter
--
Peter Pentchev roam at ringlet.net roam at FreeBSD.org roam at cpan.org
PGP key: http://people.FreeBSD.org/~roam/roam.key.asc
Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553
If this sentence were in Chinese, it would say something else.
-------------- next part --------------
diffstat for xdigger_1.0.10-13 xdigger_1.0.10-13+lenny1
debian/README.source | 17 +
debian/patches/buffers | 238 +++++++++++++++++
xdigger-1.0.10/debian/changelog | 13
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
10 files changed, 293 insertions(+), 3 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,16 @@
+xdigger (1.0.10-13+lenny1) unstable; urgency=low
+
+ * Team upload.
+ * Paul Wise made xdigger.desktop a valid file by adding ArcadeGame
+ as a category.
+ * 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.
+ * 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, 14 Jan 2011 14:53:30 +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/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,238 @@
+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-13
+
+--- 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,13 @@
+ 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);
+ break;
+ case TON_STEINE:
+- strcat(name, "/stone.au");
++ snprintf(name, sizeof(name), "%s/stone.au", XDIGGER_LIB_DIR);
+ break;
+ }
+
+@@ -510,7 +510,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++)
-------------- 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/20110116/ebc2601e/attachment-0001.pgp>
More information about the Pkg-games-devel
mailing list