[PATCH] vsprintf: BUG on %n

[PATCH] vsprintf: BUG on %n

[Kees Cook]

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.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/vsprintf.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 185b6d300ebc..a27fd7f61325 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1735,15 +1735,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 		case FORMAT_TYPE_NRCHARS: {
 			/*
 			 * Since %n poses a greater security risk than
-			 * utility, ignore %n and skip its argument.
+			 * utility, it should not be implemented. Instead,
+			 * BUG when encountering %n, since there are no
+			 * legitimate users and skipping arguments could
+			 * assist information leak attacks.
 			 */
-			void *skip_arg;
-
-			WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
-					old_fmt);
-
-			skip_arg = va_arg(args, void *);
-			break;
+			BUG();
 		}
 
 		default:
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] vsprintf: BUG on %n

[Ryan Mallon]

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.

~Ryan

> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  lib/vsprintf.c |   13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 185b6d300ebc..a27fd7f61325 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1735,15 +1735,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  		case FORMAT_TYPE_NRCHARS: {
>  			/*
>  			 * Since %n poses a greater security risk than
> -			 * utility, ignore %n and skip its argument.
> +			 * utility, it should not be implemented. Instead,
> +			 * BUG when encountering %n, since there are no
> +			 * legitimate users and skipping arguments could
> +			 * assist information leak attacks.
>  			 */
> -			void *skip_arg;
> -
> -			WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
> -					old_fmt);
> -
> -			skip_arg = va_arg(args, void *);
> -			break;
> +			BUG();
>  		}
>  
>  		default:
> 

Re: [PATCH] vsprintf: BUG on %n

[Kees Cook]

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?

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] vsprintf: BUG on %n

[Ryan Mallon]

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

Re: [PATCH] vsprintf: BUG on %n

[Kees Cook]

On Mon, Jan 27, 2014 at 4:11 PM, Ryan Mallon <rmallon@gmail.com> wrote:
> 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?

Yeah, that's in my tree. Since gcc loves reporting false positives, I
haven't wanted to try to push it into mainline. But with Fengguang
Wu's robots doing the builds, I've been trying to keep real flaws out
of the tree.

> 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.

Right, I totally agree.

> %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.

In that regard, then, I think we should just not skip the argument.
(Imagine, as an attacker trying to find some deep value on the stack,
but using %x would fill up the buffer that is being written to. %n
doing arg-skipping would help in that case.)

I'll send a different patch.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] vsprintf: BUG on %n

[Joe Perches]

On Mon, 2014-01-27 at 15:03 -0800, 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.
[]
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1735,15 +1735,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
>  		case FORMAT_TYPE_NRCHARS: {
>  			/*
>  			 * Since %n poses a greater security risk than
> -			 * utility, ignore %n and skip its argument.
> +			 * utility, it should not be implemented. Instead,
> +			 * BUG when encountering %n, since there are no
> +			 * legitimate users and skipping arguments could
> +			 * assist information leak attacks.
>  			 */
> -			void *skip_arg;
> -
> -			WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
> -					old_fmt);
> -
> -			skip_arg = va_arg(args, void *);
> -			break;
> +			BUG();

BUGs should be avoided where possible.

I think using BUG here isn't necessary or good.

Re: [PATCH] vsprintf: BUG on %n

[Andrew Morton]

On Mon, 27 Jan 2014 15:12:34 -0800 Joe Perches <joe@perches.com> wrote:

> On Mon, 2014-01-27 at 15:03 -0800, 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.
> []
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > @@ -1735,15 +1735,12 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
> >  		case FORMAT_TYPE_NRCHARS: {
> >  			/*
> >  			 * Since %n poses a greater security risk than
> > -			 * utility, ignore %n and skip its argument.
> > +			 * utility, it should not be implemented. Instead,
> > +			 * BUG when encountering %n, since there are no
> > +			 * legitimate users and skipping arguments could
> > +			 * assist information leak attacks.
> >  			 */
> > -			void *skip_arg;
> > -
> > -			WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
> > -					old_fmt);
> > -
> > -			skip_arg = va_arg(args, void *);
> > -			break;
> > +			BUG();
> 
> BUGs should be avoided where possible.
> 
> I think using BUG here isn't necessary or good.
> 

Good point(s).  

In fact the patch makes the kernel less secure - it provides a way for
people to make the kernel crash if they a) are able to inject printk
control strings and b) don't know about %s ;)