* [PATCH ipsec v2] xfrm: Preserve xfrm interface secpath for packets forwarded
@ 2023-04-12 8:56 Martin Willi
2023-04-13 18:04 ` Benedict Wong
0 siblings, 1 reply; 6+ messages in thread
From: Martin Willi @ 2023-04-12 8:56 UTC (permalink / raw)
To: Steffen Klassert, Benedict Wong, Eyal Birger
Cc: Herbert Xu, David S . Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev
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 not using a
XFRM forward policy. 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")
Suggested-by: Eyal Birger <eyal.birger@gmail.com>
Signed-off-by: Martin Willi <martin@strongswan.org>
---
v1 -> v2: Use policy dir instead of flowi outif to check for forwarding
net/xfrm/xfrm_policy.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 5c61ec04b839..669c3c0880a6 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 && dir != XFRM_POLICY_FWD)
secpath_reset(skb);
xfrm_pols_put(pols, npols);
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH ipsec v2] xfrm: Preserve xfrm interface secpath for packets forwarded
2023-04-12 8:56 [PATCH ipsec v2] xfrm: Preserve xfrm interface secpath for packets forwarded Martin Willi
@ 2023-04-13 18:04 ` Benedict Wong
2023-04-17 22:01 ` Benedict Wong
2023-04-25 7:45 ` Martin Willi
0 siblings, 2 replies; 6+ messages in thread
From: Benedict Wong @ 2023-04-13 18:04 UTC (permalink / raw)
To: Martin Willi
Cc: Steffen Klassert, Eyal Birger, Herbert Xu, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev
Not directly related to this change, but in testing these on a broader
swath of Android tests, we've found that my original change also
happens to break Transport-in-Tunnel mode (which attempts to match the
outer tunnel mode policy twice.). I wonder if it's worth just
reverting first, and going back to a previous iteration of the nested
policy checks that allows multiple lookups of the same
template/secpath pair.
On Wed, Apr 12, 2023 at 1:56 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 not using a
> XFRM forward policy. 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")
> Suggested-by: Eyal Birger <eyal.birger@gmail.com>
> Signed-off-by: Martin Willi <martin@strongswan.org>
> ---
> v1 -> v2: Use policy dir instead of flowi outif to check for forwarding
>
> net/xfrm/xfrm_policy.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 5c61ec04b839..669c3c0880a6 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 && dir != XFRM_POLICY_FWD)
> secpath_reset(skb);
>
> xfrm_pols_put(pols, npols);
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH ipsec v2] xfrm: Preserve xfrm interface secpath for packets forwarded
2023-04-13 18:04 ` Benedict Wong
@ 2023-04-17 22:01 ` Benedict Wong
2023-04-19 6:06 ` Steffen Klassert
2023-04-25 7:45 ` Martin Willi
1 sibling, 1 reply; 6+ messages in thread
From: Benedict Wong @ 2023-04-17 22:01 UTC (permalink / raw)
To: Martin Willi, Steffen Klassert
Cc: Eyal Birger, Herbert Xu, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
I believe I have a potential solution that caches the policy matches,
rather than clearing the secpath, which should allow for repeated
matches against a secpath entry, while allowing other already-matched
secpath entries to not need to match nested policies. That should
solve for the general case where the secpath gets checked against
policies multiple times (both in the forwarding case, as well as in
the nested transport mode in tunnel mode case.
Forgive my not knowing of convention; should I send that as a separate
patch, or append it as a reply to this thread?
On Thu, Apr 13, 2023 at 11:04 AM Benedict Wong <benedictwong@google.com> wrote:
>
> Not directly related to this change, but in testing these on a broader
> swath of Android tests, we've found that my original change also
> happens to break Transport-in-Tunnel mode (which attempts to match the
> outer tunnel mode policy twice.). I wonder if it's worth just
> reverting first, and going back to a previous iteration of the nested
> policy checks that allows multiple lookups of the same
> template/secpath pair.
>
>
> On Wed, Apr 12, 2023 at 1:56 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 not using a
> > XFRM forward policy. 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")
> > Suggested-by: Eyal Birger <eyal.birger@gmail.com>
> > Signed-off-by: Martin Willi <martin@strongswan.org>
> > ---
> > v1 -> v2: Use policy dir instead of flowi outif to check for forwarding
> >
> > net/xfrm/xfrm_policy.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> > index 5c61ec04b839..669c3c0880a6 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 && dir != XFRM_POLICY_FWD)
> > secpath_reset(skb);
> >
> > xfrm_pols_put(pols, npols);
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH ipsec v2] xfrm: Preserve xfrm interface secpath for packets forwarded
2023-04-17 22:01 ` Benedict Wong
@ 2023-04-19 6:06 ` Steffen Klassert
0 siblings, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2023-04-19 6:06 UTC (permalink / raw)
To: Benedict Wong
Cc: Martin Willi, Eyal Birger, Herbert Xu, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev
On Mon, Apr 17, 2023 at 03:01:26PM -0700, Benedict Wong wrote:
> I believe I have a potential solution that caches the policy matches,
> rather than clearing the secpath, which should allow for repeated
> matches against a secpath entry, while allowing other already-matched
> secpath entries to not need to match nested policies. That should
> solve for the general case where the secpath gets checked against
> policies multiple times (both in the forwarding case, as well as in
> the nested transport mode in tunnel mode case.
>
> Forgive my not knowing of convention; should I send that as a separate
> patch, or append it as a reply to this thread?
Send it as a separate patch.
Thanks!
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH ipsec v2] xfrm: Preserve xfrm interface secpath for packets forwarded
2023-04-13 18:04 ` Benedict Wong
2023-04-17 22:01 ` Benedict Wong
@ 2023-04-25 7:45 ` Martin Willi
2023-04-25 7:47 ` Steffen Klassert
1 sibling, 1 reply; 6+ messages in thread
From: Martin Willi @ 2023-04-25 7:45 UTC (permalink / raw)
To: Steffen Klassert, Benedict Wong
Cc: Eyal Birger, Herbert Xu, David S . Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, netdev
> [...] my original change also happens to break Transport-in-Tunnel
> mode (which attempts to match the outer tunnel mode policy twice.). I
> wonder if it's worth just reverting first
Given that the offending commit has been picked up by -stable and now
by distros, I guess this regression will start affecting more IPsec
users.
May I suggest to go with a revert of the offending commit as an
immediate fix, and then bring in a fixed nested policy check from
Benedict in a separate effort?
I'll post a patch with the revert.
Thanks,
Martin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH ipsec v2] xfrm: Preserve xfrm interface secpath for packets forwarded
2023-04-25 7:45 ` Martin Willi
@ 2023-04-25 7:47 ` Steffen Klassert
0 siblings, 0 replies; 6+ messages in thread
From: Steffen Klassert @ 2023-04-25 7:47 UTC (permalink / raw)
To: Martin Willi
Cc: Benedict Wong, Eyal Birger, Herbert Xu, David S . Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, netdev
On Tue, Apr 25, 2023 at 09:45:40AM +0200, Martin Willi wrote:
>
>
> > [...] my original change also happens to break Transport-in-Tunnel
> > mode (which attempts to match the outer tunnel mode policy twice.). I
> > wonder if it's worth just reverting first
>
> Given that the offending commit has been picked up by -stable and now
> by distros, I guess this regression will start affecting more IPsec
> users.
>
> May I suggest to go with a revert of the offending commit as an
> immediate fix, and then bring in a fixed nested policy check from
> Benedict in a separate effort?
>
> I'll post a patch with the revert.
I'm fine with that.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-04-25 7:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-12 8:56 [PATCH ipsec v2] xfrm: Preserve xfrm interface secpath for packets forwarded Martin Willi
2023-04-13 18:04 ` Benedict Wong
2023-04-17 22:01 ` Benedict Wong
2023-04-19 6:06 ` Steffen Klassert
2023-04-25 7:45 ` Martin Willi
2023-04-25 7:47 ` Steffen Klassert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.