Re: [PATCH] vsprintf: BUG on %n

[Ryan Mallon <rmallon@gmail.com>]

On 28/01/14 10:56, Kees Cook wrote:
> On Mon, Jan 27, 2014 at 3:11 PM, Ryan Mallon <rmallon@gmail.com> wrote:
>> On 28/01/14 10:03, Kees Cook wrote:
>>> Now that there has been a full release of the kernel, and all users
>>> of %n have been dropped, switch to %n use triggering a BUG. Ignoring
>>> arguments could be used to assist in information leaks if an arbitrary
>>> format string was under the control of an attacker.
>>
>> Not sure I follow the reasoning. %n no longer does anything in the
>> kernel, so there is no risk if it does manage to find its way into a
>> printed string. BUG() is for unrecoverable errors, which this clearly isn't.
>>
>> Information leaks via injectable strings are still possible if an
>> attacker can insert %x, %d, etc. %n is more problematic since it allows
>> for code injection, which is why it got removed. %n is not however,
>> required to get an infoleak via a format string, so I think the summary
>> is also a bit misleading.
> 
> Yeah, I'm a bit uncomfortable with the BUG() too. The issue with %n is
> that it would produce no output at all to skip arguments. With other
> things, you have to take up output space, which may be limited. How
> about just not skipping the argument? Leave the warn_on, etc?

If you are trying to catch in kernel users of %n, then the warning is
probably fine. I don't think the presense of a %n in a format string,
without any injection vulnerability is going to cause a problem.

If you are trying to catch %n being injected by a malicious user into a
vulnerable string then a warning is fine as long as the string doesn't
allow code injection through some other means. I don't think you can
easily prevent infoleaks at runtime, since any vulnerable can have %x,
%s, or whatever injected to leak information on the stack. There was
some work on detecting potentially vulnerable strings at compile time I
think?

The reason to get rid of %n is to remove the ability to escalate an
infoleak on a vulnerable format string into code execution. Vulnerable
strings and infoleaks via them are really a separate issue, and
detecting %n does nothing to solve them.

%n should probably just be treated the same as any other %FOO which is
not a valid format string directive. Keeping the warning might be useful
for kernel developers who don't know that they shouldn't be using it.
Then again, sparse, checkpatch or code review might be the better place
to do that.

~Ryan