[PATCH] D110436: Add %n format specifier warning

Jayson Yan via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 5 15:36:21 PDT 2021


Jaysonyan added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9230
+def warn_printf_n_specifier : Warning<
+  "usage of '%%n' can lead to unsafe writing to memory">, InGroup<FormatNSpecifier>;
 def warn_printf_data_arg_not_used : Warning<
----------------
aaron.ballman wrote:
> Quuxplusone wrote:
> > FWIW, I don't understand why this is "unsafe" either. The problem with `%n` is not that it might be used //intentionally//; the problem is that it opens up an attack vector for //unintentional// (malicious) use. The programmer writes `printf(buf, args...)` where `buf` is under the attacker's control (for example a debug-log format string supplied in a config file), and then the //attacker// configures something like `"%n"` instead of `"%s%d"` (so the debug-logging routine ends up poking data instead of peeking it). This vulnerable `printf(buf, ...)` is exactly what `-Wformat-security` warns about.
> > I am not aware of any vulnerability from //intentional// use of `%n`. At best, one could argue that there's a moral hazard: we might like to remove `%n`-support from our libc's printf, but we can't do that as long as there's any code out there in the wild that relies on intentional use of `%n`. Therefore, this is essentially a "deprecation warning" — but for a feature that AFAIK has never been deprecated, neither by C nor C++! (Am I wrong? Has someone officially deprecated `%n`?)
> FWIW, that's effectively how I view this as well -- it's kinda like `-Wvla` -- a diagnostic to say "congrats, you're using the feature!". However, unlike `-Wvla`, no one accidentally uses `%n` when they were expecting something else.
> 
> `%n` isn't deprecated in either C or C++.
That's a really good point, I think you're right that this wouldn't be effective in catching string format vulnerabilities, because as you said, the attacker would need to control some portion of the format string which would already be caught by `-Wformat-security`. 

Although in fuchsia I think we still have a need for warning against even explicit usages of `%n` since it could allow an external user to pass a pointers into `printf("%n", ptr)` and write to addresses that we don't want to be written to. 

Thank you for the suggestion of clang-tidy. I think moving this warning to the `bugprone` module sounds like a good way to surface a warning while not impacting intended usages of `%n`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110436/new/

https://reviews.llvm.org/D110436



More information about the cfe-commits mailing list