Buffer overflows

[Lukas Geyer]

Hi Simon,

here is a patch which should fix the buffer overflows in pgn.c and
epd.c. I have slightly rewritten the EPD parser to check more for
correct EPD format. Also the parser now gives an error code if it
encounters incorrect EPD. I have not yet checked whether the handling
of that error code is sane, but the parser itself should be fine for
the moment. Of course, if David Wheeler rewrites it in lex, even
better.

Greetings, Lukas

P.S.: The patch is against the latest Debian package, which means
5.04 release. However, it should be pretty non-intrusive and apply to
the CVS head, too

--- gnuchess-5.04.orig/src/common.h
+++ gnuchess-5.04/src/common.h
@@ -524,9 +524,15 @@
 
 /*  The EPD routines  */
 short ReadEPDFile (const char *, short);
-void ParseEPD (char *);
+int ParseEPD (char *);
 void LoadEPD (char *);
 void SaveEPD (char *);
+
+/* Error codes for ParseEPD */
+enum {
+   EPD_SUCCESS,
+   EPD_ERROR
+};
 
 /*  The command routines */
 void InputCmd (void);
--- 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)
       {
--- gnuchess-5.04.orig/src/epd.c
+++ gnuchess-5.04/src/epd.c
@@ -48,7 +48,7 @@
  ****************************************************************************/
 {
    static FILE *fp = NULL;
-   char line[255];
+   char line[1025];
 
    /*  If first time through, must open file  */
    if (fp == NULL)
@@ -70,10 +70,12 @@
    }
 
    /*  Okay, we read in an EPD entry  */
-   fgets (line, 255, fp);
+   fgets (line, 1024, fp);
    if (!feof(fp)) 
    {
-      ParseEPD (line);
+      int ret = ParseEPD (line);
+
+      if (ret != EPD_SUCCESS) abort();
       if (op != 2)
          printf ("\n%s : Best move = %s\n", id, solution);
       return (true);
@@ -87,8 +89,14 @@
    }
 }
 
+/*
+ * Returns EPD_SUCCESS on success, EPD_ERROR on error. We try to be
+ * quite tough on the format. However, as of yet no legality checking
+ * is done and the board is not reset on error, this should be done by
+ * the caller.
+ */
 
-void ParseEPD (char *p)
+int ParseEPD (char *p)
 /**************************************************************************
  *   
  *  Parses an EPD input line.  A few global variables are updated e.g.
@@ -97,13 +105,13 @@
  **************************************************************************/
 {
    short r, c, sq;
-   char s[8];
+   char *str_p;
 
    r = 56;
    c = 0;
    memset (&board, 0, sizeof (board));
 
-   while (*p != ' ')
+   while (p && *p != ' ')
    {
      sq = r + c;
      switch (*p)
@@ -187,6 +195,8 @@
         c += (*p - '0');
      else
         c++;
+     if (r < 0 || c > 8) return EPD_ERROR;
+     if (c == 8 && p[1] != '/' && p[1] != ' ') EPD_ERROR;
      p++;
    }
 
@@ -201,41 +211,58 @@
    UpdateMvboard ();
 
    /*  Get side to move  */
-   sscanf (p, " %s %[^\n]", s, p);
-   if (s[0] == 'w')
-      board.side = white;
-   else if (s[0] == 'b')
-      board.side = black;
-
+   if (!++p) return EPD_ERROR;
+   if      (*p == 'w') board.side = white; 
+   else if (*p == 'b') board.side = black;
+   else return EPD_ERROR;
+
+   /* Isn't this one cute? */
+   if (!++p || *p != ' ' || !++p) return EPD_ERROR;
+  
    /*  Castling status  */
-   sscanf (p, " %s %[^\n]", s, p);
-   if (strchr (s, 'K'))
-      board.flag |= WKINGCASTLE;
-   if (strchr (s, 'Q'))
-      board.flag |= WQUEENCASTLE;
-   if (strchr (s, 'k'))
-      board.flag |= BKINGCASTLE;
-   if (strchr (s, 'q'))
-      board.flag |= BQUEENCASTLE;
-
-   /*  En passant square */
-   sscanf (p, " %s %[^\n]", s, p);
-   if (s[0] != '-')
-      board.ep = (s[0] - 'a') + (s[1] - '1')*8;
-   else
+   while (p && *p != ' ') {
+      if      (*p == 'K') board.flag |= WKINGCASTLE;
+      else if (*p == 'Q') board.flag |= WQUEENCASTLE;
+      else if (*p == 'k') board.flag |= BKINGCASTLE;
+      else if (*p == 'q') board.flag |= BQUEENCASTLE;
+      else if (*p == '-') { p++; break; }
+      else return EPD_ERROR;
+      p++;
+   }
+   if (!p || *p != ' ' || !++p) return EPD_ERROR;
+
+   /*
+    * En passant square, can only be '-' or [a-h][36]
+    * In fact, one could add more sanity checks here.
+    */
+   if (*p != '-') {
+      if (!p[1] || *p < 'a' || *p > 'h' ||
+         !(p[1] == '3' || p[1] == '6')) return EPD_ERROR;
+      board.ep = (*p - 'a') + (p[1] - '1')*8;
+      p++;
+   } else {
       board.ep = -1;
+   }
+
+   solution[0] = '\0';
+   id[0] = '\0';
+
+   if (!++p) return EPD_SUCCESS;
+
+   /* The opcodes are optional, so we should not generate errors here */
 
    /*  Read in best move; "bm" operator */
-   if (!strncmp (p, "bm", 2))
-      sscanf (p, "%*s %[^;]; %[^\n]", solution, p); 
+   str_p = strstr(p, "bm");
+   if (str_p) sscanf (str_p, "bm %63[^;];", solution); 
 
    /*  Read in the description; "id" operator */
-   if (!strncmp (p, "id", 2))
-      sscanf (p, "%*s %[^;]; %[^\n]", id, p); 
+   str_p = strstr(p, "id");
+   if (str_p) sscanf (p, "id %31[^;];", id);
 
    CalcHashKey ();
    phase = PHASE;
-   return;
+
+   return EPD_SUCCESS;
 }
 
 
@@ -249,7 +276,7 @@
    char file[32];
    short N = 1;
 
-   sscanf (p, "%s %hd ", file, &N);
+   sscanf (p, "%31s %hd ", file, &N);
    if (strcmp (file, "next") == 0)
    {
       ReadEPDFile (file, 0);