[PATCH] Netfilter: Fix integer overflow in net/ipv6/netfilter/ip6_tables.c
wzt...@gmail.com
correctly. The size is used to determine the total entry size.
The size is bounded, but can overflow and so the size checks may
not be sufficient to catch invalid size. Fix it by catching size
values that would cause overflows before calculating the size.
Signed-off-by: Zhitong Wang <zhitong...@alibaba-inc.com>
---
net/ipv4/netfilter/ip_tables.c | 4 ++++
net/ipv6/netfilter/ip6_tables.c | 4 ++++
2 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 4e7c719..6abd3d2 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1164,6 +1164,10 @@ get_entries(struct net *net, struct ipt_get_entries __user *uptr, int *len)
}
if (copy_from_user(&get, uptr, sizeof(get)) != 0)
return -EFAULT;
+
+ if (get.size >= INT_MAX / sizeof(struct ipt_get_entries))
+ return -EINVAL;
+
if (*len != sizeof(struct ipt_get_entries) + get.size) {
duprintf("get_entries: %u != %zu\n",
*len, sizeof(get) + get.size);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 0b4557e..5185822 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1190,6 +1190,10 @@ get_entries(struct net *net, struct ip6t_get_entries __user *uptr, int *len)
}
if (copy_from_user(&get, uptr, sizeof(get)) != 0)
return -EFAULT;
+
+ if (get.size >= INT_MAX / sizeof(struct ip6t_get_entries))
+ return -EINVAL;
+
if (*len != sizeof(struct ip6t_get_entries) + get.size) {
duprintf("get_entries: %u != %zu\n",
*len, sizeof(get) + get.size);
--
1.6.5.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Patrick McHardy
> The get.size field in the get_entries() interface is not bounded
> correctly. The size is used to determine the total entry size.
> The size is bounded, but can overflow and so the size checks may
> not be sufficient to catch invalid size. Fix it by catching size
> values that would cause overflows before calculating the size.
>
> Signed-off-by: Zhitong Wang <zhitong...@alibaba-inc.com>
>
> ---
> net/ipv4/netfilter/ip_tables.c | 4 ++++
> net/ipv6/netfilter/ip6_tables.c | 4 ++++
> 2 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
> index 4e7c719..6abd3d2 100644
> --- a/net/ipv4/netfilter/ip_tables.c
> +++ b/net/ipv4/netfilter/ip_tables.c
> @@ -1164,6 +1164,10 @@ get_entries(struct net *net, struct ipt_get_entries __user *uptr, int *len)
> }
> if (copy_from_user(&get, uptr, sizeof(get)) != 0)
> return -EFAULT;
> +
> + if (get.size >= INT_MAX / sizeof(struct ipt_get_entries))
> + return -EINVAL;
I can see that the size might cause an overflow in the addition with
sizeof(struct ipt_get_entries), but that would most likely cause a mismatch
with the actual table size and get aborted (should be fixed anyways I
guess). But I fail to find the overflow you're trying to prevent, which
I guess would be the result of a multiplication.
Please point me to the specific line in question. Thanks :)
wzt wzt
> sizeof(struct ipt_get_entries)
get.size is copy from the user space, it can be set as 0x7fffffff,
addition with sizeof(struct ipt_get_entries) can be overflow.
if (*len != sizeof(struct ipt_get_entries) + get.size) {
duprintf("get_entries: %u != %zu\n",
*len, sizeof(get) + get.size);
return -EINVAL;
}
so, check get.size max value before addition with sizeof(struct
ipt_get_entries) to prevent the integer overflow.
Xiaotian Feng
>> I can see that the size might cause an overflow in the addition with
>> sizeof(struct ipt_get_entries)
> That's the integer overflow i pointed.
> get.size is copy from the user space, it can be set as 0x7fffffff,
> addition with sizeof(struct ipt_get_entries) can be overflow.
Patrick's point is that you're using "if (get.size >= INT_MAX /
sizeof(struct ipt_get_entries))"
So, did you find any chance that get.size * sizeof(struct
ipt_get_entries) >= INT_MAX ?
And, for the addition overflow, can it be caught by
"if (*len != sizeof(struct ipt_get_entries) + get.size)" ???
wzt wzt
> sizeof(struct ipt_get_entries))"
> So, did you find any chance that get.size * sizeof(struct
> ipt_get_entries) >= INT_MAX ?
>
get.size is copy from the user space, it can be set as 0x7fffffff,
addition with sizeof(struct ipt_get_entries) can be overflow.
> And, for the addition overflow, can it be caught by
>
> "if (*len != sizeof(struct ipt_get_entries) + get.size)" ???
>
sizeof(struct ipt_get_entries) + get.size can be overflow as *len,
get.size is control by user space with copy_from_user().
Jan Engelhardt
On Tuesday 2010-03-23 03:37, wzt wzt wrote:
>> And, for the addition overflow, can it be caught by
>>
>> "if (*len != sizeof(struct ipt_get_entries) + get.size)" ???
>
>sizeof(struct ipt_get_entries) + get.size can be overflow as *len,
>get.size is control by user space with copy_from_user().
The != should catch it.
For 64-bit environments:
* + invoked with size_t, unsigned int
=> right side promoted to size_t, result type is size_t
* != invoked with int and size_t
=> left-side promoted to ssize_t (probably; but something as large as size_t)
* get.size is 32-bit bounded, as is *len,
so no overflow to worry about at all unless you make
sizeof(X) hilariously big close to 2^64 which is rather unlikely.
For 32-bit environments:
* Let *len be a number of choice (e.g. 36)
* Find a sizeof(X)+get.size that equals 36 mod 2^32.
* Since sizeof(X) is const, get.size must be 0 mod 2^32.
* So get.size must be a multiple of 2^32 to fool the system.
* Since get.size itself is only a 32-bit quantity, you cannot
represent any value larger than 4294967295.
What Was What Was Wanted.
Xiaotian Feng
>> Patrick's point is that you're using "if (get.size >= INT_MAX /
>> sizeof(struct ipt_get_entries))"
>> So, did you find any chance that get.size * sizeof(struct
>> ipt_get_entries) >= INT_MAX ?
>>
> would you carefully read my explain???
> get.size is copy from the user space, it can be set as 0x7fffffff,
> addition with sizeof(struct ipt_get_entries) can be overflow.
>
get.size is unsigned int, UINT_MAX is 0x FFFFFFFF, not 0x7FFFFFFF
And you're metioning "addition", then why you're checking as "multiplication"?
wzt wzt
> And you're metioning "addition", then why you're checking as "multiplication"?
oh, my falut:( the patch is multiplication check, not addition check.
thanks for helping me:)
wzt wzt
2、 set get.size = 0xffffffff from user space
3、 sizeof(struct ipt_get_entries) + get.size = 36 + 0xffffffff = 35;
4、 if (*len != sizeof(struct ipt_get_entries) + get.size) was bypassed.
you can test with c code:
#include <stdio.h>
int main(void)
{
unsigned int arg = 0xffffffff;
printf("%u\n", arg + 36);
if (35 != arg + 36) {
printf("not over flow.\n");
return -1;
}
printf("arg over flow.\n");
return 0;
Xiaotian Feng
> 1、 suppose *len = 35, sizeof(struct ipt_get_entries) = 36
> 2、 set get.size = 0xffffffff from user space
> 3、 sizeof(struct ipt_get_entries) + get.size = 36 + 0xffffffff = 35;
> 4、 if (*len != sizeof(struct ipt_get_entries) + get.size) was bypassed.
>
> you can test with c code:
>
> #include <stdio.h>
>
> int main(void)
> {
> unsigned int arg = 0xffffffff;
>
> printf("%u\n", arg + 36);
> if (35 != arg + 36) {
> printf("not over flow.\n");
> return -1;
> }
> printf("arg over flow.\n");
>
> return 0;
> }
>
You didn't get his point... The key point is that sizeof() result type
is size_t, slightly modify you code, try the result.
> int main(void)
> {
> unsigned int arg = 0xffffffff;
> unsigned int foo;
> printf("%lu\n", arg + sizeof(foo));
> if (sizeof(foo) - 1 != arg + sizeof(foo)) {
wzt wzt
> is size_t, slightly modify you code, try the result.
>> int main(void)
>> {
>> unsigned int arg = 0xffffffff;
>> unsigned int foo;
>> printf("%lu\n", arg + sizeof(foo));
>> if (sizeof(foo) - 1 != arg + sizeof(foo)) {
>> printf("not over flow.\n");
>> return -1;
>> }
>> printf("arg over flow.\n");
>>
>> return 0;
>> }
sizeof() is size_t(unsigned int), get.size is also unsigned int, arg
+ sizeof(foo) can overflow as sizeof(foo) - 1
[root@localhost test]# ./test
3
arg over flow.
my fault is *len is check with:
if (*len < sizeof(get)) {
duprintf("get_entries: %u < %zu\n", *len, sizeof(get));
return -EINVAL;
}
so, *len must >= sizeof(get), then:
if (*len != sizeof(struct ipt_get_entries) + get.size) {
duprintf("get_entries: %u != %zu\n",
*len, sizeof(get) + get.size);
return -EINVAL;
}
if sizeof(struct ipt_get_entries) + get.size oveflow, it must <
sizeof(struct ipt_get_entries), It not make any problem, just return.
get.size overflow can't make any problem, thx Feng and Jan for helping me.