About *printf %n fortifications

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

About *printf %n fortifications

Gwenole Beauchesne
Hi,

Why a printf() with %n in the format string would require this string to
be non-writable? (debug/tst-chk1.c, stdio-common/vfprintf.c)

See the attached test case (-O2 -D_FORTIFY_SOURCE=2)
  char fmt[] = "%s%n\n";
  printf(fmt, "bar", &count);
looks valid to me, but causes an abort() with
*** %n in writable segment detected ***

The check probably meant to be against the %n argument itself.

The following patch fixes this but I have not updated tst-chk1.c yet.
WDYT?

2006-02-24  Gwenole Beauchesne  <[hidden email]>

        * stdio-common/vfprintf.c (vfprintf): Fix fortify checks for %n
        specifier.

--- glibc-2.3.6/stdio-common/vfprintf.c.vfprintf-fortify-form-number 2004-11-25 17:40:23.000000000 +0100
+++ glibc-2.3.6/stdio-common/vfprintf.c 2006-02-24 16:46:57.000000000 +0100
@@ -882,44 +882,46 @@ vfprintf (FILE *s, const CHAR_T *format,
       /* NOTREACHED */      \
       \
     LABEL (form_number):      \
-      if (s->_flags2 & _IO_FLAGS2_FORTIFY)      \
- {      \
-  if (! readonly_format)      \
-    {      \
-      extern int __readonly_area (const void *, size_t)      \
- attribute_hidden;      \
-      readonly_format      \
- = __readonly_area (format, ((STR_LEN (format) + 1)      \
-    * sizeof (CHAR_T)));      \
-    }      \
-  if (readonly_format < 0)      \
-    __libc_fatal ("*** %n in writable segment detected ***\n");      \
- }      \
-      /* Answer the count of characters written.  */      \
-      if (fspec == NULL)      \
- {      \
-  if (is_longlong)      \
-    *(long long int *) va_arg (ap, void *) = done;      \
-  else if (is_long_num)      \
-    *(long int *) va_arg (ap, void *) = done;      \
-  else if (is_char)      \
-    *(char *) va_arg (ap, void *) = done;      \
-  else if (!is_short)      \
-    *(int *) va_arg (ap, void *) = done;      \
-  else      \
-    *(short int *) va_arg (ap, void *) = done;      \
- }      \
-      else      \
+      /* Pointer to signed integer.  */      \
+      {      \
+ const void *ptr;      \
+ if (fspec == NULL)      \
+  ptr = va_arg (ap, void *);      \
+ else      \
+  ptr = args_value[fspec->data_arg].pa_pointer;      \
+      \
+ if (s->_flags2 & _IO_FLAGS2_FORTIFY)      \
+  {      \
+    extern int __readonly_area (const void *, size_t)      \
+      attribute_hidden;            \
+      \
+    int objsize;      \
+    if (is_longlong)      \
+      objsize = sizeof(long long int);      \
+    else if (is_long_num)      \
+      objsize = sizeof(long int);      \
+    else if (is_char)      \
+      objsize = 1;      \
+    else if (!is_short)      \
+      objsize = sizeof(int);      \
+    else      \
+      objsize = sizeof(short);      \
+    if (__readonly_area (ptr, objsize) > 0)      \
+      __libc_fatal ("*** %n into read-only segment detected ***\n");  \
+  }      \
+      \
+ /* Answer the count of characters written.  */      \
  if (is_longlong)      \
-  *(long long int *) args_value[fspec->data_arg].pa_pointer = done;   \
+  *(long long int *) ptr = done;      \
  else if (is_long_num)      \
-  *(long int *) args_value[fspec->data_arg].pa_pointer = done;      \
+  *(long int *) ptr = done;      \
  else if (is_char)      \
-  *(char *) args_value[fspec->data_arg].pa_pointer = done;      \
+  *(char *) ptr = done;      \
  else if (!is_short)      \
-  *(int *) args_value[fspec->data_arg].pa_pointer = done;      \
+  *(int *) ptr = done;      \
  else      \
-  *(short int *) args_value[fspec->data_arg].pa_pointer = done;      \
+  *(short int *) ptr = done;      \
+      }      \
       break;      \
       \
     LABEL (form_strerror):      \

tst-printf-fortify.c (510 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: About *printf %n fortifications

Daniel Jacobowitz-2
On Fri, Feb 24, 2006 at 07:04:08PM +0100, Gwenole Beauchesne wrote:

> Hi,
>
> Why a printf() with %n in the format string would require this string to
> be non-writable? (debug/tst-chk1.c, stdio-common/vfprintf.c)
>
> See the attached test case (-O2 -D_FORTIFY_SOURCE=2)
>   char fmt[] = "%s%n\n";
>   printf(fmt, "bar", &count);
> looks valid to me, but causes an abort() with
> *** %n in writable segment detected ***
>
> The check probably meant to be against the %n argument itself.
>
> The following patch fixes this but I have not updated tst-chk1.c yet.
> WDYT?

No, that's not the point.  It doesn't matter whether the target of the
%n is writable; if it's not, we'll just segfault.  The test is supposed
to prevent a malicious attacker inserting %n into the application
somewhere where it will be passed to printf, causing an unexpected
store.

Of course your testcase is valid - but it's a bad idea.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

Re: About *printf %n fortifications

Gwenole Beauchesne
Le vendredi, 24 fév 2006, à 19:30 Europe/Paris, Daniel Jacobowitz a
écrit :

> Of course your testcase is valid - but it's a bad idea.

OK thanks, this makes sense but then gnulib's vasnprintf()
implementation needs to be fixed (when compiled with USE_SNPRINTF) or
cvs (e.g. 1.12.13) should stop using this function. This is out of
glibc's scope though.
Reply | Threaded
Open this post in threaded view
|

Re: About *printf %n fortifications

Jakub Jelinek
In reply to this post by Daniel Jacobowitz-2
On Fri, Feb 24, 2006 at 01:30:36PM -0500, Daniel Jacobowitz wrote:

> On Fri, Feb 24, 2006 at 07:04:08PM +0100, Gwenole Beauchesne wrote:
> > Hi,
> >
> > Why a printf() with %n in the format string would require this string to
> > be non-writable? (debug/tst-chk1.c, stdio-common/vfprintf.c)
> >
> > See the attached test case (-O2 -D_FORTIFY_SOURCE=2)
> >   char fmt[] = "%s%n\n";
> >   printf(fmt, "bar", &count);
> > looks valid to me, but causes an abort() with
> > *** %n in writable segment detected ***
> >
> > The check probably meant to be against the %n argument itself.
> >
> > The following patch fixes this but I have not updated tst-chk1.c yet.
> > WDYT?
>
> No, that's not the point.  It doesn't matter whether the target of the
> %n is writable; if it's not, we'll just segfault.  The test is supposed
> to prevent a malicious attacker inserting %n into the application
> somewhere where it will be passed to printf, causing an unexpected
> store.
>
> Of course your testcase is valid - but it's a bad idea.

Actually, the test is invalid with -D_FORTIFY_SOURCE=2.
-D_FORTIFY_SOURCE=1 are checks which will just prevent some programs
violating standards or triggering undefined behavior from doing bad things.
-D_FORTIFY_SOURCE=2 actually imposes further restrictions beyond
the standards.  One of the restrictions is that %n is only permitted
in read-only strings (i.e. string literals or even gettext returned
strings).  It is very rare that you need a writable format string
with %n, and on the other side it is quite common exploit technique.
Another -D_FORTIFY_SOURCE=2 limitation is that you can't use certain
str* functions accross structure field boundaries.
E.g. while
struct { char buf[10]; char buf2[10]; } a;
strcpy (a.buf, "abcdefghijklmn");
is valid program and works with -D_FORTIFY_SOURCE=1, it is invalid
under -D_FORTIFY_SOURCE=2.
memcpy (a.buf, "abcdefghijklmn", 15);
is valid in all modes.

        Jakub