Bug#609096: Buffer overflow in xdigger with long argv[0]

[Adam D. Barratt]

Hi,

On Sun, 2011-01-09 at 01:16 +0200, Peter Pentchev wrote:
> On Thu, Jan 06, 2011 at 04:47:16PM +1100, Silvio Cesare wrote:
> > 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.
[...]
> 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.

Thanks for preparing a stable upload for this.  Most of the code changes
look okay, if possible a little overly cautious in places. :-)

This change looked a little odd:

+ 	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;

Why have the filenames changed from foo.au to XDIGGER_LIB_DIR/foo.au?

In general, we try to avoid introducing changes in stable updates which
aren't directly related to fixing the main issue; this has the dual
advantages of reducing the risk of inadvertently introducing new issues
and making the diff easier to review.

Have you verified whether the addition of ${misc:Depends} makes any
practical difference to the generated binary packages, rather than
simply quietening lintian?

Were the update to xdigger.desktop and the addition of
debian/source/format intentional?  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.

Regards,

Adam