FYI: head warning fix, security hole

[Eric Blake]

> I noticed ill-formated when I found a potential security hole in
> CVS head: there, freeze.c can dereference random memory as
> a function pointer if given a malicious frozen file.  Patch to follow.

Compiling with warnings on found this on CVS head; branch-1_4 was
immune to these problems.

In file included from modules/mpeval.c:428:
modules/evalparse.c: In function `or_term':
modules/evalparse.c:359: warning: dereferencing type-punned pointer will
break strict-aliasing rules

This might be harmless, but annoying.  GMP defines mpq_t as
a 1-element array of struct, meaning that 'mpq_t*' is not
assignable to 'const mpq_t*' without type-punning.

src/freeze.c: In function `reload_frozen_state':
src/freeze.c:335: warning: 'version' might be used uninitialized in this
function
src/freeze.c:342: warning: 'bp' might be used uninitialized in this function

This is an actual security hole.  Although I did not try to come
up with an exploit, my analysis of the data flow was that it might be
possible to generate a malicious frozen file where the stack can be
manipulated to leave a value in bp, then dereference it to install a
bogus builtin with an arbitrary function pointer, thereby allowing
the exploit to execute arbitrary code.  But even if executing
arbitrary code is not possible, this demonstrates the bug:

$ echo | m4 -F foo.m4f
$ echo F1,1,1 >> foo.m4f
$ echo aaa >> foo.m4f
$ tail -n 5 foo.m4f
F7,7,3
builtinbuiltingnu
# End of frozen state file
F1,1,1
aaa
$ echo "a(\`dumpdef',\`a')" | m4 -R foo.m4f

a:      <builtin>
$

Hmm, I just installed `a' as though it were `builtin', rather than
getting an error message that module a could not be loaded.
Then, if I edit out all previous F commands from foo.m4f, I get:

$ emacs foo.m4f
$ echo "a(\`dumpdef',\`a')" | m4 -R foo.m4f

m4: stdin: 1: Warning: a: too few arguments: 2 < 1931503731

At least I got lucky that on my platform, a random function pointer
wasn't dereferenced, because the random argument limits were
seen.

Meanwhile, we should probably tighten up the parsing so that the
'V' version command in a valid frozen file must be the first
non-comment, and can only appear once, but I did not do that for
this patch.

2006-06-15  Eric Blake  <address@hidden>

        Reduce compiler warnings.  Inside GMP, mpq_t is an array type, so
        const mpq_t is not assignable from plain mpq_t.  Avoid
        type-punning warnings caused trying to mix these types.
        * modules/mpeval.c (numb_ior, numb_eor, numb_and, numb_lshift),
        (numb_rshift, numb_divide, numb_modulo): Remove const qualifier.
        * modules/evalparse.c (or_term, xor_term, and_term, shift_term),
        (mult_term, exp_term): Remove type-punning casts.
        (numb_pow): Remove const qualifier.
        * src/freeze.c (reload_frozen_state): Fix typo in messages.
        Fix variables that can be used uninitialized, which fixes
        security hole where malicious frozen file can execute arbitrary
        code.

Index: modules/evalparse.c
===================================================================
RCS file: /sources/m4/m4/modules/evalparse.c,v
retrieving revision 1.12
diff -u -r1.12 evalparse.c
--- modules/evalparse.c 1 May 2005 11:10:05 -0000       1.12
+++ modules/evalparse.c 15 Jun 2006 21:55:44 -0000
@@ -1,5 +1,5 @@
 /* GNU m4 -- A simple macro processor
-   Copyright (C) 1989, 1990, 1991, 1992, 1993, 1994, 2001
+   Copyright (C) 1989, 1990, 1991, 1992, 1993, 1994, 2001, 2006
    Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
@@ -80,7 +80,7 @@
 static eval_error exp_term         (m4 *, eval_token, number *);
 static eval_error unary_term       (m4 *, eval_token, number *);
 static eval_error simple_term      (m4 *, eval_token, number *);
-static void      numb_pow          (number *x, const number *y);
+static void      numb_pow          (number *x, number *y);
 
 
 
@@ -356,7 +356,7 @@
       if ((er = xor_term (context, et, &v2)) != NO_ERROR)
        return er;
 
-      numb_ior(context, v1, (const number *)&v2);
+      numb_ior(context, v1, &v2);
     }
   numb_fini(v2);
   if (et == ERROR)
@@ -385,7 +385,7 @@
       if ((er = and_term (context, et, &v2)) != NO_ERROR)
        return er;
 
-      numb_eor(context, v1, (const number *)&v2);
+      numb_eor(context, v1, &v2);
     }
   numb_fini(v2);
   if (et == ERROR)
@@ -414,7 +414,7 @@
       if ((er = not_term (context, et, &v2)) != NO_ERROR)
        return er;
 
-      numb_and(context, v1, (const number *)&v2);
+      numb_and(context, v1, &v2);
     }
   numb_fini(v2);
   if (et == ERROR)
@@ -555,11 +555,11 @@
       switch (op)
        {
        case LSHIFT:
-         numb_lshift(context, v1, (const number *)&v2);
+         numb_lshift(context, v1, &v2);
          break;
 
        case RSHIFT:
-         numb_rshift(context, v1, (const number *)&v2);
+         numb_rshift(context, v1, &v2);
          break;
 
        default:
@@ -644,7 +644,7 @@
          if (numb_zerop(v2))
            return DIVIDE_ZERO;
          else {
-           numb_divide(v1, (const number *)&v2);
+           numb_divide(v1, &v2);
          }
          break;
 
@@ -660,7 +660,7 @@
          if (numb_zerop(v2))
            return MODULO_ZERO;
          else {
-           numb_modulo(context, v1, (const number *)&v2);
+           numb_modulo(context, v1, &v2);
          }
          break;
 
@@ -699,7 +699,7 @@
       if ((er = exp_term (context, et, &v2)) != NO_ERROR)
        return er;
 
-      numb_pow(v1, (const number *)&v2);
+      numb_pow(v1, &v2);
     }
   numb_fini(v2);
   if (et == ERROR)
@@ -864,7 +864,7 @@
 }
 
 static void
-numb_pow (number *x, const number *y)
+numb_pow (number *x, number *y)
 {
   /* y should be integral */
 
Index: modules/mpeval.c
===================================================================
RCS file: /sources/m4/m4/modules/mpeval.c,v
retrieving revision 1.16
diff -u -r1.16 mpeval.c
--- modules/mpeval.c    1 May 2005 11:10:05 -0000       1.16
+++ modules/mpeval.c    15 Jun 2006 21:55:44 -0000
@@ -1,5 +1,5 @@
 /* GNU m4 -- A simple macro processor
-   Copyright (C) 2000, 2001 Free Software Foundation, Inc.
+   Copyright (C) 2000, 2001, 2006 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
    it under the terms of the GNU General Public License as published by
@@ -38,7 +38,7 @@
 
                function        macros  blind minargs maxargs */
 #define builtin_functions                                      \
-       BUILTIN(mpeval,         false,  true,   2,      4  )    \
+       BUILTIN(mpeval,         false,  true,   2,      4  )    \
 
 
 
@@ -105,7 +105,8 @@
 };
 
 
-/* number should be at least 32 bits.  */
+/* GMP defines mpq_t as a 1-element array of struct.  Therefore, `mpq_t'
+   is not compatible with `const mpq_t'.  */
 typedef mpq_t number;
 
 static void numb_initialise (void);
@@ -113,14 +114,14 @@
                          const int radix, int min);
 static void mpq2mpz (m4 *context, mpz_t z, const number q, const char 
*noisily);
 static void mpz2mpq (number q, const mpz_t z);
-static void numb_divide (number *x, const number *y);
-static void numb_modulo (m4 *context, number *x, const number *y);
-static void numb_and (m4 *context, number *x, const number *y);
-static void numb_ior (m4 *context, number *x, const number *y);
-static void numb_eor (m4 *context, number *x, const number *y);
+static void numb_divide (number *x, number *y);
+static void numb_modulo (m4 *context, number *x, number *y);
+static void numb_and (m4 *context, number *x, number *y);
+static void numb_ior (m4 *context, number *x, number *y);
+static void numb_eor (m4 *context, number *x, number *y);
 static void numb_not (m4 *context, number *x);
-static void numb_lshift (m4 *context, number *x, const number *y);
-static void numb_rshift (m4 *context, number *x, const number *y);
+static void numb_lshift (m4 *context, number *x, number *y);
+static void numb_rshift (m4 *context, number *x, number *y);
 
 
 static number numb_ZERO;
@@ -200,7 +201,7 @@
 }
 
 static void
-numb_divide (number * x, const number * y)
+numb_divide (number * x, number * y)
 {
   mpq_t qres;
   mpz_t zres;
@@ -217,7 +218,7 @@
 }
 
 static void
-numb_modulo (m4 *context, number * x, const number * y)
+numb_modulo (m4 *context, number * x, number * y)
 {
   mpz_t xx, yy, res;
 
@@ -241,7 +242,7 @@
 }
 
 static void
-numb_and (m4 *context, number * x, const number * y)
+numb_and (m4 *context, number * x, number * y)
 {
   mpz_t xx, yy, res;
 
@@ -265,7 +266,7 @@
 }
 
 static void
-numb_ior (m4 *context, number * x, const number * y)
+numb_ior (m4 *context, number * x, number * y)
 {
   mpz_t xx, yy, res;
 
@@ -289,7 +290,7 @@
 }
 
 static void
-numb_eor (m4 *context, number * x, const number * y)
+numb_eor (m4 *context, number * x, number * y)
 {
   mpz_t xx, yy, res;
 
@@ -355,7 +356,7 @@
 }
 
 static void
-numb_lshift (m4 *context, number * x, const number * y)
+numb_lshift (m4 *context, number * x, number * y)
 {
   mpz_t xx, yy, res;
 
@@ -390,7 +391,7 @@
 }
 
 static void
-numb_rshift (m4 *context, number * x, const number * y)
+numb_rshift (m4 *context, number * x, number * y)
 {
   mpz_t xx, yy, res;
 
Index: src/freeze.c
===================================================================
RCS file: /sources/m4/m4/src/freeze.c,v
retrieving revision 1.41
diff -u -r1.41 freeze.c
--- src/freeze.c        27 Oct 2005 16:04:03 -0000      1.41
+++ src/freeze.c        15 Jun 2006 21:55:44 -0000
@@ -1,5 +1,5 @@
 /* GNU m4 -- A simple macro processor
-   Copyright (C) 1989, 90, 91, 92, 93, 94, 2004, 2005
+   Copyright (C) 1989, 1990, 1991, 1992, 1993, 1994, 2004, 2005, 2006
                  Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify
@@ -332,14 +332,13 @@
 reload_frozen_state (m4 *context, const char *name)
 {
   FILE *file;
-  int version;
+  int version = 0;
   int character;
   int operation;
   char syntax;
   unsigned char *string[3];
   int allocated[3];
   int number[3];
-  const m4_builtin *bp;
 
 #define GET_CHARACTER \
   (character = getc (file))
@@ -362,7 +361,7 @@
       CHECK_ALLOCATION((Buf), (BufSize), (StrLen));            \
       if ((StrLen) > 0)                                                \
        if (!fread ((Buf), (size_t) (StrLen), 1, (File)))       \
-           M4ERROR ((EXIT_FAILURE, 0,                          \
+           M4ERROR ((EXIT_FAILURE, 0,                          \
                       _("Premature end of frozen file")));     \
       (Buf)[(StrLen)] = '\0';                                  \
     }                                                          \
@@ -403,7 +402,7 @@
     switch (character)
       {
       default:
-       M4ERROR ((EXIT_FAILURE, 0, _("Ill-formated frozen file")));
+       M4ERROR ((EXIT_FAILURE, 0, _("Ill-formed frozen file")));
 
       case '\n':
 
@@ -445,7 +444,7 @@
        else
          {
            /* 3 argument 'F' operations are invalid for format version 1.  */
-           M4ERROR ((EXIT_FAILURE, 0, _("Ill-formated frozen file")));
+           M4ERROR ((EXIT_FAILURE, 0, _("Ill-formed frozen file")));
          }
 
        VALIDATE ('\n');
@@ -461,6 +460,7 @@
 
        /* Enter a macro having a builtin function as a definition.  */
        {
+          const m4_builtin *bp = NULL;
          lt_dlhandle handle   = 0;
 
          if (number[2] > 0)
@@ -502,7 +502,7 @@
        if (version < 2)
          {
            /* 'M' operator is not supported in format version 1. */
-           M4ERROR ((EXIT_FAILURE, 0, _("Ill-formated frozen file")));
+           M4ERROR ((EXIT_FAILURE, 0, _("Ill-formed frozen file")));
          }
 
        GET_CHARACTER;
@@ -520,7 +520,7 @@
        if (version < 2)
          {
            /* 'S' operator is not supported in format version 1. */
-           M4ERROR ((EXIT_FAILURE, 0, _("Ill-formated frozen file")));
+           M4ERROR ((EXIT_FAILURE, 0, _("Ill-formed frozen file")));
          }
 
        GET_CHARACTER;
@@ -643,7 +643,7 @@
        else
          {
            /* 3 argument 'T' operations are invalid for format version 1.  */
-           M4ERROR ((EXIT_FAILURE, 0, _("Ill-formated frozen file")));
+           M4ERROR ((EXIT_FAILURE, 0, _("Ill-formed frozen file")));
          }
 
        VALIDATE ('\n');