Re: fix ping(8) and traceroute(8) source selection
Le Sat, Dec 18, 2021 at 10:02:32AM +0100, Florian Obser a écrit : > On 2021-12-17 22:12 +01, Denis Fondras wrote: > > Here is an attempt to fix ping(8) and traceroute(8) source selection. > > > > Currently these tools do not obey route sourceaddr set by the operator. This > > leads to frustration at best and erroneous diagnosis at worse on multi-homed > > systems. > > I did not closely follow route(8)'s sourceaddr feature. Is this only an > issue with IPv4 or would ping6 / traceroute6 need a similar fix (which > is going to be difficult). > IPv6 is immune because it is the responsability of the caller to set a valid source address (unless it is DAD packet).
Re: fix ping(8) and traceroute(8) source selection
On 2021-12-17 22:12 +01, Denis Fondras wrote: > Here is an attempt to fix ping(8) and traceroute(8) source selection. > > Currently these tools do not obey route sourceaddr set by the operator. This > leads to frustration at best and erroneous diagnosis at worse on multi-homed > systems. I did not closely follow route(8)'s sourceaddr feature. Is this only an issue with IPv4 or would ping6 / traceroute6 need a similar fix (which is going to be difficult). > > The "real" fix would be to rework source selection in the kernel stack but > this > is a huge work which not happen overnight nor in the coming days. > > In the mean time, I propose the following diff. > > I removed -R (route recording) in ping(8) because it is not compatible with > sending a full IP header to the rip_output(). It should not impact anyone as > RR > is most of the time ignored by routers. I was toying with removing -R when I was unifying ping and ping6, but then I found a system that did allow it. IIRC one needs to disable pf on OpenBSD, so the utility is indeed very limited. Maybe it's time for it to go. > > Denis > > Index: sbin/ping/ping.c > === > RCS file: /cvs/src/sbin/ping/ping.c,v > retrieving revision 1.245 > diff -u -p -r1.245 ping.c > --- sbin/ping/ping.c 12 Jul 2021 15:09:19 - 1.245 > +++ sbin/ping/ping.c 17 Dec 2021 20:27:31 - > @@ -143,16 +143,14 @@ int options; > #define F_HOSTNAME 0x0004 > #define F_PINGFILLED0x0008 > #define F_QUIET 0x0010 > -#define F_RROUTE0x0020 > -#define F_SO_DEBUG 0x0040 > -#define F_SHOWCHAR 0x0080 > -#define F_VERBOSE 0x0100 > +#define F_SO_DEBUG 0x0020 > +#define F_SHOWCHAR 0x0040 > +#define F_VERBOSE 0x0080 no need for this churn > /* 0x0200 */ you could just add a comment like this ^ > -#define F_HDRINCL 0x0400 > -#define F_TTL 0x0800 > -#define F_TOS 0x1000 > -#define F_AUD_RECV 0x2000 > -#define F_AUD_MISS 0x4000 > +#define F_TTL 0x0100 > +#define F_TOS 0x0200 > +#define F_AUD_RECV 0x0400 > +#define F_AUD_MISS 0x0800 I'm with deraadt@ that the kernel implementation should be finished. -- I'm not entirely sure you are real.
Re: fix ping(8) and traceroute(8) source selection
I am not a fan of solving this in ping and traceroute, because there are other places this alternative-srcaddr mechanism is not working, and it feels like all of the solutions need to be in the kernel. Also, there is no urgency to fix it immediately. So a kernel option can be invested in a leisurely way.
fix ping(8) and traceroute(8) source selection
Here is an attempt to fix ping(8) and traceroute(8) source selection.
Currently these tools do not obey route sourceaddr set by the operator. This
leads to frustration at best and erroneous diagnosis at worse on multi-homed
systems.
The "real" fix would be to rework source selection in the kernel stack but this
is a huge work which not happen overnight nor in the coming days.
In the mean time, I propose the following diff.
I removed -R (route recording) in ping(8) because it is not compatible with
sending a full IP header to the rip_output(). It should not impact anyone as RR
is most of the time ignored by routers.
Denis
Index: sbin/ping/ping.c
===
RCS file: /cvs/src/sbin/ping/ping.c,v
retrieving revision 1.245
diff -u -p -r1.245 ping.c
--- sbin/ping/ping.c12 Jul 2021 15:09:19 - 1.245
+++ sbin/ping/ping.c17 Dec 2021 20:27:31 -
@@ -143,16 +143,14 @@ int options;
#defineF_HOSTNAME 0x0004
#defineF_PINGFILLED0x0008
#defineF_QUIET 0x0010
-#defineF_RROUTE0x0020
-#defineF_SO_DEBUG 0x0040
-#defineF_SHOWCHAR 0x0080
-#defineF_VERBOSE 0x0100
+#defineF_SO_DEBUG 0x0020
+#defineF_SHOWCHAR 0x0040
+#defineF_VERBOSE 0x0080
/* 0x0200 */
-#defineF_HDRINCL 0x0400
-#defineF_TTL 0x0800
-#defineF_TOS 0x1000
-#defineF_AUD_RECV 0x2000
-#defineF_AUD_MISS 0x4000
+#defineF_TTL 0x0100
+#defineF_TOS 0x0200
+#defineF_AUD_RECV 0x0400
+#defineF_AUD_MISS 0x0800
/* multicast options */
int moptions;
@@ -256,7 +254,6 @@ main(int argc, char *argv[])
u_char *datap, *packet;
u_char ttl = MAXTTL;
char *e, *target, hbuf[NI_MAXHOST], *source = NULL;
- char rspace[3 + 4 * NROUTES + 1]; /* record route space */
const char *errstr;
double fraction, integral, seconds;
uid_t ouid, uid;
@@ -308,7 +305,6 @@ main(int argc, char *argv[])
errstr, optarg);
break;
case 'D':
- options |= F_HDRINCL;
df = 1;
break;
case 'd':
@@ -383,7 +379,7 @@ main(int argc, char *argv[])
options |= F_QUIET;
break;
case 'R':
- options |= F_RROUTE;
+ printf("-R option is not supported anymore\n");
break;
case 's': /* size of packet to send */
datalen = strtonum(optarg, 0, maxpayload, );
@@ -393,7 +389,6 @@ main(int argc, char *argv[])
break;
#ifndef SMALL
case 'T':
- options |= F_HDRINCL;
options |= F_TOS;
errno = 0;
errstr = NULL;
@@ -509,7 +504,7 @@ main(int argc, char *argv[])
if (bind(s, from, from->sa_len) == -1)
err(1, "bind");
}
- } else if (options & F_VERBOSE) {
+ } else {
/*
* get the source address. XXX since we revoked the root
* privilege, we cannot use a raw socket for this.
@@ -711,51 +706,26 @@ main(int argc, char *argv[])
err(1, "setsockopt(IPV6_RECVHOPLIMIT)");
} else {
u_char loop = 0;
+ struct ip *ip = (struct ip *)outpackhdr;
if (options & F_TTL) {
if (IN_MULTICAST(ntohl(dst4.sin_addr.s_addr)))
moptions |= MULTICAST_TTL;
- else
- options |= F_HDRINCL;
}
- if ((options & F_RROUTE) && (options & F_HDRINCL))
- errx(1, "-R option and -D or -T, or -t to unicast"
- " destinations are incompatible");
-
- if (options & F_HDRINCL) {
- struct ip *ip = (struct ip *)outpackhdr;
-
- if (setsockopt(s, IPPROTO_IP, IP_HDRINCL,
- , sizeof(optval)) == -1)
- err(1, "setsockopt(IP_HDRINCL)");
- ip->ip_v = IPVERSION;
- ip->ip_hl = sizeof(struct ip) >> 2;
- ip->ip_tos = tos;
- ip->ip_id = 0;
- ip->ip_off = htons(df ? IP_DF : 0);
- ip->ip_ttl = ttl;
- ip->ip_p = IPPROTO_ICM
