From mboxrd@z Thu Jan 1 00:00:00 1970 From: fw@strlen.de (Florian Westphal) Date: Tue, 31 Oct 2017 12:11:22 +0100 Subject: [RFC PATCH] xfrm: fix regression introduced by xdst pcpu cache In-Reply-To: <20171030145843.13496-1-sds@tycho.nsa.gov> References: <20171030145843.13496-1-sds@tycho.nsa.gov> Message-ID: <20171031111122.GB7663@breakpoint.cc> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org Stephen Smalley 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 > --- > 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()? Also, should we perform this > matching before (as in this patch) or after calling xfrm_bundle_ok()? 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? No idea. I looked at the old flow cache but i don't see any of these extra checks there either. However, old flow cache stored flowi struct as key, and that contains a flowi_secid, populated by the decode_session hooks. Was it enough to check for identical flowi_secid in the flowi structs to avoid this problem or am i missing something? -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html