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

[Peter Pentchev]

On Wed, Jan 12, 2011 at 09:10:53PM +0000, Adam D. Barratt wrote:
> 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. :-)

Well, what can I say - I do get a little paranoid sometimes :)

> This change looked a little odd:
> 
> + 	case TON_DIAMANT:
> +-	  strcat(name, "/diamond.au");
> ++	  snprintf(name, sizeof(name), "%s/diamond.au", XDIGGER_LIB_DIR);
> + 	  break;

That part is okay, see below.

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

I've corrected the patch to remove the strncat() that I'd put there before
deciding to change it to snprintf() :)

> 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.

Yes, I understand that.

> 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.

> 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?

> 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.

Thanks for taking the time to review the changes!

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
If the meanings of 'true' and 'false' were switched, then this sentence wouldn't be false.
-------------- 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/20110113/811fae3e/attachment.pgp>