[PATCH] sysctl: terminate strings also on \r

[PATCH] sysctl: terminate strings also on \r

[Kees Cook]

From: Paul Wise <pabs3@bonedaddy.net>

This partially mitigates a common strategy used by attackers for hiding
the full contents of strings in procfs from naive sysadmins who use cat,
more or sysctl to inspect the contents of strings in procfs.

References: http://www.jakoblell.com/blog/2014/05/07/hacking-contest-hiding-stuff-from-the-terminal/
Signed-off-by: Paul Wise <pabs3@bonedaddy.net>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/sysctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 4aada6d9fe74..c34c9414caac 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
 		while ((p - buffer) < *lenp && len < maxlen - 1) {
 			if (get_user(c, p++))
 				return -EFAULT;
-			if (c == 0 || c == '\n')
+			if (c == 0 || c == '\n' || c == '\r')
 				break;
 			data[len++] = c;
 		}
-- 
1.9.1


-- 
Kees Cook
Chrome OS Security

Re: [PATCH] sysctl: terminate strings also on \r

[Aaron Tomlin]

On Tue, Oct 21, 2014 at 01:21:37PM -0700, Kees Cook wrote:
> From: Paul Wise <pabs3@bonedaddy.net>
> 
> This partially mitigates a common strategy used by attackers for hiding
> the full contents of strings in procfs from naive sysadmins who use cat,
> more or sysctl to inspect the contents of strings in procfs.
> 
> References: http://www.jakoblell.com/blog/2014/05/07/hacking-contest-hiding-stuff-from-the-terminal/
> Signed-off-by: Paul Wise <pabs3@bonedaddy.net>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  kernel/sysctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 4aada6d9fe74..c34c9414caac 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
>  		while ((p - buffer) < *lenp && len < maxlen - 1) {
>  			if (get_user(c, p++))
>  				return -EFAULT;
> -			if (c == 0 || c == '\n')
> +			if (c == 0 || c == '\n' || c == '\r')
>  				break;
>  			data[len++] = c;
>  		}
> -- 

Acked-by: Aaron Tomlin <atomlin@redhat.com>

-- 
Aaron Tomlin

Re: [PATCH] sysctl: terminate strings also on \r

[Paul E. McKenney]

On Tue, Oct 21, 2014 at 01:21:37PM -0700, Kees Cook wrote:
> From: Paul Wise <pabs3@bonedaddy.net>
> 
> This partially mitigates a common strategy used by attackers for hiding
> the full contents of strings in procfs from naive sysadmins who use cat,
> more or sysctl to inspect the contents of strings in procfs.
> 
> References: http://www.jakoblell.com/blog/2014/05/07/hacking-contest-hiding-stuff-from-the-terminal/
> Signed-off-by: Paul Wise <pabs3@bonedaddy.net>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>

> ---
>  kernel/sysctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 4aada6d9fe74..c34c9414caac 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
>  		while ((p - buffer) < *lenp && len < maxlen - 1) {
>  			if (get_user(c, p++))
>  				return -EFAULT;
> -			if (c == 0 || c == '\n')
> +			if (c == 0 || c == '\n' || c == '\r')
>  				break;
>  			data[len++] = c;
>  		}
> -- 
> 1.9.1
> 
> 
> -- 
> Kees Cook
> Chrome OS Security
> 

Re: [PATCH] sysctl: terminate strings also on \r

[David Rientjes]

On Tue, 21 Oct 2014, Kees Cook wrote:

> From: Paul Wise <pabs3@bonedaddy.net>
> 
> This partially mitigates a common strategy used by attackers for hiding
> the full contents of strings in procfs from naive sysadmins who use cat,
> more or sysctl to inspect the contents of strings in procfs.
> 
> References: http://www.jakoblell.com/blog/2014/05/07/hacking-contest-hiding-stuff-from-the-terminal/
> Signed-off-by: Paul Wise <pabs3@bonedaddy.net>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Acked-by: David Rientjes <rientjes@google.com>

Re: [PATCH] sysctl: terminate strings also on \r

[Andrew Morton]

On Tue, 21 Oct 2014 13:21:37 -0700 Kees Cook <keescook@chromium.org> wrote:

> From: Paul Wise <pabs3@bonedaddy.net>
> 
> This partially mitigates a common strategy used by attackers for hiding
> the full contents of strings in procfs from naive sysadmins who use cat,
> more or sysctl to inspect the contents of strings in procfs.
> 
> ...
>
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
>  		while ((p - buffer) < *lenp && len < maxlen - 1) {
>  			if (get_user(c, p++))
>  				return -EFAULT;
> -			if (c == 0 || c == '\n')
> +			if (c == 0 || c == '\n' || c == '\r')
>  				break;
>  			data[len++] = c;
>  		}

There are no valid uses of \r in a procfs write?

Re: [PATCH] sysctl: terminate strings also on \r

[Kees Cook]

On Wed, Oct 22, 2014 at 4:26 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 21 Oct 2014 13:21:37 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> From: Paul Wise <pabs3@bonedaddy.net>
>>
>> This partially mitigates a common strategy used by attackers for hiding
>> the full contents of strings in procfs from naive sysadmins who use cat,
>> more or sysctl to inspect the contents of strings in procfs.
>>
>> ...
>>
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
>>               while ((p - buffer) < *lenp && len < maxlen - 1) {
>>                       if (get_user(c, p++))
>>                               return -EFAULT;
>> -                     if (c == 0 || c == '\n')
>> +                     if (c == 0 || c == '\n' || c == '\r')
>>                               break;
>>                       data[len++] = c;
>>               }
>
> There are no valid uses of \r in a procfs write?

I struggle to imagine one; everything I found that uses proc_dostring
seems to be names, paths, and commands.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] sysctl: terminate strings also on \r

[Andrew Morton]

On Wed, 22 Oct 2014 16:43:10 -0700 Kees Cook <keescook@chromium.org> wrote:

> On Wed, Oct 22, 2014 at 4:26 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Tue, 21 Oct 2014 13:21:37 -0700 Kees Cook <keescook@chromium.org> wrote:
> >
> >> From: Paul Wise <pabs3@bonedaddy.net>
> >>
> >> This partially mitigates a common strategy used by attackers for hiding
> >> the full contents of strings in procfs from naive sysadmins who use cat,
> >> more or sysctl to inspect the contents of strings in procfs.
> >>
> >> ...
> >>
> >> --- a/kernel/sysctl.c
> >> +++ b/kernel/sysctl.c
> >> @@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
> >>               while ((p - buffer) < *lenp && len < maxlen - 1) {
> >>                       if (get_user(c, p++))
> >>                               return -EFAULT;
> >> -                     if (c == 0 || c == '\n')
> >> +                     if (c == 0 || c == '\n' || c == '\r')
> >>                               break;
> >>                       data[len++] = c;
> >>               }
> >
> > There are no valid uses of \r in a procfs write?
> 
> I struggle to imagine one; everything I found that uses proc_dostring
> seems to be names, paths, and commands.
> 

You're insufficiently pessimistic.

I wonder if the chances of damage would be lower if we were to continue
to accept the \r, but turn it into something else ("\r"?) when it is
read.

Re: [PATCH] sysctl: terminate strings also on \r

[Kees Cook]

On Wed, Oct 22, 2014 at 7:00 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Wed, 22 Oct 2014 16:43:10 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> On Wed, Oct 22, 2014 at 4:26 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>> > On Tue, 21 Oct 2014 13:21:37 -0700 Kees Cook <keescook@chromium.org> wrote:
>> >
>> >> From: Paul Wise <pabs3@bonedaddy.net>
>> >>
>> >> This partially mitigates a common strategy used by attackers for hiding
>> >> the full contents of strings in procfs from naive sysadmins who use cat,
>> >> more or sysctl to inspect the contents of strings in procfs.
>> >>
>> >> ...
>> >>
>> >> --- a/kernel/sysctl.c
>> >> +++ b/kernel/sysctl.c
>> >> @@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
>> >>               while ((p - buffer) < *lenp && len < maxlen - 1) {
>> >>                       if (get_user(c, p++))
>> >>                               return -EFAULT;
>> >> -                     if (c == 0 || c == '\n')
>> >> +                     if (c == 0 || c == '\n' || c == '\r')
>> >>                               break;
>> >>                       data[len++] = c;
>> >>               }
>> >
>> > There are no valid uses of \r in a procfs write?
>>
>> I struggle to imagine one; everything I found that uses proc_dostring
>> seems to be names, paths, and commands.
>>
>
> You're insufficiently pessimistic.

Haha, I haven't had that accusation made about me before; I'll keep
this quote around! :)

> I wonder if the chances of damage would be lower if we were to continue
> to accept the \r, but turn it into something else ("\r"?) when it is
> read.

I think that would complicate things more than help them. If there's a
legit use of \r, I'll let Paul Wise debate how to proceed. :)

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] sysctl: terminate strings also on \r

[Andrew Morton]

On Thu, 23 Oct 2014 09:39:09 -0700 Kees Cook <keescook@chromium.org> wrote:

> > I wonder if the chances of damage would be lower if we were to continue
> > to accept the \r, but turn it into something else ("\r"?) when it is
> > read.
> 
> I think that would complicate things more than help them.

Why.

> If there's a
> legit use of \r, I'll let Paul Wise debate how to proceed. :)

I don't know who Paul is.  Please take this seriously; we don't want to
have to revert this after breaking a bunch of people's setups.

Re: [PATCH] sysctl: terminate strings also on \r

[Kees Cook]

On Thu, Oct 23, 2014 at 11:23 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Thu, 23 Oct 2014 09:39:09 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> > I wonder if the chances of damage would be lower if we were to continue
>> > to accept the \r, but turn it into something else ("\r"?) when it is
>> > read.
>>
>> I think that would complicate things more than help them.
>
> Why.

My thinking is that currently we're only aware of \r being used to
assist an attacker. If we "escape" \r's in our output, we might run
the risk of shells interpreting things differently than expected, etc.
Right now, both the "cat" of a sysctl, and the internal use of the
string (for say, a command line) just reads directly from the string
memory. If we add escaping to that on output, we're adding
complication to this system that I think exceeds the utility of the
protection.

More simply, I would rather leave \r as-is than introduce an escaping
mechanism for just \r. If there were other cases of equally
problematic inputs, there may be a benefit, but for just \r I think
adding complexity to the sysctl code would have a net negative result.

>> If there's a
>> legit use of \r, I'll let Paul Wise debate how to proceed. :)
>
> I don't know who Paul is.  Please take this seriously; we don't want to
> have to revert this after breaking a bunch of people's setups.

Paul is the author of the patch I forwarded. I am taking it seriously
-- the only case I can come up with for \r after continuing to ponder
this is that someone has done really strange things with a
command-line or core dump pattern. Disallowing \r would break that
case, but I cannot come up with any reason why someone would attempt
to inject \r into core files or command lines via sysctl.

If there is a legit use for \r, then we can ignore this patch
entirely. It seemed reasonable to me to send it upstream since the
only use I (and others) have seen is to obfuscate attacks. If it's an
easy fix, let's do it. If it's going to break people or add code
complexity, I don't want it.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] sysctl: terminate strings also on \r

[Pavel Machek]

On Wed 2014-10-22 16:43:10, Kees Cook wrote:
> On Wed, Oct 22, 2014 at 4:26 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Tue, 21 Oct 2014 13:21:37 -0700 Kees Cook <keescook@chromium.org> wrote:
> >
> >> From: Paul Wise <pabs3@bonedaddy.net>
> >>
> >> This partially mitigates a common strategy used by attackers for hiding
> >> the full contents of strings in procfs from naive sysadmins who use cat,
> >> more or sysctl to inspect the contents of strings in procfs.
> >>
> >> ...
> >>
> >> --- a/kernel/sysctl.c
> >> +++ b/kernel/sysctl.c
> >> @@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
> >>               while ((p - buffer) < *lenp && len < maxlen - 1) {
> >>                       if (get_user(c, p++))
> >>                               return -EFAULT;
> >> -                     if (c == 0 || c == '\n')
> >> +                     if (c == 0 || c == '\n' || c == '\r')
> >>                               break;
> >>                       data[len++] = c;
> >>               }
> >
> > There are no valid uses of \r in a procfs write?
> 
> I struggle to imagine one; everything I found that uses proc_dostring
> seems to be names, paths, and commands.

Well, filename can contain \r, right?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] sysctl: terminate strings also on \r

[Geert Uytterhoeven]

On Mon, Oct 27, 2014 at 10:56 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Wed 2014-10-22 16:43:10, Kees Cook wrote:
>> On Wed, Oct 22, 2014 at 4:26 PM, Andrew Morton
>> <akpm@linux-foundation.org> wrote:
>> > On Tue, 21 Oct 2014 13:21:37 -0700 Kees Cook <keescook@chromium.org> wrote:
>> >
>> >> From: Paul Wise <pabs3@bonedaddy.net>
>> >>
>> >> This partially mitigates a common strategy used by attackers for hiding
>> >> the full contents of strings in procfs from naive sysadmins who use cat,
>> >> more or sysctl to inspect the contents of strings in procfs.
>> >>
>> >> ...
>> >>
>> >> --- a/kernel/sysctl.c
>> >> +++ b/kernel/sysctl.c
>> >> @@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
>> >>               while ((p - buffer) < *lenp && len < maxlen - 1) {
>> >>                       if (get_user(c, p++))
>> >>                               return -EFAULT;
>> >> -                     if (c == 0 || c == '\n')
>> >> +                     if (c == 0 || c == '\n' || c == '\r')
>> >>                               break;
>> >>                       data[len++] = c;
>> >>               }
>> >
>> > There are no valid uses of \r in a procfs write?
>>
>> I struggle to imagine one; everything I found that uses proc_dostring
>> seems to be names, paths, and commands.
>
> Well, filename can contain \r, right?

Even \n. AFAIK, the only thing a filename cannot contain is the nul character
and a forward slash, as that's used to separate path components (so slash
is valid for a path name).

Still, we can hide stuff using ANSI ESC sequences, and e.g. backspaces,
right?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH] sysctl: terminate strings also on \r

[Pavel Machek]

On Mon 2014-10-27 11:11:53, Geert Uytterhoeven wrote:
> On Mon, Oct 27, 2014 at 10:56 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > On Wed 2014-10-22 16:43:10, Kees Cook wrote:
> >> On Wed, Oct 22, 2014 at 4:26 PM, Andrew Morton
> >> <akpm@linux-foundation.org> wrote:
> >> > On Tue, 21 Oct 2014 13:21:37 -0700 Kees Cook <keescook@chromium.org> wrote:
> >> >
> >> >> From: Paul Wise <pabs3@bonedaddy.net>
> >> >>
> >> >> This partially mitigates a common strategy used by attackers for hiding
> >> >> the full contents of strings in procfs from naive sysadmins who use cat,
> >> >> more or sysctl to inspect the contents of strings in procfs.
> >> >>
> >> >> ...
> >> >>
> >> >> --- a/kernel/sysctl.c
> >> >> +++ b/kernel/sysctl.c
> >> >> @@ -1739,7 +1739,7 @@ static int _proc_do_string(char *data, int maxlen, int write,
> >> >>               while ((p - buffer) < *lenp && len < maxlen - 1) {
> >> >>                       if (get_user(c, p++))
> >> >>                               return -EFAULT;
> >> >> -                     if (c == 0 || c == '\n')
> >> >> +                     if (c == 0 || c == '\n' || c == '\r')
> >> >>                               break;
> >> >>                       data[len++] = c;
> >> >>               }
> >> >
> >> > There are no valid uses of \r in a procfs write?
> >>
> >> I struggle to imagine one; everything I found that uses proc_dostring
> >> seems to be names, paths, and commands.
> >
> > Well, filename can contain \r, right?
> 
> Even \n. AFAIK, the only thing a filename cannot contain is the nul character
> and a forward slash, as that's used to separate path components (so slash
> is valid for a path name).
> 
> Still, we can hide stuff using ANSI ESC sequences, and e.g. backspaces,
> right?

Right :-(. So this patch makes no sense, it would have to forbid any
character < 32 to be effective.

And more sfun is... with ESCape sequences, you can actually cause
stuff to be forced as an input on poor admin's terminal. cat could be
"fixed", but pretty much all the command line tools could be abused to
do this... I see no reasonable way forward.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html