| Message ID | 20230411072502.21315-1-martin@strongswan.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec] xfrm: Preserve xfrm interface secpath for packets forwarded | expand |
Hi, On Tue, Apr 11, 2023 at 10:54 AM Martin Willi <martin@strongswan.org> wrote: > > The commit referenced below clears the secpath on packets received via > xfrm interfaces to support nested IPsec tunnels. This breaks Netfilter > policy matching using xt_policy in the FORWARD chain, as the secpath > is missing during forwarding. INPUT matching is not affected, as it is > done before secpath reset. > > A work-around could use XFRM input interface matching for such rules, > but this does not work if the XFRM interface is part of a VRF; the > Netfilter input interface is replaced by the VRF interface, making a > sufficient match for IPsec-protected packets difficult. > > So instead, limit the secpath reset to packets that are targeting the > local host, in the default or a specific VRF. This should allow nested > tunnels, but keeps the secpath intact on packets that are passed to > Netfilter chains with potential IPsec policy matches. > > Fixes: b0355dbbf13c ("Fix XFRM-I support for nested ESP tunnels") > Signed-off-by: Martin Willi <martin@strongswan.org> > --- > include/net/xfrm.h | 10 ++++++++++ > net/xfrm/xfrm_policy.c | 2 +- > 2 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h > index 3e1f70e8e424..f16df2f07a83 100644 > --- a/include/net/xfrm.h > +++ b/include/net/xfrm.h > @@ -1349,6 +1349,16 @@ void xfrm_flowi_addr_get(const struct flowi *fl, > } > } > > +static inline bool xfrm_flowi_is_forwarding(struct net *net, > + const struct flowi *fl) > +{ > + if (fl->flowi_oif == LOOPBACK_IFINDEX) > + return false; > + if (netif_index_is_l3_master(net, fl->flowi_oif)) > + return false; > + return true; > +} > + > static __inline__ int > __xfrm4_state_addr_check(const struct xfrm_state *x, > const xfrm_address_t *daddr, const xfrm_address_t *saddr) > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 5c61ec04b839..4f49698eb29f 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -3745,7 +3745,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, > goto reject; > } > > - if (if_id) > + if (if_id && !xfrm_flowi_is_forwarding(net, &fl)) At first I thought that "dir" would just be "XFRM_POLICY_FWD" from the forwarding path, so you could just do: if (if_id && dir != XFRM_POLICY_FWD) secpath_reset(skb); But I think the problem with this would be when the xfrmi is moved to a different NS in which case the policy check is done using XFRM_POLICY_IN right? if so maybe this can be passed somehow, maybe using a bit in the "dir" outside of XFRM_POLICY_MASK? something like: no_reset_sp = dir & XFRM_POLICY_NO_RESET_SP || dir == XFRM_POLICY_FWD; dir &= XFRM_POLICY_MASK; ... if (if_id && !no_reset_sp) secpath_reset(skb); The benefit I think is in not deducing whether we are in forwarding. Maybe there's some other logic that I'm missing? Eyal.
Hi, On Tue, Apr 11, 2023 at 7:35 PM Eyal Birger <eyal.birger@gmail.com> wrote: > > Hi, > > On Tue, Apr 11, 2023 at 10:54 AM Martin Willi <martin@strongswan.org> wrote: > > > > The commit referenced below clears the secpath on packets received via > > xfrm interfaces to support nested IPsec tunnels. This breaks Netfilter > > policy matching using xt_policy in the FORWARD chain, as the secpath > > is missing during forwarding. INPUT matching is not affected, as it is > > done before secpath reset. > > > > A work-around could use XFRM input interface matching for such rules, > > but this does not work if the XFRM interface is part of a VRF; the > > Netfilter input interface is replaced by the VRF interface, making a > > sufficient match for IPsec-protected packets difficult. > > > > So instead, limit the secpath reset to packets that are targeting the > > local host, in the default or a specific VRF. This should allow nested > > tunnels, but keeps the secpath intact on packets that are passed to > > Netfilter chains with potential IPsec policy matches. > > > > Fixes: b0355dbbf13c ("Fix XFRM-I support for nested ESP tunnels") > > Signed-off-by: Martin Willi <martin@strongswan.org> > > --- > > include/net/xfrm.h | 10 ++++++++++ > > net/xfrm/xfrm_policy.c | 2 +- > > 2 files changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/include/net/xfrm.h b/include/net/xfrm.h > > index 3e1f70e8e424..f16df2f07a83 100644 > > --- a/include/net/xfrm.h > > +++ b/include/net/xfrm.h > > @@ -1349,6 +1349,16 @@ void xfrm_flowi_addr_get(const struct flowi *fl, > > } > > } > > > > +static inline bool xfrm_flowi_is_forwarding(struct net *net, > > + const struct flowi *fl) > > +{ > > + if (fl->flowi_oif == LOOPBACK_IFINDEX) > > + return false; > > + if (netif_index_is_l3_master(net, fl->flowi_oif)) > > + return false; > > + return true; > > +} > > + > > static __inline__ int > > __xfrm4_state_addr_check(const struct xfrm_state *x, > > const xfrm_address_t *daddr, const xfrm_address_t *saddr) > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > > index 5c61ec04b839..4f49698eb29f 100644 > > --- a/net/xfrm/xfrm_policy.c > > +++ b/net/xfrm/xfrm_policy.c > > @@ -3745,7 +3745,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, > > goto reject; > > } > > > > - if (if_id) > > + if (if_id && !xfrm_flowi_is_forwarding(net, &fl)) > > At first I thought that "dir" would just be "XFRM_POLICY_FWD" from the > forwarding path, so you could just do: > > if (if_id && dir != XFRM_POLICY_FWD) > secpath_reset(skb); > > But I think the problem with this would be when the xfrmi is moved to a > different NS in which case the policy check is done using XFRM_POLICY_IN > right? if so maybe this can be passed somehow, maybe using a bit in the "dir" > outside of XFRM_POLICY_MASK? > > something like: > > no_reset_sp = dir & XFRM_POLICY_NO_RESET_SP || dir == XFRM_POLICY_FWD; > dir &= XFRM_POLICY_MASK; > > ... > if (if_id && !no_reset_sp) > secpath_reset(skb); > > The benefit I think is in not deducing whether we are in forwarding. > > Maybe there's some other logic that I'm missing? After another look the secpath is reset in that case anyway. So in that case, which flow is missing when just using: if (if_id && dir != XFRM_POLICY_FWD) secpath_reset(skb); Eyal.
Hi Eyal, > > The benefit I think is in not deducing whether we are in > > forwarding. > > After another look the secpath is reset in that case anyway. > So in that case, which flow is missing when just using: > > if (if_id && dir != XFRM_POLICY_FWD) > secpath_reset(skb); This is obviously the better and simpler approach, and it works just fine in my testing. I'll post a v2. Thanks, Martin
diff --git a/include/net/xfrm.h b/include/net/xfrm.h index 3e1f70e8e424..f16df2f07a83 100644 --- a/include/net/xfrm.h +++ b/include/net/xfrm.h @@ -1349,6 +1349,16 @@ void xfrm_flowi_addr_get(const struct flowi *fl, } } +static inline bool xfrm_flowi_is_forwarding(struct net *net, + const struct flowi *fl) +{ + if (fl->flowi_oif == LOOPBACK_IFINDEX) + return false; + if (netif_index_is_l3_master(net, fl->flowi_oif)) + return false; + return true; +} + static __inline__ int __xfrm4_state_addr_check(const struct xfrm_state *x, const xfrm_address_t *daddr, const xfrm_address_t *saddr) diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 5c61ec04b839..4f49698eb29f 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -3745,7 +3745,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, goto reject; } - if (if_id) + if (if_id && !xfrm_flowi_is_forwarding(net, &fl)) secpath_reset(skb); xfrm_pols_put(pols, npols);
The commit referenced below clears the secpath on packets received via xfrm interfaces to support nested IPsec tunnels. This breaks Netfilter policy matching using xt_policy in the FORWARD chain, as the secpath is missing during forwarding. INPUT matching is not affected, as it is done before secpath reset. A work-around could use XFRM input interface matching for such rules, but this does not work if the XFRM interface is part of a VRF; the Netfilter input interface is replaced by the VRF interface, making a sufficient match for IPsec-protected packets difficult. So instead, limit the secpath reset to packets that are targeting the local host, in the default or a specific VRF. This should allow nested tunnels, but keeps the secpath intact on packets that are passed to Netfilter chains with potential IPsec policy matches. Fixes: b0355dbbf13c ("Fix XFRM-I support for nested ESP tunnels") Signed-off-by: Martin Willi <martin@strongswan.org> --- include/net/xfrm.h | 10 ++++++++++ net/xfrm/xfrm_policy.c | 2 +- 2 files changed, 11 insertions(+), 1 deletion(-)