OK. Thanks for the comments. I'll get back soon.
Regards,
Trent.
------------------------------------------------------------
Trent Jaeger
IBM T.J. Watson Research Center
19 Skyline Drive, Hawthorne, NY 10532
(914) 784-7225, FAX (914) 784-7225
Herbert Xu <[EMAIL PROTECTED]>
08/06/2005 03:45 AM
To: Trent Jaeger/Watson/[EMAIL PROTECTED]
cc: [EMAIL PROTECTED], [EMAIL PROTECTED], [EMAIL PROTECTED],
netdev@vger.kernel.org, [EMAIL PROTECTED], Serge E
Hallyn/Austin/[EMAIL PROTECTED], [EMAIL PROTECTED], [EMAIL PROTECTED]
Subject: Re: [PATCH 1/2] LSM-IPSec Networking Hooks --
revised flow cache [resend]
On Tue, Aug 02, 2005 at 02:04:41PM -0400, jaegert wrote:
> Resend of 20 July patch that repaired the flow_cache_lookup
> authorization (now for 2.6.13-rc4-git4).
Thanks Trent. I'm happy with the flow cache stuff now.
However, there are still some technical details to take
care of.
> diff -puN include/linux/xfrm.h~lsm-xfrm-nethooks include/linux/xfrm.h
> --- linux-2.6.13-rc4-xfrm/include/linux/xfrm.h~lsm-xfrm-nethooks
2005-08-01 16:11:22.000000000 -0400
> +++ linux-2.6.13-rc4-xfrm-root/include/linux/xfrm.h 2005-08-01
16:11:22.000000000 -0400
> @@ -173,6 +201,7 @@ enum xfrm_attr_type_t {
> XFRMA_ALG_CRYPT, /* struct xfrm_algo */
> XFRMA_ALG_COMP, /* struct
xfrm_algo */
> XFRMA_ENCAP, /* struct
xfrm_algo + struct xfrm_encap_tmpl */
> + XFRMA_SEC_CTX, /* struct xfrm_sec_ctx */
> XFRMA_TMPL, /* 1 or more
struct xfrm_user_tmpl */
> XFRMA_SA,
> XFRMA_POLICY,
Please add it at the end of the enum as otherwise you may break
existing user-space applications. In this particular case the
breakage isn't serious since those three XFRMA types are fairly
recent but still it's better to be safe than sorry :)
> diff -puN include/net/xfrm.h~lsm-xfrm-nethooks include/net/xfrm.h
> --- linux-2.6.13-rc4-xfrm/include/net/xfrm.h~lsm-xfrm-nethooks
2005-08-01 16:11:22.000000000 -0400
> +++ linux-2.6.13-rc4-xfrm-root/include/net/xfrm.h 2005-08-01
16:11:22.000000000 -0400
> @@ -510,6 +514,27 @@ xfrm_selector_match(struct xfrm_selector
> return 0;
> }
>
> +/* If neither has a context --> match
> + Otherwise, both must have a context and the sids, doi, alg must
match */
> +static inline int xfrm_sec_ctx_match(struct xfrm_sec_ctx *s1, struct
xfrm_sec_ctx *s2)
> +{
> + return ((!s1 && !s2) ||
> + (s1 && s2 &&
> + (s1->ctx_sid == s2->ctx_sid) &&
> + (s1->ctx_doi == s2->ctx_doi) &&
> + (s1->ctx_alg == s2->ctx_alg)));
> +}
Would it be possible to make this conditional on CONFIG_SECURITY_NETWORK?
> +static inline struct xfrm_sec_ctx *xfrm_policy_security(struct
xfrm_policy *xp)
> +{
> + return (xp ? xp->security : NULL);
> +}
> +
> +static inline struct xfrm_sec_ctx *xfrm_state_security(struct
xfrm_state *x)
> +{
> + return (x ? x->security : NULL);
> +}
> +
Do you really need these NULL checks? If not I'd suggest getting rid
of these altogether.
A quick glance at all the users of xfrm_policy_security in Patch 1
seems to indicate that none of those places can have xp being NULL.
> diff -puN net/core/flow.c~lsm-xfrm-nethooks net/core/flow.c
> --- linux-2.6.13-rc4-xfrm/net/core/flow.c~lsm-xfrm-nethooks 2005-08-01
16:11:22.000000000 -0400
> +++ linux-2.6.13-rc4-xfrm-root/net/core/flow.c 2005-08-01
16:12:03.000000000 -0400
> @@ -23,6 +23,7 @@
> #include <net/flow.h>
> #include <asm/atomic.h>
> #include <asm/semaphore.h>
> +#include <linux/security.h>
This appears to be unnecessary.
> diff -puN net/ipv4/xfrm4_policy.c~lsm-xfrm-nethooks
net/ipv4/xfrm4_policy.c
> --- linux-2.6.13-rc4-xfrm/net/ipv4/xfrm4_policy.c~lsm-xfrm-nethooks
2005-08-01 16:11:22.000000000 -0400
> +++ linux-2.6.13-rc4-xfrm-root/net/ipv4/xfrm4_policy.c 2005-08-01
16:11:22.000000000 -0400
> @@ -36,6 +36,8 @@ __xfrm4_find_bundle(struct flowi *fl, st
> if (xdst->u.rt.fl.oif == fl->oif &&
/*XXX*/
> xdst->u.rt.fl.fl4_dst == fl->fl4_dst
&&
> xdst->u.rt.fl.fl4_src == fl->fl4_src
&&
> + xfrm_sec_ctx_match(xfrm_policy_security(policy),
> + xfrm_state_security(dst->xfrm)) &&
Is this necessary? The policy's context must've matched the state's
context at its creation time. AFAIK there is no way for the security
context to change during their life-cycle.
> diff -puN net/ipv6/xfrm6_policy.c~lsm-xfrm-nethooks
net/ipv6/xfrm6_policy.c
> --- linux-2.6.13-rc4-xfrm/net/ipv6/xfrm6_policy.c~lsm-xfrm-nethooks
2005-08-01 16:11:22.000000000 -0400
> +++ linux-2.6.13-rc4-xfrm-root/net/ipv6/xfrm6_policy.c 2005-08-01
16:11:22.000000000 -0400
> @@ -54,6 +54,8 @@ __xfrm6_find_bundle(struct flowi *fl, st
> xdst->u.rt6.rt6i_src.plen);
> if
(ipv6_addr_equal(&xdst->u.rt6.rt6i_dst.addr, &fl_dst_prefix) &&
> ipv6_addr_equal(&xdst->u.rt6.rt6i_src.addr, &fl_src_prefix) &&
> + xfrm_sec_ctx_match(xfrm_policy_security(policy),
> + xfrm_state_security(dst->xfrm)) &&
> xfrm_bundle_ok(xdst, fl, AF_INET6)) {
> dst_clone(dst);
> break;
Ditto.
> diff -puN net/key/af_key.c~lsm-xfrm-nethooks net/key/af_key.c
> --- linux-2.6.13-rc4-xfrm/net/key/af_key.c~lsm-xfrm-nethooks 2005-08-01
16:11:22.000000000 -0400
> +++ linux-2.6.13-rc4-xfrm-root/net/key/af_key.c 2005-08-01
16:11:22.000000000 -0400
> @@ -383,6 +384,44 @@ static int verify_address_len(void *p)
> return 0;
> }
>
> +static inline int verify_sec_ctx_len(void *p)
> +{
> + struct sadb_x_sec_ctx *sec_ctx = (struct sadb_x_sec_ctx
*)p;
> + int len = 0;
> +
> + len += sizeof(struct sadb_x_sec_ctx);
> + len += sec_ctx->sadb_x_ctx_len;
> + len += sizeof(uint64_t) - 1;
> + len /= sizeof(uint64_t);
> +
> + if (sec_ctx->sadb_x_sec_len != len)
> + return -EINVAL;
> +
> + return 0;
> +}
What if I pass in sadb_x_ctx_len == -1?
> +static inline struct xfrm_user_sec_ctx *pfkey_sadb2xfrm_user_ctx(struct
sadb_x_sec_ctx *sec_ctx)
> +{
> + struct xfrm_user_sec_ctx *uctx = NULL;
> +
> + if (sec_ctx) {
This is unnecessary since callers have already checked it.
> + if (!uctx)
> + return NULL;
You need to fix the callers to handle this case.
> + struct xfrm_user_sec_ctx *uctx =
pfkey_sadb2xfrm_user_ctx(sec_ctx);
> +
> + err = security_xfrm_state_alloc(x, uctx);
So either check uctx directly or make sure that security_xfrm_state_alloc
doesn't crash with uctx == NULL.
> @@ -2027,6 +2136,18 @@ static int pfkey_spdadd(struct sock *sk,
> if (xp->selector.dport)
> xp->selector.dport_mask = ~0;
>
> + sec_ctx = (struct sadb_x_sec_ctx *)
ext_hdrs[SADB_X_EXT_SEC_CTX-1];
> + if (sec_ctx != NULL) {
> + struct xfrm_user_sec_ctx *uctx =
pfkey_sadb2xfrm_user_ctx(sec_ctx);
> +
> + err = security_xfrm_policy_alloc(xp,
uctx);
Ditto.
> @@ -2108,7 +2230,18 @@ static int pfkey_spddelete(struct sock *
> if (sel.dport)
> sel.dport_mask = ~0;
>
> - xp = xfrm_policy_bysel(pol->sadb_x_policy_dir-1, &sel,
1);
> + sec_ctx = (struct sadb_x_sec_ctx *)
ext_hdrs[SADB_X_EXT_SEC_CTX-1];
> + memset(&tmp, 0, sizeof(struct xfrm_policy));
> +
> + if (sec_ctx != NULL) {
> + err = security_xfrm_policy_alloc(
> + &tmp, (struct
xfrm_user_sec_ctx *)sec_ctx);
What makes spddelete different from spdadd?
> @@ -2703,10 +2837,22 @@ static struct xfrm_policy *pfkey_compile
> (*dir = parse_ipsecrequests(xp, pol)) < 0)
> goto out;
>
> + /* security context too */
> + if (len >= (pol->sadb_x_policy_len*8 +
> + sizeof(struct sadb_x_sec_ctx))) {
> + char *p = (char *) pol;
> + p += pol->sadb_x_policy_len*8;
> + sec_ctx = (struct sadb_x_sec_ctx *) p;
> + if (security_xfrm_policy_alloc(
> + xp, (struct
xfrm_user_sec_ctx *)sec_ctx))
> + goto out;
> + }
> +
Do we really need socket-specific policies with security context?
Where are the length checks here?
Too many errors encountered, you'll have to resend :)
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[EMAIL PROTECTED]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
