From: Casey Schaufler <casey@schaufler-ca.com>
To: Paul Moore <paul@paul-moore.com>,
selinux@vger.kernel.org, linux-security-module@vger.kernel.org
Cc: Herbert Xu <herbert@gondor.apana.org.au>, netdev@vger.kernel.org
Subject: Re: [RFC PATCH] lsm,selinux: pass the family information along with xfrm flow
Date: Tue, 29 Sep 2020 15:38:56 -0700 [thread overview]
Message-ID: <d42b766d-595a-b360-df13-0a3fe7a8bd7f@schaufler-ca.com> (raw)
In-Reply-To: <160141647786.7997.5490924406329369782.stgit@sifl>
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@gondor.apana.org.au>
> Signed-off-by: Paul Moore <paul@paul-moore.com>
For what it may be worth
Acked-by: Casey Schaufler <casey@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;
>
>
next prev parent reply other threads:[~2020-09-29 22:39 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-29 21:54 [RFC PATCH] lsm,selinux: pass the family information along with xfrm flow Paul Moore
2020-09-29 22:38 ` Casey Schaufler [this message]
2020-09-29 23:09 ` James Morris
2020-09-30 10:14 ` Herbert Xu
2020-09-30 13:44 ` Paul Moore
2020-10-28 2:09 ` Paul Moore
2020-10-28 5:27 ` James Morris
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d42b766d-595a-b360-df13-0a3fe7a8bd7f@schaufler-ca.com \
--to=casey@schaufler-ca.com \
--cc=herbert@gondor.apana.org.au \
--cc=linux-security-module@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=selinux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox