[PATCH] Fix user data corrupted by old value return of sysctl

[PATCH] Fix user data corrupted by old value return of sysctl

[Yi Yang]

If the user reads a sysctl entry which is of string type
  by sysctl syscall, this call probably corrupts the user data
  right after the old value buffer, the issue lies in sysctl_string
  seting 0 to oldval[len], len is the available buffer size
  specified by the user, obviously, this will write to the first
  byte of the user memory place immediate after the old value buffer
, the correct way is that sysctl_string doesn't set 0, the user
should do it by self in the program.

The following program verifies this point:

#include <linux/unistd.h>
#include <linux/types.h>
#include <linux/sysctl.h>
#include <errno.h>

_syscall1(int, _sysctl, struct __sysctl_args *, args);
int sysctl(int *name, int nlen, void *oldval, size_t *oldlenp,
            void *newval, size_t newlen)
{
         struct __sysctl_args args
		= {name,nlen,oldval,oldlenp,newval,newlen};

         return _sysctl(&args);
}

#define SIZE(x) sizeof(x)/sizeof(x[0])
#define OSNAMESZ 4

struct mystruct {
	char osname[OSNAMESZ];
	int target;
	int osnamelth;
} myos;

int name[] = { CTL_KERN, KERN_NODENAME };

int main(int argc, char * argv[])
{
	myos.target = 1;
	printf("target = %d\n", myos.target);
	myos.osnamelth = SIZE(myos.osname);
         if (sysctl(name, SIZE(name), myos.osname,
			&myos.osnamelth, 0, 0))
                 perror("sysctl");
	else {
                 printf("Current host name: %s\n", myos.osname);
	}
	printf("target = %d\n", myos.target);
         return 0;
}

Copy it to file sysctl-safe.c, then
$ hostname
mylocalmachine
$ gcc sysctl-safe.c
$ ./a.out
target = 1
Current host name: mylo
target = 0
$

After apply this patch:
$ hostname
mylocalmachine
$ gcc sysctl-safe.c
$ ./a.out
target = 1
Current host name: mylo
target = 1

Signed-off-by: Yi Yang <yang.y.yi@gmail.com>


--- a/kernel/sysctl.c.orig	2005-12-30 09:21:34.000000000 +0000
+++ b/kernel/sysctl.c	2005-12-30 15:58:15.000000000 +0000
@@ -2207,8 +2207,6 @@ int sysctl_string(ctl_table *table, int
  				len = table->maxlen;
  			if(copy_to_user(oldval, table->data, len))
  				return -EFAULT;
-			if(put_user(0, ((char __user *) oldval) + len))
-				return -EFAULT;
  			if(put_user(len, oldlenp))
  				return -EFAULT;
  		}

Re: [PATCH] Fix user data corrupted by old value return of sysctl

[Linus Torvalds]

On Fri, 30 Dec 2005, Yi Yang wrote:
>
> If the user reads a sysctl entry which is of string type
> by sysctl syscall, this call probably corrupts the user data
> right after the old value buffer, the issue lies in sysctl_string
> seting 0 to oldval[len], len is the available buffer size
> specified by the user, obviously, this will write to the first
> byte of the user memory place immediate after the old value buffer,
> the correct way is that sysctl_string doesn't set 0, the user
> should do it by self in the program.

Hmm.. I think this patch is incomplete.

We _should_ zero-pad the data, at least if the result fits in the buffer.

So I think the correct fix is to just _copy_ the last zero if it fits in 
the buffer, rather than do the unconditional "add NUL at the end" thing. 
The simplest way to do that is to just make "l" be "strlen(str)+1", so 
that we count the ending NUL in the length (and then, if the buffer isn't 
big enough, we will truncate it).

In other words, I would instead suggest a patch like the appended.

But even that is questionable: one alternative is to always zero-pad (like 
we used to), but make sure that the buffer size is sufficient for it (ie 
instead of adding one to the length of the string, we'd subtract one from 
the buffer length and make sure that the '\0' fits..

Comments?

		Linus
---
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 9990e10..ad0425a 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2201,14 +2201,12 @@ int sysctl_string(ctl_table *table, int 
 		if (get_user(len, oldlenp))
 			return -EFAULT;
 		if (len) {
-			l = strlen(table->data);
+			l = strlen(table->data)+1;
 			if (len > l) len = l;
 			if (len >= table->maxlen)
 				len = table->maxlen;
 			if(copy_to_user(oldval, table->data, len))
 				return -EFAULT;
-			if(put_user(0, ((char __user *) oldval) + len))
-				return -EFAULT;
 			if(put_user(len, oldlenp))
 				return -EFAULT;
 		}

Re: [PATCH] Fix user data corrupted by old value return of sysctl

[Yi Yang]

Linus Torvalds wrote:

>On Fri, 30 Dec 2005, Yi Yang wrote:
>  
>
>>If the user reads a sysctl entry which is of string type
>>by sysctl syscall, this call probably corrupts the user data
>>right after the old value buffer, the issue lies in sysctl_string
>>seting 0 to oldval[len], len is the available buffer size
>>specified by the user, obviously, this will write to the first
>>byte of the user memory place immediate after the old value buffer,
>>the correct way is that sysctl_string doesn't set 0, the user
>>should do it by self in the program.
>>    
>>
>
>Hmm.. I think this patch is incomplete.
>
>We _should_ zero-pad the data, at least if the result fits in the buffer.
>
>So I think the correct fix is to just _copy_ the last zero if it fits in 
>the buffer, rather than do the unconditional "add NUL at the end" thing. 
>The simplest way to do that is to just make "l" be "strlen(str)+1", so 
>that we count the ending NUL in the length (and then, if the buffer isn't 
>big enough, we will truncate it).
>
>In other words, I would instead suggest a patch like the appended.
>
>But even that is questionable: one alternative is to always zero-pad (like 
>we used to), but make sure that the buffer size is sufficient for it (ie 
>instead of adding one to the length of the string, we'd subtract one from 
>the buffer length and make sure that the '\0' fits..
>
>Comments?
>  
>
Yes, you are more complete, I agree with it very much.

>		Linus
>---
>diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>index 9990e10..ad0425a 100644
>--- a/kernel/sysctl.c
>+++ b/kernel/sysctl.c
>@@ -2201,14 +2201,12 @@ int sysctl_string(ctl_table *table, int 
> 		if (get_user(len, oldlenp))
> 			return -EFAULT;
> 		if (len) {
>-			l = strlen(table->data);
>+			l = strlen(table->data)+1;
> 			if (len > l) len = l;
> 			if (len >= table->maxlen)
> 				len = table->maxlen;
> 			if(copy_to_user(oldval, table->data, len))
> 				return -EFAULT;
>-			if(put_user(0, ((char __user *) oldval) + len))
>-				return -EFAULT;
> 			if(put_user(len, oldlenp))
> 				return -EFAULT;
> 		}
>
>  
>

Re: [PATCH] Fix user data corrupted by old value return of sysctl

[Coywolf Qi Hunt]

On Fri, Dec 30, 2005 at 09:25:35AM -0800, Linus Torvalds wrote:
> 
> 
> On Fri, 30 Dec 2005, Yi Yang wrote:
> >
> > If the user reads a sysctl entry which is of string type
> > by sysctl syscall, this call probably corrupts the user data
> > right after the old value buffer, the issue lies in sysctl_string
> > seting 0 to oldval[len], len is the available buffer size
> > specified by the user, obviously, this will write to the first
> > byte of the user memory place immediate after the old value buffer,
> > the correct way is that sysctl_string doesn't set 0, the user
> > should do it by self in the program.
> 
> Hmm.. I think this patch is incomplete.
> 
> We _should_ zero-pad the data, at least if the result fits in the buffer.
> 
> So I think the correct fix is to just _copy_ the last zero if it fits in 
> the buffer, rather than do the unconditional "add NUL at the end" thing. 
> The simplest way to do that is to just make "l" be "strlen(str)+1", so 
> that we count the ending NUL in the length (and then, if the buffer isn't 
> big enough, we will truncate it).
> 
> In other words, I would instead suggest a patch like the appended.
> 
> But even that is questionable: one alternative is to always zero-pad (like 
> we used to), but make sure that the buffer size is sufficient for it (ie 
> instead of adding one to the length of the string, we'd subtract one from 
> the buffer length and make sure that the '\0' fits..
> 
> Comments?

Always do zero-pad please. I'd feel more comfortable with C strings (NULL
terminated). Don't you?

Poor code, also a small cleanup attached.

Signed-off-by: Coywolf Qi Hunt <qiyong@fc-cn.com>
---

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e5102ea..9960a26 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -2203,7 +2203,7 @@ int sysctl_string(ctl_table *table, int 
 		if (len) {
 			l = strlen(table->data)+1;
 			if (len > l) len = l;
-			if (len >= table->maxlen)
+			if (len > table->maxlen)
 				len = table->maxlen;
 			if(copy_to_user(oldval, table->data, len))
 				return -EFAULT;

Re: [PATCH] Fix user data corrupted by old value return of sysctl

[YOSHIFUJI Hideaki / 吉藤英明]

Hello.

In article <Pine.LNX.4.64.0512300916220.3249@g5.osdl.org> (at Fri, 30 Dec 2005 09:25:35 -0800 (PST)), Linus Torvalds <torvalds@osdl.org> says:

> On Fri, 30 Dec 2005, Yi Yang wrote:
> >
> > If the user reads a sysctl entry which is of string type
> > by sysctl syscall, this call probably corrupts the user data
> > right after the old value buffer, the issue lies in sysctl_string
> > seting 0 to oldval[len], len is the available buffer size
> > specified by the user, obviously, this will write to the first
> > byte of the user memory place immediate after the old value buffer,
> > the correct way is that sysctl_string doesn't set 0, the user
> > should do it by self in the program.
:
> We _should_ zero-pad the data, at least if the result fits in the buffer.
:
> But even that is questionable: one alternative is to always zero-pad (like 
> we used to), but make sure that the buffer size is sufficient for it (ie 
> instead of adding one to the length of the string, we'd subtract one from 
> the buffer length and make sure that the '\0' fits..

How about returning -ENOMEM, as BSDs (FreeBSD and NetBSD
at least) do.  No?

--yoshfuji

Re: [PATCH] Fix user data corrupted by old value return of sysctl

[David Wagner]

Yi Yang  wrote:
>If the user reads a sysctl entry which is of string type
>  by sysctl syscall, this call probably corrupts the user data
>  right after the old value buffer, the issue lies in sysctl_string
>  seting 0 to oldval[len], len is the available buffer size
>  specified by the user, obviously, this will write to the first
>  byte of the user memory place immediate after the old value buffer
>, the correct way is that sysctl_string doesn't set 0, the user
>should do it by self in the program.

That's not just data corruption -- it's also a buffer overrun.
Granted, it's "only" a one-byte overrun, but I have seen one-byte
overruns be exploitable occasionally in the past.  So this sounds
to me like a potential security issue, too.

Re: [PATCH] Fix user data corrupted by old value return of sysctl

[Coywolf Qi Hunt]

2005/12/30, Yi Yang <yang.y.yi@gmail.com>:
> If the user reads a sysctl entry which is of string type
>   by sysctl syscall, this call probably corrupts the user data
>   right after the old value buffer, the issue lies in sysctl_string
>   seting 0 to oldval[len], len is the available buffer size
>   specified by the user, obviously, this will write to the first
>   byte of the user memory place immediate after the old value buffer
> , the correct way is that sysctl_string doesn't set 0, the user
> should do it by self in the program.
>
> The following program verifies this point:
>
> #include <linux/unistd.h>
> #include <linux/types.h>
> #include <linux/sysctl.h>
> #include <errno.h>
>
> _syscall1(int, _sysctl, struct __sysctl_args *, args);
> int sysctl(int *name, int nlen, void *oldval, size_t *oldlenp,
>             void *newval, size_t newlen)
> {
>          struct __sysctl_args args
>                 = {name,nlen,oldval,oldlenp,newval,newlen};
>
>          return _sysctl(&args);
> }
>
> #define SIZE(x) sizeof(x)/sizeof(x[0])
> #define OSNAMESZ 4
>
> struct mystruct {
>         char osname[OSNAMESZ];
>         int target;
>         int osnamelth;
> } myos;
>
> int name[] = { CTL_KERN, KERN_NODENAME };
>
> int main(int argc, char * argv[])
> {
>         myos.target = 1;
>         printf("target = %d\n", myos.target);
>         myos.osnamelth = SIZE(myos.osname);
>          if (sysctl(name, SIZE(name), myos.osname,
>                         &myos.osnamelth, 0, 0))
>                  perror("sysctl");
>         else {
>                  printf("Current host name: %s\n", myos.osname);
>         }
>         printf("target = %d\n", myos.target);
>          return 0;
> }
>
> Copy it to file sysctl-safe.c, then
> $ hostname
> mylocalmachine
> $ gcc sysctl-safe.c
> $ ./a.out
> target = 1
> Current host name: mylo
> target = 0
> $
>
> After apply this patch:
> $ hostname
> mylocalmachine
> $ gcc sysctl-safe.c
> $ ./a.out
> target = 1
> Current host name: mylo

You didn't set the trailing '\0', I wonder how your printf did work
properly ever. You've just been lucky or something.

-- Coywolf


> target = 1
>
> Signed-off-by: Yi Yang <yang.y.yi@gmail.com>
>
>
> --- a/kernel/sysctl.c.orig      2005-12-30 09:21:34.000000000 +0000
> +++ b/kernel/sysctl.c   2005-12-30 15:58:15.000000000 +0000
> @@ -2207,8 +2207,6 @@ int sysctl_string(ctl_table *table, int
>                                 len = table->maxlen;
>                         if(copy_to_user(oldval, table->data, len))
>                                 return -EFAULT;
> -                       if(put_user(0, ((char __user *) oldval) + len))
> -                               return -EFAULT;
>                         if(put_user(len, oldlenp))
>                                 return -EFAULT;
>                 }
>
>
>
--
Coywolf Qi Hunt

Re: [PATCH] Fix user data corrupted by old value return of sysctl

[Yi Yang]

Coywolf Qi Hunt wrote:

>2005/12/30, Yi Yang <yang.y.yi@gmail.com>:
>  
>
>>If the user reads a sysctl entry which is of string type
>>  by sysctl syscall, this call probably corrupts the user data
>>  right after the old value buffer, the issue lies in sysctl_string
>>  seting 0 to oldval[len], len is the available buffer size
>>  specified by the user, obviously, this will write to the first
>>  byte of the user memory place immediate after the old value buffer
>>, the correct way is that sysctl_string doesn't set 0, the user
>>should do it by self in the program.
>>
>>The following program verifies this point:
>>
>>#include <linux/unistd.h>
>>#include <linux/types.h>
>>#include <linux/sysctl.h>
>>#include <errno.h>
>>
>>_syscall1(int, _sysctl, struct __sysctl_args *, args);
>>int sysctl(int *name, int nlen, void *oldval, size_t *oldlenp,
>>            void *newval, size_t newlen)
>>{
>>         struct __sysctl_args args
>>                = {name,nlen,oldval,oldlenp,newval,newlen};
>>
>>         return _sysctl(&args);
>>}
>>
>>#define SIZE(x) sizeof(x)/sizeof(x[0])
>>#define OSNAMESZ 4
>>
>>struct mystruct {
>>        char osname[OSNAMESZ];
>>        int target;
>>        int osnamelth;
>>} myos;
>>
>>int name[] = { CTL_KERN, KERN_NODENAME };
>>
>>int main(int argc, char * argv[])
>>{
>>        myos.target = 1;
>>        printf("target = %d\n", myos.target);
>>        myos.osnamelth = SIZE(myos.osname);
>>         if (sysctl(name, SIZE(name), myos.osname,
>>                        &myos.osnamelth, 0, 0))
>>                 perror("sysctl");
>>        else {
>>                 printf("Current host name: %s\n", myos.osname);
>>        }
>>        printf("target = %d\n", myos.target);
>>         return 0;
>>}
>>
>>Copy it to file sysctl-safe.c, then
>>$ hostname
>>mylocalmachine
>>$ gcc sysctl-safe.c
>>$ ./a.out
>>target = 1
>>Current host name: mylo
>>target = 0
>>$
>>
>>After apply this patch:
>>$ hostname
>>mylocalmachine
>>$ gcc sysctl-safe.c
>>$ ./a.out
>>target = 1
>>Current host name: mylo
>>    
>>
>
>You didn't set the trailing '\0', I wonder how your printf did work
>properly ever. You've just been lucky or something.
>
>-- Coywolf
>  
>
The variable target does it, its value is 0x00000001, so you mustn't 
worry it.
osname only has 4-bytes space, so if you set '\0' to its tail, a byte 
information will be lost.

>
>  
>
>>target = 1
>>
>>Signed-off-by: Yi Yang <yang.y.yi@gmail.com>
>>
>>
>>--- a/kernel/sysctl.c.orig      2005-12-30 09:21:34.000000000 +0000
>>+++ b/kernel/sysctl.c   2005-12-30 15:58:15.000000000 +0000
>>@@ -2207,8 +2207,6 @@ int sysctl_string(ctl_table *table, int
>>                                len = table->maxlen;
>>                        if(copy_to_user(oldval, table->data, len))
>>                                return -EFAULT;
>>-                       if(put_user(0, ((char __user *) oldval) + len))
>>-                               return -EFAULT;
>>                        if(put_user(len, oldlenp))
>>                                return -EFAULT;
>>                }
>>
>>
>>
>>    
>>
>--
>Coywolf Qi Hunt
>
>  
>

Re: [PATCH] Fix user data corrupted by old value return of sysctl

[Coywolf Qi Hunt]

2005/12/31, Yi Yang <yang.y.yi@gmail.com>:
> Coywolf Qi Hunt wrote:
> >You didn't set the trailing '\0', I wonder how your printf did work
> >properly ever. You've just been lucky or something.
> >
> >-- Coywolf
> >
> >
> The variable target does it, its value is 0x00000001, so you mustn't
> worry it.
> osname only has 4-bytes space, so if you set '\0' to its tail, a byte
> information will be lost.

I'm worrying more. We should set '\0'. Let the one byte information
lost, the caller deserve that. Actually here printf sees "mylo"+'\01'
if little endian.

Linus, besides fixing bug, your commit certainly breaks userland
compatibility. Please consider.
--
Coywolf Qi Hunt

Re: [PATCH] Fix user data corrupted by old value return of sysctl

[Yi Yang]

Coywolf Qi Hunt wrote:

>2005/12/31, Yi Yang <yang.y.yi@gmail.com>:
>  
>
>>Coywolf Qi Hunt wrote:
>>    
>>
>>>You didn't set the trailing '\0', I wonder how your printf did work
>>>properly ever. You've just been lucky or something.
>>>
>>>-- Coywolf
>>>
>>>
>>>      
>>>
>>The variable target does it, its value is 0x00000001, so you mustn't
>>worry it.
>>osname only has 4-bytes space, so if you set '\0' to its tail, a byte
>>information will be lost.
>>    
>>
>
>I'm worrying more. We should set '\0'. Let the one byte information
>lost, the caller deserve that. Actually here printf sees "mylo"+'\01'
>if little endian.
>  
>
I want to remind of you, my program is an example to verify this bug, it 
can run on both little-endian and big-endian computer correctly, because 
the variable target  is 0x00000001 or 0x00000000, it sets 0 to the tail 
of osname, only a byte '\01' isn't expected for little-endian, but my 
program really can run correctly.

For compatibility, you don't worry, you can try the application 'sysctl -a'.

This bug is very serious as far as security is concerned, so it is 
necessary, the kernel shouldn't corrupt the user data no matter what the 
reason is, if the user provides more space, Linus's patch has set 0 to 
the tail, but if the user space is not enough, to set 0 to the tail is 
not necessary, because a user mode application should do it by itself, 
but not depends on kernel.

>Linus, besides fixing bug, your commit certainly breaks userland
>compatibility. Please consider.
>--
>Coywolf Qi Hunt
>
>  
>