Thanks for the comments.
I'll be mostly on vacation (moving) this week, so I will be getting back
to you after Aug 22.
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/13/2005 02:32 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 -- mods
based on Herbert's comments
Hi Trent:
Thanks for your good work. Here are the comments for your first patch.
I won't comment on the second patch since others have already looked
through it and I don't know enough about SELINUX to be of much help.
On Thu, Aug 11, 2005 at 02:21:15PM -0400, jaegert wrote:
>
> +static inline struct xfrm_sec_ctx *xfrm_policy_security(struct
xfrm_policy *xp)
> +{
> + return xp->security;
> +}
> +
> +static inline struct xfrm_sec_ctx *xfrm_state_security(struct
xfrm_state *x)
> +{
> + return x->security;
> +}
Any reason why we can't use xp->security and x->security directly?
> @@ -2027,6 +2150,21 @@ 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);
> +
> + if (!uctx)
> + goto out;
Please set err.
> @@ -2703,10 +2858,25 @@ 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;
> + if (verify_sec_ctx_len(p))
> + goto out;
I think this might be insufficient. When verify_sec_ctx_len is called
by parse_exthdr the caller has already verified that sadb_x_sec_len is
within the packet.
So you should explicitly check sadb_x_sec_len here.
> + sec_ctx = (struct sadb_x_sec_ctx *) p;
BTW, please remove all the spaces between cast operators and their
operand.
> + if (security_xfrm_policy_alloc(
> + xp, (struct
xfrm_user_sec_ctx *)sec_ctx))
You need to do the sadb->xfrm conversion here just like spddelete.
You should also stored the returned error value in *dir.
> diff -puN net/xfrm/xfrm_policy.c~lsm-xfrm-nethooks
net/xfrm/xfrm_policy.c
> @@ -491,23 +494,25 @@ EXPORT_SYMBOL(xfrm_policy_walk);
>
> /* Find policy to apply to this flow. */
>
> -static void xfrm_policy_lookup(struct flowi *fl, u16 family, u8 dir,
> +static void xfrm_policy_lookup(struct flowi *fl, struct sock *sk, u16
family, u8 dir,
> void **objp,
atomic_t **obj_refp)
> {
> struct xfrm_policy *pol;
>
> read_lock_bh(&xfrm_policy_lock);
> for (pol = xfrm_policy_list[dir]; pol; pol = pol->next) {
> - struct xfrm_selector *sel =
&pol->selector;
> int match;
>
> if (pol->family != family)
> continue;
>
> - match = xfrm_selector_match(sel, fl,
family);
> + match =
xfrm_selector_match(&pol->selector, fl, family);
> +
This looks like a left-over from previous changes. Please preserve
the existing code.
}
> @@ -383,6 +386,13 @@ xfrm_state_find(xfrm_address_t *daddr, x
> xfrm_init_tempsel(x, fl, tmpl, daddr,
saddr, family);
>
> if (km_query(x, tmpl, pol) == 0) {
> + if
(!xfrm_sec_ctx_match(xfrm_policy_security(pol), xfrm_state_security(x))) {
> + x->km.state = XFRM_STATE_DEAD;
??? At this point we've only sent a message to user-space so
x is not going to have any security context.
> diff -puN net/xfrm/xfrm_user.c~lsm-xfrm-nethooks net/xfrm/xfrm_user.c
> @@ -88,6 +88,31 @@ static int verify_encap_tmpl(struct rtat
> return 0;
> }
>
> +
> +static inline int verify_sec_ctx_len(struct rtattr **xfrma)
> +{
> + struct rtattr *rt = xfrma[XFRMA_SEC_CTX - 1];
> + struct xfrm_user_sec_ctx *uctx;
> + int len = 0;
> +
> + if (!rt)
> + return 0;
> +
> + uctx = RTA_DATA(rt);
> +
> + if (uctx->ctx_len > PAGE_SIZE)
> + return -EINVAL;
You need to verify that rta_len isn't less than sizeof(*uctx) first.
> +
> + len += sizeof(struct xfrm_user_sec_ctx);
> + len += uctx->ctx_len;
> +
> + if (uctx->len != len)
> + return -EINVAL;
error: structure has no member named `len' :)
> @@ -390,6 +471,21 @@ static int dump_one_state(struct xfrm_st
> if (x->encap)
> RTA_PUT(skb, XFRMA_ENCAP,
sizeof(*x->encap), x->encap);
>
> + if ((xfrm_ctx = xfrm_state_security(x))) {
> + int ctx_size = sizeof(struct
xfrm_user_sec_ctx) +
> + xfrm_ctx->ctx_len;
> + struct xfrm_user_sec_ctx *uctx =
kmalloc(ctx_size, GFP_KERNEL);
> + int err;
> +
> + if (!uctx)
> + goto rtattr_failure;
I feel uneasy about this. Netlink dumping isn't used to these sort
of error conditions. Perhaps you could just dump it straight from
x->security and avoid the kmalloc altogether.
> +static int copy_sec_ctx(struct xfrm_policy *pol, struct
xfrm_user_sec_ctx *uctx)
> +{
> + int err = 0;
> +
> + if (uctx) {
> + err = security_xfrm_policy_alloc(pol,
uctx);
> + }
> +
> + return err;
> +}
Do we really need this function?
> +static int copy_from_user_sec_ctx(struct xfrm_policy *pol, struct
rtattr **xfrma)
> +{
> + struct rtattr *rt = xfrma[XFRMA_SEC_CTX-1];
> + struct xfrm_user_sec_ctx *uctx;
> +
> + if (!rt)
> + return 0;
> +
> + uctx = RTA_DATA(rt);
> + return copy_sec_ctx(pol, uctx);
> +}
Certainly not here since uctx can't be NULL. As there is only one other
user you might as well get rid of it.
> +static int copy_to_user_sec_ctx(struct xfrm_policy *xp, struct sk_buff
*skb, int src)
src is unused.
> +{
> + int err = 0;
> + struct xfrm_sec_ctx *xfrm_ctx = xfrm_policy_security(xp);
> +
> + if (xfrm_ctx) {
> + int ctx_size = sizeof(struct
xfrm_user_sec_ctx) +
> + xfrm_ctx->ctx_len;
> + struct xfrm_user_sec_ctx *uctx =
kmalloc(ctx_size, GFP_ATOMIC);
> +
> + if (!uctx)
> + return -ENOMEM;
> +
> + err = dump_one_sec_ctx(xfrm_ctx, uctx, skb,
> + ctx_size);
Same comment as dump_one_state. It'd be better to just dump the context
directly onto the skb and avoid the kmalloc.
> @@ -852,8 +1000,21 @@ static int xfrm_get_policy(struct sk_buf
>
> if (p->index)
> xp = xfrm_policy_byid(p->dir, p->index,
delete);
> - else
> - xp = xfrm_policy_bysel(p->dir, &p->sel,
delete);
> + else {
> + struct rtattr **rtattrs = (struct rtattr
**) xfrma;
> + struct rtattr *rt =
rtattrs[XFRMA_SEC_CTX-1];
> + struct xfrm_policy tmp;
> +
> + memset(&tmp, 0, sizeof(struct
xfrm_policy));
> + if (rt) {
> + struct xfrm_user_sec_ctx
*uctx = RTA_DATA(rt);
> +
> + if ((err =
security_xfrm_policy_alloc(&tmp, uctx)))
> + return
err;
You need to verify the validity of uctx.
> @@ -1346,9 +1511,26 @@ static struct xfrm_policy *xfrm_compile_
> verify_newpolicy_info(p))
> return NULL;
>
> + if (len > (sizeof(*p) + (XFRM_MAX_DEPTH *
> + sizeof(struct xfrm_user_tmpl)))) {
Sorry, this isn't going to fly. This stops us from ever increasing
XFRM_MAX_DEPTH. You'll need to think of some other way to do this.
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
