[PATCH] D110436: Add %n format specifier warning

Arthur O'Dwyer via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Oct 5 16:35:56 PDT 2021


Quuxplusone 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<
----------------
Jaysonyan wrote:
> 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`.
Once it targets clang-tidy instead of clang, I stop caring. ;) But for the record, I still think you haven't understood how `%n` works. If you have let a malicious external user control the pointer value of `ptr` and point it at memory they don't own, //you have already lost//; at that point the arcane incantation `printf("%n", ptr)` is no more dangerous than a simple assignment `*ptr = 42` (because both are ways of poking into memory that you've already postulated was chosen by the attacker). If that's really your concern (which, again, it shouldn't be), then maybe you should instead warn on `scanf("%p")` — off the top of my head I can't think of any other way for an external attacker to control the value of an arbitrary pointer `ptr`.


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