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 <[email protected]>
Cc: [email protected] # 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
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 &&
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;
On Mon, Jan 27, 2014 at 5:02 PM, Ryan Mallon <[email protected]> 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 <[email protected]>
It builds and boots fine for me, FWIW.
-Kees
--
Kees Cook
Chrome OS Security
On 29/01/14 09:51, Kees Cook wrote:
> On Mon, Jan 27, 2014 at 5:02 PM, Ryan Mallon <[email protected]> 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 <[email protected]>
>
> 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
On Tue, Jan 28, 2014 at 10:54 AM, Ryan Mallon <[email protected]> wrote:
> On 29/01/14 09:51, Kees Cook wrote:
>
>> On Mon, Jan 27, 2014 at 5:02 PM, Ryan Mallon <[email protected]> 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 <[email protected]>
>>
>> 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