Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1232971 > unrolled thread
| Started by | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| First post | 2015-09-25 19:50 +0200 |
| Last post | 2015-09-29 00:40 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
[PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-09-25 19:50 +0200
Re: [PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2015-09-28 10:10 +0200
Re: [PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-09-28 22:20 +0200
Re: [PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly Kees Cook <keescook@chromium.org> - 2015-09-29 00:40 +0200
| From | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| Date | 2015-09-25 19:50 +0200 |
| Subject | [PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly |
| Message-ID | <qcFia-8p1-15@gated-at.bofh.it> |
If we meet any invalid or unsupported format specifier, 'handling' it
by just printing it as a literal string is not safe: Presumably the
format string and the arguments passed gcc's type checking, but that
means something like sprintf(buf, "%n %pd", &intvar, dentry) would end
up interpreting &intvar as a struct dentry*.
When the offending specifier was %n it used to be at the end of the
format string, but we can't rely on that always being the case. Also,
gcc doesn't complain about some more or less exotic qualifiers (or
'length modifiers' in posix-speak) such as 'j' or 'q', but being
unrecognized by the kernel's printf implementation, they'd be
interpreted as unknown specifiers, and the rest of arguments would be
interpreted wrongly.
So let's complain about anything we don't understand, not just %n, and
stop pretending that we'd be able to make sense of the rest of the
format/arguments. If the offending specifier is in a printk() call we
unfortunately only get a "BUG: recent printk recursion!", but at least
direct users of the sprintf family will be caught.
Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
lib/vsprintf.c | 31 +++++++++++++++++++++----------
1 file changed, 21 insertions(+), 10 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 95cd63b43b99..f2590a80937f 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1769,14 +1769,14 @@ qualifier:
case 'n':
/*
- * Since %n poses a greater security risk than utility, treat
- * it as an invalid format specifier. Warn about its use so
- * that new instances don't get added.
+ * Since %n poses a greater security risk than
+ * utility, treat it as any other invalid or
+ * unsupported format specifier.
*/
- WARN_ONCE(1, "Please remove ignored %%n in '%s'\n", fmt);
/* Fall-through */
default:
+ WARN_ONCE(1, "Please remove unsupported %%%c in format string\n", *fmt);
spec->type = FORMAT_TYPE_INVALID;
return fmt - start;
}
@@ -1944,10 +1944,15 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
break;
case FORMAT_TYPE_INVALID:
- if (str < end)
- *str = '%';
- ++str;
- break;
+ /*
+ * Presumably the arguments passed gcc's type
+ * checking, but there is no safe or sane way
+ * for us to continue parsing the format and
+ * fetching from the va_list; the remaining
+ * specifiers and arguments would be out of
+ * sync.
+ */
+ goto out;
default:
switch (spec.type) {
@@ -1992,6 +1997,7 @@ int vsnprintf(char *buf, size_t size, const char *fmt, va_list args)
}
}
+out:
if (size > 0) {
if (str < end)
*str = '\0';
@@ -2189,9 +2195,10 @@ do { \
switch (spec.type) {
case FORMAT_TYPE_NONE:
- case FORMAT_TYPE_INVALID:
case FORMAT_TYPE_PERCENT_CHAR:
break;
+ case FORMAT_TYPE_INVALID:
+ goto out;
case FORMAT_TYPE_WIDTH:
case FORMAT_TYPE_PRECISION:
@@ -2253,6 +2260,7 @@ do { \
}
}
+out:
return (u32 *)(PTR_ALIGN(str, sizeof(u32))) - bin_buf;
#undef save_arg
}
@@ -2375,12 +2383,14 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
break;
case FORMAT_TYPE_PERCENT_CHAR:
- case FORMAT_TYPE_INVALID:
if (str < end)
*str = '%';
++str;
break;
+ case FORMAT_TYPE_INVALID:
+ goto out;
+
default: {
unsigned long long num;
@@ -2423,6 +2433,7 @@ int bstr_printf(char *buf, size_t size, const char *fmt, const u32 *bin_buf)
} /* switch(spec.type) */
} /* while(*fmt) */
+out:
if (size > 0) {
if (str < end)
*str = '\0';
--
2.1.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Andy Shevchenko <andriy.shevchenko@linux.intel.com> |
|---|---|
| Date | 2015-09-28 10:10 +0200 |
| Subject | Re: [PATCH 1/4] lib/vsprintf.c: handle invalid format specifiers more robustly |
| Message-ID | <qdBFw-8kb-15@gated-at.bofh.it> |
| In reply to | #1232971 |
On Fri, 2015-09-25 at 19:41 +0200, Rasmus Villemoes wrote:
> If we meet any invalid or unsupported format specifier, 'handling' it
> by just printing it as a literal string is not safe: Presumably the
> format string and the arguments passed gcc's type checking, but that
> means something like sprintf(buf, "%n %pd", &intvar, dentry) would
> end
> up interpreting &intvar as a struct dentry*.
>
> When the offending specifier was %n it used to be at the end of the
> format string, but we can't rely on that always being the case. Also,
> gcc doesn't complain about some more or less exotic qualifiers (or
> 'length modifiers' in posix-speak) such as 'j' or 'q', but being
> unrecognized by the kernel's printf implementation, they'd be
> interpreted as unknown specifiers, and the rest of arguments would be
> interpreted wrongly.
>
> So let's complain about anything we don't understand, not just %n,
> and
> stop pretending that we'd be able to make sense of the rest of the
> format/arguments. If the offending specifier is in a printk() call we
> unfortunately only get a "BUG: recent printk recursion!", but at
> least
> direct users of the sprintf family will be caught.
I like the fix (I noticed the same issue after commented out Martin's
patch).
Few minor comments below.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
> lib/vsprintf.c | 31 +++++++++++++++++++++----------
> 1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 95cd63b43b99..f2590a80937f 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1769,14 +1769,14 @@ qualifier:
>
> case 'n':
> /*
> - * Since %n poses a greater security risk than
> utility, treat
> - * it as an invalid format specifier. Warn about its
> use so
> - * that new instances don't get added.
> + * Since %n poses a greater security risk than
Any reason to wrap first string?
> + * utility, treat it as any other invalid or
> + * unsupported format specifier.
> */
> - WARN_ONCE(1, "Please remove ignored %%n in '%s'\n",
> fmt);
> /* Fall-through */
>
> default:
> + WARN_ONCE(1, "Please remove unsupported %%%c in
> format string\n", *fmt);
> spec->type = FORMAT_TYPE_INVALID;
> return fmt - start;
> }
> @@ -1944,10 +1944,15 @@ int vsnprintf(char *buf, size_t size, const
> char *fmt, va_list args)
> break;
>
> case FORMAT_TYPE_INVALID:
> - if (str < end)
> - *str = '%';
> - ++str;
> - break;
> + /*
> + * Presumably the arguments passed gcc's
> type
> + * checking, but there is no safe or sane
> way
> + * for us to continue parsing the format and
> + * fetching from the va_list; the remaining
> + * specifiers and arguments would be out of
> + * sync.
Could we use wider strings in the commentary here?
> + */
> + goto out;
>
> default:
> switch (spec.type) {
> @@ -1992,6 +1997,7 @@ int vsnprintf(char *buf, size_t size, const
> char *fmt, va_list args)
> }
> }
>
> +out:
> if (size > 0) {
> if (str < end)
> *str = '\0';
> @@ -2189,9 +2195,10 @@ do { > > > > \
>
>
> switch (spec.type) {
> case FORMAT_TYPE_NONE:
> - case FORMAT_TYPE_INVALID:
> case FORMAT_TYPE_PERCENT_CHAR:
> break;
> + case FORMAT_TYPE_INVALID:
> + goto out;
>
> case FORMAT_TYPE_WIDTH:
> case FORMAT_TYPE_PRECISION:
> @@ -2253,6 +2260,7 @@ do { > > > > \
>
> }
> }
>
> +out:
> return (u32 *)(PTR_ALIGN(str, sizeof(u32))) - bin_buf;
> #undef save_arg
> }
> @@ -2375,12 +2383,14 @@ int bstr_printf(char *buf, size_t size, const
> char *fmt, const u32 *bin_buf)
> break;
>
> case FORMAT_TYPE_PERCENT_CHAR:
> - case FORMAT_TYPE_INVALID:
> if (str < end)
> *str = '%';
> ++str;
> break;
>
> + case FORMAT_TYPE_INVALID:
> + goto out;
> +
> default: {
> unsigned long long num;
>
> @@ -2423,6 +2433,7 @@ int bstr_printf(char *buf, size_t size, const
> char *fmt, const u32 *bin_buf)
> } /* switch(spec.type) */
> } /* while(*fmt) */
>
> +out:
> if (size > 0) {
> if (str < end)
> *str = '\0';
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| Date | 2015-09-28 22:20 +0200 |
| Message-ID | <qdN3Y-1rA-19@gated-at.bofh.it> |
| In reply to | #1233931 |
On Mon, Sep 28 2015, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: >> /* >> - * Since %n poses a greater security risk than >> utility, treat >> - * it as an invalid format specifier. Warn about its >> use so >> - * that new instances don't get added. >> + * Since %n poses a greater security risk than > > Any reason to wrap first string? I just hit M-q in emacs and let that take care of somewhat sane wrapping. I don't play diff golf. >> + /* >> + * Presumably the arguments passed gcc's >> type >> + * checking, but there is no safe or sane >> way >> + * for us to continue parsing the format and >> + * fetching from the va_list; the remaining >> + * specifiers and arguments would be out of >> + * sync. > > Could we use wider strings in the commentary here? Ditto. Rasmus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2015-09-29 00:40 +0200 |
| Message-ID | <qdPfr-4Ba-1@gated-at.bofh.it> |
| In reply to | #1232971 |
On Fri, Sep 25, 2015 at 10:41 AM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > If we meet any invalid or unsupported format specifier, 'handling' it > by just printing it as a literal string is not safe: Presumably the > format string and the arguments passed gcc's type checking, but that > means something like sprintf(buf, "%n %pd", &intvar, dentry) would end > up interpreting &intvar as a struct dentry*. > > When the offending specifier was %n it used to be at the end of the > format string, but we can't rely on that always being the case. Also, > gcc doesn't complain about some more or less exotic qualifiers (or > 'length modifiers' in posix-speak) such as 'j' or 'q', but being > unrecognized by the kernel's printf implementation, they'd be > interpreted as unknown specifiers, and the rest of arguments would be > interpreted wrongly. > > So let's complain about anything we don't understand, not just %n, and > stop pretending that we'd be able to make sense of the rest of the > format/arguments. If the offending specifier is in a printk() call we > unfortunately only get a "BUG: recent printk recursion!", but at least > direct users of the sprintf family will be caught. I like it! Thanks :) Acked-by: Kees Cook <keescook@chromium.org> -Kees -- Kees Cook Chrome OS Security -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web