Enabling "-Werror=format-security" by default

[Dhiru Kholia]

Hi,

We are working on a proposal to enable "-Werror=format-security"
compilation flag for all packages in Fedora.

Once this flag is enabled, GCC will refuse to compile code that could be
vulnerable to a string format security flaw. For more details, please
see https://fedorahosted.org/fesco/ticket/1185 page.

Enabling this option eliminates an entire class of security issues! To
further understand why it is important to fix such bugs, please see
https://fedoraproject.org/wiki/Format-Security-FAQ page.

Currently, around 400 packages FTBFS if this flag is enabled. I am all
set to start filing the bugs (once given the green signal). In addition,
I am willing to help in patching these packages. I believe that this
work is important and will benefit everyone (including upstream and
other distributions).

I am attaching a sample Bugzilla bug report - this is what the actual
bug reports will look like.

--
Dhiru

[Jerry James]

On Wed, Nov 20, 2013 at 8:57 AM, Dhiru Kholia <dhiru.kholia(a)gmail.com&gt; wrote:
>  Currently, around 400 packages FTBFS if this flag is enabled. I am
> all
>  set to start filing the bugs (once given the green signal). In addition,
>  I am willing to help in patching these packages. I believe that this
>  work is important and will benefit everyone (including upstream and
>  other distributions). 
It would have been nice if you had mentioned which packages failed to
build, so maintainers could start looking at them.  I found this by
digging around a little:

http://people.fedoraproject.org/~halfie/rebuild-logs.txt

And the very first package I maintain that appears on that list, abe,
is an interesting one.  The game has an internal function,
path_sprintf(), which is static in Game.c.  All callers of that
function are visible in the same file, and all pass constant strings
into the function, which passes those constant strings to sprintf().
The function's purpose is to produce a pathname for a file of interest
to the caller in the game's installed location.  It's too bad that
gcc's analysis cannot span function calls inside a compilation unit.
There really is nothing wrong with this code.
-- 
Jerry James
http://www.jamezone.org/

[Josh Bressers]

>  
>  And the very first package I maintain that appears on that list, abe,
>  is an interesting one.  The game has an internal function,
>  path_sprintf(), which is static in Game.c.  All callers of that
>  function are visible in the same file, and all pass constant strings
>  into the function, which passes those constant strings to sprintf().
>  The function's purpose is to produce a pathname for a file of interest
>  to the caller in the game's installed location.  It's too bad that
>  gcc's analysis cannot span function calls inside a compilation unit.
>  There really is nothing wrong with this code. 
You're right, some of the "bugs" aren't really bugs. It would be handy
if
there was some form of taint checking in gcc, but we have to work with what
we have. We mention this in the FAQ.

https://fedoraproject.org/wiki/Format-Security-FAQ#I_don.27t_process_untr...

I would rather see us future proof all code than try to figure out each
possible use case. This is a bit of a blanket strategy, but I do think it
will have a net benefit. It's not every day you can remove an entire class
of security issues (I can count the number of times we've done this on one
hand).

Thanks.

-- 
Josh Bressers / Red Hat Product Security Team

[Adam Jackson]

On Wed, 2013-11-20 at 09:13 -0700, Jerry James wrote:
>  On Wed, Nov 20, 2013 at 8:57 AM, Dhiru Kholia
> <dhiru.kholia(a)gmail.com&gt; wrote:
>  > Currently, around 400 packages FTBFS if this flag is enabled. I am all
>  > set to start filing the bugs (once given the green signal). In addition,
>  > I am willing to help in patching these packages. I believe that this
>  > work is important and will benefit everyone (including upstream and
>  > other distributions).
>  
>  It would have been nice if you had mentioned which packages failed to
>  build, so maintainers could start looking at them.  I found this by
>  digging around a little:
>  
>  http://people.fedoraproject.org/~halfie/rebuild-logs.txt 
The implementation of this flag needs some work.  The sis X driver
apparently fails the check for this code:

    const char *rectxine = "\t... setting up rectangular Xinerama layout\n";
    // ...
    if (infochanged && !usenonrect) {
         xf86DrvMsg(pScrn1->scrnIndex, X_INFO,
                    "Virtual screen size does not match maximum display
modes...\n");
         xf86DrvMsg(pScrn1->scrnIndex, X_INFO, rectxine);
    }

Presumably gcc means something very precise by "string literal" here.
If I change the declaration to be const char rectxine[] it builds fine.
Which is... somewhat understandable?  I mean you _could_ assign to
rectxine-the-pointer and change what it points to, but the code does
not, so you'd hope constant-propagation would figure this out.

- ajax

[Przemek Klosowski]

On 11/20/2013 11:13 AM, Jerry James wrote:
>  path_sprintf(), which is static in Game.c. All callers of that 
>  function are visible in the same file, and all pass constant strings 
>  into the function, which passes those constant strings to sprintf(). 
>  The function's purpose is to produce a pathname for a file of interest 
>  to the caller in the game's installed location. It's too bad that 
>  gcc's analysis cannot span function calls inside a compilation unit. 
>  There really is nothing wrong with this code.  
Well, the code is inelegant:

  sprintf(path + len, formatted_name);

looks better and avoids the warning if you write it as

  sprintf(&(path[len]), "%s", formatted_name);

which should lead the reader to reflect on whether it makes sense to prevent buffer
overflow by
using %NNs to limit the size of appended name so that it fits within the limits of the
path buffer.

[Florian Weimer]

On 11/20/2013 06:45 PM, Przemek Klosowski wrote:
>  On 11/20/2013 11:13 AM, Jerry James wrote:
> > path_sprintf(), which is static in Game.c. All callers of that
> > function are visible in the same file, and all pass constant strings
> > into the function, which passes those constant strings to sprintf().
> > The function's purpose is to produce a pathname for a file of interest
> > to the caller in the game's installed location. It's too bad that
> > gcc's analysis cannot span function calls inside a compilation unit.
> > There really is nothing wrong with this code.
>  Well, the code is inelegant:
>
>    sprintf(path + len, formatted_name);
>
>  looks better and avoids the warning if you write it as
>
>    sprintf(&(path[len]), "%s", formatted_name);
>
>  which should lead the reader to reflect on whether it makes sense to prevent buffer
> overflow by
>  using %NNs to limit the size of appended name so that it fits within the limits of the
> path buffer. 
You should be using snprintf anyway.  And neither sprintf nor snprintf 
are really suitable for build strings piece-by-piece, unfortunately.

Anyway, adding the "%s" trades a bit of text segment size increase for a 
likely decrease in execution time because the non-format-string argument 
does not have to be parsed for format strings.

-- 
Florian Weimer / Red Hat Product Security Team

[Jerry James]

On Thu, Nov 21, 2013 at 2:04 AM, Florian Weimer <fweimer(a)redhat.com&gt; wrote:
>  On 11/20/2013 06:45 PM, Przemek Klosowski wrote:
> > Well, the code is inelegant:
> >
> >   sprintf(path + len, formatted_name);
> >
> > looks better and avoids the warning if you write it as
> >
> >   sprintf(&(path[len]), "%s", formatted_name);
> >
> > which should lead the reader to reflect on whether it makes sense to
> > prevent buffer overflow by
> > using %NNs to limit the size of appended name so that it fits within the
> > limits of the path buffer.
>
>
>  You should be using snprintf anyway.  And neither sprintf nor snprintf are
>  really suitable for build strings piece-by-piece, unfortunately.
>
>  Anyway, adding the "%s" trades a bit of text segment size increase for a
>  likely decrease in execution time because the non-format-string argument
>  does not have to be parsed for format strings. 
Thanks for the suggestions, everyone.  I have added a patch to fix
this for abe.  I also pulled a patch for apron from upstream, which
had already fixed their code, and made a patch for cmusphinx which I
also submitted upstream.  So there's 3 packages you can cross off the
list.

Regards,
-- 
Jerry James
http://www.jamezone.org/

[Dan Winship]

On 11/20/2013 11:13 AM, Jerry James wrote:
>  And the very first package I maintain that appears on that list,
> abe,
>  is an interesting one.  The game has an internal function,
>  path_sprintf(), which is static in Game.c.  All callers of that
>  function are visible in the same file, and all pass constant strings
>  into the function, which passes those constant strings to sprintf().
>  The function's purpose is to produce a pathname for a file of interest
>  to the caller in the game's installed location.  It's too bad that
>  gcc's analysis cannot span function calls inside a compilation unit.
>  There really is nothing wrong with this code. 
If you change its prototype to:

static void path_sprintf (char *path, char *format, ...) 
__attribute__((__format__(__printf, 2, 3)));

(and update it to use varargs and vsprintf() instead of sprintf())

then the warnings will go away, because gcc will now know that it's a 
function that behaves like printf(), with argument 2 being the format 
string and argument 3 being the "...", and so then it can do the 
-Wformat-security checking at each of the path_sprintf() callpoints. 
(And you also get warnings when the arguments don't match the format 
string, like you would if you were calling sprintf() directly.) (And now 
you can use formats other than a single "%d" in the future if you want...)

-- Dan

[Kevin Fenzi]

On Wed, 20 Nov 2013 21:27:39 +0530
Dhiru Kholia <dhiru.kholia(a)gmail.com&gt; wrote:

>  Hi,
>  
>  We are working on a proposal to enable "-Werror=format-security"
>  compilation flag for all packages in Fedora.
>  
>  Once this flag is enabled, GCC will refuse to compile code that could
>  be vulnerable to a string format security flaw. For more details,
>  please see https://fedorahosted.org/fesco/ticket/1185 page.
>  
>  Enabling this option eliminates an entire class of security issues! To
>  further understand why it is important to fix such bugs, please see
>  https://fedoraproject.org/wiki/Format-Security-FAQ page.
>  
>  Currently, around 400 packages FTBFS if this flag is enabled. I am all
>  set to start filing the bugs (once given the green signal). In
>  addition, I am willing to help in patching these packages. I believe
>  that this work is important and will benefit everyone (including
>  upstream and other distributions).
>  
>  I am attaching a sample Bugzilla bug report - this is what the actual
>  bug reports will look like. 
Great. Thanks for doing this. 

First... I'd suggest posting the list of packages and give maintainers
a week or two to just fix them. Then before filing anything you can run
a quick check to see which packages are still needing fixing. 

Looking at: 

http://fedoraproject.org/wiki/Mass_bug_filing

I'd ask for a bit more in the bug report. ;) 

Might repeat the info from
https://fedoraproject.org/wiki/Format-Security-FAQ#How_do_I_fix_these_err...
in the bug text (just to save people a trip to the wiki for such a
simple fixing process)

And I would add: 

Please fix this issue in rawhide with a patch (which you should submit
to upstream to merge moving forward). Please do a new build with the
fix in rawhide. Other releases do not need to be directly fixed, but
there should be no harm in pushing out this fix/patch with other needed
changes to those branches. 

And we might say: 

In the event you don't fix this bug before the next mass rebuild,
provenpackagers may step in and update your package(s) to fix this
issue. 

Otherwise looks great. ;) 

kevin

[Simone Caronni]

On 20 November 2013 17:25, Kevin Fenzi <kevin(a)scrye.com&gt; wrote:

>  First... I'd suggest posting the list of packages and give
> maintainers
>  a week or two to just fix them. Then before filing anything you can run
>  a quick check to see which packages are still needing fixing.
>  
Yes please, sometimes the automated bug reports are a bit confusing and/or
misleading.

Thanks,
--Simone


-- 
You cannot discover new oceans unless you have the courage to lose sight of
the shore (R. W. Emerson).

http://xkcd.com/229/
http://negativo17.org/

[Dhiru Kholia]

On 11/20/13 at 09:27pm, Dhiru Kholia wrote:
>  We are working on a proposal to enable
> "-Werror=format-security"
>  compilation flag for all packages in Fedora.
>
>  Currently, around 400 packages FTBFS if this flag is enabled. 
A list of packages which FTBFS is available at,

http://people.fedoraproject.org/~halfie/rebuild-logs.txt

The fix for these errors is quite simple. It's a matter of changing a
line like,

   printf(foo);

to read,

   printf("%s", foo);

That's it.

...

Jerry, Kevin, all,

Thanks for the feedback. I will incorporate your suggestions.

--
Dhiru

[David Smith]

On 11/20/2013 10:51 AM, Dhiru Kholia wrote:
>  On 11/20/13 at 09:27pm, Dhiru Kholia wrote:
> > We are working on a proposal to enable "-Werror=format-security"
> > compilation flag for all packages in Fedora.
> >
> > Currently, around 400 packages FTBFS if this flag is enabled.
>  
>  A list of packages which FTBFS is available at,
>  
>  http://people.fedoraproject.org/~halfie/rebuild-logs.txt 
Looking at the list, I see several (~17) packages with errors of the form:

====
error: -Wformat-security ignored without -Wformat [-Werror=format-security]
====

Which is an error, but not exactly what you are trying to catch. Got any
ideas on what is going on here?

-- 
David Smith
dsmith(a)redhat.com
Red Hat
http://www.redhat.com
256.217.0141 (direct)
256.837.0057 (fax)

[Dhiru Kholia]

On 11/20/13 at 11:16am, David Smith wrote:
>  > On 11/20/13 at 09:27pm, Dhiru Kholia wrote:
>  > A list of packages which FTBFS is available at,
>  >
>  > http://people.fedoraproject.org/~halfie/rebuild-logs.txt
>
>  Looking at the list, I see several (~17) packages with errors of the form:
>
>  error: -Wformat-security ignored without -Wformat [-Werror=format-security]
>
>  Which is an error, but not exactly what you are trying to catch. Got any
>  ideas on what is going on here? 
Hi David,

Excellent catch! I took a quick look and it seems that these packages
are trying to use custom compilation flags.

E.g. p0f-3.06b-3.fc20.src.rpm has a line which says,

BASIC_CFLAGS="-Wall -Wno-format -I/usr/local/include/ \
              -I/opt/local/include/ -DVERSION=\"$VERSION\" $CFLAGS"


The usage of hard-coded "-Wno-format" flag conflicts with our desired
"-Werror=format-security" flag. p0f is a "security package" and it
should know better, I believe.

Additionally, p0f packaging seems to be violating the Fedora packaging
guidelines,

https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

The very next project I am (and was) planning to work on, is to detect
packages which try to use custom compilation flags ;)

--
Dhiru

[Mark Wielaard]

On Wed, 2013-11-20 at 23:15 +0530, Dhiru Kholia wrote:
>  On 11/20/13 at 11:16am, David Smith wrote:
>  > > On 11/20/13 at 09:27pm, Dhiru Kholia wrote:
>  > > A list of packages which FTBFS is available at,
>  > >
>  > > http://people.fedoraproject.org/~halfie/rebuild-logs.txt
>  >
>  > Looking at the list, I see several (~17) packages with errors of the form:
>  >
>  > error: -Wformat-security ignored without -Wformat [-Werror=format-security]
>  >
>  > Which is an error, but not exactly what you are trying to catch. Got any
>  > ideas on what is going on here?
>  
>  Hi David,
>  
>  Excellent catch! I took a quick look and it seems that these packages
>  are trying to use custom compilation flags.
>  
>  E.g. p0f-3.06b-3.fc20.src.rpm has a line which says,
>  
>  BASIC_CFLAGS="-Wall -Wno-format -I/usr/local/include/ \
>                -I/opt/local/include/ -DVERSION=\"$VERSION\" $CFLAGS"
>  
>  
>  The usage of hard-coded "-Wno-format" flag conflicts with our desired
>  "-Werror=format-security" flag.
>  [...]
>  The very next project I am (and was) planning to work on, is to detect
>  packages which try to use custom compilation flags ;) 
elfutils seems to be in somewhat of the same situation, although
slightly different. Upstream does actually explicitly enable -Werror
-Wformat=2 for all files, but has 5 exceptions for which it uses
-Wno-format which then clashes with the setting of -Wformat-security.

The reason such files use -Wno-format is either because they have some
helper method such as:

ssize_t
regtype (const char *setname, int type, const char *fmt, int arg)
{
   [...]
   int s = snprintf (name, namelen, fmt, arg);

which is always called with a static fmt string, but gcc is unable to
detect that.

Or it contains code that creates a format string such as by:

/* Location print format string.  */
static const char *locfmt;

[...]

parse_opt()

      switch (arg[0])
        {
        case 'd':
          locfmt = "%7" PRId64 " ";
          break;

        case 'o':
        octfmt:
          locfmt = "%7" PRIo64 " ";
          break;

        case 'x':
          locfmt = "%7" PRIx64 " ";
          break;

        default:
          error (0, 0, gettext ("invalid value '%s' ...

[...]

process()
              if (unlikely (locfmt != NULL))
                printf (locfmt, (int64_t) to - len - (buf - start));

Where gcc again seems unable to detect that the locfmt string is a
constant string.

How to deal with such cases?

Thanks,

Mark

[Till Maas]

On Wed, Nov 20, 2013 at 10:21:10PM +0530, Dhiru Kholia wrote:
>  On 11/20/13 at 09:27pm, Dhiru Kholia wrote:
>  > We are working on a proposal to enable "-Werror=format-security"
>  > compilation flag for all packages in Fedora.
>  >
>  > Currently, around 400 packages FTBFS if this flag is enabled.
>  
>  A list of packages which FTBFS is available at,
>  
>  http://people.fedoraproject.org/~halfie/rebuild-logs.txt 
Here is a list with owners of affected packages:
http://till.fedorapeople.org/tmp/format-security-owners.txt

It was created as follows:
curl http://people.fedoraproject.org/~halfie/rebuild-logs.txt | cut -d/ -f1 | rev | cut
-d- -f 3-  | rev  | sort -u | fedoradev-pkgowners | sort

Regards
Till

[Alexander Bokovoy]

On Wed, 20 Nov 2013, Dhiru Kholia wrote:
> Hi,
>
> We are working on a proposal to enable "-Werror=format-security"
> compilation flag for all packages in Fedora.
>
> Once this flag is enabled, GCC will refuse to compile code that could be
> vulnerable to a string format security flaw. For more details, please
> see https://fedorahosted.org/fesco/ticket/1185 page.
>
> Enabling this option eliminates an entire class of security issues! To
> further understand why it is important to fix such bugs, please see
> https://fedoraproject.org/wiki/Format-Security-FAQ page.
>
> Currently, around 400 packages FTBFS if this flag is enabled. I am all
> set to start filing the bugs (once given the green signal). In addition,
> I am willing to help in patching these packages. I believe that this
> work is important and will benefit everyone (including upstream and
> other distributions).
>
> I am attaching a sample Bugzilla bug report - this is what the actual
> bug reports will look like. 
I think these reports are misleading, at least in
FreeIPA case.
freeipa-3.3.1-2.fc21.src.rpm/build.log:ipa_enrollment.c:320:5: error: format not a string
literal and no format arguments [-Werror=format-security]
freeipa-3.3.1-2.fc21.src.rpm/build.log:ipa_enrollment.c:347:9: error: format not a string
literal and no format arguments [-Werror=format-security]
freeipa-3.3.1-2.fc21.src.rpm/build.log:ipa_enrollment.c:360:5: error: format not a string
literal and no format arguments [-Werror=format-security]

All three cases are dealing with following lines:
    LOG("%s", errMesg ? errMesg : "success\n");
    LOG("%s", errMesg);
    LOG("%s", errMesg);

where LOG macro expands to 
    slapi_log_error(SLAPI_LOG_PLUGIN, NAME, format, arguments ... );

(SLAPI_LOG_PLUGIN and NAME are constants)

as you can see, in all these cases format *is* a string literal and
there are exact format arguments passed.

-- 
/ Alexander Bokovoy

[Jaroslav Reznik]

----- Original Message -----
>  Hi,
>  
>  We are working on a proposal to enable "-Werror=format-security"
>  compilation flag for all packages in Fedora. 
Hi!
FESCo on yesterdays meeting agreed to ask you for a formal proposal
as a Change page [1] - for more details see [2]. Don't hesitate to ask
any question regarding the process, I'll help and will drive this
change through.

AGREED: Please file a Change page for this. (+7, 0, 0)

Jaroslav

[1] https://fedorahosted.org/fesco/ticket/1185#comment:14
[2] https://fedoraproject.org/wiki/Changes/Policy