netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net] netfilter: complete validation of user input
@ 2024-04-09 12:07 Eric Dumazet
  2024-04-10  9:23 ` Pablo Neira Ayuso
  2024-04-11  2:50 ` patchwork-bot+netdevbpf
  0 siblings, 2 replies; 3+ messages in thread
From: Eric Dumazet @ 2024-04-09 12:07 UTC (permalink / raw)
  To: David S . Miller, Jakub Kicinski, Paolo Abeni, Pablo Neira Ayuso,
	Jozsef Kadlecsik
  Cc: netdev, netfilter-devel, coreteam, eric.dumazet, Eric Dumazet, syzbot

In my recent commit, I missed that do_replace() handlers
use copy_from_sockptr() (which I fixed), followed
by unsafe copy_from_sockptr_offset() calls.

In all functions, we can perform the @optlen validation
before even calling xt_alloc_table_info() with the following
check:

if ((u64)optlen < (u64)tmp.size + sizeof(tmp))
        return -EINVAL;

Fixes: 0c83842df40f ("netfilter: validate user input for expected length")
Reported-by: syzbot <syzkaller@googlegroups.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv4/netfilter/arp_tables.c | 4 ++++
 net/ipv4/netfilter/ip_tables.c  | 4 ++++
 net/ipv6/netfilter/ip6_tables.c | 4 ++++
 3 files changed, 12 insertions(+)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index b150c9929b12e86219a55c77da480e0c538b3449..14365b20f1c5c09964dd7024060116737f22cb63 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -966,6 +966,8 @@ static int do_replace(struct net *net, sockptr_t arg, unsigned int len)
 		return -ENOMEM;
 	if (tmp.num_counters == 0)
 		return -EINVAL;
+	if ((u64)len < (u64)tmp.size + sizeof(tmp))
+		return -EINVAL;
 
 	tmp.name[sizeof(tmp.name)-1] = 0;
 
@@ -1266,6 +1268,8 @@ static int compat_do_replace(struct net *net, sockptr_t arg, unsigned int len)
 		return -ENOMEM;
 	if (tmp.num_counters == 0)
 		return -EINVAL;
+	if ((u64)len < (u64)tmp.size + sizeof(tmp))
+		return -EINVAL;
 
 	tmp.name[sizeof(tmp.name)-1] = 0;
 
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 487670759578168c5ff53bce6642898fc41936b3..fe89a056eb06c43743b2d7449e59f4e9360ba223 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1118,6 +1118,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len)
 		return -ENOMEM;
 	if (tmp.num_counters == 0)
 		return -EINVAL;
+	if ((u64)len < (u64)tmp.size + sizeof(tmp))
+		return -EINVAL;
 
 	tmp.name[sizeof(tmp.name)-1] = 0;
 
@@ -1504,6 +1506,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len)
 		return -ENOMEM;
 	if (tmp.num_counters == 0)
 		return -EINVAL;
+	if ((u64)len < (u64)tmp.size + sizeof(tmp))
+		return -EINVAL;
 
 	tmp.name[sizeof(tmp.name)-1] = 0;
 
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 636b360311c5365fba2330f6ca2f7f1b6dd1363e..131f7bb2110d3a08244c6da40ff9be45a2be711b 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1135,6 +1135,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len)
 		return -ENOMEM;
 	if (tmp.num_counters == 0)
 		return -EINVAL;
+	if ((u64)len < (u64)tmp.size + sizeof(tmp))
+		return -EINVAL;
 
 	tmp.name[sizeof(tmp.name)-1] = 0;
 
@@ -1513,6 +1515,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len)
 		return -ENOMEM;
 	if (tmp.num_counters == 0)
 		return -EINVAL;
+	if ((u64)len < (u64)tmp.size + sizeof(tmp))
+		return -EINVAL;
 
 	tmp.name[sizeof(tmp.name)-1] = 0;
 
-- 
2.44.0.478.gd926399ef9-goog


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH net] netfilter: complete validation of user input
  2024-04-09 12:07 [PATCH net] netfilter: complete validation of user input Eric Dumazet
@ 2024-04-10  9:23 ` Pablo Neira Ayuso
  2024-04-11  2:50 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 3+ messages in thread
From: Pablo Neira Ayuso @ 2024-04-10  9:23 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Jozsef Kadlecsik,
	netdev, netfilter-devel, coreteam, eric.dumazet, syzbot

On Tue, Apr 09, 2024 at 12:07:41PM +0000, Eric Dumazet wrote:
> In my recent commit, I missed that do_replace() handlers
> use copy_from_sockptr() (which I fixed), followed
> by unsafe copy_from_sockptr_offset() calls.

I forgot too to git grep away from net/netfilter/ folder for some
reason.

> In all functions, we can perform the @optlen validation
> before even calling xt_alloc_table_info() with the following
> check:
> 
> if ((u64)optlen < (u64)tmp.size + sizeof(tmp))
>         return -EINVAL;

Thanks for this fix.

> Fixes: 0c83842df40f ("netfilter: validate user input for expected length")
> Reported-by: syzbot <syzkaller@googlegroups.com>

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/ipv4/netfilter/arp_tables.c | 4 ++++
>  net/ipv4/netfilter/ip_tables.c  | 4 ++++
>  net/ipv6/netfilter/ip6_tables.c | 4 ++++
>  3 files changed, 12 insertions(+)
> 
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index b150c9929b12e86219a55c77da480e0c538b3449..14365b20f1c5c09964dd7024060116737f22cb63 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -966,6 +966,8 @@ static int do_replace(struct net *net, sockptr_t arg, unsigned int len)
>  		return -ENOMEM;
>  	if (tmp.num_counters == 0)
>  		return -EINVAL;
> +	if ((u64)len < (u64)tmp.size + sizeof(tmp))
> +		return -EINVAL;
>  
>  	tmp.name[sizeof(tmp.name)-1] = 0;
>  
> @@ -1266,6 +1268,8 @@ static int compat_do_replace(struct net *net, sockptr_t arg, unsigned int len)
>  		return -ENOMEM;
>  	if (tmp.num_counters == 0)
>  		return -EINVAL;
> +	if ((u64)len < (u64)tmp.size + sizeof(tmp))
> +		return -EINVAL;
>  
>  	tmp.name[sizeof(tmp.name)-1] = 0;
>  
> diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
> index 487670759578168c5ff53bce6642898fc41936b3..fe89a056eb06c43743b2d7449e59f4e9360ba223 100644
> --- a/net/ipv4/netfilter/ip_tables.c
> +++ b/net/ipv4/netfilter/ip_tables.c
> @@ -1118,6 +1118,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len)
>  		return -ENOMEM;
>  	if (tmp.num_counters == 0)
>  		return -EINVAL;
> +	if ((u64)len < (u64)tmp.size + sizeof(tmp))
> +		return -EINVAL;
>  
>  	tmp.name[sizeof(tmp.name)-1] = 0;
>  
> @@ -1504,6 +1506,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len)
>  		return -ENOMEM;
>  	if (tmp.num_counters == 0)
>  		return -EINVAL;
> +	if ((u64)len < (u64)tmp.size + sizeof(tmp))
> +		return -EINVAL;
>  
>  	tmp.name[sizeof(tmp.name)-1] = 0;
>  
> diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
> index 636b360311c5365fba2330f6ca2f7f1b6dd1363e..131f7bb2110d3a08244c6da40ff9be45a2be711b 100644
> --- a/net/ipv6/netfilter/ip6_tables.c
> +++ b/net/ipv6/netfilter/ip6_tables.c
> @@ -1135,6 +1135,8 @@ do_replace(struct net *net, sockptr_t arg, unsigned int len)
>  		return -ENOMEM;
>  	if (tmp.num_counters == 0)
>  		return -EINVAL;
> +	if ((u64)len < (u64)tmp.size + sizeof(tmp))
> +		return -EINVAL;
>  
>  	tmp.name[sizeof(tmp.name)-1] = 0;
>  
> @@ -1513,6 +1515,8 @@ compat_do_replace(struct net *net, sockptr_t arg, unsigned int len)
>  		return -ENOMEM;
>  	if (tmp.num_counters == 0)
>  		return -EINVAL;
> +	if ((u64)len < (u64)tmp.size + sizeof(tmp))
> +		return -EINVAL;
>  
>  	tmp.name[sizeof(tmp.name)-1] = 0;
>  
> -- 
> 2.44.0.478.gd926399ef9-goog
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH net] netfilter: complete validation of user input
  2024-04-09 12:07 [PATCH net] netfilter: complete validation of user input Eric Dumazet
  2024-04-10  9:23 ` Pablo Neira Ayuso
@ 2024-04-11  2:50 ` patchwork-bot+netdevbpf
  1 sibling, 0 replies; 3+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-04-11  2:50 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: davem, kuba, pabeni, pablo, kadlec, netdev, netfilter-devel,
	coreteam, eric.dumazet, syzkaller

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  9 Apr 2024 12:07:41 +0000 you wrote:
> In my recent commit, I missed that do_replace() handlers
> use copy_from_sockptr() (which I fixed), followed
> by unsafe copy_from_sockptr_offset() calls.
> 
> In all functions, we can perform the @optlen validation
> before even calling xt_alloc_table_info() with the following
> check:
> 
> [...]

Here is the summary with links:
  - [net] netfilter: complete validation of user input
    https://git.kernel.org/netdev/net/c/65acf6e0501a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-11  2:50 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-09 12:07 [PATCH net] netfilter: complete validation of user input Eric Dumazet
2024-04-10  9:23 ` Pablo Neira Ayuso
2024-04-11  2:50 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).