On Thu, Nov 2, 2017 at 8:58 AM, Stephen Smalley [off-list ref] wrote:

On Wed, 2017-11-01 at 17:39 -0400, Paul Moore wrote:

quoted

On Tue, Oct 31, 2017 at 7:08 PM, Florian Westphal [off-list ref]
wrote:

quoted

Paul Moore [off-list ref] wrote:

quoted

On Mon, Oct 30, 2017 at 10:58 AM, Stephen Smalley <sds@tycho.nsa.
gov> wrote:

quoted

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.

quoted

... 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?

The assumption was that identical policies would yield the same
SAs,
but thats not correct.

Well, to be fair, I think the assumption is valid for normal,
unlabeled IPsec.  The problem comes when SELinux starts labeling SAs
and now you have multiple SAs for a given policy, each differing only
in the SELinux/LSM label.

No, it is invalid for normal, unlabeled IPSEC too, in the case where
one has defined xfrm state selectors.  That's what my other testsuite
patch (which is presently only on the xfrmselectortest branch) is
exercising - matching of xfrm state selectors.  But in any event,
Florian's patch fixes both, so I'm fine with it.  I don't know though
how it compares performance-wise with walking the bundle and just
calling security_xfrm_state_pol_flow_match() and xfrm_selector_match()
on each one.

quoted

Considering that adding the SELinux/LSM label effectively adds an
additional selector, I'm wondering if we should simply add the
SELinux/LSM label matching to xfrm_selector_match()?  Looking quickly
at the code it seems as though we always follow xfrm_selector_match()
with a LSM check anyway, the one exception being in
__xfrm_policy_check() ... which *might* be a valid exception, as we
don't do our access checks for inbound traffic at that point in the
stack.

Possibly, but that should probably be a separate patch. We should just
fix this regression for 4.14, either via Florian's patch or by
augmenting my patch to perform the matching calls on all of the xfrms.

I agree that v4.14 should get the smallest patch possible that fixes
the problem.  I was just looking at the patches presented so far and
thinking out loud.

-- 
paul moore
www.paul-moore.com