vasnprintf's "%n in writable segment" chokes with _FORTIFY_SOURCE == 2

[Jim Meyering]

Hi Bruno,

When I build a coreutils snapshot with -D_FORTIFY_SOURCE=2 on a
relatively recent fedora-based system, seq always aborts like this:

    $ ./seq 1
    *** %n in writable segment detected ***
    1zsh: abort      ./seq 1
    [Exit 134 (ABRT)]

That is due to the fact that vasnprintf writes %n into a format
string that is subsequently used by snprintf.
And why is that done?  For portability to crufty systems on which
s*printf doesn't even return a valid count.

But the particular bug that code is working around doesn't
even affect glibc's snprintf function.

This is why gnulib should be written to rely on posix- (or c99-)
compliant functions whenever possible: so that conforming systems
aren't penalized.  I haven't looked closely enough, but perhaps
vasnprintf itself could require snprintf-posix?  It looks like
that might induce a dependency loop, though, since the snprintf-posix
module uses vasnprintf.c.

Here's a minimal change to avoid using %n in this specific case.
It works because there is already code to handle the case in which
there is no %n directive, i.e., when fbp[1] == '\0'.

Another approach is to avoid using %n with any glibc-based system.

diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index f563823..9692562 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -3384,7 +3384,7 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
  else
 #endif
   *fbp = dp->conversion;
-#if USE_SNPRINTF
+#if USE_SNPRINTF && _FORTIFY_SOURCE != 2
  fbp[1] = '%';
  fbp[2] = 'n';
  fbp[3] = '\0';

[Bruno Haible]

Jim Meyering wrote:

> When I build a coreutils snapshot with -D_FORTIFY_SOURCE=2 on a
> relatively recent fedora-based system, seq always aborts like this:
>
>     $ ./seq 1
>     *** %n in writable segment detected ***
>     1zsh: abort      ./seq 1
>     [Exit 134 (ABRT)]
>
> That is due to the fact that vasnprintf writes %n into a format
> string that is subsequently used by snprintf.

Use of %n is valid in ISO C99 and in POSIX.

At least Microsoft's C99 violation [1] [2] returns without processing the
%n. But letting a program crash for the attempt to use a C99 feature is
gross.

I haven't seen a valid rationale for this. Not on Microsoft's site.
[3] is vague. In [4] the real problem is the use of a user-provided string
as format string.

Can you ask these "Fortify" advocates for some rationale? Cutting down
innocent features without properly thought-out solution isn't going to win.

> This is why gnulib should be written to rely on posix- (or c99-)
> compliant functions whenever possible: so that conforming systems
> aren't penalized.

A system that ignores %n or even crashes upon it is not POSIX or C99
compliant.

Bruno

[1] http://lists.gnu.org/archive/html/bug-gnulib/2007-06/msg00048.html
[2] http://gcc.gnu.org/ml/gcc/2007-06/msg00122.html
[3] http://gcc.gnu.org/ml/gcc/2007-06/msg00145.html
[4] http://seclists.org/bugtraq/1999/Sep/0328.html

[Bruno Haible]

Jim Meyering wrote:
>     $ ./seq 1
>     *** %n in writable segment detected ***

The use of format strings in writable memory is valid. A use case is, for
example, in
    printf (gettext ("foo %d bar %x %n"), ...);

When the translator's and the user's charset encoding are not the same,
or when mmap() is not available, the format string returned by the gettext
function will be in writable memory.

The distinction between read-only and writable memory is something I had not
thought of in m4/printf.m4. Making the test more accurate like this.


2007-10-18  Bruno Haible  <bruno@...>

        * m4/printf.m4 (gl_PRINTF_DIRECTIVE_N, gl_SNPRINTF_DIRECTIVE_N): Put
        the format string into writable memory. Needed in Fortify conditions.

*** m4/printf.m4.orig 2007-10-18 13:10:26.000000000 +0200
--- m4/printf.m4 2007-10-18 13:05:44.000000000 +0200
***************
*** 1,4 ****
! # printf.m4 serial 16
  dnl Copyright (C) 2003, 2007 Free Software Foundation, Inc.
  dnl This file is free software; the Free Software Foundation
  dnl gives unlimited permission to copy and/or distribute it,
--- 1,4 ----
! # printf.m4 serial 17
  dnl Copyright (C) 2003, 2007 Free Software Foundation, Inc.
  dnl This file is free software; the Free Software Foundation
  dnl gives unlimited permission to copy and/or distribute it,
***************
*** 585,595 ****
        AC_TRY_RUN([
  #include <stdio.h>
  #include <string.h>
  static char buf[100];
  int main ()
  {
    int count = -1;
!   if (sprintf (buf, "%d %n", 123, &count, 33, 44, 55) < 0
        || strcmp (buf, "123 ") != 0
        || count != 4)
      return 1;
--- 585,600 ----
        AC_TRY_RUN([
  #include <stdio.h>
  #include <string.h>
+ static char fmtstring[10];
  static char buf[100];
  int main ()
  {
    int count = -1;
!   /* Copy the format string.  Some systems (glibc with _FORTIFY_SOURCE=2)
!      support %n in format strings in read-only memory but not in writable
!      memory.  */
!   strcpy (fmtstring, "%d %n");
!   if (sprintf (buf, fmtstring, 123, &count, 33, 44, 55) < 0
        || strcmp (buf, "123 ") != 0
        || count != 4)
      return 1;
***************
*** 872,882 ****
        AC_TRY_RUN([
  #include <stdio.h>
  #include <string.h>
  static char buf[100];
  int main ()
  {
    int count = -1;
!   snprintf (buf, 4, "%d %n", 12345, &count, 33, 44, 55);
    if (count != 6)
      return 1;
    return 0;
--- 877,892 ----
        AC_TRY_RUN([
  #include <stdio.h>
  #include <string.h>
+ static char fmtstring[10];
  static char buf[100];
  int main ()
  {
    int count = -1;
!   /* Copy the format string.  Some systems (glibc with _FORTIFY_SOURCE=2)
!      support %n in format strings in read-only memory but not in writable
!      memory.  */
!   strcpy (fmtstring, "%d %n");
!   snprintf (buf, 4, fmtstring, 12345, &count, 33, 44, 55);
    if (count != 6)
      return 1;
    return 0;

[Jim Meyering]

Bruno Haible <bruno@...> wrote:
> Jim Meyering wrote:
>>     $ ./seq 1
>>     *** %n in writable segment detected ***
>
> The use of format strings in writable memory is valid.

Of course it is valid.  No one questions that.
But disallowing %n in a writable format string does
protect applications from an entire class of exploits.
That is worth more than enough to compensate for the minor limitation.

> A use case is, for example, in
>     printf (gettext ("foo %d bar %x %n"), ...);
>
> When the translator's and the user's charset encoding are not the same,
> or when mmap() is not available, the format string returned by the gettext
> function will be in writable memory.
>
> The distinction between read-only and writable memory is something I had not
> thought of in m4/printf.m4. Making the test more accurate like this.
>
> 2007-10-18  Bruno Haible  <bruno@...>
>
> * m4/printf.m4 (gl_PRINTF_DIRECTIVE_N, gl_SNPRINTF_DIRECTIVE_N): Put
> the format string into writable memory. Needed in Fortify conditions.

Thanks, that is an improvement, but as you know, it doesn't solve
the problem with seq segfaulting.

IMHO, glibc is important enough that gnulib should cater to it,
or at least not penalize it, regardless of whether you find this
particular aspect of _FORTIFY_SOURCE worthwhile.

For now, I'll stick with my patch.

BTW, this problem was also encountered last year by CVS developers.

[Jim Meyering]

Bruno Haible <bruno@...> wrote:

> Jim Meyering wrote:
>> When I build a coreutils snapshot with -D_FORTIFY_SOURCE=2 on a
>> relatively recent fedora-based system, seq always aborts like this:
>>
>>     $ ./seq 1
>>     *** %n in writable segment detected ***
>>     1zsh: abort      ./seq 1
>>     [Exit 134 (ABRT)]
>>
>> That is due to the fact that vasnprintf writes %n into a format
>> string that is subsequently used by snprintf.
>
> Use of %n is valid in ISO C99 and in POSIX.
>
> At least Microsoft's C99 violation [1] [2] returns without processing the
> %n. But letting a program crash for the attempt to use a C99 feature is
> gross.
>
> I haven't seen a valid rationale for this. Not on Microsoft's site.
> [3] is vague. In [4] the real problem is the use of a user-provided string
> as format string.
>
> Can you ask these "Fortify" advocates for some rationale? Cutting down
> innocent features without properly thought-out solution isn't going to win.

%n is most definitely not "innocent".
It is a commonly-exploited feature.

>> This is why gnulib should be written to rely on posix- (or c99-)
>> compliant functions whenever possible: so that conforming systems
>> aren't penalized.
>
> A system that ignores %n or even crashes upon it is not POSIX or C99
> compliant.

That is not relevant to my point.
The fact is that the current implementation in vasnprintf.c
penalizes *all* systems for the sake of the few with snprintf
that don't return a valid count.  Plus, that code is full of
#ifdefs, many of which would disappear if it relied on a
posix-conforming snprintf.

[Jim Meyering]

Bruno Haible <bruno@...> wrote:

> Jim Meyering wrote:
>> When I build a coreutils snapshot with -D_FORTIFY_SOURCE=2 on a
>> relatively recent fedora-based system, seq always aborts like this:
>>
>>     $ ./seq 1
>>     *** %n in writable segment detected ***
>>     1zsh: abort      ./seq 1
>>     [Exit 134 (ABRT)]
>>
>> That is due to the fact that vasnprintf writes %n into a format
>> string that is subsequently used by snprintf.
>
> Use of %n is valid in ISO C99 and in POSIX.

As you know I don't dispute its validity.
However, vasnprintf.c should resort to incurring the
expense of that portability kludge only when necessary.
How about if it does that only when a configure-time test
has detected that snprintf's return value is not conforming?

Then, vasnprintf.c will be more efficient, and won't use the offending %n,
on the vast majority of systems that have an snprintf implementation
with a conforming return value.

[Paul Eggert]

Jim Meyering <jim@...> writes:

> vasnprintf.c should resort to incurring the
> expense of that portability kludge only when necessary.
> How about if it does that only when a configure-time test
> has detected that snprintf's return value is not conforming?

That sounds like a good idea.

How common is it for snprintf to return the wrong value?  If it's not
that common, perhaps we can just not worry about those systems at all.

I can't resist mentioning a more-fun workaround.  We can put a
snprintf wrapper into gnulib that copies the format string into a
buffer, uses mmap to make the buffer read-only, and then calls the
real snprintf.  Sort of an "anti-Fortify" wrapper, if you will.

[Bruno Haible]

Jim Meyering wrote:
> The fact is that the current implementation in vasnprintf.c
> penalizes *all* systems for the sake of the few with snprintf
> that don't return a valid count.

It has a few more instructions than needed, for portability. When cross-
compiling, the gl_SNPRINTF_DIRECTIVE_N autoconf test can guess wrong.
But in the case you mention (glibc-2.3.4 or newer), we know that snprintf's
return value is usable. We have no other choice than to work around this broken
glibc behaviour:

2007-10-18  Bruno Haible  <bruno@...>

        * m4/vasnprintf.m4 (VASNPRINTF): Don't use %n on glibc >= 2.3 systems.
        Reported by Jim Meyering.

*** lib/vasnprintf.c.orig 2007-10-19 01:49:53.000000000 +0200
--- lib/vasnprintf.c 2007-10-19 01:47:50.000000000 +0200
***************
*** 3385,3393 ****
--- 3385,3405 ----
  #endif
   *fbp = dp->conversion;
  #if USE_SNPRINTF
+ # if !(__GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 3))
  fbp[1] = '%';
  fbp[2] = 'n';
  fbp[3] = '\0';
+ # else
+ /* On glibc2 systems from glibc >= 2.3 - probably also older
+   ones - we know that snprintf's returns value conforms to
+   ISO C 99: the gl_SNPRINTF_DIRECTIVE_N test passes.
+   Therefore we can avoid using %n in this situation.
+   On glibc2 systems from 2004-10-18 or newer, the use of %n
+   in format strings in writable memory may crash the program
+   (if compiled with _FORTIFY_SOURCE=2), so we should avoid it
+   in this situation.  */
+ fbp[1] = '\0';
+ # endif
  #else
  fbp[1] = '\0';
  #endif

[Bruno Haible]

Jim Meyering wrote:
> But disallowing %n in a writable format string does
> protect applications from an entire class of exploits.
> That is worth more than enough to compensate for the minor limitation.

Two remarks:

* The %n has to serve as a scapegoat here. The exploit in [1] is a
  combination of
    1. a runtime system that allows modifications of arbitrary memory
       locations without the concept of compartments inside the memory
       of a process (C combined with the Unix memory model),
    2. a user-provided string that is used as a format string,
    3. a format directive that causes a write into memory.

  #1 is the real root of so many security issues, but its solution is
  out of scope here.

  #2 is the cause of this particular issue. #3 is not an issue by itself.

  So why don't people think more about how to fix #2?

2) Does it have to be done through abort()? Can't it be silent like on
   Windows Vista? IMO, library functions should not crash a program when
   the input is standards-compliant.

> BTW, this problem was also encountered last year by CVS developers.

I must have missed that, sorry.

Bruno

[1] http://seclists.org/bugtraq/1999/Sep/0328.html

[Jim Meyering]

Bruno Haible <bruno@...> wrote:
> Jim Meyering wrote:
>> But disallowing %n in a writable format string does
>> protect applications from an entire class of exploits.
>> That is worth more than enough to compensate for the minor limitation.
>
> Two remarks:
>
> * The %n has to serve as a scapegoat here. The exploit in [1] is a
>   combination of

%n is not a scapegoat at all.
It is the key that gives format-abusers so much latitude
in choosing what value to write where.  Without that
feature, coming up with a real exploit is much harder.

>     1. a runtime system that allows modifications of arbitrary memory
>        locations without the concept of compartments inside the memory
>        of a process (C combined with the Unix memory model),
>     2. a user-provided string that is used as a format string,
>     3. a format directive that causes a write into memory.
>
>   #1 is the real root of so many security issues, but its solution is
>   out of scope here.
>
>   #2 is the cause of this particular issue. #3 is not an issue by itself.
>
>   So why don't people think more about how to fix #2?

Because removing such vulnerabilities at the source is much more
labor-intensive.  Think of -D_FORTIFY_SOURCE=2 as a _global_ stop-gap
compromise while we wait (maybe forever :-) for all application code to
be cleaned up.

Limiting the use of %n in this small way is a good _compromise_.
No one forces the average developer to use -D_FORTIFY_SOURCE=2,
but those who deem it useful should be able to.

> 2) Does it have to be done through abort()? Can't it be silent like on
>    Windows Vista? IMO, library functions should not crash a program when
>    the input is standards-compliant.

I think that using abort is perfect.
Anyone who is picky enough about standards-compliance -- and
willing to sacrifice some added security -- can simply refrain
from using -D_FORTIFY_SOURCE=2.

People have made the same "it shouldn't abort" argument about glibc's
abort-on-double-free behavior.  %n-in-writable-segment is less grave,
(and hence calling abort is not the default) but still worth addressing.

[Jim Meyering]

Bruno Haible <bruno@...> wrote:
>
> * m4/vasnprintf.m4 (VASNPRINTF): Don't use %n on glibc >= 2.3 systems.
> Reported by Jim Meyering.
>
> *** lib/vasnprintf.c.orig 2007-10-19 01:49:53.000000000 +0200

Thanks for fixing it.  I've corrected the ChangeLog.

[Bruno Haible]

Jim Meyering wrote:
> It is the key that gives format-abusers so much latitude
> in choosing what value to write where.  Without that
> feature, coming up with a real exploit is much harder.

Without %n, one can still use format strings like
  %.10000000f%.10000000f%.10000000f%.10000000f%.10000000f%.10000000f
to conduct denial-of-service attacks.

> >     2. a user-provided string that is used as a format string,
> >   So why don't people think more about how to fix #2?
>
> Because removing such vulnerabilities at the source is much more
> labor-intensive.

It doesn't need to be at the source level. If, for example,
gcc was changed to emit a certain signature (4 bytes) in front of every
read-only format string, and vfprintf would verify this signature by
looking at fmt[-4..-1], then only the few applications which willfully
construct format strings at runtime would have to change their source code.

Bruno

[Jim Meyering]

Bruno Haible <bruno@...> wrote:
> Jim Meyering wrote:
>> It is the key that gives format-abusers so much latitude
>> in choosing what value to write where.  Without that
>> feature, coming up with a real exploit is much harder.
>
> Without %n, one can still use format strings like
>   %.10000000f%.10000000f%.10000000f%.10000000f%.10000000f%.10000000f
> to conduct denial-of-service attacks.

Yes, it'd be great if all exploits resulted only in a DoS.
But limiting use of %n makes it much harder to construct more
serious exploits e.g., resulting in arbitrary code execution.