This patch escapes all characters outside of allowed '\n' plus 0x20-0x7E
charset passed to printk().
There are numerous printk() instances with user supplied input as "%s"
data, and unprivileged user may craft log messages with substrings
containing control characters via these printk()s. Control characters
might fool root viewing the logs via tty.
Printing non-ASCII characters is not portable since not everyone sees
the same characters after 0xFF. If any driver use it to print some
binary data, it should be fixed. Not fixed code will print hex codes
of the binary data.
On testing Samsung Q310 laptop there are no users of chars outside of the
restricted charset.
Signed-off-by: Vasiliy Kulikov <[email protected]>
---
This patch does nothing with crafted "%s" data with '\n' inside. It
allows unprivileged user to craft arbitrary log messages via breaking
log lines boundaries. It is a bit tricky to fix it compatible way.
Limiting "%s" to one line in vscnprintf() would break legitimate users
of the multiline feature. Intoducing new "%S" format for single lines
makes little sense as there are tons of printk() calls that should be
already restricted to one line.
Proposals about '\n' inside of '%s" are welcome.
kernel/printk.c | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/kernel/printk.c b/kernel/printk.c
index 3518539..1f23988 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -671,6 +671,20 @@ static void emit_log_char(char c)
logged_chars++;
}
+static void emit_log_char_escaped(char c)
+{
+ char buffer[8];
+ int i, len;
+
+ if ((c >= ' ' && c < 127) || c == '\n')
+ emit_log_char(c);
+ else {
+ len = sprintf(buffer, "#%02x", c);
+ for (i = 0; i < len; i++)
+ emit_log_char(buffer[i]);
+ }
+}
+
/*
* Zap console related locks when oopsing. Only zap at most once
* every 10 seconds, to leave time for slow consoles to print a
@@ -938,7 +952,7 @@ asmlinkage int vprintk(const char *fmt, va_list args)
break;
}
- emit_log_char(*p);
+ emit_log_char_escaped(*p);
if (*p == '\n')
new_text_line = 1;
}
--
1.7.0.4
On Wed, Jun 22, 2011 at 01:53:41PM +0400, Vasiliy Kulikov wrote:
> This patch escapes all characters outside of allowed '\n' plus 0x20-0x7E
> charset passed to printk().
>
> There are numerous printk() instances with user supplied input as "%s"
> data, and unprivileged user may craft log messages with substrings
> containing control characters via these printk()s. Control characters
> might fool root viewing the logs via tty.
There are "numerous" places this could happen? Shouldn't this be
handled by the viewers of the log file and not the kernel itself?
And what could these control characters cause to be "fooled"?
greg k-h
Hi Greg,
On Wed, Jun 22, 2011 at 08:37 -0700, Greg KH wrote:
> On Wed, Jun 22, 2011 at 01:53:41PM +0400, Vasiliy Kulikov wrote:
> > This patch escapes all characters outside of allowed '\n' plus 0x20-0x7E
> > charset passed to printk().
> >
> > There are numerous printk() instances with user supplied input as "%s"
> > data, and unprivileged user may craft log messages with substrings
> > containing control characters via these printk()s. Control characters
> > might fool root viewing the logs via tty.
>
> There are "numerous" places this could happen?
>From where I'm working now: warn_setuid_and_fcaps_mixed(),
get_file_caps(). task->comm is not filtered and sometimes it is used in
log entries related to the process. I don't know a global policy of
filtering such user supplied strings and didn't spot such filtering
(however, maybe I've missed it somewhere). It's MUCH simplier to filter
it in one place rather than hunt after the callers all over the kernel.
> Shouldn't this be
> handled by the viewers of the log file and not the kernel itself?
What viewers? Do you mean syslog implementation? IMO it's not its
duty, it logs what it was fed. It should be entiely app's care to
maintain consisten logs, syslog might not know precise log structure.
What should syslog do if app's log structure changes?
> And what could these control characters cause to be "fooled"?
E.g. line up:
ALERT!
^[1AUseless line.
TTY will interpret it as a single line "Useless line", ALERT will be
fully losen.
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Wed, 2011-06-22 at 13:53 +0400, Vasiliy Kulikov wrote:
> This patch escapes all characters outside of allowed '\n' plus 0x20-0x7E
> charset passed to printk().
[]
> diff --git a/kernel/printk.c b/kernel/printk.c
[]
> +static void emit_log_char_escaped(char c)
> +{
> + char buffer[8];
> + int i, len;
> +
> + if ((c >= ' ' && c < 127) || c == '\n')
if (isprint(c))
Why not add this to emit_log_char?
On Wed, Jun 22, 2011 at 09:38 -0700, Joe Perches wrote:
> On Wed, 2011-06-22 at 13:53 +0400, Vasiliy Kulikov wrote:
> > This patch escapes all characters outside of allowed '\n' plus 0x20-0x7E
> > charset passed to printk().
>
> []
>
> > diff --git a/kernel/printk.c b/kernel/printk.c
> []
> > +static void emit_log_char_escaped(char c)
> > +{
> > + char buffer[8];
> > + int i, len;
> > +
> > + if ((c >= ' ' && c < 127) || c == '\n')
>
> if (isprint(c))
#define isprint(c) ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0)
It slightly differs from what I've written. It (1) lacks '\n', (2)
passes non-ASCII symbols. How would non-ASCII symbols look like if
terminal doesn't support it? (I don't know, merely asking).
But if >159 (the number got from _ctype[]) is not an issue then
(isprint(c) || (c == '\n')) looks really better.
> Why not add this to emit_log_char?
No real need, but someone may want to explicitly bypass the check, it
should use emit_log_char() instead of emit_log_char_escaped().
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Wed, 2011-06-22 at 20:53 +0400, Vasiliy Kulikov wrote:
> On Wed, Jun 22, 2011 at 09:38 -0700, Joe Perches wrote:
> > > + if ((c >= ' ' && c < 127) || c == '\n')
> > if (isprint(c))
> #define isprint(c) ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0)
> It slightly differs from what I've written. It (1) lacks '\n',
You still need tab, so:
if (isprint(c) || isspace(c))
> (2) passes non-ASCII symbols.
> How would non-ASCII symbols look like if
> terminal doesn't support it? (I don't know, merely asking).
I believe most would work fine.
Are there any lp01's or la36's still connected
to a serial console?
I don't know what would happen to a 7 bit
ascii only device.
On Wed, Jun 22, 2011 at 10:14 -0700, Joe Perches wrote:
> On Wed, 2011-06-22 at 20:53 +0400, Vasiliy Kulikov wrote:
> > On Wed, Jun 22, 2011 at 09:38 -0700, Joe Perches wrote:
> > > > + if ((c >= ' ' && c < 127) || c == '\n')
> > > if (isprint(c))
> > #define isprint(c) ((__ismask(c)&(_P|_U|_L|_D|_SP)) != 0)
> > It slightly differs from what I've written. It (1) lacks '\n',
>
> You still need tab,
Correct.
> so:
>
> if (isprint(c) || isspace(c))
No, it also allows vertical tabs. Looking into __ctype only ' ', '\n' and
'\t' should be allowed among all _S, so
if (isprint(c) || (c == '\n') || (c == '\t'))
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Wed, 22 Jun 2011 09:38:03 -0700
Joe Perches <[email protected]> wrote:
> On Wed, 2011-06-22 at 13:53 +0400, Vasiliy Kulikov wrote:
> > This patch escapes all characters outside of allowed '\n' plus 0x20-0x7E
> > charset passed to printk().
I think this is fundamentally wrong.
It makes sense for some interfaces but not others and arbitarily doing it
makes a nasty mess of anything like file name printing in non English
languages.
The right way to do this IMHO is for the console device itself to have a
filter function, the default would be the 0x20-0x7E but for example with
any console which has an accompanying tty device the right behaviour
depends upon the port UTF8 flag (IUTF8).
If that is set you shouldn't be filtering out unicode, just control codes.
Minor other nit is that you might want to allow BEL through and you
certainly want to allow tab through.
The core code should not be hardcoding policy assumptions about symbol
sets and ASCII, for an awful lot of consoles today that assumption is
just plain wrong, for others it makes sense
So with tty maintainer hat on - NAK to the current approach but a good
idea to do it properly.
Hi Alan,
On Wed, Jun 22, 2011 at 19:10 +0100, Alan Cox wrote:
> If that is set you shouldn't be filtering out unicode, just control codes.
OK.
> Minor other nit is that you might want to allow BEL through and you
> certainly want to allow tab through.
In what situation do you think BEL makes sense in kernel log? I cannot
image the situation. Alarms should use KERN_EMERG/KERN_ALERT log level.
> The core code should not be hardcoding policy assumptions about symbol
> sets and ASCII, for an awful lot of consoles today that assumption is
> just plain wrong, for others it makes sense
No problem, I don't insist.
> So with tty maintainer hat on - NAK to the current approach but a good
> idea to do it properly.
The final check should be:
if (iscntrl(c) && (c != '\n') && (c != '\t'))
Any comments against this variant?
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Wed, Jun 22, 2011 at 08:37:42AM -0700, Greg KH wrote:
> On Wed, Jun 22, 2011 at 01:53:41PM +0400, Vasiliy Kulikov wrote:
> > This patch escapes all characters outside of allowed '\n' plus 0x20-0x7E
> > charset passed to printk().
> >
> > There are numerous printk() instances with user supplied input as "%s"
> > data, and unprivileged user may craft log messages with substrings
> > containing control characters via these printk()s. Control characters
> > might fool root viewing the logs via tty.
>
> There are "numerous" places this could happen?
USB product identifiers?
--
Matthew Garrett | [email protected]
On Wed, Jun 22, 2011 at 21:07, Vasiliy Kulikov <[email protected]> wrote:
> On Wed, Jun 22, 2011 at 19:10 +0100, Alan Cox wrote:
>> If that is set you shouldn't be filtering out unicode, just control codes.
>
> OK.
>
>> Minor other nit is that you might want to allow BEL through and you
>> certainly want to allow tab through.
>
> In what situation do you think BEL makes sense in kernel log? I cannot
> image the situation. Alarms should use KERN_EMERG/KERN_ALERT log level.
Does BEL work? Last time I tried fancy things (e.g. color output
depending on the
message level), it didn't work. I only got strange characters.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On Thu, Jun 23, 2011 at 02:36:05PM +0100, Matthew Garrett wrote:
> On Wed, Jun 22, 2011 at 08:37:42AM -0700, Greg KH wrote:
> > On Wed, Jun 22, 2011 at 01:53:41PM +0400, Vasiliy Kulikov wrote:
> > > This patch escapes all characters outside of allowed '\n' plus 0x20-0x7E
> > > charset passed to printk().
> > >
> > > There are numerous printk() instances with user supplied input as "%s"
> > > data, and unprivileged user may craft log messages with substrings
> > > containing control characters via these printk()s. Control characters
> > > might fool root viewing the logs via tty.
> >
> > There are "numerous" places this could happen?
>
> USB product identifiers?
That's one, sure, but the ability to overwrite something else that you
don't want someone to see based on plugging in a USB device is pretty
slim. If I can plug any type of USB device I want into the system, odds
are I just owned it anyway...
greg k-h
Hi Vasiliy,
On Wed, Jun 22, 2011 at 11:07:39PM +0400, Vasiliy Kulikov wrote:
> The final check should be:
>
> if (iscntrl(c) && (c != '\n') && (c != '\t'))
>
> Any comments against this variant?
In fact, I'm not sure we're adding that much protection with such a check
because as long as the '\n' is allowed, it's easy to fake logs. For instance :
$ cd /tmp
$ echo "main() { *(int*)0=0; }" | gcc -xc -o fail -
$ ln -s fail $'Oops: 000\nklogd'
$ ./Oops*
$ dmesg|tail -2
Oops: 000
klogd[1927]: segfault at 0 ip 0000000008048337 sp 00000000ffb54ba4 error 6 in fail[8048000+1000]
$
In an ideal world, only \n should be escaped since it's the only delimitor,
and klogd would get the raw logs with lines correctly sequenced. Other
characters should probably be escaped before going to log files if those
files are supposed to be readable on a terminal.
But I recall it was not possible to escape \n when we worked on the subject
several years ago on 2.4, because some drivers used to send multi-line logs
in a single printk().
The fundamental issue we're facing is that neither inputs nor outputs have
been clearly typed in the past. I tend to consider that a log file is readable
by "tail -f" and a such should not contain dangerous chars, however I also
tend to prefer sending raw logs over the network when they are archived by
different means. In the end it makes sense for the kernel and klogd to
exchange raw logs and syslogd should encode them when pushing them to a
file.
Best regards,
Willy