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

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

[Jason A. Donenfeld]

Hey Dmitry,

I see you got wireguard's netlink stuff hooked up to syzkaller.
Excellent work, and thanks! It's already finding bugs.

Right now it seems to know about 5 different keys you've come up with,
and not much in the way of endpoints. I think we can improve this.

For keys, there are a few cases we care about:

1) Low order keys
2) Negative keys
3) Normal keys
4) Keys that correspond to other keys (private ==> public)

For this last point, if we just have a few with that correspondance
quality in there, syzkaller will eventually wind up configuring two
interfaces that can talk to each other, which is good. Here's a
collection of keys you can use, in base64, that will cover those
cases, if you want to add these instead of the current ones in there:

1)
AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
4Ot6fDtBuK4WVuP68Z/EatoJjeucMrH9hmIFFl9JuAA=

2)
2/////////////////////////////////////////8=
TJyVvKNQjCSx0LFVnIPvWwREXMRYHI6G2CJO3dCfEdc=

3,4)
oFyoT2ycjjhT4v16cK4Psg+hUmAMsAhFF08IB2+NeEM=
l1ydgcmDyCCe54ElS4mfjtklrp8JI8I8YvU8V82/aRw=
sIBz6NROkePakiwiQ4JEu4hcaeJpyOnYNbEUKTpN3G4=
0XMomfYRzYmUA01/QT3JV2MOVJPChaykAGXLYxG+aWs=
oMuHmkf1vGRMDmk/ptAxx0oVU7bpAbn/L1GMeAQvtUI=
9E2jZ6iO5lZPAgIRRWcnCC9c6+6LG/Xrczc0G0WbOSI=

That's 10 keys total, which should be a decent collection to replace
your current set of hard coded keys in there. You can unbase64 these
into C format with commands like:

$ echo '2/////////////////////////////////////////8=' | base64 -d | xxd -i
  0xdb, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
  0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff

The second thing is getting two wireguard interfaces to talk to each
other. This probably should happen over localhost. That means the
listen port of one should be the endpoint of the other. So maybe you
can get away fuzzing these with:

Listen ports:
51820
51821
51822
[randomly selected]

and

Endpoints:
127.0.0.1:51820
127.0.0.1:51821
127.0.0.1:51822
[::1]:51820
[::1]:51821
[::1]:51822
[randomly selected]

Finally the "allowed ips" for a peer, the routing table entry that
points to wireguard, and the packet that's being sent, should all
somehow correspond. But probably an allowed ips of 0.0.0.0/0 will
eventually be fuzzed to, which covers everything for the first part,
so let's see if the rest falls into place on its own.

What do you think of all that?

Jason
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Dmitry Vyukov]

On Tue, Feb 4, 2020 at 10:39 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hey Dmitry,
>
> I see you got wireguard's netlink stuff hooked up to syzkaller.
> Excellent work, and thanks! It's already finding bugs.
>
> Right now it seems to know about 5 different keys you've come up with,
> and not much in the way of endpoints. I think we can improve this.
>
> For keys, there are a few cases we care about:
>
> 1) Low order keys
> 2) Negative keys
> 3) Normal keys
> 4) Keys that correspond to other keys (private ==> public)
>
> For this last point, if we just have a few with that correspondance
> quality in there, syzkaller will eventually wind up configuring two
> interfaces that can talk to each other, which is good. Here's a
> collection of keys you can use, in base64, that will cover those
> cases, if you want to add these instead of the current ones in there:
>
> 1)
> AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
> 4Ot6fDtBuK4WVuP68Z/EatoJjeucMrH9hmIFFl9JuAA=
>
> 2)
> 2/////////////////////////////////////////8=
> TJyVvKNQjCSx0LFVnIPvWwREXMRYHI6G2CJO3dCfEdc=
>
> 3,4)
> oFyoT2ycjjhT4v16cK4Psg+hUmAMsAhFF08IB2+NeEM=
> l1ydgcmDyCCe54ElS4mfjtklrp8JI8I8YvU8V82/aRw=
> sIBz6NROkePakiwiQ4JEu4hcaeJpyOnYNbEUKTpN3G4=
> 0XMomfYRzYmUA01/QT3JV2MOVJPChaykAGXLYxG+aWs=
> oMuHmkf1vGRMDmk/ptAxx0oVU7bpAbn/L1GMeAQvtUI=
> 9E2jZ6iO5lZPAgIRRWcnCC9c6+6LG/Xrczc0G0WbOSI=
>
> That's 10 keys total, which should be a decent collection to replace
> your current set of hard coded keys in there. You can unbase64 these
> into C format with commands like:
>
> $ echo '2/////////////////////////////////////////8=' | base64 -d | xxd -i
>   0xdb, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
>   0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
>   0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff
>
> The second thing is getting two wireguard interfaces to talk to each
> other. This probably should happen over localhost. That means the
> listen port of one should be the endpoint of the other. So maybe you
> can get away fuzzing these with:
>
> Listen ports:
> 51820
> 51821
> 51822
> [randomly selected]
>
> and
>
> Endpoints:
> 127.0.0.1:51820
> 127.0.0.1:51821
> 127.0.0.1:51822
> [::1]:51820
> [::1]:51821
> [::1]:51822
> [randomly selected]
>
> Finally the "allowed ips" for a peer, the routing table entry that
> points to wireguard, and the packet that's being sent, should all
> somehow correspond. But probably an allowed ips of 0.0.0.0/0 will
> eventually be fuzzed to, which covers everything for the first part,
> so let's see if the rest falls into place on its own.
>
> What do you think of all that?
>
> Jason

Hi Jason,

[getting through backlog after a tip...]

I think you addressed all of this by now, right? And we got decent
coverage of wireguard. Anything else low hanging left?

https://github.com/google/syzkaller/commit/2c71f1a9122cc3cb0abacbbec6359c40db02be35
https://github.com/google/syzkaller/commit/4d1ab643be2091f794ec55d83ec8acf7b0a60be3
https://github.com/google/syzkaller/commit/c5ed587f4af5e639f7373d8ebf10ac049cb9c71b

Thanks!
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Jason A. Donenfeld]

Hey Dmitry,

Yes! Our side discussions wound up getting everything pretty squared
away, and coverage on syzkaller looks pretty good to me. By inference,
I think we're hitting most code paths in WireGuard. Syzkaller, though,
is missing non-userspace-process coverage from:

- workqueues
- napi callback
- timer callback
- udp tunnel callback

Seems like there might be some future research to be done on how we
can track these. But tracking it or not, the fact that packets are
flowing on some path implies that other code paths are being hit. So I
feel pretty good about syzkaller's ability to dig up nice wireguard
bugs.

Jason
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Jason A. Donenfeld]

Observation:

It seems to be starting to synthesize packets sent to the wireguard
socket. These aren't the proper handshake packets generated internally
by that triangle commit, but rather ones that syzkaller creates
itself. That's why we have coverage on wg_receive, which otherwise
wouldn't be called from a userspace process, since syzbot is sending
its own packets to that function.

However, the packets it generates aren't getting very far, failing all
of the tests in validate_header_len. None of those checks are at all
cryptographic, which means it should be able to hit those eventually.
Anything we should be doing to help it out? After it gets past that
check, it'll wind up in the handshake queue or the data queue, and
then (in theory) it should be rejected on a cryptographic basis. But
maybe syzbot will figure out how to crash it instead :-P.
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Dmitry Vyukov]

On Mon, Feb 17, 2020 at 12:44 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Observation:
>
> It seems to be starting to synthesize packets sent to the wireguard
> socket. These aren't the proper handshake packets generated internally
> by that triangle commit, but rather ones that syzkaller creates
> itself. That's why we have coverage on wg_receive, which otherwise
> wouldn't be called from a userspace process, since syzbot is sending
> its own packets to that function.
>
> However, the packets it generates aren't getting very far, failing all
> of the tests in validate_header_len. None of those checks are at all
> cryptographic, which means it should be able to hit those eventually.
> Anything we should be doing to help it out? After it gets past that
> check, it'll wind up in the handshake queue or the data queue, and
> then (in theory) it should be rejected on a cryptographic basis. But
> maybe syzbot will figure out how to crash it instead :-P.

Looking into this.

Found the program that gives wg_receive coverage:

r0 = openat$tun(0xffffffffffffff9c,
&(0x7f0000000080)='/dev/net/tun\x00', 0x88002, 0x0)
ioctl$TUNSETIFF(r0, 0x400454ca, &(0x7f00000000c0)={'syzkaller1\x00',
0x420000015001})
r1 = socket$netlink(0x10, 0x3, 0x0)
ioctl$sock_inet_SIOCSIFADDR(r1, 0x8914,
&(0x7f0000000140)={'syzkaller1\x00', {0x7, 0x0, @empty}})
write$tun(r0, &(0x7f00000002c0)={@void, @val, @ipv4=@udp={{0x5, 0x4,
0x0, 0x0, 0x1c, 0x0, 0x0, 0x0, 0x11, 0x0, @remote, @broadcast}, {0x0,
0x4e21, 0x8}}}, 0x26)

Checked that doing SIOCSIFADDR is also required, otherwise the packet
does not reach wg_receive.
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Dmitry Vyukov]

On Mon, Feb 17, 2020 at 4:19 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Feb 17, 2020 at 12:44 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >
> > Observation:
> >
> > It seems to be starting to synthesize packets sent to the wireguard
> > socket. These aren't the proper handshake packets generated internally
> > by that triangle commit, but rather ones that syzkaller creates
> > itself. That's why we have coverage on wg_receive, which otherwise
> > wouldn't be called from a userspace process, since syzbot is sending
> > its own packets to that function.
> >
> > However, the packets it generates aren't getting very far, failing all
> > of the tests in validate_header_len. None of those checks are at all
> > cryptographic, which means it should be able to hit those eventually.
> > Anything we should be doing to help it out? After it gets past that
> > check, it'll wind up in the handshake queue or the data queue, and
> > then (in theory) it should be rejected on a cryptographic basis. But
> > maybe syzbot will figure out how to crash it instead :-P.
>
> Looking into this.
>
> Found the program that gives wg_receive coverage:
>
> r0 = openat$tun(0xffffffffffffff9c,
> &(0x7f0000000080)='/dev/net/tun\x00', 0x88002, 0x0)
> ioctl$TUNSETIFF(r0, 0x400454ca, &(0x7f00000000c0)={'syzkaller1\x00',
> 0x420000015001})
> r1 = socket$netlink(0x10, 0x3, 0x0)
> ioctl$sock_inet_SIOCSIFADDR(r1, 0x8914,
> &(0x7f0000000140)={'syzkaller1\x00', {0x7, 0x0, @empty}})
> write$tun(r0, &(0x7f00000002c0)={@void, @val, @ipv4=@udp={{0x5, 0x4,
> 0x0, 0x0, 0x1c, 0x0, 0x0, 0x0, 0x11, 0x0, @remote, @broadcast}, {0x0,
> 0x4e21, 0x8}}}, 0x26)
>
> Checked that doing SIOCSIFADDR is also required, otherwise the packet
> does not reach wg_receive.


All packets we inject with standard means (syz_emit_ethernet) get
rejected on the following check:

static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
{
const struct iphdr *iph;
u32 len;

/* When the interface is in promisc. mode, drop all the crap
* that it receives, do not try to analyse it.
*/
if (skb->pkt_type == PACKET_OTHERHOST)
goto drop;

Even if we drop IFF_NAPI_FRAGS which diverges packets who-knows-where.

Somehow we need to get something other than PACKET_OTHERHOST...
Why is it dropping all remote packets?...
How do remote packets get into stack then?...
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Dmitry Vyukov]

On Mon, Feb 17, 2020 at 4:42 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> > >
> > > Observation:
> > >
> > > It seems to be starting to synthesize packets sent to the wireguard
> > > socket. These aren't the proper handshake packets generated internally
> > > by that triangle commit, but rather ones that syzkaller creates
> > > itself. That's why we have coverage on wg_receive, which otherwise
> > > wouldn't be called from a userspace process, since syzbot is sending
> > > its own packets to that function.
> > >
> > > However, the packets it generates aren't getting very far, failing all
> > > of the tests in validate_header_len. None of those checks are at all
> > > cryptographic, which means it should be able to hit those eventually.
> > > Anything we should be doing to help it out? After it gets past that
> > > check, it'll wind up in the handshake queue or the data queue, and
> > > then (in theory) it should be rejected on a cryptographic basis. But
> > > maybe syzbot will figure out how to crash it instead :-P.
> >
> > Looking into this.
> >
> > Found the program that gives wg_receive coverage:
> >
> > r0 = openat$tun(0xffffffffffffff9c,
> > &(0x7f0000000080)='/dev/net/tun\x00', 0x88002, 0x0)
> > ioctl$TUNSETIFF(r0, 0x400454ca, &(0x7f00000000c0)={'syzkaller1\x00',
> > 0x420000015001})
> > r1 = socket$netlink(0x10, 0x3, 0x0)
> > ioctl$sock_inet_SIOCSIFADDR(r1, 0x8914,
> > &(0x7f0000000140)={'syzkaller1\x00', {0x7, 0x0, @empty}})
> > write$tun(r0, &(0x7f00000002c0)={@void, @val, @ipv4=@udp={{0x5, 0x4,
> > 0x0, 0x0, 0x1c, 0x0, 0x0, 0x0, 0x11, 0x0, @remote, @broadcast}, {0x0,
> > 0x4e21, 0x8}}}, 0x26)
> >
> > Checked that doing SIOCSIFADDR is also required, otherwise the packet
> > does not reach wg_receive.
>
>
> All packets we inject with standard means (syz_emit_ethernet) get
> rejected on the following check:
>
> static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
> {
> const struct iphdr *iph;
> u32 len;
>
> /* When the interface is in promisc. mode, drop all the crap
> * that it receives, do not try to analyse it.
> */
> if (skb->pkt_type == PACKET_OTHERHOST)
> goto drop;
>
> Even if we drop IFF_NAPI_FRAGS which diverges packets who-knows-where.
>
> Somehow we need to get something other than PACKET_OTHERHOST...
> Why is it dropping all remote packets?...
> How do remote packets get into stack then?...

I've managed to create a packet that reaches wg_receive, that is:

syz_emit_ethernet(AUTO, &AUTO={@local, @empty, @void, {@ipv4={AUTO,
@udp={{AUTO, AUTO, 0x0, 0x0, AUTO, 0x0, 0x0, 0x0, AUTO, 0x0, @empty,
@empty, {[]}}, {0x0, 0x4e22, AUTO, 0x0, [], ""/10}}}}}, 0x0)

Had to enumerate all possible combinations of local/remote mac,
local/report ip, local/remote port.

However, this is only without IFF_NAPI_FRAGS. With IFF_NAPI_FRAGS it
reaches udp_gro_receive, but does not get past:

if (!sk || NAPI_GRO_CB(skb)->encap_mark ||
    (skb->ip_summed != CHECKSUM_PARTIAL &&
     NAPI_GRO_CB(skb)->csum_cnt == 0 &&
     !NAPI_GRO_CB(skb)->csum_valid) ||
    !udp_sk(sk)->gro_receive)
    goto out;
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Dmitry Vyukov]

On Mon, Feb 17, 2020 at 8:24 PM Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Feb 17, 2020 at 4:42 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> > > >
> > > > Observation:
> > > >
> > > > It seems to be starting to synthesize packets sent to the wireguard
> > > > socket. These aren't the proper handshake packets generated internally
> > > > by that triangle commit, but rather ones that syzkaller creates
> > > > itself. That's why we have coverage on wg_receive, which otherwise
> > > > wouldn't be called from a userspace process, since syzbot is sending
> > > > its own packets to that function.
> > > >
> > > > However, the packets it generates aren't getting very far, failing all
> > > > of the tests in validate_header_len. None of those checks are at all
> > > > cryptographic, which means it should be able to hit those eventually.
> > > > Anything we should be doing to help it out? After it gets past that
> > > > check, it'll wind up in the handshake queue or the data queue, and
> > > > then (in theory) it should be rejected on a cryptographic basis. But
> > > > maybe syzbot will figure out how to crash it instead :-P.
> > >
> > > Looking into this.
> > >
> > > Found the program that gives wg_receive coverage:
> > >
> > > r0 = openat$tun(0xffffffffffffff9c,
> > > &(0x7f0000000080)='/dev/net/tun\x00', 0x88002, 0x0)
> > > ioctl$TUNSETIFF(r0, 0x400454ca, &(0x7f00000000c0)={'syzkaller1\x00',
> > > 0x420000015001})
> > > r1 = socket$netlink(0x10, 0x3, 0x0)
> > > ioctl$sock_inet_SIOCSIFADDR(r1, 0x8914,
> > > &(0x7f0000000140)={'syzkaller1\x00', {0x7, 0x0, @empty}})
> > > write$tun(r0, &(0x7f00000002c0)={@void, @val, @ipv4=@udp={{0x5, 0x4,
> > > 0x0, 0x0, 0x1c, 0x0, 0x0, 0x0, 0x11, 0x0, @remote, @broadcast}, {0x0,
> > > 0x4e21, 0x8}}}, 0x26)
> > >
> > > Checked that doing SIOCSIFADDR is also required, otherwise the packet
> > > does not reach wg_receive.
> >
> >
> > All packets we inject with standard means (syz_emit_ethernet) get
> > rejected on the following check:
> >
> > static struct sk_buff *ip_rcv_core(struct sk_buff *skb, struct net *net)
> > {
> > const struct iphdr *iph;
> > u32 len;
> >
> > /* When the interface is in promisc. mode, drop all the crap
> > * that it receives, do not try to analyse it.
> > */
> > if (skb->pkt_type == PACKET_OTHERHOST)
> > goto drop;
> >
> > Even if we drop IFF_NAPI_FRAGS which diverges packets who-knows-where.
> >
> > Somehow we need to get something other than PACKET_OTHERHOST...
> > Why is it dropping all remote packets?...
> > How do remote packets get into stack then?...
>
> I've managed to create a packet that reaches wg_receive, that is:
>
> syz_emit_ethernet(AUTO, &AUTO={@local, @empty, @void, {@ipv4={AUTO,
> @udp={{AUTO, AUTO, 0x0, 0x0, AUTO, 0x0, 0x0, 0x0, AUTO, 0x0, @empty,
> @empty, {[]}}, {0x0, 0x4e22, AUTO, 0x0, [], ""/10}}}}}, 0x0)
>
> Had to enumerate all possible combinations of local/remote mac,
> local/report ip, local/remote port.
>
> However, this is only without IFF_NAPI_FRAGS. With IFF_NAPI_FRAGS it
> reaches udp_gro_receive, but does not get past:
>
> if (!sk || NAPI_GRO_CB(skb)->encap_mark ||
>     (skb->ip_summed != CHECKSUM_PARTIAL &&
>      NAPI_GRO_CB(skb)->csum_cnt == 0 &&
>      !NAPI_GRO_CB(skb)->csum_valid) ||
>     !udp_sk(sk)->gro_receive)
>     goto out;


I've added descriptions for wireguard packets:
https://github.com/google/syzkaller/commit/012fbc3229ebef871a201ea431b16610e6e0d345
It gives all reachable coverage (without breaking crypto).

Strictly saying, for tcp we experimented with receiving ACKs back from
tun and exposing them to fuzzer to form proper SYNACKs:
https://github.com/google/syzkaller/blob/012fbc3229ebef871a201ea431b16610e6e0d345/executor/common_linux.h#L1390-L1441
https://github.com/google/syzkaller/blob/012fbc3229ebef871a201ea431b16610e6e0d345/sys/linux/vnet.txt#L24-L27

Theoretically, it could receive wireguard handshakes and form proper
replies with valid signatures and stuff.

I disabled IFF_NAPI_FRAGS entirely, it seems to prevent from getting
any meaningful coverage:
https://github.com/google/syzkaller/commit/39cd0f85a1ac60b88c793bd8f4a981227614da88
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

Re: 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@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?
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Dmitry Vyukov]

On Wed, Feb 19, 2020 at 11:23 AM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> On Tue, Feb 18, 2020 at 11:00 AM Dmitry Vyukov <dvyukov@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?

Running an instance aimed at these functions:

"enable_syscalls": [
"socket$nl_generic",
"syz_genetlink_get_family_id$wireguard",
"sendmsg$WG_CMD*",
"ioctl$ifreq_SIOCGIFINDEX_wireguard*",
"socket$nl_route",
"sendmsg$nl_route",
"setsockopt",
"syz_emit_ethernet",
"syz_extract_tcp_res",
"socket$inet*",
"accept4$inet*",
"bind$inet*",
"connect$inet*",
"sendto$inet*",
"recvfrom$inet*",
"syz_genetlink_get_family_id$fou",
"sendmsg$FOU*",
"unshare",
"setns",
"socket",
"socketpair",
"getuid",
"getgid",
"bpf",
"setsockopt",
"getsockopt",
"ioctl",
"accept4",
"connect",
"bind",
"listen",
"sendmsg",
"recvmsg",
"syz_emit_ethernet",
"syz_extract_tcp_res",
"write$tun",
"syz_init_net_socket",
"syz_genetlink_get_family_id",
"syz_open_procfs$namespace",
"ioctl$NS_GET*",
"ioctl$TUN*",
"openat$tun",
"getpid",
"mmap"
]

I got some coverage in wg_netdevice_notification:
https://imgur.com/a/1sJZKtp

Or you mean the parts that are still red?

I think theoretically these parts should be reachable too because
syzkaller can do unshare and obtain net ns fd's.

It's quite hard to test because it just crashes all the time on known bugs.
So maybe the most profitable way to get more coverage throughout the
networking subsystem now is to fix the top layer of crashers ;)

2020/02/20 16:29:08 vm-9: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 16:32:25 vm-6: crash: kernel BUG at net/core/skbuff.c:LINE!
2020/02/20 16:33:13 vm-16: crash: INFO: task hung in register_netdevice_notifier
2020/02/20 16:33:46 vm-5: crash: INFO: task hung in register_netdevice_notifier
2020/02/20 16:34:22 vm-2: crash: WARNING in restore_regulatory_settings
2020/02/20 16:34:27 vm-4: crash: unregister_netdevice: waiting for DEV
to become free
2020/02/20 16:35:04 vm-26: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 16:35:17 vm-34: crash: no output from test machine
2020/02/20 16:35:20 vm-0: crash: INFO: task hung in register_netdevice_notifier
2020/02/20 16:36:42 vm-3: crash: WARNING in restore_regulatory_settings
2020/02/20 16:37:47 vm-29: crash: INFO: task hung in register_netdevice_notifier
2020/02/20 16:38:54 vm-31: crash: INFO: task hung in register_netdevice_notifier
2020/02/20 16:39:00 vm-9: crash: INFO: task hung in register_netdevice_notifier
2020/02/20 16:41:41 vm-4: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 16:43:00 vm-23: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 16:44:01 vm-2: crash: possible deadlock in htab_lru_map_delete_node
2020/02/20 16:44:06 vm-38: crash: INFO: task hung in register_netdevice_notifier
2020/02/20 16:47:13 vm-8: crash: unregister_netdevice: waiting for DEV
to become free
2020/02/20 16:47:20 vm-25: crash: unregister_netdevice: waiting for
DEV to become free
2020/02/20 16:47:33 vm-18: crash: BUG: using smp_processor_id() in
preemptible [ADDR] code: syz-executor
2020/02/20 16:49:37 vm-37: crash: unregister_netdevice: waiting for
DEV to become free
2020/02/20 16:51:51 vm-12: crash: WARNING in print_bfs_bug
2020/02/20 16:53:28 vm-4: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 16:55:24 vm-17: crash: INFO: task hung in register_netdevice_notifier
2020/02/20 16:56:26 vm-34: crash: BUG: MAX_LOCKDEP_KEYS too low!
2020/02/20 16:57:11 vm-3: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 16:57:44 vm-31: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 16:58:13 vm-33: crash: BUG: MAX_LOCKDEP_KEYS too low!
2020/02/20 16:58:35 vm-32: crash: BUG: using smp_processor_id() in
preemptible [ADDR] code: syz-executor
2020/02/20 17:03:12 vm-9: crash: INFO: task hung in raw_release
2020/02/20 17:03:20 vm-3: crash: unregister_netdevice: waiting for DEV
to become free
2020/02/20 17:03:26 vm-6: crash: BUG: using smp_processor_id() in
preemptible [ADDR] code: syz-executor
2020/02/20 17:05:08 vm-31: crash: INFO: task hung in lock_sock_nested
2020/02/20 17:06:01 vm-16: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 17:06:24 vm-26: crash: INFO: task hung in bcm_release
2020/02/20 17:07:20 vm-27: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 17:08:03 vm-11: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
2020/02/20 17:08:05 vm-17: crash: WARNING in hsr_addr_subst_dest
2020/02/20 17:09:12 vm-22: crash: BUG: MAX_LOCKDEP_ENTRIES too low!
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Jason A. Donenfeld]

Hi Dmitry,

On Thu, Feb 20, 2020 at 5:14 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> I got some coverage in wg_netdevice_notification:
> https://imgur.com/a/1sJZKtp
>
> Or you mean the parts that are still red?

Yes, it's the red parts that interest me. Intermixing those with
various wireguard-specific netlink calls and setting devices up and
down and putting traffic through those sockets, in weird ways, could
dig up bugs.

> I think theoretically these parts should be reachable too because
> syzkaller can do unshare and obtain net ns fd's.
>
> It's quite hard to test because it just crashes all the time on known bugs.
> So maybe the most profitable way to get more coverage throughout the
> networking subsystem now is to fix the top layer of crashers ;)

Ahhh, interesting, so the issue is that syzkaller is finding too many
other networking stack bugs before it gets to being able to play with
wireguard. Shucks.

Jason
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Dmitry Vyukov]

On Thu, Feb 20, 2020 at 5:34 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Dmitry,
>
> On Thu, Feb 20, 2020 at 5:14 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> > I got some coverage in wg_netdevice_notification:
> > https://imgur.com/a/1sJZKtp
> >
> > Or you mean the parts that are still red?
>
> Yes, it's the red parts that interest me. Intermixing those with
> various wireguard-specific netlink calls and setting devices up and
> down and putting traffic through those sockets, in weird ways, could
> dig up bugs.
>
> > I think theoretically these parts should be reachable too because
> > syzkaller can do unshare and obtain net ns fd's.
> >
> > It's quite hard to test because it just crashes all the time on known bugs.
> > So maybe the most profitable way to get more coverage throughout the
> > networking subsystem now is to fix the top layer of crashers ;)
>
> Ahhh, interesting, so the issue is that syzkaller is finding too many
> other networking stack bugs before it gets to being able to play with
> wireguard. Shucks.


If it's aimed only at, say, wireguard netlink interface, then it's not
distracted by bugs in other parts. But as you add some ipv4/6 tcp/udp
sockets, more netlink to change these net namespaces, namespaces
related syscalls, packet injection, etc, in the end it covers quite a
significant part of kernel. You know how fuzzing works, right. You
really need to fix the current layer of bugs to get to the next one.
And we accumulated 600+ open bugs. It still finds some new ones, but I
guess these are really primitive ones (as compared to its full bug
finding potential).
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard

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

[Jason A. Donenfeld]

On Thu, Feb 20, 2020 at 5:45 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> If it's aimed only at, say, wireguard netlink interface, then it's not
> distracted by bugs in other parts. But as you add some ipv4/6 tcp/udp
> sockets, more netlink to change these net namespaces, namespaces
> related syscalls, packet injection, etc, in the end it covers quite a
> significant part of kernel. You know how fuzzing works, right. You
> really need to fix the current layer of bugs to get to the next one.
> And we accumulated 600+ open bugs. It still finds some new ones, but I
> guess these are really primitive ones (as compared to its full bug
> finding potential).

Yea, seems reasonable. I need to get a local syzkaller instance set up
for customization and then start patching the things that seem to be
standing in the way. Either way, so long as there isn't some
implementation issue or logical problem getting in the way of calling
that codepath, I'm satisfied in knowing that syzkaller will get there
eventually.
_______________________________________________
WireGuard mailing list
WireGuard@lists.zx2c4.com
https://lists.zx2c4.com/mailman/listinfo/wireguard