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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
