Re: Latest patch

[Lukas Geyer]

Simon Waters <address@hidden> writes:

> Lukas Geyer wrote:
> > 
> > Doing all that, I wondered why we had in all copyright notices as contact
> > info address@hidden and 3 addresses of Stuart. I added my
> > address to two or three of the files, but maybe we should think of
> > something consistent there.
> 
> It should be bugs-gnu-chess, Stuart reads that list, but still
> gets quite a lot of direct emails for stuff I volunteered to
> handle.

> I'll straighten inconsistencies with email addresses and the
> like as I find them. Just email me a note on silly errors like
> that, rather than worry about including them in patches, keeps
> the patches small, and keeps the differences small as well.

OK, I don't really know whether I am on that list or not. If we just
leave that address in there, please put me on the mailing list, if you
have not done so yet.

> > I will also put my package as a tarball somewhere and announce it on
> > gnu.chess to get people to alpha-test it. I would really like to have
> > that security fix in 5.05 really soon, so either we have to work hard
> > to test all that pondering stuff or the burden is more or less on you
> > to make the current CVS head releasable. If you plan to do so, could
> > you announce it some days before? We could just make sure it compiles
> > and everything basically works on as many architectures as we can
> > get hold of.
> 
> My idea was to get 5.05 out without major changes, but since to
> secure it we want the PGN change amongst others, I'm happy to
> include other changes that seem ready.

The best preliminary fix is the following. David Wheeler said he
rewrote the parser with lex, which is a good thing to do, but the
buffer overflow is fixed by the following. (7, not 8...)

--- gnuchess-5.04.orig/src/pgn.c
+++ gnuchess-5.04/src/pgn.c
@@ -148,7 +148,7 @@
       c = fgetc(fp);
       if (c == '*') break;
       ungetc (c, fp);
-      fscanf (fp, "%d. %s %s ", &moveno, wmv, bmv);
+      fscanf (fp, "%d. %7s %7s ", &moveno, wmv, bmv);
       p = ValidateMove (wmv);
       if (!p)
       {

> In retrospect not mentioning the security problems in gnu.chess
> newsgroup might have been better, not that I think it is a major
> issue as I suspect most PGN and EPD files go through a parser by
> Tim Mann, and not gnuchess's own parser.

I am not sure about that, but I would always favor full disclosure of
security problems. Distributions and people who compile binary
packages like Tim Mann and Dann Corbit can then fix it and users are
reminded of being careful (I hope), and not feeding random garbage to
chess programs...

Best, Lukas

P.S.: The package which I put up on

http://people.debian.org/~lukas/gnuchess/gnuchess-5.05luk1.tar.gz 

slightly differs from the patch I sent you, I removed some stupid
debugging output and added an #include <sys/types.h> in src/common.h
which seems to be needed by older glibc's or such to have pthread_t
defined.