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 <[email protected]>
--- 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;
}
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;
}
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.
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;
> }
>
>
>
2005/12/30, Yi Yang <[email protected]>:
> 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 <[email protected]>
>
>
> --- 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
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 <[email protected]>
---
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;
Coywolf Qi Hunt wrote:
>2005/12/30, Yi Yang <[email protected]>:
>
>
>>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 <[email protected]>
>>
>>
>>--- 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
>
>
>
2005/12/31, Yi Yang <[email protected]>:
> 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
Hello.
In article <[email protected]> (at Fri, 30 Dec 2005 09:25:35 -0800 (PST)), Linus Torvalds <[email protected]> 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
Coywolf Qi Hunt wrote:
>2005/12/31, Yi Yang <[email protected]>:
>
>
>>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
>
>
>