Re: Raw socket should comply with selected source address
On Thu, Dec 16, 2021 at 11:48:58AM -0700, Theo de Raadt wrote:
> 'route sourceaddr' support is incomplete.
> In particular it does not work in ping or traceroute.
Thanks for the explanation.
> > On Thu, Dec 16, 2021 at 07:20:04PM +0100, Denis Fondras wrote:
> > > Raw sockets do not comply with route sourceaddr.
So it is about unconnected sockets. Should it be fixed there?
> > > + struct sockaddr *ip4_source = NULL;
Initialization = NULL is not necessay.
> > > + ip4_source = rtable_getsource(ro->ro_tableid, AF_INET);
> > > + if (ip4_source != NULL) {
> > > + struct ifaddr *ifa;
> > > + if ((ifa = ifa_ifwithaddr(ip4_source,
I would not call a function from the IP output path that grabs
kernel lock and loops over all interfaces and addresses.
Should we store it at the socket and use it if inp_laddr is not
set?
struct art_root {
struct sockaddr *source;/* [K] optional src addr to use
*/
Why does this field not have an ar_ prefix like all the others?
Why does it need kernel lock? You don't have kernel lock here.
It is set with kernel lock, cleared with net lock and unset with
the routing socket lock. The memory of the sockaddr seems to
be protected by ifaddr refcounting and net lock.
Can we assume that a store to a pointer and also dereferencing it
is atomic?
> > > + ro->ro_tableid)) != NULL &&
> > > + ISSET(ifa->ifa_ifp->if_flags, IFF_UP)) {
> > > + ip->ip_src =
> > > satosin(ip4_source)->sin_addr;
> > > + }
> > > + } else
> > > + ip->ip_src = ia->ia_addr.sin_addr;
> > > + }
> > > }
> > >
> > > #ifdef IPSEC
> >
Re: Raw socket should comply with selected source address
'route sourceaddr' support is incomplete.
In particular it does not work in ping or traceroute.
The original idea of this option is to replace the default src address
allocation algorithm, with a static default, particularily on routers.
It is only working for non-bound sockets, but it should also work for
SOCK_RAW or potentially other configurations.
I believe this was implimented too cynically, as a "oh we only need to
fix these few cases", but we are better off applying the replacement
algorithm for all unbound cases, then there is only algorithm A or B,
not a mix of algorithms depending on the type of socket being used.
It is really fun when you realize ping -v uses a dummy connect() to
figure out a srcaddr to print, then the kernel chooses a different
address for the sent packets.
Alexander Bluhm wrote:
> On Thu, Dec 16, 2021 at 07:20:04PM +0100, Denis Fondras wrote:
> > Raw sockets do not comply with route sourceaddr.
> >
> > Use set address if source is not set by the caller.
>
> Which problem do you want to solve?
> Which setups do you break?
>
> bluhm
>
> > Index: netinet/ip_output.c
> > ===
> > RCS file: /cvs/src/sys/netinet/ip_output.c,v
> > retrieving revision 1.377
> > diff -u -p -r1.377 ip_output.c
> > --- netinet/ip_output.c 3 Dec 2021 17:18:34 - 1.377
> > +++ netinet/ip_output.c 16 Dec 2021 18:12:44 -
> > @@ -110,6 +110,7 @@ ip_output(struct mbuf *m, struct mbuf *o
> > struct route iproute;
> > struct sockaddr_in *dst;
> > struct tdb *tdb = NULL;
> > + struct sockaddr *ip4_source = NULL;
> > u_long mtu;
> > #if NPF > 0
> > u_int orig_rtableid;
> > @@ -237,8 +238,18 @@ reroute:
> > dst = satosin(ro->ro_rt->rt_gateway);
> >
> > /* Set the source IP address */
> > - if (ip->ip_src.s_addr == INADDR_ANY && ia)
> > - ip->ip_src = ia->ia_addr.sin_addr;
> > + if (ip->ip_src.s_addr == INADDR_ANY && ia) {
> > + ip4_source = rtable_getsource(ro->ro_tableid, AF_INET);
> > + if (ip4_source != NULL) {
> > + struct ifaddr *ifa;
> > + if ((ifa = ifa_ifwithaddr(ip4_source,
> > + ro->ro_tableid)) != NULL &&
> > + ISSET(ifa->ifa_ifp->if_flags, IFF_UP)) {
> > + ip->ip_src =
> > satosin(ip4_source)->sin_addr;
> > + }
> > + } else
> > + ip->ip_src = ia->ia_addr.sin_addr;
> > + }
> > }
> >
> > #ifdef IPSEC
>
Re: Raw socket should comply with selected source address
On Thu, Dec 16, 2021 at 07:20:04PM +0100, Denis Fondras wrote:
> Raw sockets do not comply with route sourceaddr.
>
> Use set address if source is not set by the caller.
Which problem do you want to solve?
Which setups do you break?
bluhm
> Index: netinet/ip_output.c
> ===
> RCS file: /cvs/src/sys/netinet/ip_output.c,v
> retrieving revision 1.377
> diff -u -p -r1.377 ip_output.c
> --- netinet/ip_output.c 3 Dec 2021 17:18:34 - 1.377
> +++ netinet/ip_output.c 16 Dec 2021 18:12:44 -
> @@ -110,6 +110,7 @@ ip_output(struct mbuf *m, struct mbuf *o
> struct route iproute;
> struct sockaddr_in *dst;
> struct tdb *tdb = NULL;
> + struct sockaddr *ip4_source = NULL;
> u_long mtu;
> #if NPF > 0
> u_int orig_rtableid;
> @@ -237,8 +238,18 @@ reroute:
> dst = satosin(ro->ro_rt->rt_gateway);
>
> /* Set the source IP address */
> - if (ip->ip_src.s_addr == INADDR_ANY && ia)
> - ip->ip_src = ia->ia_addr.sin_addr;
> + if (ip->ip_src.s_addr == INADDR_ANY && ia) {
> + ip4_source = rtable_getsource(ro->ro_tableid, AF_INET);
> + if (ip4_source != NULL) {
> + struct ifaddr *ifa;
> + if ((ifa = ifa_ifwithaddr(ip4_source,
> + ro->ro_tableid)) != NULL &&
> + ISSET(ifa->ifa_ifp->if_flags, IFF_UP)) {
> + ip->ip_src =
> satosin(ip4_source)->sin_addr;
> + }
> + } else
> + ip->ip_src = ia->ia_addr.sin_addr;
> + }
> }
>
> #ifdef IPSEC
Raw socket should comply with selected source address
Raw sockets do not comply with route sourceaddr.
Use set address if source is not set by the caller.
Index: netinet/ip_output.c
===
RCS file: /cvs/src/sys/netinet/ip_output.c,v
retrieving revision 1.377
diff -u -p -r1.377 ip_output.c
--- netinet/ip_output.c 3 Dec 2021 17:18:34 - 1.377
+++ netinet/ip_output.c 16 Dec 2021 18:12:44 -
@@ -110,6 +110,7 @@ ip_output(struct mbuf *m, struct mbuf *o
struct route iproute;
struct sockaddr_in *dst;
struct tdb *tdb = NULL;
+ struct sockaddr *ip4_source = NULL;
u_long mtu;
#if NPF > 0
u_int orig_rtableid;
@@ -237,8 +238,18 @@ reroute:
dst = satosin(ro->ro_rt->rt_gateway);
/* Set the source IP address */
- if (ip->ip_src.s_addr == INADDR_ANY && ia)
- ip->ip_src = ia->ia_addr.sin_addr;
+ if (ip->ip_src.s_addr == INADDR_ANY && ia) {
+ ip4_source = rtable_getsource(ro->ro_tableid, AF_INET);
+ if (ip4_source != NULL) {
+ struct ifaddr *ifa;
+ if ((ifa = ifa_ifwithaddr(ip4_source,
+ ro->ro_tableid)) != NULL &&
+ ISSET(ifa->ifa_ifp->if_flags, IFF_UP)) {
+ ip->ip_src =
satosin(ip4_source)->sin_addr;
+ }
+ } else
+ ip->ip_src = ia->ia_addr.sin_addr;
+ }
}
#ifdef IPSEC
