kernel: escape non-ASCII and control characters in printk()

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.

---

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(-)

index 3518539..1f23988 100644
@@ -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

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,

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.

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?

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

USB product identifiers?

Matthew Garrett | [email protected]

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

well, currently cat /proc/kmsg is bad idea on multiuser system... right?

terminals are powerful (or shall we say insecure?) these days. Including replies.

cat /proc/kmsg can result in something like "9q" be added to the next bash command. It would not surprise me if nastier stuff was possible with some xterm variant.

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[]

	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.

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

You still need tab, so:

if (isprint(c) || isspace(c))

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.

Correct.

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

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,

OK.

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.

No problem, I don't insist.

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

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

Hi Vasiliy,

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