Page MenuHomeFreeBSD

udp: Let jail policy rewrite the dstaddr for v6 sendto()s
AcceptedPublic

Authored by markj on Thu, Sep 17, 8:32 PM.

Details

Reviewers
bz
glebius
Group Reviewers
network
Summary

When performing an unconnected sendto() on a v6 UDP socket in a classic
jail, we were not applying the usual policy of replacing the loopback
addr with the jail's primary IP. Compare with, e.g., udp6_connect() or
the IPv4 udp_send(). Fix that.

Reported by: Yuxiang Yang, Yizhou Zhao, Ao Wang, Xuewei Feng, Qi Li,

		and Ke Xu from Tsinghua University using GLM-5.1 from Z.ai

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 77040
Build 73923: arc lint + arc unit

Event Timeline

markj requested review of this revision.Thu, Sep 17, 8:32 PM
sys/netinet6/udp6_usrreq.c
819

I would have assume in_selectsrc() would do that properly as compared to IPv4 we DO (or did who knows what happened to all this) do source address selection based on destination in IPv6. And it seems it does something given laddr is sret to a result of in6_selectsrc_socket().

So taking the primary address is definitively not the right thing in a an IPv6 jail.
It needs to come out of the right prefix if there are multiple.

If or how this works these days I have no clue without starting to read a lot of code.
But based on the description I am not sure what problem the prison_remote_ip6() call here is supposed to solve?

bz requested changes to this revision.Thu, Sep 17, 8:51 PM
bz added inline comments.
sys/netinet6/udp6_usrreq.c
819

Also if at all (in6_selectsrc[_socket] does not do the right job), I would put this below the next KASSERT, just to be sure.

This revision now requires changes to proceed.Thu, Sep 17, 8:51 PM
markj added inline comments.
sys/netinet6/udp6_usrreq.c
819

The problem tis that a jail restricted to ip6.addr=fd00::1 can use sendto() to send a UDP packet to a socket bound to ::1. The kernel is supposed to transparently rewrite ::1 -> fd00::1 within the jail. It does that when one connect()s the socket to ::1, but not when using unconnected sendto().

I can't see any code in in_selectsrc() which handles this.

sys/netinet6/udp6_usrreq.c
819

I went back to 8.0, which appears to be the first release which supports jailed v6 addresses, and I cannot see how it is different in this respect. udp6_connect() silently rewrites the dstaddr, but udp6_send() does not. Nothing in in6_selectsrc() applies the policy implemented in prison_remote_ip6().

Move the rewrite to after the assertion.

sys/netinet6/udp6_usrreq.c
819

Assume you have a jail with (in order):

fd00::1
and
2001:db8::1

your destination is 2001:db8::123

We ought to pick 2001:db8::1 not fd00::1 and I am almost certain we used to once upon a time because I wrote the code for IPv4 which never made it as no one wanted to benchmark it and it was considered too expensive. I can go and dig history and look through code for IPv6 but if you want to get this fixed for now my suggestion is:

(a) move it below the KASSERT
and
(b) add a comment detailing in the lines of above that in a multi-IPv6 jail we may pick the wrong source address.

And yes, it seems udp6_connect() is equally broken and would need a similar comment? *sigh*.

And someone ought to open a PR for the two cases then.

sys/netinet6/udp6_usrreq.c
819

I don't understand your example. This change only makes a difference when your destination is ::1. The kernel is not supposed to let the jailed socket send packets to a UDP socket bound to ::1 in the jail host.

bz added inline comments.
sys/netinet6/udp6_usrreq.c
819

Oh forgive me. Ignore all the above. I unrolled Phab to the beginning of the function and sin6 is an alias for addr6. My brain was on the unbound, not the unconnected problem.

This revision is now accepted and ready to land.Thu, Sep 17, 9:53 PM