Skip to content

Commit

Permalink
IPsec: propagate security module errors up from flow_cache_lookup
Browse files Browse the repository at this point in the history 
When a security module is loaded (in this case, SELinux), the
security_xfrm_policy_lookup() hook can return an access denied permission
(or other error).  We were not handling that correctly, and in fact
inverting the return logic and propagating a false "ok" back up to
xfrm_lookup(), which then allowed packets to pass as if they were not
associated with an xfrm policy.

The way I was seeing the problem was when connecting via IPsec to a
confined service on an SELinux box (vsftpd), which did not have the
appropriate SELinux policy permissions to send packets via IPsec.

The first SYNACK would be blocked, because of an uncached lookup via
flow_cache_lookup(), which would fail to resolve an xfrm policy because
the SELinux policy is checked at that point via the resolver.

However, retransmitted SYNACKs would then find a cached flow entry when
calling into flow_cache_lookup() with a null xfrm policy, which is
interpreted by xfrm_lookup() as the packet not having any associated
policy and similarly to the first case, allowing it to pass without
transformation.

The solution presented here is to first ensure that errno values are
correctly propagated all the way back up through the various call chains
from security_xfrm_policy_lookup(), and handled correctly.

Then, flow_cache_lookup() is modified, so that if the policy resolver
fails (typically a permission denied via the security module), the flow
cache entry is killed rather than having a null policy assigned (which
indicates that the packet can pass freely).  This also forces any future
lookups for the same flow to consult the security module (e.g. SELinux)
for current security policy (rather than, say, caching the error on the
flow cache entry).

Signed-off-by: James Morris <[email protected]>
  • Loading branch information
James Morris authored and David S. Miller committed Oct 12, 2006
1 parent 388b240 commit 134b0fc
Show file tree
Hide file tree
Showing 3 changed files with 82 additions and 30 deletions.
2 changes: 1 addition & 1 deletion include/net/flow.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ struct flowi {
#define FLOW_DIR_FWD 2

struct sock;
typedef void (*flow_resolve_t)(struct flowi *key, u16 family, u8 dir,
typedef int (*flow_resolve_t)(struct flowi *key, u16 family, u8 dir,
void **objp, atomic_t **obj_refp);

extern void *flow_cache_lookup(struct flowi *key, u16 family, u8 dir,
Expand Down
42 changes: 28 additions & 14 deletions net/core/flow.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,14 @@ static void flow_cache_new_hashrnd(unsigned long arg)
add_timer(&flow_hash_rnd_timer);
}

static void flow_entry_kill(int cpu, struct flow_cache_entry *fle)
{
if (fle->object)
atomic_dec(fle->object_ref);
kmem_cache_free(flow_cachep, fle);
flow_count(cpu)--;
}

static void __flow_cache_shrink(int cpu, int shrink_to)
{
struct flow_cache_entry *fle, **flp;
Expand All @@ -100,10 +108,7 @@ static void __flow_cache_shrink(int cpu, int shrink_to)
}
while ((fle = *flp) != NULL) {
*flp = fle->next;
if (fle->object)
atomic_dec(fle->object_ref);
kmem_cache_free(flow_cachep, fle);
flow_count(cpu)--;
flow_entry_kill(cpu, fle);
}
}
}
Expand Down Expand Up @@ -220,24 +225,33 @@ void *flow_cache_lookup(struct flowi *key, u16 family, u8 dir,

nocache:
{
int err;
void *obj;
atomic_t *obj_ref;

resolver(key, family, dir, &obj, &obj_ref);
err = resolver(key, family, dir, &obj, &obj_ref);

if (fle) {
fle->genid = atomic_read(&flow_cache_genid);

if (fle->object)
atomic_dec(fle->object_ref);

fle->object = obj;
fle->object_ref = obj_ref;
if (obj)
atomic_inc(fle->object_ref);
if (err) {
/* Force security policy check on next lookup */
*head = fle->next;
flow_entry_kill(cpu, fle);
} else {
fle->genid = atomic_read(&flow_cache_genid);

if (fle->object)
atomic_dec(fle->object_ref);

fle->object = obj;
fle->object_ref = obj_ref;
if (obj)
atomic_inc(fle->object_ref);
}
}
local_bh_enable();

if (err)
obj = ERR_PTR(err);
return obj;
}
}
Expand Down
68 changes: 53 additions & 15 deletions net/xfrm/xfrm_policy.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -883,30 +883,32 @@ int xfrm_policy_walk(u8 type, int (*func)(struct xfrm_policy *, int, int, void*)
}
EXPORT_SYMBOL(xfrm_policy_walk);

/* Find policy to apply to this flow. */

/*
* Find policy to apply to this flow.
*
* Returns 0 if policy found, else an -errno.
*/
static int xfrm_policy_match(struct xfrm_policy *pol, struct flowi *fl,
u8 type, u16 family, int dir)
{
struct xfrm_selector *sel = &pol->selector;
int match;
int match, ret = -ESRCH;

if (pol->family != family ||
pol->type != type)
return 0;
return ret;

match = xfrm_selector_match(sel, fl, family);
if (match) {
if (!security_xfrm_policy_lookup(pol, fl->secid, dir))
return 1;
}
if (match)
ret = security_xfrm_policy_lookup(pol, fl->secid, dir);

return 0;
return ret;
}

static struct xfrm_policy *xfrm_policy_lookup_bytype(u8 type, struct flowi *fl,
u16 family, u8 dir)
{
int err;
struct xfrm_policy *pol, *ret;
xfrm_address_t *daddr, *saddr;
struct hlist_node *entry;
Expand All @@ -922,44 +924,69 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(u8 type, struct flowi *fl,
chain = policy_hash_direct(daddr, saddr, family, dir);
ret = NULL;
hlist_for_each_entry(pol, entry, chain, bydst) {
if (xfrm_policy_match(pol, fl, type, family, dir)) {
err = xfrm_policy_match(pol, fl, type, family, dir);
if (err) {
if (err == -ESRCH)
continue;
else {
ret = ERR_PTR(err);
goto fail;
}
} else {
ret = pol;
priority = ret->priority;
break;
}
}
chain = &xfrm_policy_inexact[dir];
hlist_for_each_entry(pol, entry, chain, bydst) {
if (xfrm_policy_match(pol, fl, type, family, dir) &&
pol->priority < priority) {
err = xfrm_policy_match(pol, fl, type, family, dir);
if (err) {
if (err == -ESRCH)
continue;
else {
ret = ERR_PTR(err);
goto fail;
}
} else if (pol->priority < priority) {
ret = pol;
break;
}
}
if (ret)
xfrm_pol_hold(ret);
fail:
read_unlock_bh(&xfrm_policy_lock);

return ret;
}

static void xfrm_policy_lookup(struct flowi *fl, u16 family, u8 dir,
static int xfrm_policy_lookup(struct flowi *fl, u16 family, u8 dir,
void **objp, atomic_t **obj_refp)
{
struct xfrm_policy *pol;
int err = 0;

#ifdef CONFIG_XFRM_SUB_POLICY
pol = xfrm_policy_lookup_bytype(XFRM_POLICY_TYPE_SUB, fl, family, dir);
if (pol)
if (IS_ERR(pol)) {
err = PTR_ERR(pol);
pol = NULL;
}
if (pol || err)
goto end;
#endif
pol = xfrm_policy_lookup_bytype(XFRM_POLICY_TYPE_MAIN, fl, family, dir);

if (IS_ERR(pol)) {
err = PTR_ERR(pol);
pol = NULL;
}
#ifdef CONFIG_XFRM_SUB_POLICY
end:
#endif
if ((*objp = (void *) pol) != NULL)
*obj_refp = &pol->refcnt;
return err;
}

static inline int policy_to_flow_dir(int dir)
Expand Down Expand Up @@ -1297,6 +1324,8 @@ int xfrm_lookup(struct dst_entry **dst_p, struct flowi *fl,

policy = flow_cache_lookup(fl, dst_orig->ops->family,
dir, xfrm_policy_lookup);
if (IS_ERR(policy))
return PTR_ERR(policy);
}

if (!policy)
Expand Down Expand Up @@ -1343,6 +1372,10 @@ int xfrm_lookup(struct dst_entry **dst_p, struct flowi *fl,
fl, family,
XFRM_POLICY_OUT);
if (pols[1]) {
if (IS_ERR(pols[1])) {
err = PTR_ERR(pols[1]);
goto error;
}
if (pols[1]->action == XFRM_POLICY_BLOCK) {
err = -EPERM;
goto error;
Expand Down Expand Up @@ -1581,6 +1614,9 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
pol = flow_cache_lookup(&fl, family, fl_dir,
xfrm_policy_lookup);

if (IS_ERR(pol))
return 0;

if (!pol) {
if (skb->sp && secpath_has_nontransport(skb->sp, 0, &xerr_idx)) {
xfrm_secpath_reject(xerr_idx, skb, &fl);
Expand All @@ -1599,6 +1635,8 @@ int __xfrm_policy_check(struct sock *sk, int dir, struct sk_buff *skb,
&fl, family,
XFRM_POLICY_IN);
if (pols[1]) {
if (IS_ERR(pols[1]))
return 0;
pols[1]->curlft.use_time = (unsigned long)xtime.tv_sec;
npols ++;
}
Expand Down

0 comments on commit 134b0fc

Please sign in to comment.