RFC: default to -Werror=format-security

We’ve had a number of requests to make the format-security warning default to an error. This warning complains about a printf-like format string that is not a literal string and is used without any arguments. E.G.:

format-security.c:4:10: warning: format string is not a string literal (potentially insecure) [-Wformat-security]
  printf(fmt);
         ^~~
1 warning generated.

For background, if the format string can be controlled by external input, the security risk is that it could contain “%” characters and be used to clobber memory. The alternative is to use a fixed “%s” format, e.g., printf(“%s”, fmt).

This catches real-world security holes, but sometimes people don’t pay attention to warnings. Promoting this warning to an error by default would get people’s attention and help motivate them to fix their code. But, the obvious downside is that it could be disruptive. Existing code might fail to build and would either require source code fixes or build changes to specify -Wno-error=format-security.

Opinions?

What other warnings do we default to error? Do we seem to have any (defacto or explicit) guideline for deciding?

I’m great with this being an error. “printf(fmt, x, y, z);” is suspicious, and can remain a warning, but “printf(fmt);” should just default to being an error. Just be sure there is a way to turn it back into a warning, or squelched entirely.

You should strongly consider adding a fix-it for this situation. That should make it slightly more palatable to turn the warning into an error.

We’ve had a number of requests to make the format-security warning default
to an error. This warning complains about a printf-like format string that
is not a literal string and is used without any arguments. E.G.:

format-security.c:4:10: warning: format string is not a string literal
(potentially insecure) [-Wformat-security]
  printf(fmt);
         ^~~
1 warning generated.

For background, if the format string can be controlled by external input,
the security risk is that it could contain “%” characters and be used to
clobber memory. The alternative is to use a fixed “%s” format, e.g.,
printf(“%s”, fmt).

This catches real-world security holes, but sometimes people don’t pay
attention to warnings

Won't this line of reasoning lead to all useful warnings being in -Werror
eventually? Say, forgetting a return statement in a function is also "just"
a warning...

My feeling is that we shouldn't do this in upstream clang. The vast
majority of our default-error warnings are actually checks for invalid code
that we want to be able to accept in system headers. To my knowledge, none
of our existing default-error warnings fire on well-formed code, which this
is.

I think it's totally reasonable for vendors to want to make this default to
an error, though. :slight_smile:

What other warnings do we default to error? Do we seem to have any (defacto or explicit) guideline for deciding?

Here is a list of the diagnostics that currently default to errors:

warn_module_config_mismatch
ext_reserved_user_defined_literal
ext_pp_bad_paste_ms
warn_use_of_private_header_outside_module
ext_cc_narrowing
ext_register_storage_class
warn_builtin_unknown
warn_cxx_ms_struct
warn_delegating_tor_cycle
ext_constexpr_function_never_constant_expr
ext_init_list_type_narrowing
ext_init_list_variable_narrowing
ext_init_list_constant_narrowing
ext_out_of_line_declaration
ext_array_init_parens
ext_typecheck_addrof_temporary
ext_increment_bool
warn_non_pod_vararg_with_format_string
warn_cannot_pass_non_pod_arg_to_vararg
ext_in_class_initializer_float_type_cxx11
warn_second_parameter_to_va_arg_not_pod
warn_second_parameter_to_va_arg_ownership_qualified
warn_return_missing_expr
ext_return_missing_expr
ext_return_has_expr
ext_module_import_in_extern_c
ext_module_import_not_at_top_level_noop
warn_nullability_declspec

Many of them are extensions. I’m not aware of any specific guidelines for this.

We’ve had a number of requests to make the format-security warning default
to an error. This warning complains about a printf-like format string that
is not a literal string and is used without any arguments. E.G.:

format-security.c:4:10: warning: format string is not a string literal
(potentially insecure) [-Wformat-security]
  printf(fmt);
         ^~~
1 warning generated.

For background, if the format string can be controlled by external input,
the security risk is that it could contain “%” characters and be used to
clobber memory. The alternative is to use a fixed “%s” format, e.g.,
printf(“%s”, fmt).

This catches real-world security holes, but sometimes people don’t pay
attention to warnings

Won't this line of reasoning lead to all useful warnings being in -Werror
eventually? Say, forgetting a return statement in a function is also "just"
a warning...

I would not be opposed to that either. If it leads to an exploitable
security vulnerability (such as format specifiers, failing to return
on all control paths, obvious buffer under/overruns, etc), I think we
should at least consider making it an error. Perhaps it can be
controlled with a flag (treat-security-diagnostics-as-an-error) that
is on by default.

~Aaron

I’m with Reid on this. Useful warnings should be on by default, less useful warnings shouldn’t be, and if our warnings are high-signal enough then most people will look at them.

I think one problem we have for some warnings (probably including -Wformat-security) is that it’s not clear from the warning text what the problem is. It’d be useful to have some document that for each warning lists the warning’s motivation and common fixes and workarounds. If people don’t understand a warning and just try to shut up the compiler, things usually don’t end well.

For this warning, it’d probably help a lot to add a fixit like Ben suggested.

Not all of them :slight_smile:

Visual Studio groups warnings into big warning level buckets. Level 1 has the most important / severe (obvious use of uninitialized value), level 4 has fairly minor warnings (unused parameter), and /Weverything will tell you about really useless stuff (warning! you just used __declspec(align)! ). I could imagine a world where the "Level 1", and maybe "Level 2" warnings were errors by default.

We should make it harder to compile broken code, and easier to write correct code. We can't change it all at once without angering the world though :slight_smile:

Won't this line of reasoning lead to all useful warnings being in -Werror
eventually? Say, forgetting a return statement in a function is also "just"
a warning...

Not all of them :slight_smile:

Visual Studio groups warnings into big warning level buckets. Level 1 has
the most important / severe (obvious use of uninitialized value), level 4
has fairly minor warnings (unused parameter), and /Weverything will tell
you about really useless stuff (warning! you just used __declspec(align)!
). I could imagine a world where the "Level 1", and maybe "Level 2"
warnings were errors by default.

We have this too: on-by-default warnings, -Wall, -Wextra, -Weverything. I
don't think we should turn on-by-default warnings into errors.

We should make it harder to compile broken code, and easier to write
correct code. We can't change it all at once without angering the world
though :slight_smile:

People who don't like writing broken code don't ignore warnings. People who
do like writing broken code will just pass -Wno-error. I don't think this
proposal helps either party.

Won't this line of reasoning lead to all useful warnings being in -Werror
eventually? Say, forgetting a return statement in a function is also "just"
a warning...

Not all of them :slight_smile:

Visual Studio groups warnings into big warning level buckets. Level 1 has
the most important / severe (obvious use of uninitialized value), level 4
has fairly minor warnings (unused parameter), and /Weverything will tell you
about really useless stuff (warning! you just used __declspec(align)! ). I
could imagine a world where the "Level 1", and maybe "Level 2" warnings were
errors by default.

We have this too: on-by-default warnings, -Wall, -Wextra, -Weverything. I
don't think we should turn on-by-default warnings into errors.

We already do (in fact, we have over two dozen that we turn on,
according to the list posted earlier in the thread).

We should make it harder to compile broken code, and easier to write
correct code. We can't change it all at once without angering the world
though :slight_smile:

People who don't like writing broken code don't ignore warnings. People who
do like writing broken code will just pass -Wno-error. I don't think this
proposal helps either party.

I think there's a reasonable distinction between "this code may not do
what you expect" and "this code may not do what you expect, and is
likely to result in security vulnerabilities." If someone elects to
turn on -Wno-error to get their insecure code to compile, there's
basically no help for them. But for projects that don't compile
particularly cleanly (so security-related diagnostics get lost in the
noise), being alerted, "no, really, this is bad" will definitely help
*some* programmers. I am interested in alerting people who might be
willing to fix security vulnerabilities in their code. I'm unconcerned
about people who will go to any lengths to get their code to compile
without actually fixing their code when it comes to security concerns.

~Aaron

>>
>>
>>>
>>>
>>> Won't this line of reasoning lead to all useful warnings being in
-Werror
>>> eventually? Say, forgetting a return statement in a function is also
"just"
>>> a warning...
>>
>>
>> Not all of them :slight_smile:
>>
>> Visual Studio groups warnings into big warning level buckets. Level 1
has
>> the most important / severe (obvious use of uninitialized value), level
4
>> has fairly minor warnings (unused parameter), and /Weverything will
tell you
>> about really useless stuff (warning! you just used __declspec(align)!
). I
>> could imagine a world where the "Level 1", and maybe "Level 2" warnings
were
>> errors by default.
>
>
> We have this too: on-by-default warnings, -Wall, -Wextra, -Weverything. I
> don't think we should turn on-by-default warnings into errors.

We already do (in fact, we have over two dozen that we turn on,
according to the list posted earlier in the thread).

But that's for things that are supposed to be errors but can't be due to
system headers, not the other way round.

>> We should make it harder to compile broken code, and easier to write
>> correct code. We can't change it all at once without angering the world
>> though :slight_smile:
>
>
> People who don't like writing broken code don't ignore warnings. People
who
> do like writing broken code will just pass -Wno-error. I don't think this
> proposal helps either party.

I think there's a reasonable distinction between "this code may not do
what you expect" and "this code may not do what you expect, and is
likely to result in security vulnerabilities."

"may not be what you expect" can always be a security vulnerability – think
of -Wfor-loop-analysis, -Wunreachable, etc. We have warnings with much
higher true positive rate than -Wformat-security.

If someone elects to
turn on -Wno-error to get their insecure code to compile, there's
basically no help for them. But for projects that don't compile
particularly cleanly (so security-related diagnostics get lost in the
noise)

If our default warning set is too noisy, that's a bug. Every on-by-default
warning should be super useful -- that's the bar for on-by-default warnings.

(I generally feel like Nico’s capturing my line-of-thinking here, FYI)

>>
>>
>>>
>>>
>>> Won't this line of reasoning lead to all useful warnings being in
>>> -Werror
>>> eventually? Say, forgetting a return statement in a function is also
>>> "just"
>>> a warning...
>>
>>
>> Not all of them :slight_smile:
>>
>> Visual Studio groups warnings into big warning level buckets. Level 1
>> has
>> the most important / severe (obvious use of uninitialized value), level
>> 4
>> has fairly minor warnings (unused parameter), and /Weverything will
>> tell you
>> about really useless stuff (warning! you just used __declspec(align)!
>> ). I
>> could imagine a world where the "Level 1", and maybe "Level 2" warnings
>> were
>> errors by default.
>
>
> We have this too: on-by-default warnings, -Wall, -Wextra, -Weverything.
> I
> don't think we should turn on-by-default warnings into errors.

We already do (in fact, we have over two dozen that we turn on,
according to the list posted earlier in the thread).

But that's for things that are supposed to be errors but can't be due to
system headers, not the other way round.

Fair point.

>> We should make it harder to compile broken code, and easier to write
>> correct code. We can't change it all at once without angering the
>> world
>> though :slight_smile:
>
>
> People who don't like writing broken code don't ignore warnings. People
> who
> do like writing broken code will just pass -Wno-error. I don't think
> this
> proposal helps either party.

I think there's a reasonable distinction between "this code may not do
what you expect" and "this code may not do what you expect, and is
likely to result in security vulnerabilities."

"may not be what you expect" can always be a security vulnerability – think
of -Wfor-loop-analysis, -Wunreachable, etc. We have warnings with much
higher true positive rate than -Wformat-security.

Sorry, but printf(fmt); is *always* a true positive in my book. Same
with failing to return from all code paths. (etc)

I do hear what you are saying and am not suggesting we should go about
this in a shotgun fashion. Each warning would require careful thought,
and possibly also require alterations to the diagnostic before turning
it into something that is an error by default.

If someone elects to
turn on -Wno-error to get their insecure code to compile, there's
basically no help for them. But for projects that don't compile
particularly cleanly (so security-related diagnostics get lost in the
noise)

If our default warning set is too noisy, that's a bug. Every on-by-default
warning should be super useful -- that's the bar for on-by-default warnings.

Super useful doesn't mean "is actually fixed by the developer." I have
been on projects that have a significant number of super useful
diagnostics that live in the source for *years* because of the sheer
quantity of diagnostics. When attempting to improve those kinds of
projects, they would get serious benefit from the notion of "some
diagnostics are more important than others." In fact, one such project
like that used Visual Studio's warning levels to do exactly that.
Unfortunately, clang has no such mechanism -- this sounds like a
proposal to add one. Perhaps the discussion should not be "warning vs
error" but instead about warning levels or some other mechanism to
call out the severity of a diagnostic. I want to get some utilities in
place that help existing code bases that are diagnostic-heavy get
security-related warnings under control.

~Aaron

As a note on this, I'm only mildly in favor of making this an error by default. The points raised by Nico are good, though they haven't convinced me to abandon the cause entirely :). I won't be super upset if we keep this a warning.

Regardless of the default-ness of this particular issue, I would really love to see a fixit for the trivial "printf(fmt);" case.

I’ve just noticed that there’s no warning when printf has more than one argument, is this a bug?

You are wrong. The most common reason for printf(fmt) to appear is that fmt is the result of doing a lookup of the locale-aware version of some constant string. In this case, the contents of fmt is entirely under the control of whoever shipped the application, and will have been checked for format string vulnerabilities by the localisation tools (at least, assuming that the original that is being translated are free from vulnerabilities). If you are not doing any caching in the application, then you can mark the translation function with the attribute that indicates that its input and output have the same format string compatibility. If you are caching, then there is no easy way of silencing this warning.

Making this an error will cause valid and correct code to fail to compile and will result in people simply disabling the warning, rather than checking it.

David

If the expected string does not have any format specifiers, then
printf("%s", fmt) is definitely the correct way to write that because
the assumption "entirely under the control of whoever shipped the
application" is a poor one. If it does have format specifiers, I agree
that we should not err, but I don't believe that was on the table.

~Aaron

Or use puts, if applicable.

>>
>> Sorry, but printf(fmt); is *always* a true positive in my book. Same
>> with failing to return from all code paths. (etc)
>
> You are wrong. The most common reason for printf(fmt) to appear is that
fmt is the result of doing a lookup of the locale-aware version of some
constant string. In this case, the contents of fmt is entirely under the
control of whoever shipped the application, and will have been checked for
format string vulnerabilities by the localisation tools (at least, assuming
that the original that is being translated are free from vulnerabilities).
If you are not doing any caching in the application, then you can mark the
translation function with the attribute that indicates that its input and
output have the same format string compatibility. If you are caching, then
there is no easy way of silencing this warning.
>
> Making this an error will cause valid and correct code to fail to
compile and will result in people simply disabling the warning, rather than
checking it.

If the expected string does not have any format specifiers, then
printf("%s", fmt) is definitely the correct way to write that because
the assumption "entirely under the control of whoever shipped the
application" is a poor one. If it does have format specifiers, I agree
that we should not err, but I don't believe that was on the table.

I think David is talking about a situation where it is e.g.

printf(translate("Please enter a number from %d-%d\n"), lo, hi);

-- Sean Silva