Re: [PATCH 0/2] vsprintf: ignore %n again

[Kees Cook]

On Mon, Sep 16, 2013 at 8:55 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Sep 16, 2013 at 12:43:55AM -0700, Kees Cook wrote:
>> Whether seq_printf should return void or error, %n still needs to be removed.
>> As such, instead of changing the seq_file structure and adding instructions
>> to all callers of seq_printf, just examine seq->count for the callers that
>> care about how many characters were put into the buffer, as suggested by
>> George Spelvin. First patch removes all %n usage in favor of checking
>> seq->count before/after. Second patch makes %n ignore its argument.
>
> This is completely pointless.  *ANY* untrusted format string kernel-side
> is pretty much it.  Blocking %n is not "defense in depth", it's security
> theater.  Again, if attacker can feed an arbitrary format string to
> vsnprintf(), it's over - you've lost.  It's not just about information
> leaks vs. ability to store a value of attacker's choosing at the address
> of attacker's choosing as it was in userland.  Kernel-side an ability to
> trigger read from an arbitrary address is much nastier than information
> leak risk; consider iomem, for starters.

I completely disagree. (Surprise!) Eliminating %n _does_ change
things. Why have the risk when solutions for getting the same behavior
take fewer instructions and are safer? Yes, I agree that arbitrary
format strings are still a very very bad thing, but %n makes it an
order of magnitude worse. Attacking format strings with %n is very
well understood by exploiters, so why give them this primitive for no
sensible reason?

> What we ought to do is prevention of _that_.  AFAICS, we have reasonably
> few call chains that might transmit format string; most of the calls
> are with plain and simple string literal.  I wonder if could get away
> with reasonable amount of annotations to catch such crap...

Yes. And if gcc would ignore const char arguments, we could do this:

diff --git a/Makefile b/Makefile
index e73f758..2bc680b 100644
--- a/Makefile
+++ b/Makefile
@@ -372,7 +372,6 @@ KBUILD_CPPFLAGS := -D__KERNEL__
 KBUILD_CFLAGS   := -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs \
                   -fno-strict-aliasing -fno-common \
                   -Werror-implicit-function-declaration \
-                  -Wno-format-security \
                   -fno-delete-null-pointer-checks
 KBUILD_AFLAGS_KERNEL :=
 KBUILD_CFLAGS_KERNEL :=
@@ -659,6 +658,11 @@ KBUILD_CFLAGS      += $(call cc-option,-fno-strict-overflow
 # conserve stack if available
 KBUILD_CFLAGS   += $(call cc-option,-fconserve-stack)

+# Enable format-security when it can stop the build, otherwise disable.
+KBUILD_CFLAGS  += $(call cc-option,\
+                       -Wformat -Wformat-security -Werror=format-security,\
+                       -Wno-format-security)
+
 # use the deterministic mode of AR if available
 KBUILD_ARFLAGS := $(call ar-option,D)

I have a patch series that fixes all the warnings produced by gcc in
this state. All of our printf uses are already marked up in the
kernel, so this checks them all.

> Consider, e.g. introducing __vsnprint(), with vsnprintf(s, n, fmt, ...)
> expanding to __vsnprintf(1, s, n, fmt, ...) if fmt is a string literal
> and __vsnprintf(0, s, n, fmt, ...) otherwise.  Now,
> int __sprintf(int safe, char *buf, const char *fmt, ...)
> {
>         va_list args;
>         int i;
>
>         va_start(args, fmt);
>         i = __vsnprintf(safe, buf, INT_MAX, fmt, args);
>         va_end(args);
>
>         return i;
> }
> and #define for sprintf (expanding it to either __sprintf(1, ...)
> or __sprintf(0, ...)).  That plus similar for snprintf and seq_printf
> will already take care of most of the call chains leading to __vsnprintf() -
> relatively few calls with have 0 passed to it.  Add WARN_ON(!safe) to
> __vsnprintf and we probably won't drown in warnings.  Now, we can start
> adding things like that to remaining call chains *and* do things like
> replacing
>                         snd_iprintf(buffer, fields[i].format,
>                                 *get_dummy_ll_ptr(dummy, fields[i].offset));
> with
>                         /* fields[i].format is known to be a valid format */
>                         __snd_iprintf(1, buffer, fields[i].format,
>                                 *get_dummy_ll_ptr(dummy, fields[i].offset));
> to deal with the places where the origin of format string is provably safe,
> but not a string literal (actually, s/1/__FORMAT_IS_SAFE/, to make it
> greppable).

Unless I've misunderstood, I think we'd already get very close to this
with the gcc options instead. This patch set is what I've been using
to generate the format string fixes over the last few months, with 7
sent this last round:

http://git.kernel.org/cgit/linux/kernel/git/kees/linux.git/log/?h=format-security

-Kees

-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/