This patch escapes control characters fed to printk() except '\n' and '\t'.
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, e.g. using ^[1A to suppress
the previous log line.
On the testing Samsung Q310 laptop there are no users of chars outside
of the restricted charset.
v2 - Allow chars with code >127. Allow tabs.
Reported-by: Solar Designer <[email protected]>
Signed-off-by: Vasiliy Kulikov <[email protected]>
---
kernel/printk.c | 17 ++++++++++++++++-
1 files changed, 16 insertions(+), 1 deletions(-)
---
diff --git a/kernel/printk.c b/kernel/printk.c
index 3518539..727ff7d 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -41,6 +41,7 @@
#include <linux/cpu.h>
#include <linux/notifier.h>
#include <linux/rculist.h>
+#include <linux/ctype.h>
#include <asm/uaccess.h>
@@ -671,6 +672,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 (!iscntrl(c) || (c == '\n') || (c == '\t'))
+ emit_log_char(c);
+ else {
+ len = sprintf(buffer, "#x%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 +953,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;
}
---
* Vasiliy Kulikov <[email protected]> wrote:
> This patch escapes control characters fed to printk() except '\n' and '\t'.
>
> 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, e.g. using ^[1A to suppress
> the previous log line.
>
> On the testing Samsung Q310 laptop there are no users of chars outside
> of the restricted charset.
>
> v2 - Allow chars with code >127. Allow tabs.
>
> Reported-by: Solar Designer <[email protected]>
> Signed-off-by: Vasiliy Kulikov <[email protected]>
> ---
> kernel/printk.c | 17 ++++++++++++++++-
> 1 files changed, 16 insertions(+), 1 deletions(-)
>
> ---
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 3518539..727ff7d 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -41,6 +41,7 @@
> #include <linux/cpu.h>
> #include <linux/notifier.h>
> #include <linux/rculist.h>
> +#include <linux/ctype.h>
>
> #include <asm/uaccess.h>
>
> @@ -671,6 +672,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 (!iscntrl(c) || (c == '\n') || (c == '\t'))
> + emit_log_char(c);
> + else {
> + len = sprintf(buffer, "#x%02x", c);
> + for (i = 0; i < len; i++)
> + emit_log_char(buffer[i]);
> + }
Nit: please use balanced curly braces.
Also, i think it would be better to make this opt-out, i.e. exclude
the handful of control characters that are harmful (such as backline
and console escape), instead of trying to include the known-useful
ones.
The whole non-ASCII-languages issue would not have happened if such
an approach was taken.
It's also the better approach for the kernel: we handle known harmful
things and are permissive otherwise.
Thanks,
Ingo
Hi Ingo,
On Sun, Jun 26, 2011 at 12:39 +0200, Ingo Molnar wrote:
> > + if (!iscntrl(c) || (c == '\n') || (c == '\t'))
> > + emit_log_char(c);
> > + else {
> > + len = sprintf(buffer, "#x%02x", c);
> > + for (i = 0; i < len; i++)
> > + emit_log_char(buffer[i]);
> > + }
>
> Nit: please use balanced curly braces.
OK.
> Also, i think it would be better to make this opt-out, i.e. exclude
> the handful of control characters that are harmful (such as backline
> and console escape), instead of trying to include the known-useful
> ones.
Do you see any issue with the check above?
> The whole non-ASCII-languages issue would not have happened if such
> an approach was taken.
>
> It's also the better approach for the kernel: we handle known harmful
> things and are permissive otherwise.
I hope it is not a universal tip for the whole kernel development.
Black lists are almost always suck.
Could you instantly answer without reading the previous discussion what
control characters are harmful, what are sometimes harmful (on some
ttys), and what are always safe and why (or even answer why it is
harmful at all)? I'm not a tty guy and I have to read console_codes(4)
or similar docs to answer this question, the majority of kernel devs
might have to read the docs too.
Writing the black list implies the full knowledge of _all_ possible
malformed input values, which is somewhat hard to achieve (or even
impossible). Some developers might not be interested in learning such
details, but still interested in how this API can be used.
Quite the contrary, the allowed values set makes sense to the developer
and more stricktly defines the API in question. Discussing the API
goals and reaching the consensus about its usage is much more
productive. It might catch some wrong and dangerous API misuses. If the
allowed set becomes too strict one day, no problem - just explicitly
relax the check. If you lose some value in the black list (e.g. it
becomes known that some control char sequence can be used to fake the
logs), the miss significance would be higher.
And from the cynical point of view the white list is simply smaller and
cleaner than the black list.
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
* Vasiliy Kulikov <[email protected]> wrote:
> > Also, i think it would be better to make this opt-out, i.e.
> > exclude the handful of control characters that are harmful (such
> > as backline and console escape), instead of trying to include the
> > known-useful ones.
>
> Do you see any issue with the check above?
There were clear problems with the first version you posted and
that's enough proof to request the exclusion of known-dangerous
characters instead of including known-useful characters.
> > The whole non-ASCII-languages issue would not have happened if
> > such an approach was taken.
> >
> > It's also the better approach for the kernel: we handle known
> > harmful things and are permissive otherwise.
>
> I hope it is not a universal tip for the whole kernel development.
> Black lists are almost always suck.
>
> Could you instantly answer without reading the previous discussion
> what control characters are harmful, what are sometimes harmful (on
> some ttys), and what are always safe and why (or even answer why it
> is harmful at all)? I'm not a tty guy and I have to read
> console_codes(4) or similar docs to answer this question, the
> majority of kernel devs might have to read the docs too.
>
> Writing the black list implies the full knowledge of _all_ possible
> malformed input values, which is somewhat hard to achieve (or even
> impossible). Some developers might not be interested in learning
> such details, but still interested in how this API can be used.
The point is to protect against known harm and not to turn it into
"lets disable things broadly, just in case something unusual is
harmful" kind of voodoo programming.
And yes, i very much expect someone *restricting* Linux utility to
have *full* knowledge of the topic involved.
> Quite the contrary, the allowed values set makes sense to the
> developer and more stricktly defines the API in question.
> Discussing the API goals and reaching the consensus about its usage
> is much more productive. It might catch some wrong and dangerous
> API misuses. If the allowed set becomes too strict one day, no
> problem - just explicitly relax the check. If you lose some value
> in the black list (e.g. it becomes known that some control char
> sequence can be used to fake the logs), the miss significance would
> be higher.
>
> And from the cynical point of view the white list is simply smaller
> and cleaner than the black list.
A black list is well-defined: it disables the display of certain
characters because they are *known to be dangerous*.
A white list on the other hand does it the wrong way around: it tries
to put the 'burden of proof' on the useful, good guys - and that's
counter-productive really.
I don't mind protecting against known threats, with emphasis on
'known'.
Thanks,
Ingo
On Sun, Jun 26, 2011 at 20:26 +0200, Ingo Molnar wrote:
> > > Also, i think it would be better to make this opt-out, i.e.
> > > exclude the handful of control characters that are harmful (such
> > > as backline and console escape), instead of trying to include the
> > > known-useful ones.
> >
> > Do you see any issue with the check above?
>
> There were clear problems with the first version you posted and
> that's enough proof to request the exclusion of known-dangerous
> characters instead of including known-useful characters.
It doesn't proof anything. If I/someone else did a mistake with
blacklisting would you say it is enough proof to request the inclusion
of well-known allowed characters?
> A black list is well-defined: it disables the display of certain
> characters because they are *known to be dangerous*.
What do you do with dangerous characters that are *not yet known* to be
dangerous?
> A white list on the other hand does it the wrong way around: it tries
> to put the 'burden of proof' on the useful, good guys - and that's
> counter-productive really.
Really? I think strict API definition is productive, unlike using it
in cases where it looks like working, but creating tricky and obscure
bugs.
Yes, drawing multicolor logs is funny, but ...egrrr... printk() is not
written for these things.
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
* Vasiliy Kulikov <[email protected]> wrote:
> On Sun, Jun 26, 2011 at 20:26 +0200, Ingo Molnar wrote:
>
> > > > Also, i think it would be better to make this opt-out, i.e.
> > > > exclude the handful of control characters that are harmful
> > > > (such as backline and console escape), instead of trying to
> > > > include the known-useful ones.
> > >
> > > Do you see any issue with the check above?
> >
> > There were clear problems with the first version you posted and
> > that's enough proof to request the exclusion of known-dangerous
> > characters instead of including known-useful characters.
>
> It doesn't proof anything. If I/someone else did a mistake with
> blacklisting would you say it is enough proof to request the
> inclusion of well-known allowed characters?
No, because the problems such a mistake causes are not equivalent: it
would have been far more harmful to not print out the *very real*
product names written in some non-US language than to accidentally
include some control character you did not think of.
> > A black list is well-defined: it disables the display of certain
> > characters because they are *known to be dangerous*.
>
> What do you do with dangerous characters that are *not yet known*
> to be dangerous?
I'm ready to act on facts only. Also, i really prefer the policy of
acting on known dangers instead of being afraid of the unknown.
The whole 'trust but verify' thing.
> > A white list on the other hand does it the wrong way around: it
> > tries to put the 'burden of proof' on the useful, good guys - and
> > that's counter-productive really.
>
> Really? I think strict API definition is productive, unlike using
> it in cases where it looks like working, but creating tricky and
> obscure bugs.
You werent really creating a well-defined API here, were you?
> Yes, drawing multicolor logs is funny, but ...egrrr... printk() is
> not written for these things.
maybe, but i still think that such a change works better, has fewer
unintended side effects and is better documented if it excludes known
dangers instead of trying to include known useful bits imperfectly.
Thanks,
Ingo
On Sun, Jun 26, 2011 at 21:46 +0200, Ingo Molnar wrote:
> > > > > Also, i think it would be better to make this opt-out, i.e.
> > > > > exclude the handful of control characters that are harmful
> > > > > (such as backline and console escape), instead of trying to
> > > > > include the known-useful ones.
> > > >
> > > > Do you see any issue with the check above?
> > >
> > > There were clear problems with the first version you posted and
> > > that's enough proof to request the exclusion of known-dangerous
> > > characters instead of including known-useful characters.
> >
> > It doesn't proof anything. If I/someone else did a mistake with
> > blacklisting would you say it is enough proof to request the
> > inclusion of well-known allowed characters?
>
> No, because the problems such a mistake causes are not equivalent: it
> would have been far more harmful to not print out the *very real*
> product names written in some non-US language than to accidentally
> include some control character you did not think of.
???
Not "not print", but print in "crypted" form. The information is
still not lost, you can obviously restore it to the original form, with
some effort, but possible. Compare it with the harm of log spoofing -
it is not "restorable".
> > > A black list is well-defined: it disables the display of certain
> > > characters because they are *known to be dangerous*.
> >
> > What do you do with dangerous characters that are *not yet known*
> > to be dangerous?
>
> I'm ready to act on facts only.
The *fact* is you/anybody/everybody might not know all bad things. If
you just don't care because it is yet unknown then you will be
vulnerable as soon as it disclosured.
> Also, i really prefer the policy of
> acting on known dangers instead of being afraid of the unknown.
Do you know the principle "Attacks always get better, never worse"? If
you are protected against only of known attack, you will be vulnerable
to *every* danger not known to you.
Maybe you don't know, but it is really possible to be protected against
some *yet unknown* attack techniques. (The assessment of what attacks it
protects against is undefined too, though.) And upstream Linux is *already*
protected against some *yet unknown* bugs, not the whole bug classes,
but at least small kinds of it.
> > > A white list on the other hand does it the wrong way around: it
> > > tries to put the 'burden of proof' on the useful, good guys - and
> > > that's counter-productive really.
> >
> > Really? I think strict API definition is productive, unlike using
> > it in cases where it looks like working, but creating tricky and
> > obscure bugs.
>
> You werent really creating a well-defined API here, were you?
No, I was - only ascii chars and \n are allowed. In v2 all ascii chars,
the upper charset and 2 control chars are allowed. Rather clear, IMO.
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
* Vasiliy Kulikov <[email protected]> wrote:
> On Sun, Jun 26, 2011 at 21:46 +0200, Ingo Molnar wrote:
> > > > > > Also, i think it would be better to make this opt-out, i.e.
> > > > > > exclude the handful of control characters that are harmful
> > > > > > (such as backline and console escape), instead of trying to
> > > > > > include the known-useful ones.
> > > > >
> > > > > Do you see any issue with the check above?
> > > >
> > > > There were clear problems with the first version you posted and
> > > > that's enough proof to request the exclusion of known-dangerous
> > > > characters instead of including known-useful characters.
> > >
> > > It doesn't proof anything. If I/someone else did a mistake with
> > > blacklisting would you say it is enough proof to request the
> > > inclusion of well-known allowed characters?
> >
> > No, because the problems such a mistake causes are not equivalent: it
> > would have been far more harmful to not print out the *very real*
> > product names written in some non-US language than to accidentally
> > include some control character you did not think of.
>
> ???
>
> Not "not print", but print in "crypted" form. The information is
> still not lost, you can obviously restore it to the original form,
> with some effort, but possible. Compare it with the harm of log
> spoofing - it is not "restorable".
The harm of 'potential' log spoofing affecting exactly zero known
users right now, versus the harm of obfuscating the output for a
known space of USB devices that print in non-US characters, at
minimum.
> > > > A black list is well-defined: it disables the display of
> > > > certain characters because they are *known to be dangerous*.
> > >
> > > What do you do with dangerous characters that are *not yet known*
> > > to be dangerous?
> >
> > I'm ready to act on facts only.
>
> The *fact* is you/anybody/everybody might not know all bad things.
> If you just don't care because it is yet unknown then you will be
> vulnerable as soon as it disclosured.
Erm, do you claim that it's not possible to know which characters are
dangerous and which ones not?
> > Also, i really prefer the policy of acting on known dangers
> > instead of being afraid of the unknown.
>
> Do you know the principle "Attacks always get better, never worse"?
> If you are protected against only of known attack, you will be
> vulnerable to *every* danger not known to you.
>
> Maybe you don't know, but it is really possible to be protected
> against some *yet unknown* attack techniques. (The assessment of
> what attacks it protects against is undefined too, though.) And
> upstream Linux is *already* protected against some *yet unknown*
> bugs, not the whole bug classes, but at least small kinds of it.
This claim is silly - do you claim some 'unknown bug' in the ASCII
printout space?
Cannot you be bothered to enumerate the known 'bad' control and
escape characters?
You *clearly* did not consider the full utility spectrum in the first
version of the patch so i think it's necessary due diligence on our
part to ask you to be more thoughtful with this ...
> > > > A white list on the other hand does it the wrong way around:
> > > > it tries to put the 'burden of proof' on the useful, good
> > > > guys - and that's counter-productive really.
> > >
> > > Really? I think strict API definition is productive, unlike
> > > using it in cases where it looks like working, but creating
> > > tricky and obscure bugs.
> >
> > You werent really creating a well-defined API here, were you?
>
> No, I was - only ascii chars and \n are allowed. In v2 all ascii
> chars, the upper charset and 2 control chars are allowed. Rather
> clear, IMO.
Please enumerate the space you excluded and the reason for exclusion.
Terminals are not some unknown and unknowable space.
Thanks,
Ingo
On Mon, Jun 27, 2011 at 00:01 +0200, Ingo Molnar wrote:
> * Vasiliy Kulikov <[email protected]> wrote:
> > On Sun, Jun 26, 2011 at 21:46 +0200, Ingo Molnar wrote:
> > > No, because the problems such a mistake causes are not equivalent: it
> > > would have been far more harmful to not print out the *very real*
> > > product names written in some non-US language than to accidentally
> > > include some control character you did not think of.
> >
> > ???
> >
> > Not "not print", but print in "crypted" form. The information is
> > still not lost, you can obviously restore it to the original form,
> > with some effort, but possible. Compare it with the harm of log
> > spoofing - it is not "restorable".
>
> The harm of 'potential' log spoofing affecting exactly zero known
> users right now,
???
A potential thing affects all users that *can be* affected by actual
log spoofing. This is what the word "potential" means.
Analogy: if some privilege escalation bug is found in some very core
code then all users iteracting with an untrusted security domains
(local users, network, etc.) being able to exploit it would be affected.
It is silly to say that nobody is affected because you just don't know
any such cases of this bug exploitation in the past.
> > > > > A black list is well-defined: it disables the display of
> > > > > certain characters because they are *known to be dangerous*.
> > > >
> > > > What do you do with dangerous characters that are *not yet known*
> > > > to be dangerous?
> > >
> > > I'm ready to act on facts only.
> >
> > The *fact* is you/anybody/everybody might not know all bad things.
> > If you just don't care because it is yet unknown then you will be
> > vulnerable as soon as it disclosured.
>
> Erm, do you claim that it's not possible to know which characters are
> dangerous and which ones not?
A sample:
Is BEL dangerous? I can imagine 200 BELs/scrolls down would hang a
slow tty by a considerable amount of time without ^C ability to kill the
log viewer. On the rest ttys it is harmless. Even Alan wanted to add
it to the white list, not just exclude from the black list.
If someone skilled in the TTY codes provides us the full analysis of tty
code table with *a proof* what codes are harmfull (in the sense of
logging) and what are not, the question would be closed. I'm not
skilled it in, I'd not try to do it (maybe the manpage is incomplete?).
I've reported just the *known for sure* farmfull sequence (the first
report of cntrl chars harm belogs to Solar Designer and dates to 2006:
http://lists.openwall.net/linux-kernel/2006/08/22/29).
> > > Also, i really prefer the policy of acting on known dangers
> > > instead of being afraid of the unknown.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Do you know the principle "Attacks always get better, never worse"?
> > If you are protected against only of known attack, you will be
> > vulnerable to *every* danger not known to you.
> >
> > Maybe you don't know, but it is really possible to be protected
> > against some *yet unknown* attack techniques. (The assessment of
> > what attacks it protects against is undefined too, though.) And
> > upstream Linux is *already* protected against some *yet unknown*
> > bugs, not the whole bug classes, but at least small kinds of it.
>
> This claim is silly - do you claim some 'unknown bug' in the ASCII
> printout space?
You were talking about dangers in general, I'm answering about it in
general too.
> Cannot you be bothered to enumerate the known 'bad' control and
> escape characters?
Note it is not bad characters, but character sequences. You cannot just
grep for bad substrings because of KERN_CONT (because the black list means
everything not denied is permitted, and breaking ctrl sequence is then
permitted, right?), so you have to define a state machine for this
filtering (if you want to filter just only bad charsequences). It looks
too complicated for me for this little thing - just to write to a log
with user supplied data. But you say it is simplier and safer, maybe
you will do it?
> You *clearly* did not consider the full utility spectrum in the first
> version of the patch
Sure, that's why maintainers exist and they should be competent to
ack/nak the solution that does/doesn't fit the actual needs.
BTW, it has happened because of missing clear conversions about what
characters printk() can be fed. In the comment the encoding is not
mentioned, only "See also: printf(3)", which might be related to format
strings only.
Also '\n' problem with multiline messages wouldn't arrise if printk()
was clearly declared as a function logging exactly one line.
> > > > > A white list on the other hand does it the wrong way around:
> > > > > it tries to put the 'burden of proof' on the useful, good
> > > > > guys - and that's counter-productive really.
> > > >
> > > > Really? I think strict API definition is productive, unlike
> > > > using it in cases where it looks like working, but creating
> > > > tricky and obscure bugs.
> > >
> > > You werent really creating a well-defined API here, were you?
> >
> > No, I was - only ascii chars and \n are allowed. In v2 all ascii
> > chars, the upper charset and 2 control chars are allowed. Rather
> > clear, IMO.
>
> Please enumerate the space you excluded
All control characters except space chars (tabs and \n) are not needed
for the log purposes, they define the cursor position, output colors,
and tty modes. All this is not needed for log purposes, it is a cynical
log, not a cristmas tree.
> and the reason for exclusion.
Note that *you* want to define a black list with a clear denied
semantics, I want to do the reverse - to define the allowed semantics,
which additionally implies better API semantics understanding. I don't
need to enumerate all evils, I'm evading it by defining what one can do
for sure.
I don't understand why you so resist to define API more clear to avoid
other possible problems in the future.
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Mon, Jun 27, 2011 at 12:36 +0400, Vasiliy Kulikov wrote:
> > and the reason for exclusion.
>
> Note that *you* want to define a black list with a clear denied
> semantics,
Sorry, s/semantics/charsets with a proof of the harm/
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
Go back and read what I said originally, and think about it.
You have to look at the IUTF bit of the relevant console you are logging
to.
As far as I can see we need:
No IUTF: block various control codes + CSI
IUTF: block various control codes only
But arbitarily blocking anything non ASCII is going to make a mess in
anything non American. Even English needs UTF-8.
Alan
On Mon, Jun 27, 2011 at 10:40 +0100, Alan Cox wrote:
>
> Go back and read what I said originally, and think about it.
>
> You have to look at the IUTF bit of the relevant console you are logging
> to.
The knowledge of what specific console is relevant is unknown at the
moment of printk(). Do you approve only filtering on klogd side?
> But arbitarily blocking anything non ASCII is going to make a mess in
> anything non American. Even English needs UTF-8.
Sure, I don't propose it anymore (v2 goes without it).
Thanks,
--
Vasiliy Kulikov
http://www.openwall.com - bringing security into open computing environments
On Mon, Jun 27, 2011 at 11:38 AM, Vasiliy Kulikov <[email protected]> wrote:
>
> Sure, I don't propose it anymore (v2 goes without it).
What point would you like to filter things at?
I really think that user space should do its own filtering - nobody
does a plain 'cat' on dmesg. Or if they do, they really have
themselves to blame.
And afaik, we don't do any escape sequence handling at the console
level either, so you cannot mess up the console with control
characters.
And the most dangerous character seems to be one that you don't
filter: the one we really do react to is '\n', and you could possibly
make confusing log messages by embedding a newline in your string and
then trying to make the rest look like something bad (say, an oops).
So I'm not entirely convinced about this filtering at all.
Linus