syzkaller wireguard key situation [was: Re: [PATCH net-next v2] net: WireGuard secure network tunnel]

[Jason A. Donenfeld]

On Tue, Feb 18, 2020 at 11:00 AM Dmitry Vyukov <dvyukov at google.com> wrote:
> I've added descriptions for wireguard packets:
> https://github.com/google/syzkaller/commit/012fbc3229ebef871a201ea431b16610e6e0d345
> It gives all reachable coverage (without breaking crypto).

Oh, great, that looks really good. It now fails at the index match,
which is basically trying to brute force a 32-bit integer that's
changing every 3 minutes, which syzkaller is probably too slow to do
on it's own. But that's fine.

Your commit has some questions in it, like "# Not clear if these
indexes are also generated randomly and we need to guess them or
not.". Here's what's up with those indices:

Message message_handshake_initiation: the sender picks a random 32-bit
number, and places it in the "sender_index" field.
Message message_handshake_response: the sender picks a random 32-bit
number, and places it in the "sender_index" field. It places the
32-bit number that it received from message_handshake_initiation into
the "receiver_index" field.
Message message_handshake_cookie: the sender places the 32-bit number
that it received from message_handshake_initiation or
message_handshake_response into the "receive_index" field.
Message message_data: the sender places the 32-bit number that it
picked for the  message_handshake_initiation or
message_handshake_response into the "key_idx" field.

I'm not sure it'll be too feasible to correlate these relations via
fuzzing. And either way, I think we've got decent enough non-crypto
coverage now on the receive path.

I noticed a small seemingly insignificant function with low coverage
that's on the rtnl path that might benefit from some attention and
also help find bugs in other devices: wg_netdevice_notification. This
triggers on various things, but the only case it really cares about is
NETDEV_REGISTER, which happens when the interface changes network
namespaces. WireGuard holds a reference to its underlying creating
namespace, and in order to avoid circular reference counting or UaF it
needs to either relinquish or get a reference. There are other drivers
with similar type of reference counting management, I would assume.
This sort of thing seems up the ally of the types of bugs and races
syzkaller likes to find. The way to trigger it is with `ip link set
dev wg0 netns blah`, and then back to its original netns. That's this
code in net/core/rtnetlink.c:

       if (tb[IFLA_NET_NS_PID] || tb[IFLA_NET_NS_FD] ||
tb[IFLA_TARGET_NETNSID]) {
               struct net *net = rtnl_link_get_net_capable(skb, dev_net(dev),
                                                           tb, CAP_NET_ADMIN);
               if (IS_ERR(net)) {
                       err = PTR_ERR(net);
                       goto errout;
               }

               err = dev_change_net_namespace(dev, net, ifname);
               put_net(net);
               if (err)
                       goto errout;
               status |= DO_SETLINK_MODIFIED;
       }

That seems to have decent coverage, but not over wireguard. Is that
just a result of the syzkaller @devname not yet being expanded to
wg{0,1,2}, and it'll take a few more weeks for it to learn that?
@netns_id seems probably good, being restricted to 0:4; is it possible
these weren't created though a priori?