[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Re: mass bug filing for undefined sn?printf use



On Sun, 28 Dec 2008 00:42:46 -0800
Kees Cook <kees@outflux.net> wrote:

> I'd like to seek advice before I perform a mass-bug filing for this
> unstable (though semi-common) use of "sprintf" and "snprintf":
> 
>     sprintf(buf, "%s foo %d %d", buf, var1, var2);
> 
> This is used in many upstreams to perform a format-string-handling
> version of strcat.
> 
> This was originally noticed by Anders Kaseorg in Ubuntu[1], since
> -D_FORTIFY_SOURCE=2 triggers a change in behavior (buf is truncated before
> handling the rest of the format string instead of performing the concat).
> 
> Upstream glibc points out[2] that using sprintf in this way is undefined
> under C99, and the man pages have now been updated[3] to reflect this.
> (Though I believe it is possible to patch glibc to avoid the change in
> behavior, it's probably best to work on fixing all the upstreams.)
> 
> In Debian, some tools already compile natively with -D_FORTIFY_SOURCE=2,
> and some have Build-Depends on "hardening-wrapper", which enables this
> compiler flag.  As such, it seems sensible to have all affected packages
> fixed since the results of such a call could change.  (Though it is not an
> RC issue.)

By all affected packages, do you mean packages that use the code or
packages that use the code *AND* compile with  or
Build-Depend on hardening-wrapper?

IMHO any bugs filed merely due to the presence of the code without the
means to trigger the error in normal builds should be wishlist.

Re:
Debian GPE team <pkg-gpe-maintainers@lists.alioth.debian.org>
   gpe-conf (U)

gpe-conf, being Gtk+ and therefore GLib can simply switch to using
g_strconcat or g_sn?printf or g_strdup_printf and avoid all these
problems. In the specific case of gpe-conf, only two of the files using
this code do not already include gtk/gtk.h or glib/glib.h so it is only
sensible to use the GLib functions instead for most if not all
occurrences. (Indeed, in many cases, the use of a newly allocated
string to be freed later, instead of a static fixed buffer has other
benefits elsewhere.)

> And, a possible solution from Anders Kaseorg...
>  This example sprintf() call could be fixed as follows:
>   -sprintf(buf, "%s plus %d", buf, k);
>   +sprintf(buf + strlen(buf), " plus %d", k);
>  Similarly, an invalid snprintf() call could be fixed as follows:
>   -snprintf(buf, buflen, "%s plus %d", buf, k);
>   +snprintf(buf + strlen(buf), buflen - strlen(buf), " plus %d", k);
> 
> Attached is a list of affected packages, generated via:
> 
>   pcregrep -M 'sprintf\s*\(\s*([^,]*)\s*,\s*"%s[^"]*"\s*,\s*\1\s*,'
>   pcregrep -M 'snprintf\s*\(\s*([^,]*)\s*,[^,]*,\s*"%s[^"]*"\s*,\s*\1\s*,'
> 
> The logs for individual packages can be seen here[4].  I've tried to trim
> out stuff that was Ubuntu-specific or not relevant, so apologies in advance
> if there are incorrect (or missing) things in the list.
> 
> Thoughts?

Split the list according to packages that merely match the regexp and
those that match the regexp *AND* match a second regexp indicating that
the build system either uses -D_FORTIFY_SOURCE=2 or hardening-wrapper?

-- 


Neil Williams
=============
http://www.data-freedom.org/
http://www.nosoftwarepatents.com/
http://www.linux.codehelp.co.uk/

Attachment: pgpRubVFN5LIn.pgp
Description: PGP signature


Reply to: