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@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Paul Moore <paul@xxxxxxxxxxxxxx>
For what it may be worth
Acked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> ---
> 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;
>
>
