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

[Lars-Peter Clausen <lars@metafoo.de>]

On 09/16/2013 05:55 PM, Al Viro 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.
> 
> 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...
> 
> 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).
> 
> Comments?

I wrote a script the other day, which first recursively collects functions
that somehow end up passing a format string to vsnprintf. And then as a
second step finds all invocations of these functions with a non-const
string. As far as I can tell callers of vsnprintf and friends usually get it
right, it's rather functions that call a function that calls a function that
calls vsnprintf that get misused (There is one subsystem which seems to be
Swiss cheese in regard to this). So doing this just for a few functions
won't help you'd have to do this for all functions that take format strings.

- Lars