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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
