On Mon, Oct 30, 2017 at 10:58 AM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote: > Since 4.14-rc1, the selinux-testsuite has been encountering sporadic > failures during testing of labeled IPSEC. git bisect pointed to > commit ec30d78c14a813db39a647b6a348b4286 ("xfrm: add xdst pcpu cache"). > The xdst pcpu cache is only checking that the policies are the same, > but does not validate that the policy, state, and flow match with respect > to security context labeling. As a result, the wrong SA could be used > and the receiver could end up performing permission checking and > providing SO_PEERSEC or SCM_SECURITY values for the wrong security context. > security_xfrm_state_pol_flow_match() exists for this purpose and is > already called from xfrm_state_look_at() for matching purposes. > Further, xfrm_state_look_at() also performs a xfrm_selector_match() test, > which is also missing from the xdst pcpu cache logic. Add calls to both > of these functions when validating the cache entry. With these changes, > the selinux-testsuite passes all tests again. > > Fixes: ec30d78c14a813db39a647b6a348b4286ba4abf5 ("xfrm: add xdst pcpu cache") > Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx> Thanks for chasing this down while I was on vacation :) > This is an RFC because I am not entirely confident in the fix, e.g. is it > sufficient to perform this matching only on the first xfrm or do they all > need to be walked as in xfrm_bundle_ok()? If you look at how we handle outgoing labeled IPsec traffic, e.g. selinux_xfrm_skb_sid_egress(), you'll see that we only check the first xfrm because I don't believe it is ever possible for us to create a xfrm bundle with mis-matching SELinux labels. Inbound traffic is another story, we need to check the entire bundle. > ... Also, should we perform this > matching before (as in this patch) or after calling xfrm_bundle_ok()? I would probably make the LSM call the last check, as you've done; but I have to say that is just so it is consistent with the "LSM last" philosophy and not because of any performance related argument. > ... Also, > do we need to test xfrm->sel.family before calling xfrm_selector_match > (as in this patch) or not - xfrm_state_look_at() does so when the > state is XFRM_STATE_VALID but not when it is _ERROR or _EXPIRED? Speaking purely from a SELinux perspective, I'm not sure it matters: as long as the labels match we are happy. However, from a general IPsec perspective it does seem like a reasonable thing. Granted I'm probably missing something, but it seems a little odd that the code isn't already checking that the selectors match (... what am I missing?). It does check the policies, maybe that is enough in the normal IPsec case? > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 2746b62..171818b 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -1820,6 +1820,11 @@ xfrm_resolve_and_create_bundle(struct xfrm_policy **pols, int num_pols, > !xfrm_pol_dead(xdst) && > memcmp(xdst->pols, pols, > sizeof(struct xfrm_policy *) * num_pols) == 0 && > + (!xdst->u.dst.xfrm->sel.family || > + xfrm_selector_match(&xdst->u.dst.xfrm->sel, fl, > + xdst->u.dst.xfrm->sel.family)) && > + security_xfrm_state_pol_flow_match(xdst->u.dst.xfrm, > + xdst->pols[0], fl) && > xfrm_bundle_ok(xdst)) { > dst_hold(&xdst->u.dst); > return xdst; > -- > 2.9.5 > -- paul moore www.paul-moore.com