Re: have pf_route bail out if it resolves a route with RTF_LOCAL set
On Fri, Jan 29, 2021 at 03:23:31PM +0100, Alexander Bluhm wrote:
> On Fri, Jan 29, 2021 at 10:53:09AM +1000, David Gwynne wrote:
> > > Are you sure that it does not break any use case? I have seen so
> > > much strange stuff. What is the advantage?
> >
> > The current behaviour is lucky at best, and quirky at worst. Usually I
> > would agree with you that breaking stuff isn't great, even if it's
> > wrong, but while I'm changing how route-to etc works I think it's
> > a good chance to clean up some of these edge cases.
>
> I have been developping products based on pf edge cases for 15
> years. I don't know which dragons are in our codebase. This should
> not prevent improvements in OpenBSD. I am just asking not to remove
> anything just because we currently don't know, how it can be used.
i understand.
> Changing syntax like address@interface can easily be adpted. Slight
> semantic changes may cause debugging sessions on productive customer
> systems. And then we might need a quick new solution for a previously
> existing feature. So please be careful.
the regress tests i just updated made it clear that using route-to with
loopback interfaces was not supposed to work. i think blacklisting
RTF_LOCAL routes is in keeping with that idea, and would go a bit
further and drop packets going out IFF_LOOPBACK interfaces too. i can't
think of a good edge case that this would break.
Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1107
diff -u -p -r1.1107 pf.c
--- pf.c3 Feb 2021 07:41:12 - 1.1107
+++ pf.c4 Feb 2021 22:43:11 -
@@ -6015,7 +6015,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
rtableid = m0->m_pkthdr.ph_rtableid;
rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
- if (!rtisvalid(rt)) {
+ if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
if (s->rt != PF_DUPTO) {
pf_send_icmp(m0, ICMP_UNREACH, ICMP_UNREACH_HOST,
0, pd->af, s->rule.ptr, pd->rdomain);
@@ -6025,7 +6025,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
}
ifp = if_get(rt->rt_ifidx);
- if (ifp == NULL)
+ if (ifp == NULL || ISSET(ifp->if_flags, IFF_LOOPBACK))
goto bad;
/* A locally generated packet may have invalid source address. */
@@ -6159,7 +6159,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
if (IN6_IS_SCOPE_EMBED(&dst->sin6_addr))
dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
- if (!rtisvalid(rt)) {
+ if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
if (s->rt != PF_DUPTO) {
pf_send_icmp(m0, ICMP6_DST_UNREACH,
ICMP6_DST_UNREACH_NOROUTE, 0,
@@ -6170,7 +6170,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
}
ifp = if_get(rt->rt_ifidx);
- if (ifp == NULL)
+ if (ifp == NULL || ISSET(ifp->if_flags, IFF_LOOPBACK))
goto bad;
/* A locally generated packet may have invalid source address. */
>
Re: have pf_route bail out if it resolves a route with RTF_LOCAL set
On Fri, Jan 29, 2021 at 10:53:09AM +1000, David Gwynne wrote: > > Are you sure that it does not break any use case? I have seen so > > much strange stuff. What is the advantage? > > The current behaviour is lucky at best, and quirky at worst. Usually I > would agree with you that breaking stuff isn't great, even if it's > wrong, but while I'm changing how route-to etc works I think it's > a good chance to clean up some of these edge cases. I have been developping products based on pf edge cases for 15 years. I don't know which dragons are in our codebase. This should not prevent improvements in OpenBSD. I am just asking not to remove anything just because we currently don't know, how it can be used. Changing syntax like address@interface can easily be adpted. Slight semantic changes may cause debugging sessions on productive customer systems. And then we might need a quick new solution for a previously existing feature. So please be careful. bluhm
Re: have pf_route bail out if it resolves a route with RTF_LOCAL set
On Thu, Jan 28, 2021 at 08:09:36PM +0100, Alexander Bluhm wrote:
> On Thu, Jan 28, 2021 at 09:57:33AM +1000, David Gwynne wrote:
> > calling if_output with a route to a local IP is confusing, and I'm not
> > sure it makes sense anyway.
> >
> > this treats a an RTF_LOCAL route like an invalid round and drops the
> > packet.
> >
> > ok?
>
> Are you sure that it does not break any use case? I have seen so
> much strange stuff. What is the advantage?
The current behaviour is lucky at best, and quirky at worst. Usually I
would agree with you that breaking stuff isn't great, even if it's
wrong, but while I'm changing how route-to etc works I think it's
a good chance to clean up some of these edge cases.
>
> bluhm
>
> > Index: pf.c
> > ===
> > RCS file: /cvs/src/sys/net/pf.c,v
> > retrieving revision 1.1104
> > diff -u -p -r1.1104 pf.c
> > --- pf.c27 Jan 2021 23:53:35 - 1.1104
> > +++ pf.c27 Jan 2021 23:55:49 -
> > @@ -6054,7 +6054,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
> > }
> >
> > rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
> > - if (!rtisvalid(rt)) {
> > + if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
> > if (r->rt != PF_DUPTO) {
> > pf_send_icmp(m0, ICMP_UNREACH, ICMP_UNREACH_HOST,
> > 0, pd->af, s->rule.ptr, pd->rdomain);
> > @@ -6213,7 +6213,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
> > if (IN6_IS_SCOPE_EMBED(&dst->sin6_addr))
> > dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
> > rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
> > - if (!rtisvalid(rt)) {
> > + if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
> > if (r->rt != PF_DUPTO) {
> > pf_send_icmp(m0, ICMP6_DST_UNREACH,
> > ICMP6_DST_UNREACH_NOROUTE, 0,
Re: have pf_route bail out if it resolves a route with RTF_LOCAL set
On Thu, Jan 28, 2021 at 09:57:33AM +1000, David Gwynne wrote:
> calling if_output with a route to a local IP is confusing, and I'm not
> sure it makes sense anyway.
>
> this treats a an RTF_LOCAL route like an invalid round and drops the
> packet.
>
> ok?
Are you sure that it does not break any use case? I have seen so
much strange stuff. What is the advantage?
bluhm
> Index: pf.c
> ===
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1104
> diff -u -p -r1.1104 pf.c
> --- pf.c 27 Jan 2021 23:53:35 - 1.1104
> +++ pf.c 27 Jan 2021 23:55:49 -
> @@ -6054,7 +6054,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
> }
>
> rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
> - if (!rtisvalid(rt)) {
> + if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
> if (r->rt != PF_DUPTO) {
> pf_send_icmp(m0, ICMP_UNREACH, ICMP_UNREACH_HOST,
> 0, pd->af, s->rule.ptr, pd->rdomain);
> @@ -6213,7 +6213,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
> if (IN6_IS_SCOPE_EMBED(&dst->sin6_addr))
> dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
> rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
> - if (!rtisvalid(rt)) {
> + if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
> if (r->rt != PF_DUPTO) {
> pf_send_icmp(m0, ICMP6_DST_UNREACH,
> ICMP6_DST_UNREACH_NOROUTE, 0,
Re: have pf_route bail out if it resolves a route with RTF_LOCAL set
On Thu, Jan 28, 2021 at 08:15:06AM +0100, Claudio Jeker wrote:
> On Thu, Jan 28, 2021 at 09:57:33AM +1000, David Gwynne wrote:
> > calling if_output with a route to a local IP is confusing, and I'm not
> > sure it makes sense anyway.
> >
> > this treats a an RTF_LOCAL route like an invalid round and drops the
> > packet.
> >
> > ok?
>
> Isn't this a change of behaviour? I think ip_output() fill pass the packet
> via loopback back into the system. Which is probably strange for route-to
> / reply-to (since there is divert-to and rdr-to for this). What about
> dup-to?
Yes, it's a deliberate change of behaviour. If pf_route resolves an
RTF_LOCAL route, it tries to push it straight out the interface output
routine. For tunnel style interfaces, it pushes the packet straight over
to the other side. For broadcast (Ethernet), I'd have to read some code,
but my guess is that we'll send an ethernet packet to ourselves.
Neither of those are desirable. So do we reject the packet like this
diff suggests? Or do I copy more of ip_output into pf_route? Or do we
look at cutting ip_output in half so pf_route can call the tail of it?
The only difference between route-to/reply-to and dup-to is that dup-to
does not get the icmp error generated. The RTF_LOCAL destined packets
would still be dropped though.
dlg
>
> > Index: pf.c
> > ===
> > RCS file: /cvs/src/sys/net/pf.c,v
> > retrieving revision 1.1104
> > diff -u -p -r1.1104 pf.c
> > --- pf.c27 Jan 2021 23:53:35 - 1.1104
> > +++ pf.c27 Jan 2021 23:55:49 -
> > @@ -6054,7 +6054,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
> > }
> >
> > rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
> > - if (!rtisvalid(rt)) {
> > + if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
> > if (r->rt != PF_DUPTO) {
> > pf_send_icmp(m0, ICMP_UNREACH, ICMP_UNREACH_HOST,
> > 0, pd->af, s->rule.ptr, pd->rdomain);
> > @@ -6213,7 +6213,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
> > if (IN6_IS_SCOPE_EMBED(&dst->sin6_addr))
> > dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
> > rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
> > - if (!rtisvalid(rt)) {
> > + if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
> > if (r->rt != PF_DUPTO) {
> > pf_send_icmp(m0, ICMP6_DST_UNREACH,
> > ICMP6_DST_UNREACH_NOROUTE, 0,
> >
>
> --
> :wq Claudio
Re: have pf_route bail out if it resolves a route with RTF_LOCAL set
On Thu, Jan 28, 2021 at 09:57:33AM +1000, David Gwynne wrote:
> calling if_output with a route to a local IP is confusing, and I'm not
> sure it makes sense anyway.
>
> this treats a an RTF_LOCAL route like an invalid round and drops the
> packet.
>
> ok?
Isn't this a change of behaviour? I think ip_output() fill pass the packet
via loopback back into the system. Which is probably strange for route-to
/ reply-to (since there is divert-to and rdr-to for this). What about
dup-to?
> Index: pf.c
> ===
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.1104
> diff -u -p -r1.1104 pf.c
> --- pf.c 27 Jan 2021 23:53:35 - 1.1104
> +++ pf.c 27 Jan 2021 23:55:49 -
> @@ -6054,7 +6054,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
> }
>
> rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
> - if (!rtisvalid(rt)) {
> + if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
> if (r->rt != PF_DUPTO) {
> pf_send_icmp(m0, ICMP_UNREACH, ICMP_UNREACH_HOST,
> 0, pd->af, s->rule.ptr, pd->rdomain);
> @@ -6213,7 +6213,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
> if (IN6_IS_SCOPE_EMBED(&dst->sin6_addr))
> dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
> rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
> - if (!rtisvalid(rt)) {
> + if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
> if (r->rt != PF_DUPTO) {
> pf_send_icmp(m0, ICMP6_DST_UNREACH,
> ICMP6_DST_UNREACH_NOROUTE, 0,
>
--
:wq Claudio
have pf_route bail out if it resolves a route with RTF_LOCAL set
calling if_output with a route to a local IP is confusing, and I'm not
sure it makes sense anyway.
this treats a an RTF_LOCAL route like an invalid round and drops the
packet.
ok?
Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1104
diff -u -p -r1.1104 pf.c
--- pf.c27 Jan 2021 23:53:35 - 1.1104
+++ pf.c27 Jan 2021 23:55:49 -
@@ -6054,7 +6054,7 @@ pf_route(struct pf_pdesc *pd, struct pf_
}
rt = rtalloc(sintosa(dst), RT_RESOLVE, rtableid);
- if (!rtisvalid(rt)) {
+ if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
if (r->rt != PF_DUPTO) {
pf_send_icmp(m0, ICMP_UNREACH, ICMP_UNREACH_HOST,
0, pd->af, s->rule.ptr, pd->rdomain);
@@ -6213,7 +6213,7 @@ pf_route6(struct pf_pdesc *pd, struct pf
if (IN6_IS_SCOPE_EMBED(&dst->sin6_addr))
dst->sin6_addr.s6_addr16[1] = htons(ifp->if_index);
rt = rtalloc(sin6tosa(dst), RT_RESOLVE, rtableid);
- if (!rtisvalid(rt)) {
+ if (!rtisvalid(rt) || ISSET(rt->rt_flags, RTF_LOCAL)) {
if (r->rt != PF_DUPTO) {
pf_send_icmp(m0, ICMP6_DST_UNREACH,
ICMP6_DST_UNREACH_NOROUTE, 0,
