| Message ID | 20220824221252.4130836-3-benedictwong@google.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xfrm: Fix bugs in stacked XFRM-I tunnels | expand |
On Wed, Aug 24, 2022 at 10:12:52PM +0000, Benedict Wong wrote: > This change ensures that all nested XFRM packets have their policy > checked before decryption of the next layer, so that policies are > verified at each intermediate step of the decryption process. > > Notably, raw ESP/AH packets do not perform policy checks inherently, > whereas all other encapsulated packets (UDP, TCP encapsulated) do policy > checks after calling xfrm_input handling in the respective encapsulation > layer. > > This is necessary especially for nested tunnels, as the IP addresses, > protocol and ports may all change, thus not matching the previous > policies. In order to ensure that packets match the relevant inbound > templates, the xfrm_policy_check should be done before handing off to > the inner XFRM protocol to decrypt and decapsulate. > > In order to prevent double-checking packets both here and in the > encapsulation layers, this check is currently limited to nested > tunnel-mode transforms and checked prior to decapsulation of inner > tunnel layers (prior to hitting a nested tunnel's xfrm_input, there > is no great way to detect a nested tunnel). This is primarily a > performance consideration, as a general blanket check at the end of > xfrm_input would suffice, but may result in multiple policy checks. > > Test: Tested against Android Kernel Unit Tests > Signed-off-by: Benedict Wong <benedictwong@google.com> > --- > net/xfrm/xfrm_input.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c > index bcb9ee25474b..a3b55d109836 100644 > --- a/net/xfrm/xfrm_input.c > +++ b/net/xfrm/xfrm_input.c > @@ -586,6 +586,20 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) > goto drop; > } > > + /* If nested tunnel, check outer states before context is lost. > + * Only nested tunnels need to be checked, since IP addresses change > + * as a result of the tunnel mode decapsulation. Similarly, this check > + * is limited to nested tunnels to avoid performing another policy > + * check on non-nested tunnels. On success, this check also updates the > + * secpath's verified_cnt variable, skipping future verifications of > + * previously-verified secpath entries. > + */ > + if ((x->outer_mode.flags & XFRM_MODE_FLAG_TUNNEL) && > + sp->verified_cnt < sp->len && > + !xfrm_policy_check(NULL, XFRM_POLICY_IN, skb, family)) { > + goto drop; > + } This is not the right place to do the policy lookup. We don't know if we should check XFRM_POLICY_IN or XFRM_POLICY_FWD. But it looks like we don't reset the secpath in the receive path like other virtual interfaces do. Would such a patch fix your issue too? diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index cc6ab79609e2..429de6a28f59 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -3516,7 +3516,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, int xerr_idx = -1; const struct xfrm_if_cb *ifcb; struct sec_path *sp; - struct xfrm_if *xi; + struct xfrm_if *xi = NULL; u32 if_id = 0; rcu_read_lock(); @@ -3668,6 +3668,9 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb, goto reject; } + if (xi) + secpath_reset(skb); + xfrm_pols_put(pols, npols); return 1; }
Thanks for the response; apologies for taking a while to re-patch this
and verify.
I think this /almost/ does what we need to. I'm still seeing v6 ESP in v6
ESP tunnels failing; I think it's due to the fact that the IPv6 ESP
codepath does not trigger policy checks in the receive codepath until it
hits the socket, or changes namespace.
Perhaps if we verify policy unconditionally in xfrmi_rcv_cb? combined
with your change above, this should ensure IPv6 ESP also checks policies,
and inside that clear the secpath?
diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index 5113fa0fbcee..4288d87c9249 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -236,23 +236,21 @@ static int xfrmi_rcv_cb(struct sk_buff *skb, int err)
xnet = !net_eq(xi->net, dev_net(skb->dev));
- if (xnet) {
- inner_mode = &x->inner_mode;
-
- if (x->sel.family == AF_UNSPEC) {
- inner_mode = xfrm_ip2inner_mode(x,
XFRM_MODE_SKB_CB(skb)->protocol);
- if (inner_mode == NULL) {
- XFRM_INC_STATS(dev_net(skb->dev),
- LINUX_MIB_XFRMINSTATEMODEERROR);
- return -EINVAL;
- }
+ inner_mode = &x->inner_mode;
+
+ if (x->sel.family == AF_UNSPEC) {
+ inner_mode = xfrm_ip2inner_mode(x,
XFRM_MODE_SKB_CB(skb)->protocol);
+ if (inner_mode == NULL) {
+ XFRM_INC_STATS(dev_net(skb->dev),
+ LINUX_MIB_XFRMINSTATEMODEERROR);
+ return -EINVAL;
}
-
- if (!xfrm_policy_check(NULL, XFRM_POLICY_IN, skb,
- inner_mode->family))
- return -EPERM;
}
+ if (!xfrm_policy_check(NULL, XFRM_POLICY_IN, skb,
+ inner_mode->family))
+ return -EPERM;
+
xfrmi_scrub_packet(skb, xnet);
dev_sw_netstats_rx_add(dev, skb->len);
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index f1a0bab920a5..04f66f6d5729 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3516,7 +3516,7 @@ int __xfrm_policy_check(struct sock *sk, int
dir, struct sk_buff *skb,
int xerr_idx = -1;
const struct xfrm_if_cb *ifcb;
struct sec_path *sp;
- struct xfrm_if *xi;
+ struct xfrm_if *xi = NULL;
u32 if_id = 0;
rcu_read_lock();
@@ -3667,6 +3667,9 @@ int __xfrm_policy_check(struct sock *sk, int
dir, struct sk_buff *skb,
goto reject;
}
+ if (xi)
+ secpath_reset(skb);
+
xfrm_pols_put(pols, npols);
return 1;
}
On Mon, Aug 29, 2022 at 11:25 PM Steffen Klassert
<steffen.klassert@secunet.com> wrote:
>
> On Wed, Aug 24, 2022 at 10:12:52PM +0000, Benedict Wong wrote:
> > This change ensures that all nested XFRM packets have their policy
> > checked before decryption of the next layer, so that policies are
> > verified at each intermediate step of the decryption process.
> >
> > Notably, raw ESP/AH packets do not perform policy checks inherently,
> > whereas all other encapsulated packets (UDP, TCP encapsulated) do policy
> > checks after calling xfrm_input handling in the respective encapsulation
> > layer.
> >
> > This is necessary especially for nested tunnels, as the IP addresses,
> > protocol and ports may all change, thus not matching the previous
> > policies. In order to ensure that packets match the relevant inbound
> > templates, the xfrm_policy_check should be done before handing off to
> > the inner XFRM protocol to decrypt and decapsulate.
> >
> > In order to prevent double-checking packets both here and in the
> > encapsulation layers, this check is currently limited to nested
> > tunnel-mode transforms and checked prior to decapsulation of inner
> > tunnel layers (prior to hitting a nested tunnel's xfrm_input, there
> > is no great way to detect a nested tunnel). This is primarily a
> > performance consideration, as a general blanket check at the end of
> > xfrm_input would suffice, but may result in multiple policy checks.
> >
> > Test: Tested against Android Kernel Unit Tests
> > Signed-off-by: Benedict Wong <benedictwong@google.com>
> > ---
> > net/xfrm/xfrm_input.c | 14 ++++++++++++++
> > 1 file changed, 14 insertions(+)
> >
> > diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> > index bcb9ee25474b..a3b55d109836 100644
> > --- a/net/xfrm/xfrm_input.c
> > +++ b/net/xfrm/xfrm_input.c
> > @@ -586,6 +586,20 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
> > goto drop;
> > }
> >
> > + /* If nested tunnel, check outer states before context is lost.
> > + * Only nested tunnels need to be checked, since IP addresses change
> > + * as a result of the tunnel mode decapsulation. Similarly, this check
> > + * is limited to nested tunnels to avoid performing another policy
> > + * check on non-nested tunnels. On success, this check also updates the
> > + * secpath's verified_cnt variable, skipping future verifications of
> > + * previously-verified secpath entries.
> > + */
> > + if ((x->outer_mode.flags & XFRM_MODE_FLAG_TUNNEL) &&
> > + sp->verified_cnt < sp->len &&
> > + !xfrm_policy_check(NULL, XFRM_POLICY_IN, skb, family)) {
> > + goto drop;
> > + }
>
> This is not the right place to do the policy lookup. We don't know
> if we should check XFRM_POLICY_IN or XFRM_POLICY_FWD.
>
> But it looks like we don't reset the secpath in the receive path
> like other virtual interfaces do.
>
> Would such a patch fix your issue too?
>
> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index cc6ab79609e2..429de6a28f59 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -3516,7 +3516,7 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
> int xerr_idx = -1;
> const struct xfrm_if_cb *ifcb;
> struct sec_path *sp;
> - struct xfrm_if *xi;
> + struct xfrm_if *xi = NULL;
> u32 if_id = 0;
>
> rcu_read_lock();
> @@ -3668,6 +3668,9 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
> goto reject;
> }
>
> + if (xi)
> + secpath_reset(skb);
> +
> xfrm_pols_put(pols, npols);
> return 1;
> }
On Fri, Sep 16, 2022 at 10:44:42PM -0700, Benedict Wong wrote: > Thanks for the response; apologies for taking a while to re-patch this > and verify. > > I think this /almost/ does what we need to. I'm still seeing v6 ESP in v6 > ESP tunnels failing; I think it's due to the fact that the IPv6 ESP > codepath does not trigger policy checks in the receive codepath until it > hits the socket, or changes namespace. > Perhaps if we verify policy unconditionally in xfrmi_rcv_cb? combined > with your change above, this should ensure IPv6 ESP also checks policies, > and inside that clear the secpath? Hm, do you know why this is different to IPv4? IPv4 and IPv6 should do the same regarding to policy checks.
Ahh, I've never had an IPv4 server without a NAT to test against, I'd presume this is identical there. The only comparison that I've been able to do was IPv4 UDP-encap vs IPv6 ESP. We could instead add the policy check to the ESP input path if that is the correct place. On Wed, Sep 21, 2022 at 11:27 PM Steffen Klassert <steffen.klassert@secunet.com> wrote: > > On Fri, Sep 16, 2022 at 10:44:42PM -0700, Benedict Wong wrote: > > Thanks for the response; apologies for taking a while to re-patch this > > and verify. > > > > I think this /almost/ does what we need to. I'm still seeing v6 ESP in v6 > > ESP tunnels failing; I think it's due to the fact that the IPv6 ESP > > codepath does not trigger policy checks in the receive codepath until it > > hits the socket, or changes namespace. > > Perhaps if we verify policy unconditionally in xfrmi_rcv_cb? combined > > with your change above, this should ensure IPv6 ESP also checks policies, > > and inside that clear the secpath? > > Hm, do you know why this is different to IPv4? IPv4 and IPv6 should > do the same regarding to policy checks. >
On Thu, Sep 22, 2022 at 06:33:55PM -0700, Benedict Wong wrote: > Ahh, I've never had an IPv4 server without a NAT to test against, I'd presume > this is identical there. The only comparison that I've been able to do was IPv4 > UDP-encap vs IPv6 ESP. > > We could instead add the policy check to the ESP input path if that is > the correct place. Ok, looks like there is a policy check missing for xfrm_interfaces when already one (or more) transformations happened. The best would be to add a separate xfrm_interfaces rcv handler (in struct xfrm6_protocol/xfrm4_protocol) for esp4/6 and do the policy check if we have a secpath present. That should fix it in combination with reseting the secpath in the policy_check as I did in my previous patch.
diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c index bcb9ee25474b..a3b55d109836 100644 --- a/net/xfrm/xfrm_input.c +++ b/net/xfrm/xfrm_input.c @@ -586,6 +586,20 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type) goto drop; } + /* If nested tunnel, check outer states before context is lost. + * Only nested tunnels need to be checked, since IP addresses change + * as a result of the tunnel mode decapsulation. Similarly, this check + * is limited to nested tunnels to avoid performing another policy + * check on non-nested tunnels. On success, this check also updates the + * secpath's verified_cnt variable, skipping future verifications of + * previously-verified secpath entries. + */ + if ((x->outer_mode.flags & XFRM_MODE_FLAG_TUNNEL) && + sp->verified_cnt < sp->len && + !xfrm_policy_check(NULL, XFRM_POLICY_IN, skb, family)) { + goto drop; + } + skb->mark = xfrm_smark_get(skb->mark, x); sp->xvec[sp->len++] = x;
This change ensures that all nested XFRM packets have their policy checked before decryption of the next layer, so that policies are verified at each intermediate step of the decryption process. Notably, raw ESP/AH packets do not perform policy checks inherently, whereas all other encapsulated packets (UDP, TCP encapsulated) do policy checks after calling xfrm_input handling in the respective encapsulation layer. This is necessary especially for nested tunnels, as the IP addresses, protocol and ports may all change, thus not matching the previous policies. In order to ensure that packets match the relevant inbound templates, the xfrm_policy_check should be done before handing off to the inner XFRM protocol to decrypt and decapsulate. In order to prevent double-checking packets both here and in the encapsulation layers, this check is currently limited to nested tunnel-mode transforms and checked prior to decapsulation of inner tunnel layers (prior to hitting a nested tunnel's xfrm_input, there is no great way to detect a nested tunnel). This is primarily a performance consideration, as a general blanket check at the end of xfrm_input would suffice, but may result in multiple policy checks. Test: Tested against Android Kernel Unit Tests Signed-off-by: Benedict Wong <benedictwong@google.com> --- net/xfrm/xfrm_input.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)