bug#10591: Bug#655118: Please enabled hardened build flags

bug#10591: Bug#655118: Please enabled hardened build flags

[Rob Browning]

> Alexandru Cardaniuc <cardaniuc@gmail.com> writes:



Moritz Mühlenhoff <jmm@inutil.org> writes:

> On Sun, Jan 08, 2012 at 06:13:59PM +0100, Moritz Muehlenhoff wrote:
>> Package: emacs23
>> Version: 23.3+1-4
>> Severity: important
>> Tags: patch
>> 
>> Hi Rob,
>> Please enabled hardened build flags through dpkg-buildflags.
>> 
>> Patch attached. (dpkg-buildflags abides "noopt" from DEB_BUILD_OPTIONS)
>
> I forgot to add that "-Wformat -Wformat-security -Werror=format-security"
> exposed missing format strings in movemail, for which I attach a patch.
>
> Cheers,
>         Moritz
>

-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

bug#10592: Bug#655118: Please enabled hardened build flags

[Rob Browning]

(Sorry, accidentally hit send too early.)

Mortiz updated Emacs to support -Wformat -Wformat-security
-Werror=format-security.

Here are the relevant changes (further details are available at
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=655118):

diff -aur emacs23-23.3+1.orig/lib-src/movemail.c emacs23-23.3+1/lib-src/movemail.c
--- emacs23-23.3+1.orig/lib-src/movemail.c	2011-12-29 05:07:27.000000000 +0100
+++ emacs23-23.3+1/lib-src/movemail.c	2012-01-08 17:31:22.000000000 +0100
@@ -615,11 +615,11 @@
 {
   fprintf (stderr, "movemail: ");
   if (s3)
-    fprintf (stderr, s1, s2, s3);
+    fprintf (stderr, "%s%s%s", s1, s2, s3);
   else if (s2)
-    fprintf (stderr, s1, s2);
+    fprintf (stderr, "%s%s", s1, s2);
   else
-    fprintf (stderr, s1);
+    fprintf (stderr, "%s", s1);
   fprintf (stderr, "\n");
 }
 
Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

bug#10592: Bug#655118: Please enabled hardened build flags

[Eli Zaretskii]

> From: Rob Browning <rlb@defaultvalue.org>
> Date: Mon, 23 Jan 2012 23:05:26 -0600
> Cc: 655118@bugs.debian.org, 655118-forwarded@bugs.debian.org,
> 	Moritz Mühlenhoff <jmm@inutil.org>
> 
> --- emacs23-23.3+1.orig/lib-src/movemail.c	2011-12-29 05:07:27.000000000 +0100
> +++ emacs23-23.3+1/lib-src/movemail.c	2012-01-08 17:31:22.000000000 +0100
> @@ -615,11 +615,11 @@
>  {
>    fprintf (stderr, "movemail: ");
>    if (s3)
> -    fprintf (stderr, s1, s2, s3);
> +    fprintf (stderr, "%s%s%s", s1, s2, s3);
>    else if (s2)
> -    fprintf (stderr, s1, s2);
> +    fprintf (stderr, "%s%s", s1, s2);
>    else
> -    fprintf (stderr, s1);
> +    fprintf (stderr, "%s", s1);
>    fprintf (stderr, "\n");
>  }

How can this possibly be TRT?  The commentary to this function says:

  /* Print error message.  `s1' is printf control string, `s2' and `s3'
     are args for it or null. */

If S1 is the printf control string, how will printing it with %s DTRT?
E.g., in this invocation:

      error ("Error connecting to POP server: %s", pop_error, 0);

or in this one:

      error ("Error in open: %s, %s", strerror (errno), outfile);

I think the right fix for this is to declare `error' with the
appropriate printf attribute.  Alternatively, you could use variable
argument lists and call vprintf instead.

bug#10592: Bug#655118: Please enabled hardened build flags

[Rob Browning]

Eli Zaretskii <eliz@gnu.org> writes:

> How can this possibly be TRT?  The commentary to this function says:

I actually wondered the same thing while I was going to sleep.
Obviously I got in too big a hurry last night.  I'll fix it correctly
myself and re-send.

Thanks, and apologies for the noise.
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

bug#10592: Bug#655118: Please enabled hardened build flags

[Rob Browning]

Eli Zaretskii <eliz@gnu.org> writes:

> I think the right fix for this is to declare `error' with the
> appropriate printf attribute.  Alternatively, you could use variable
> argument lists and call vprintf instead.

Would something like this be acceptable, and if not, how would you like
to see it adjusted?  The patch changes error() to use an ANSI
declaration, and it relies on the printf format attribute:

diff --git a/lib-src/movemail.c b/lib-src/movemail.c
index 58add49..6b2fc20 100644
--- a/lib-src/movemail.c
+++ b/lib-src/movemail.c
@@ -60,6 +60,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <sys/file.h>
 #include <stdio.h>
 #include <errno.h>
+#include <stdarg.h>
 #include <time.h>
 
 #include <getopt.h>
@@ -152,7 +153,7 @@ extern char *rindex __P((const char *, int));
 #endif
 
 void fatal ();
-void error ();
+void error (const char *template, ...) __attribute__ ((format (printf, 1, 2)));
 void pfatal_with_name ();
 void pfatal_and_delete ();
 char *concat ();
@@ -610,16 +611,13 @@ fatal (s1, s2, s3)
    are args for it or null. */
 
 void
-error (s1, s2, s3)
-     char *s1, *s2, *s3;
+error (const char *template, ...)
 {
+  va_list ap;
   fprintf (stderr, "movemail: ");
-  if (s3)
-    fprintf (stderr, s1, s2, s3);
-  else if (s2)
-    fprintf (stderr, s1, s2);
-  else
-    fprintf (stderr, s1);
+  va_start (ap, template);
+  vfprintf (stderr, template, ap);
+  va_end (ap);
   fprintf (stderr, "\n");
 }
 
@@ -733,13 +731,13 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
   server = pop_open (hostname, user, password, POP_NO_GETPASS);
   if (! server)
     {
-      error ("Error connecting to POP server: %s", pop_error, 0);
+      error ("Error connecting to POP server: %s", pop_error);
       return EXIT_FAILURE;
     }
 
   if (pop_stat (server, &nmsgs, &nbytes))
     {
-      error ("Error getting message count from POP server: %s", pop_error, 0);
+      error ("Error getting message count from POP server: %s", pop_error);
       return EXIT_FAILURE;
     }
 
@@ -761,7 +759,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
   if ((mbf = fdopen (mbfi, "wb")) == NULL)
     {
       pop_close (server);
-      error ("Error in fdopen: %s", strerror (errno), 0);
+      error ("Error in fdopen: %s", strerror (errno));
       close (mbfi);
       unlink (outfile);
       return EXIT_FAILURE;
@@ -785,7 +783,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
       mbx_delimit_begin (mbf);
       if (pop_retr (server, i, mbf) != OK)
 	{
-	  error ("%s", Errmsg, 0);
+	  error ("%s", Errmsg);
 	  close (mbfi);
 	  return EXIT_FAILURE;
 	}
@@ -793,7 +791,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
       fflush (mbf);
       if (ferror (mbf))
 	{
-	  error ("Error in fflush: %s", strerror (errno), 0);
+	  error ("Error in fflush: %s", strerror (errno));
 	  pop_close (server);
 	  close (mbfi);
 	  return EXIT_FAILURE;
@@ -809,14 +807,14 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
 #ifdef BSD_SYSTEM
   if (fsync (mbfi) < 0)
     {
-      error ("Error in fsync: %s", strerror (errno), 0);
+      error ("Error in fsync: %s", strerror (errno));
       return EXIT_FAILURE;
     }
 #endif
 
   if (close (mbfi) == -1)
     {
-      error ("Error in close: %s", strerror (errno), 0);
+      error ("Error in close: %s", strerror (errno));
       return EXIT_FAILURE;
     }
 
@@ -825,7 +823,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
       {
 	if (pop_delete (server, i))
 	  {
-	    error ("Error from POP server: %s", pop_error, 0);
+	    error ("Error from POP server: %s", pop_error);
 	    pop_close (server);
 	    return EXIT_FAILURE;
 	  }
@@ -833,7 +831,7 @@ popmail (mailbox, outfile, preserve, password, reverse_order)
 
   if (pop_quit (server))
     {
-      error ("Error from POP server: %s", pop_error, 0);
+      error ("Error from POP server: %s", pop_error);
       return EXIT_FAILURE;
     }
 
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

bug#10592: Bug#655118: Please enabled hardened build flags

[Eli Zaretskii]

> From: Rob Browning <rlb@defaultvalue.org>
> Cc: 10592@debbugs.gnu.org,  655118@bugs.debian.org,  655118-forwarded@bugs.debian.org,  jmm@inutil.org
> Date: Tue, 24 Jan 2012 20:22:52 -0600
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think the right fix for this is to declare `error' with the
> > appropriate printf attribute.  Alternatively, you could use variable
> > argument lists and call vprintf instead.
> 
> Would something like this be acceptable, and if not, how would you like
> to see it adjusted?  The patch changes error() to use an ANSI
> declaration, and it relies on the printf format attribute:

This is fine with me, but please use ATTRIBUTE_FORMAT_PRINTF (defined
in src/config.h) instead of a literal __attribute__(...), which is a
GCC-only thing.

Thanks.

bug#10592: Bug#655118: Please enabled hardened build flags

[Rob Browning]

Eli Zaretskii <eliz@gnu.org> writes:

> This is fine with me, but please use ATTRIBUTE_FORMAT_PRINTF (defined
> in src/config.h) instead of a literal __attribute__(...), which is a
> GCC-only thing.

Will do.

Thanks
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

bug#10592: Bug#655118: Please enabled hardened build flags

[Rob Browning]

Rob Browning <rlb@defaultvalue.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> This is fine with me, but please use ATTRIBUTE_FORMAT_PRINTF (defined
>> in src/config.h) instead of a literal __attribute__(...), which is a
>> GCC-only thing.
>
> Will do.

OK, that appears to be newer than 23.3, so I'll probably stick with the
gcc-only changes for now (in Debian).

Thanks again.
-- 
Rob Browning
rlb @defaultvalue.org and @debian.org
GPG as of 2002-11-03 14DD 432F AE39 534D B592 F9A0 25C8 D377 8C7E 73A4

bug#10592: Bug#655118: Please enabled hardened build flags

[Paul Eggert]

I am not observing this problem in the Emacs trunk, with either
GCC 4.6.2 or GCC 4.7.0 20120127 (experimental), when I compile
with -Wformat -Wformat-security.  I suspect the problem has
already been fixed in the trunk in a different way, by
rewriting movemail to use prototypes.  I'm therefore taking
the liberty of marking this bug as fixed in the Emacs bug
database; please feel free to reopen it if I've misunderstood
the situation.