| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 16 Sep 2022 22:44:42 -0700
From: Benedict Wong <benedictwong@...gle.com>
To: Steffen Klassert <steffen.klassert@...unet.com>
Cc: netdev@...r.kernel.org, nharold@...gle.com, lorenzo@...gle.com
Subject: Re: [PATCH v2 ipsec 2/2] xfrm: Ensure policy checked for nested ESP tunnels
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@...unet.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@...gle.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;
> }
Powered by blists - more mailing lists