Re: [PATCH v2 ipsec 2/2] xfrm: Ensure policy checked for nested ESP tunnels
From: Steffen Klassert <steffen.klassert@secunet.com>
Date: 2022-08-30 06:25:41
Subsystem:
networking [general], networking [ipsec], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Steffen Klassert, Herbert Xu, Linus Torvalds
On Wed, Aug 24, 2022 at 10:12:52PM +0000, Benedict Wong wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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; }