This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] vfprintf: validate nargs and maybe allocate from heap


On 02/16/2012 05:16 PM, Kees Cook wrote:
The nargs value can overflow when doing allocations, allowing arbitrary
memory writes via format strings, bypassing _FORTIFY_SOURCE:
http://www.phrack.org/issues.html?issue=67&id=9

So a security issue - can we get this fixed quickly, please? I'd like to ping for a review and commit!



Kees, thanks for the patch.


> [...]
diff --git a/stdio-common/vfprintf.c b/stdio-common/vfprintf.c
index 863cd5d..022e72b 100644
--- a/stdio-common/vfprintf.c
+++ b/stdio-common/vfprintf.c
> [...]
@@ -1698,13 +1702,33 @@ do_positional:

      /* Determine the number of arguments the format string consumes.  */
      nargs = MAX (nargs, max_ref_arg);
+    bytes_per_arg = sizeof (*args_value) + sizeof (*args_size)
+                    + sizeof (*args_type);
+
+    /* Check for potential integer overflow.  */
+    if (nargs>  SIZE_MAX / bytes_per_arg)
+      {
+         done = -1;
+         goto all_done;
+      }

      /* Allocate memory for the argument descriptions.  */
-    args_type = alloca (nargs * sizeof (int));
+    if (__libc_use_alloca (nargs * bytes_per_arg))
+        args_value = alloca (nargs * bytes_per_arg);
+    else
+      {
+        args_value = args_malloced = malloc (nargs * bytes_per_arg);
+        if (args_value == NULL)
+          {
+            done = -1;
+            goto all_done;
+          }
+      }
+
+    args_size =&args_value[nargs].pa_int;
+    args_type =&args_size[nargs];

don't you have an off-by-one error here? You allocate nargs arguments and access [nargs]


      memset (args_type, s->_flags2&  _IO_FLAGS2_FORTIFY ? '\xff' : '\0',
-	    nargs * sizeof (int));
-    args_value = alloca (nargs * sizeof (union printf_arg));
-    args_size = alloca (nargs * sizeof (int));
+	    nargs * sizeof (*args_type));

      /* XXX Could do sanity check here: If any element in ARGS_TYPE is
         still zero after this loop, format is invalid.  For now we
@@ -1973,8 +1997,8 @@ do_positional:
    }

  all_done:
-  if (__builtin_expect (workstart != NULL, 0))
-    free (workstart);
+  free (args_malloced);
+  free (workstart);
    /* Unlock the stream.  */
    _IO_funlockfile (s);
    _IO_cleanup_region_end (0);

Andreas -- Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg) GPG fingerprint = 93A3 365E CE47 B889 DF7F FED1 389A 563C C272 A126


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]