netdev - Re: [PATCH v2 2/2] net: return early for possible invalid uaddr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e926cd4f-9c87-8fa3-5b55-861ac299a184@linux.alibaba.com>
Date:   Tue, 17 Aug 2021 12:08:39 +0800
From:   Wen Yang <wenyang@...ux.alibaba.com>
To:     Eric Dumazet <eric.dumazet@...il.com>, davem@...emloft.net,
        David Ahern <dsahern@...nel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] net: return early for possible invalid uaddr



在 2021/8/13 上午1:35, Wen Yang 写道:
> 
> 
> 在 2021/8/12 上午12:11, Eric Dumazet 写道:
>>
>>
>> On 8/11/21 5:24 PM, Wen Yang wrote:
>>> The inet_dgram_connect() first calls inet_autobind() to select an
>>> ephemeral port, then checks uaddr in udp_pre_connect() or
>>> __ip4_datagram_connect(), but the port is not released until the socket
>>> is closed. This could cause performance issues or even exhaust ephemeral
>>> ports if a malicious user makes a large number of UDP connections with
>>> invalid uaddr and/or addr_len.
>>>
>>
>> This is a big patch.
>>
>> Can the malicious user still use a large number of UDP sockets,
>> with valid uaddr/add_len and consequently exhaust ephemeral ports ?
>>
>> If yes, it does not seem your patch is helping.
>>
> 
> Thank you for your comments.
> However, we could make these optimizations:
> 
> 1, If the user passed in some invalid parameters, we should return as
> soon as possible. We shouldn't assume that these parameters are valid
> first, then do some real work (such as select an ephemeral port), and
> then finally check that they are indeed valid or not.
> 
> 2. Unify the code for checking parameters in udp_pre_connect() and
> __ip4_datagram_connect() to make the code clearer.
> 
>> If no, have you tried instead to undo the autobind, if the connect 
>> fails ?
>>
> 
> Thanks. Undo the autobind is useful if the connect fails.
> We will add this logic and submit the v3 patch later.
> 

Hello, there is no undo autobind for udp. If this logic is added, the 
patch will be bigger; maybe we can release this ephemeral port through 
unhash()?

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 8a8dba7..43947d8 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -563,6 +563,7 @@ int inet_dgram_connect(struct socket *sock, struct 
sockaddr *uaddr,
                        int addr_len, int flags)
  {
         struct sock *sk = sock->sk;
+       bool autobind;
         int err;

         if (addr_len < sizeof(uaddr->sa_family))
@@ -581,9 +582,17 @@ int inet_dgram_connect(struct socket *sock, struct 
sockaddr *uaddr,
                         return err;
         }

-       if (data_race(!inet_sk(sk)->inet_num) && inet_autobind(sk))
+       autobind = data_race(!inet_sk(sk)->inet_num);
+       if (autobind && inet_autobind(sk))
                 return -EAGAIN;
-       return sk->sk_prot->connect(sk, uaddr, addr_len);
+
+       err = sk->sk_prot->connect(sk, uaddr, addr_len);
+       if (err && autobind) {
+               if (sk->sk_prot->unhash)
+                       sk->sk_prot->unhash(sk);
+       }
+
+       return err;
  }
  EXPORT_SYMBOL(inet_dgram_connect);

Could you kindly give some suggestions?

In addition, the previous v2 patch detects errors before bind and 
returns earlier, which should be reasonable.


-- 
Best wishes，
Wen