buffer string is copied from userspace. It is not checked whether it is
zero terminated. This may lead to overflow inside of simple_strtoul().
It was introduced before the git epoch. Files "ipt_CLUSTERIP/*" are
root writable only by default, however, on some setups permissions might be
relaxed to e.g. network admin user.
Signed-off-by: Vasiliy Kulikov <[email protected]>
---
Compile tested.
net/ipv4/netfilter/ipt_CLUSTERIP.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index 403ca57..7aabf9a 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -666,6 +666,7 @@ static ssize_t clusterip_proc_write(struct file *file, const char __user *input,
if (copy_from_user(buffer, input, PROC_WRITELEN))
return -EFAULT;
+ buffer[sizeof(buffer)-1] = 0;
if (*buffer == '+') {
nodenum = simple_strtoul(buffer+1, NULL, 10);
--
1.7.0.4
On Fri, Mar 11, 2011 at 2:12 AM, Vasiliy Kulikov <[email protected]> wrote:
> buffer string is copied from userspace. ?It is not checked whether it is
> zero terminated. ?This may lead to overflow inside of simple_strtoul().
>
> It was introduced before the git epoch. ?Files "ipt_CLUSTERIP/*" are
> root writable only by default, however, on some setups permissions might be
> relaxed to e.g. network admin user.
>
> Signed-off-by: Vasiliy Kulikov <[email protected]>
> ---
> ?Compile tested.
>
> ?net/ipv4/netfilter/ipt_CLUSTERIP.c | ? ?1 +
> ?1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
> index 403ca57..7aabf9a 100644
> --- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
> +++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
> @@ -666,6 +666,7 @@ static ssize_t clusterip_proc_write(struct file *file, const char __user *input,
>
> ? ? ? ?if (copy_from_user(buffer, input, PROC_WRITELEN))
I think size should be used instead of PROC_WRITELEN.
if (size > PROC_WRITELEN)
return -EIO;
if (copy_from_user(buffer, input, size))
return -EFAULT;
buffer[size] = '\0';
> ? ? ? ? ? ? ? ?return -EFAULT;
> + ? ? ? buffer[sizeof(buffer)-1] = 0;
>
> ? ? ? ?if (*buffer == '+') {
> ? ? ? ? ? ? ? ?nodenum = simple_strtoul(buffer+1, NULL, 10);
--
Regards,
Changli Gao([email protected])
On 13.03.2011 15:00, Changli Gao wrote:
> On Fri, Mar 11, 2011 at 2:12 AM, Vasiliy Kulikov <[email protected]> wrote:
>> buffer string is copied from userspace. It is not checked whether it is
>> zero terminated. This may lead to overflow inside of simple_strtoul().
>>
>> It was introduced before the git epoch. Files "ipt_CLUSTERIP/*" are
>> root writable only by default, however, on some setups permissions might be
>> relaxed to e.g. network admin user.
>>
>> Signed-off-by: Vasiliy Kulikov <[email protected]>
>> ---
>> Compile tested.
>>
>> net/ipv4/netfilter/ipt_CLUSTERIP.c | 1 +
>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
>> index 403ca57..7aabf9a 100644
>> --- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
>> +++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
>> @@ -666,6 +666,7 @@ static ssize_t clusterip_proc_write(struct file *file, const char __user *input,
>>
>> if (copy_from_user(buffer, input, PROC_WRITELEN))
>
> I think size should be used instead of PROC_WRITELEN.
>
> if (size > PROC_WRITELEN)
> return -EIO;
> if (copy_from_user(buffer, input, size))
> return -EFAULT;
> buffer[size] = '\0';
I agree, otherwise we might have the situation that the userspace
copy is crossing page boundaries into unmapped memory.
'buffer' string is copied from userspace. It is not checked whether it is
zero terminated. This may lead to overflow inside of simple_strtoul().
Changli Gao suggested to copy not more than user supplied 'size' bytes.
It was introduced before the git epoch. Files "ipt_CLUSTERIP/*" are
root writable only by default, however, on some setups permissions might be
relaxed to e.g. network admin user.
Signed-off-by: Vasiliy Kulikov <[email protected]>
---
net/ipv4/netfilter/ipt_CLUSTERIP.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index 1e26a48..af7dec6 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -669,8 +669,11 @@ static ssize_t clusterip_proc_write(struct file *file, const char __user *input,
char buffer[PROC_WRITELEN+1];
unsigned long nodenum;
- if (copy_from_user(buffer, input, PROC_WRITELEN))
+ if (size > PROC_WRITELEN)
+ return -EIO;
+ if (copy_from_user(buffer, input, size))
return -EFAULT;
+ buffer[size] = 0;
if (*buffer == '+') {
nodenum = simple_strtoul(buffer+1, NULL, 10);
--
1.7.0.4
On Thu, Mar 17, 2011 at 7:32 PM, Vasiliy Kulikov <[email protected]> wrote:
> 'buffer' string is copied from userspace. ?It is not checked whether it is
> zero terminated. ?This may lead to overflow inside of simple_strtoul().
> Changli Gao suggested to copy not more than user supplied 'size' bytes.
>
> It was introduced before the git epoch. ?Files "ipt_CLUSTERIP/*" are
> root writable only by default, however, on some setups permissions might be
> relaxed to e.g. network admin user.
>
> Signed-off-by: Vasiliy Kulikov <[email protected]>
Acked-by: Changli Gao <[email protected]>
--
Regards,
Changli Gao([email protected])
Am 17.03.2011 13:15, schrieb Changli Gao:
> On Thu, Mar 17, 2011 at 7:32 PM, Vasiliy Kulikov <[email protected]> wrote:
>> 'buffer' string is copied from userspace. It is not checked whether it is
>> zero terminated. This may lead to overflow inside of simple_strtoul().
>> Changli Gao suggested to copy not more than user supplied 'size' bytes.
>>
>> It was introduced before the git epoch. Files "ipt_CLUSTERIP/*" are
>> root writable only by default, however, on some setups permissions might be
>> relaxed to e.g. network admin user.
>>
>> Signed-off-by: Vasiliy Kulikov <[email protected]>
> Acked-by: Changli Gao <[email protected]>
>
>
Applied, thanks everyone.