Re: Enabling -Werror=format-security by default
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 -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
- 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 -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
On Thu, Nov 21, 2013 at 2:04 AM, Florian Weimer fwei...@redhat.com 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/ -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
On Wed, Nov 20, 2013 at 8:57 AM, Dhiru Kholia dhiru.kho...@gmail.com 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/ -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
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_untrusted_string.2C_this_isn.27t_an_error.21 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 -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
On Wed, 20 Nov 2013 21:27:39 +0530 Dhiru Kholia dhiru.kho...@gmail.com 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_errors.3F 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 signature.asc Description: PGP signature -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
On 20 November 2013 17:25, Kevin Fenzi ke...@scrye.com 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/ -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
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 -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
On Wed, 2013-11-20 at 09:13 -0700, Jerry James wrote:
On Wed, Nov 20, 2013 at 8:57 AM, Dhiru Kholia dhiru.kho...@gmail.com 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
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
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 dsm...@redhat.com Red Hat http://www.redhat.com 256.217.0141 (direct) 256.837.0057 (fax) -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
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 -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
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. -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
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 -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
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
--
devel mailing list
devel@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/devel
Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
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 -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
Re: Enabling -Werror=format-security by default
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 -- devel mailing list devel@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/devel Fedora Code of Conduct: http://fedoraproject.org/code-of-conduct
