[PATCH] vsprintf: ignore arguments to %n

[PATCH] vsprintf: ignore arguments to %n

[Kees Cook]

If arguments are consumed without output when encountering %n, it
could be used to benefit or improve information leak attacks that were
exposed via a limited size buffer. Since %n is not used by the kernel,
there is no reason to make an info leak attack any easier.

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@vger.kernel.org # 3.13+
---
 lib/vsprintf.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 185b6d300ebc..9d5c48b705f9 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1735,14 +1735,10 @@ 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,
+			 * when encountering %n, ignore the arguments.
 			 */
-			void *skip_arg;
-
-			WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
-					old_fmt);
-
-			skip_arg = va_arg(args, void *);
+			WARN_ONCE(1, "Ignored %%n in '%s'\n", old_fmt);
 			break;
 		}
 
-- 
1.7.9.5


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] vsprintf: ignore arguments to %n

[Joe Perches]

On Mon, 2014-01-27 at 16:39 -0800, Kees Cook wrote:

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1735,14 +1735,10 @@ 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,
> +			 * when encountering %n, ignore the arguments.
>  			 */
> -			void *skip_arg;
> -
> -			WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
> -					old_fmt);
> -
> -			skip_arg = va_arg(args, void *);
> +			WARN_ONCE(1, "Ignored %%n in '%s'\n", old_fmt);
>  			break;
>  		}

I don't think this is better either.

Why not tell people to fix the code?
checkpatch isn't run on quite a lot of patches.

anyway, here's a possible checkpatch test too.
---
 scripts/checkpatch.pl | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1dbd6d1..95c3264 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4069,6 +4069,15 @@ sub process {
 			}
 		}
 
+# check for unsupported uses of %n in what appear to be formats
+		if ($sline =~ /\b$logFunctions\s*\(.*"/) {
+			my $fmt = get_quoted_string($line, $rawline);
+			if ($fmt ne "" && $fmt =~ /[^\%]\%n/) {
+				WARN("PRINTF_CONTROL_N",
+				     "printf style use of \%n is not supported\n" . $herecurr);
+			}
+		}
+
 # Check for misused memsets
 		if ($^V && $^V ge 5.10.0 &&
 		    defined $stat &&

Re: [PATCH] vsprintf: ignore arguments to %n

[Ryan Mallon]

On 28/01/14 11:39, Kees Cook wrote:
> If arguments are consumed without output when encountering %n, it
> could be used to benefit or improve information leak attacks that were
> exposed via a limited size buffer. Since %n is not used by the kernel,
> there is no reason to make an info leak attack any easier.

I was thinking more like the following. Print the warning if %n is
detected in format_decode(), but otherwise just remove the handling of
%n outright and treat it like any other invalid format specifier.
Something like this completely untested patch. Thoughts?

~Ryan

---
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 10909c5..4e24009 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -364,7 +364,6 @@ enum format_type {
 	FORMAT_TYPE_SHORT,
 	FORMAT_TYPE_UINT,
 	FORMAT_TYPE_INT,
-	FORMAT_TYPE_NRCHARS,
 	FORMAT_TYPE_SIZE_T,
 	FORMAT_TYPE_PTRDIFF
 };
@@ -1512,10 +1511,6 @@ qualifier:
 		return fmt - start;
 		/* skip alnum */
 
-	case 'n':
-		spec->type = FORMAT_TYPE_NRCHARS;
-		return ++fmt - start;
-
 	case '%':
 		spec->type = FORMAT_TYPE_PERCENT_CHAR;
 		return ++fmt - start;
@@ -1538,6 +1533,15 @@ qualifier:
 	case 'u':
 		break;
 
+	case 'n':
+		/*
+		 * Since %n poses a greater security risk than utility, treat
+		 * it as an invalid format specifier. Warn about it use, so
+		 * new instances don't get added.
+		 */
+		WARN_ONCE(1, "Please remove ignored %%n in '%s'\n", fmt);
+		/* Fall-through */
+
 	default:
 		spec->type = FORMAT_TYPE_INVALID;
 		return fmt - start;
@@ -1711,20 +1715,6 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
 			++str;
 			break;
 
-		case FORMAT_TYPE_NRCHARS: {
-			/*
-			 * Since %n poses a greater security risk than
-			 * utility, ignore %n and skip its argument.
-			 */
-			void *skip_arg;
-
-			WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
-					old_fmt);
-
-			skip_arg = va_arg(args, void *);
-			break;
-		}
-
 		default:
 			switch (spec.type) {
 			case FORMAT_TYPE_LONG_LONG:
@@ -1999,19 +1989,6 @@ do {									\
 				fmt++;
 			break;
 
-		case FORMAT_TYPE_NRCHARS: {
-			/* skip %n 's argument */
-			u8 qualifier = spec.qualifier;
-			void *skip_arg;
-			if (qualifier == 'l')
-				skip_arg = va_arg(args, long *);
-			else if (_tolower(qualifier) == 'z')
-				skip_arg = va_arg(args, size_t *);
-			else
-				skip_arg = va_arg(args, int *);
-			break;
-		}
-
 		default:
 			switch (spec.type) {
 
@@ -2170,10 +2147,6 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
 			++str;
 			break;
 
-		case FORMAT_TYPE_NRCHARS:
-			/* skip */
-			break;
-
 		default: {
 			unsigned long long num;
 

Re: [PATCH] vsprintf: ignore arguments to %n

[Kees Cook]

On Mon, Jan 27, 2014 at 5:02 PM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 28/01/14 11:39, Kees Cook wrote:
>> If arguments are consumed without output when encountering %n, it
>> could be used to benefit or improve information leak attacks that were
>> exposed via a limited size buffer. Since %n is not used by the kernel,
>> there is no reason to make an info leak attack any easier.
>
> I was thinking more like the following. Print the warning if %n is
> detected in format_decode(), but otherwise just remove the handling of
> %n outright and treat it like any other invalid format specifier.
> Something like this completely untested patch. Thoughts?

I'd be totally fine with it. Minor typo in the comment before the
WARN_ONCE (should be "its" instead of "it"), but otherwise looks good.
Consider it:

Acked-by: Kees Cook <keescook@chromium.org>

It builds and boots fine for me, FWIW.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] vsprintf: ignore arguments to %n

[Ryan Mallon]

On 29/01/14 09:51, Kees Cook wrote:

> On Mon, Jan 27, 2014 at 5:02 PM, Ryan Mallon <rmallon@gmail.com> wrote:
>> On 28/01/14 11:39, Kees Cook wrote:
>>> If arguments are consumed without output when encountering %n, it
>>> could be used to benefit or improve information leak attacks that were
>>> exposed via a limited size buffer. Since %n is not used by the kernel,
>>> there is no reason to make an info leak attack any easier.
>>
>> I was thinking more like the following. Print the warning if %n is
>> detected in format_decode(), but otherwise just remove the handling of
>> %n outright and treat it like any other invalid format specifier.
>> Something like this completely untested patch. Thoughts?
> 
> I'd be totally fine with it. Minor typo in the comment before the
> WARN_ONCE (should be "its" instead of "it"), but otherwise looks good.
> Consider it:
> 
> Acked-by: Kees Cook <keescook@chromium.org>
> 
> It builds and boots fine for me, FWIW.
> 
> -Kees
> 


It looks like your second version already got added to Andrew's mm tree.
I'm happy to repost mine with a fixed typo and proper signed-off by if
you'd rather use that version.

~Ryan
 

Re: [PATCH] vsprintf: ignore arguments to %n

[Kees Cook]

On Tue, Jan 28, 2014 at 10:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
> On 29/01/14 09:51, Kees Cook wrote:
>
>> On Mon, Jan 27, 2014 at 5:02 PM, Ryan Mallon <rmallon@gmail.com> wrote:
>>> On 28/01/14 11:39, Kees Cook wrote:
>>>> If arguments are consumed without output when encountering %n, it
>>>> could be used to benefit or improve information leak attacks that were
>>>> exposed via a limited size buffer. Since %n is not used by the kernel,
>>>> there is no reason to make an info leak attack any easier.
>>>
>>> I was thinking more like the following. Print the warning if %n is
>>> detected in format_decode(), but otherwise just remove the handling of
>>> %n outright and treat it like any other invalid format specifier.
>>> Something like this completely untested patch. Thoughts?
>>
>> I'd be totally fine with it. Minor typo in the comment before the
>> WARN_ONCE (should be "its" instead of "it"), but otherwise looks good.
>> Consider it:
>>
>> Acked-by: Kees Cook <keescook@chromium.org>
>>
>> It builds and boots fine for me, FWIW.
>>
>> -Kees
>>
>
>
> It looks like your second version already got added to Andrew's mm tree.
> I'm happy to repost mine with a fixed typo and proper signed-off by if
> you'd rather use that version.

I think yours is much cleaner: it entirely removes the %n processing logic.

-Kees

-- 
Kees Cook
Chrome OS Security