[RFC PATCH] lsm,selinux: pass the family information along with xfrm flow

[Casey Schaufler]

On 9/29/2020 2:54 PM, Paul Moore wrote:
> As pointed out by Herbert in a recent related patch, the LSM hooks
> should pass the address family in addition to the xfrm flow as the
> family information is needed to safely access the flow.
>
> While this is not technically a problem for the current LSM/SELinux
> code as it only accesses fields common to all address families, we
> should still pass the address family so that the LSM hook isn't
> inherently flawed.  An alternate solution could be to simply pass
> the LSM secid instead of flow, but this introduces the problem of
> the LSM hook callers sending the wrong secid which would be much
> worse.
>
> Reported-by: Herbert Xu <herbert at gondor.apana.org.au>
> Signed-off-by: Paul Moore <paul at paul-moore.com>

For what it may be worth

Acked-by: Casey Schaufler <casey at schaufler-ca.com>


> ---
>  include/linux/lsm_hook_defs.h   |    2 +-
>  include/linux/lsm_hooks.h       |    1 +
>  include/linux/security.h        |    7 +++++--
>  net/xfrm/xfrm_state.c           |    4 ++--
>  security/security.c             |    5 +++--
>  security/selinux/include/xfrm.h |    3 ++-
>  security/selinux/xfrm.c         |    3 ++-
>  7 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 2a8c74d99015..e3c3b5d20469 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -349,7 +349,7 @@ LSM_HOOK(int, 0, xfrm_state_delete_security, struct xfrm_state *x)
>  LSM_HOOK(int, 0, xfrm_policy_lookup, struct xfrm_sec_ctx *ctx, u32 fl_secid,
>  	 u8 dir)
>  LSM_HOOK(int, 1, xfrm_state_pol_flow_match, struct xfrm_state *x,
> -	 struct xfrm_policy *xp, const struct flowi *fl)
> +	 struct xfrm_policy *xp, const struct flowi *fl, unsigned short family)
>  LSM_HOOK(int, 0, xfrm_decode_session, struct sk_buff *skb, u32 *secid,
>  	 int ckall)
>  #endif /* CONFIG_SECURITY_NETWORK_XFRM */
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 9e2e3e63719d..ea088aacfdad 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -1093,6 +1093,7 @@
>   *	@x contains the state to match.
>   *	@xp contains the policy to check for a match.
>   *	@fl contains the flow to check for a match.
> + *	@family the flow's address family.
>   *	Return 1 if there is a match.
>   * @xfrm_decode_session:
>   *	@skb points to skb to decode.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 0a0a03b36a3b..701b41eb090c 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1625,7 +1625,8 @@ void security_xfrm_state_free(struct xfrm_state *x);
>  int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir);
>  int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
>  				       struct xfrm_policy *xp,
> -				       const struct flowi *fl);
> +				       const struct flowi *fl,
> +				       unsigned short family);
>  int security_xfrm_decode_session(struct sk_buff *skb, u32 *secid);
>  void security_skb_classify_flow(struct sk_buff *skb, struct flowi *fl);
>  
> @@ -1679,7 +1680,9 @@ static inline int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_s
>  }
>  
>  static inline int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
> -			struct xfrm_policy *xp, const struct flowi *fl)
> +						     struct xfrm_policy *xp,
> +						     const struct flowi *fl,
> +						     unsigned short family)
>  {
>  	return 1;
>  }
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 69520ad3d83b..f90d2f1da44a 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1020,7 +1020,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
>  	if (x->km.state == XFRM_STATE_VALID) {
>  		if ((x->sel.family &&
>  		     !xfrm_selector_match(&x->sel, fl, x->sel.family)) ||
> -		    !security_xfrm_state_pol_flow_match(x, pol, fl))
> +		    !security_xfrm_state_pol_flow_match(x, pol, fl, family))
>  			return;
>  
>  		if (!*best ||
> @@ -1033,7 +1033,7 @@ static void xfrm_state_look_at(struct xfrm_policy *pol, struct xfrm_state *x,
>  	} else if (x->km.state == XFRM_STATE_ERROR ||
>  		   x->km.state == XFRM_STATE_EXPIRED) {
>  		if (xfrm_selector_match(&x->sel, fl, x->sel.family) &&
> -		    security_xfrm_state_pol_flow_match(x, pol, fl))
> +		    security_xfrm_state_pol_flow_match(x, pol, fl, family))
>  			*error = -ESRCH;
>  	}
>  }
> diff --git a/security/security.c b/security/security.c
> index 70a7ad357bc6..62dd0af7c6bc 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2391,7 +2391,8 @@ int security_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir)
>  
>  int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
>  				       struct xfrm_policy *xp,
> -				       const struct flowi *fl)
> +				       const struct flowi *fl,
> +				       unsigned short family)
>  {
>  	struct security_hook_list *hp;
>  	int rc = LSM_RET_DEFAULT(xfrm_state_pol_flow_match);
> @@ -2407,7 +2408,7 @@ int security_xfrm_state_pol_flow_match(struct xfrm_state *x,
>  	 */
>  	hlist_for_each_entry(hp, &security_hook_heads.xfrm_state_pol_flow_match,
>  				list) {
> -		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl);
> +		rc = hp->hook.xfrm_state_pol_flow_match(x, xp, fl, family);
>  		break;
>  	}
>  	return rc;
> diff --git a/security/selinux/include/xfrm.h b/security/selinux/include/xfrm.h
> index a0b465316292..36907dd06647 100644
> --- a/security/selinux/include/xfrm.h
> +++ b/security/selinux/include/xfrm.h
> @@ -26,7 +26,8 @@ int selinux_xfrm_state_delete(struct xfrm_state *x);
>  int selinux_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir);
>  int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x,
>  				      struct xfrm_policy *xp,
> -				      const struct flowi *fl);
> +				      const struct flowi *fl,
> +				      unsigned short family);
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
>  extern atomic_t selinux_xfrm_refcount;
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index 7314196185d1..5beb30237d3a 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -175,7 +175,8 @@ int selinux_xfrm_policy_lookup(struct xfrm_sec_ctx *ctx, u32 fl_secid, u8 dir)
>   */
>  int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x,
>  				      struct xfrm_policy *xp,
> -				      const struct flowi *fl)
> +				      const struct flowi *fl,
> +				      unsigned short family)
>  {
>  	u32 state_sid;
>  
>