PF_KEY socket: limit the length of copied socket address

[kib]

Summary

for SADB_UPDATE op SADB_X_EXT_NEW_ADDRESS_SRC/DST extensions, by
checking the sa_len matching the address family requirements before
doing the copy.

Also convert KEY_SETSECASIDX() and KEY_SETSECSPIDX() to functions and
apply the same sa_len clamping there as well.

See https://github.com/0xdeadbeefnetwork/pfkey-sadb-overflow

PR:     297264
Tested by: Wafa Hamzah <[e-mail]>

[markj]

Do the KEY_SETSECASIDX and KEY_SETSECSPIDX macros need to be fixed too? There are quite a few places in this file that copy sockaddrs around, but so far I didn't find any others without checks.

sys/netipsec/key.c

5380

I suspect these checks should be done before the copying, rather. key_checksockaddrs() validates the socket lengths, but it happens too late.

[kib]

sys/netipsec/key.c

5380

After sa_len is clamped, I do not think it matter.

[kib]

Handle KEY_SETSECASIDX() and KEY_SETSECSPIDX().

[markj]

sys/netipsec/key.c

593

Stray backslash here.

5380

But no clamping is needed, we can just check the sockaddr earlier. Moreover, since sizeof(union sockaddr_union) == sizeof(struct sockaddr_in6), the clamping will hide invalid socklens for AF_INET6 sockaddrs.

[kib]

sys/netipsec/key.c

5380

I think the current code is easier to follow, but ok.

[glebius]

Not an expert here.

[kib]

Ping?

[markj]

sys/netipsec/key.c

599

Looking through all the callers of the two macros, we are already using key_checksockaddrs() to validate the sockaddrs everywhere except in one place: key_allocsa_tunnel(). The sockaddrs passed to key_allocsa_tunnel() are set up in ipcomp4_nonexp_encapcheck() and ipcomp6_nonexp_encapcheck().

So, I think we can remove the clamping. I prefer to avoid clamping if possible, I think it will hide bugs.

ipcomp6_nonexp_encapcheck() looks weird: it sets sa_family = AF_INET but treats them as sockaddr_in6 addresses. Is that a copy-paste bug? If so, we can fix that, and then make the two helpers assert that key_checksockaddrs() passes.

[kib]

sys/netipsec/key.c

599

The mbufs passed to ipcompX_nonexp_encapcheck() are already checked to be at least the required length, in ip_encap.c::encap_input(), I believe. This is passed through the ipv4/6_encap_cfg structure min_length member. After that, the sockaddrs for key_allocsa_tunnel should pass the invariant by construction. I just removed the clamping from key_setsec* helpers.

I believe that you are right about ipcomp6_nonexp_encapcheck() setting the wrong family, I do not see how the AF_INET could be correct there. But I am not set up to test it. I just added the patch to change the family.

[kib]

Constify key_checksockaddrs() args.
Remove clamping.
Assert that key_checksockaddrs() pass for helpers.
Change the socket address family in ipcomp6_nonexp_encapcheck().