[cfe-dev] [RFC] Adding warnings against usage of %n

[Aaron Ballman via cfe-dev]

On Sun, Nov 7, 2021 at 10:26 AM Jayson Yan via cfe-dev
<cfe-dev at lists.llvm.org> wrote:
>
> Hello everyone,
>
> We’re interested in warning against the usage of the “%n” format string specifier to discourage developers from using this potentially unsafe format specifier.

Potentially unsafe in what way? As mentioned on the ongoing code
review for this topic (https://reviews.llvm.org/D110436), use of `%n`
by itself is not unsafe. What is unsafe is when the format string
itself is what's under attacker control so they can insert their own
`%n` unbeknownst to the programmer, but that is a much harder problem
to diagnose because there are legitimate cases for having a
non-literal format specifier.

> We were thinking of enabling this as a warning under the -Wformat-security flag but are open to alternatives, some ideas were:

I would be opposed to this without more justification as to what the
security concern is.

> Placing this check under its own flag (something along the lines of “-Wformat-n”) and possibly enabling it with -Wformat or -Wformat-security

I don't think it would be appropriate to add this as a flag in Clang
at all. It'd have to be off-by-default (there is nothing insecure
about many uses of %n, so the false positive rate would be too high),
and Clang doesn't typically add new off-by-default diagnostics because
experience has shown that users don't typically enable them. We have
some precedent for a "congrats, you're using this language feature"
kind of diagnostic in the form of -Wvla, but I don't think that
precedent applies here. It is easy to accidentally form a VLA without
intending to, there's really no evidence for accidentally using %n
that I am aware of.

> Adding a check to clang-tidy

This is the option I'm most comfortable with given the expected high
false positive rate, but I'll mention that we have no particularly
good category for this bug other than "misc". It's not pointing out a
bugrpone pattern, it has nothing to do with performance or
readability, and no coding guideline recommends diagnosing this (that
I spotted in my cursory looks). We could add it to "misc", but I'm not
certain the check is sufficiently motivated for inclusion in
clang-tidy either.

> Setting the default configuration based on the target triple (target runtimes may disallow %n altogether and we could detect this based on the target triple)
>
>
> Interested in any thoughts on:
>
> How should we enable this? (eg. flags, clang-tidy, etc)

I would be pretty strongly opposed to enabling it in Clang due to the
false positives, so I'd opt for clang-tidy if anything, but even there
feels somewhat unsatisfying without more evidence that this catches
any real-world issues. I'd want to see that check run over some large
corpus of C code to hear what kind of false and true positives it
returns.

> Should we surface this as an error or a warning?

Warning, if anything. There's no justification for an error that I can think of.

~Aaron

>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev