Re: [PATCHSET RFC] selinux: rework labeled IPsec networking

Re: [PATCHSET RFC] selinux: rework labeled IPsec networking

[Paul Moore]

On Mon, 2011-02-14 at 14:16 +0100, Steffen Klassert wrote:
> This patchset attempts to fix some problems I faced when I tried to use
> labeled IPsec and secmark. In particular, packet forwarding with labeled
> IPsec did not behave as I expected. I know, labeled IPsec was not designed
> to do packet forwarding in the first place, but it's possible to use it
> to secure packet forwarding too. I marked the patchset as RFC because
> I'd like to get some feedback before I continue to work on this.
> 
> There is one issue on packet forwarding with labeled IPsec for which I
> found no easy solution. If we receive IPsec encrypted packets via some
> interface and want to forward it decrypted via some other interface, we
> can (and we do) check in the postrouting hook if the SA that decrypted the
> packet is allowed to talk to the sending interface. We can do this because
> we have the secpath of the decryption. The other way arround, If we receive
> plain IP packets via some interface and want to forward it, IPsec encrypted
> via some other interface, we can't do this check because we have no secpath
> in this case. The only solution I see here, is to implement a secpath for
> outbound IPsec packets. The secpath pointer in the skb is free in this case,
> so would be possible. But that's a bigger task and I don't want to start
> with something like that before I'm getting feedback on all of this.
> 
> The patchset is also availabe at branch 'selinux-networking' of
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/klassert/linux-2.6-stk.git

Hi Steffen,

Thanks for the patches, I'm going to try and review them sometime today
or tomorrow (probably in a few different chunks).  

For future reference, it was good that you sent these to the LSM list,
but you should probably also include the SELinux list (CC'd) on patches
that are SELinux specific (as I believe all of these are).

 * http://www.nsa.gov/research/selinux/subscribe.shtml

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCHSET RFC] selinux: rework labeled IPsec networking

[Paul Moore]

On Tue, 2011-02-15 at 13:19 +0100, Steffen Klassert wrote:
> Hi Paul,
> 
> On Mon, Feb 14, 2011 at 11:59:17AM -0500, Paul Moore wrote:
> > 
> > Hi Steffen,
> > 
> > Thanks for the patches, I'm going to try and review them sometime today
> > or tomorrow (probably in a few different chunks).  
> > 
> 
> thanks for looking at the patches.

My apologies, I only got about halfway through these today and I need to
call it quits for the day.  Finishing this off is first on my todo list
for tomorrow morning.

Thanks for your patience.

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCHSET RFC] selinux: rework labeled IPsec networking

[Paul Moore]

On Monday, February 21, 2011 6:54:03 AM Steffen Klassert wrote:
> On Tue, Feb 15, 2011 at 07:02:14PM -0500, Paul Moore wrote:
> > My apologies, I only got about halfway through these today and I need to
> > call it quits for the day.  Finishing this off is first on my todo list
> > for tomorrow morning.
> > 
> > Thanks for your patience.
> 
> Thanks for the review and sorry for the delay on my side,
> I was ill for the rest of the last week.
> 
> I'll respin a patchset with the acked patches and comment
> on the rest tomorrow.

No problem, thanks for re-spinning the patches.

--
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 01/10] selinux: Fix check for xfrm selinux context algorithm

[Paul Moore]

On Mon, 2011-02-14 at 14:17 +0100, Steffen Klassert wrote:
> selinux_xfrm_sec_ctx_alloc accidentally checks the xfrm domain of
> interpretation against the selinux context algorithm. This patch
> fixes this by checking ctx_alg against the selinux context algorithm.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

Might also be a good candidate for stable.

Acked-by: Paul Moore <paul.moore@hp.com>

> ---
>  security/selinux/xfrm.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index fff78d3..728c57e 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -208,7 +208,7 @@ static int selinux_xfrm_sec_ctx_alloc(struct xfrm_sec_ctx **ctxp,
>  	if (!uctx)
>  		goto not_from_user;
>  
> -	if (uctx->ctx_doi != XFRM_SC_ALG_SELINUX)
> +	if (uctx->ctx_alg != XFRM_SC_ALG_SELINUX)
>  		return -EINVAL;
>  
>  	str_len = uctx->ctx_len;

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 02/10] selinux: Perform postroute access control checks after IPsec transfomations

[Paul Moore]

On Mon, 2011-02-14 at 14:18 +0100, Steffen Klassert wrote:
> If selinux_policycap_netpeer is not set, we do the access control checks
> whenever a packet enters the postrouting hook. This can happen mulitiple
> times if IPsec transformations are involved. If we do packet labeling
> with secmark and we allow the sending socket just to send e.g. esp
> packets, we can't check for that. If a packet is seen by the postrouting
> hook for the first time, it is not transformed yet and rejected because it
> is not labeled as an esp packet.
> 
> Fix this by doing the access control checks just if the packet is on it's
> final way out. This is the same behaviour that we have already if
> selinux_policycap_netpeer is set.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

Believe it or not, this code you are changing was done that way for a
reason: compatibility, bug-for-bug compatibility :)

When the new ingress/egress controls were first introduced (check the
archives, the patches were merged Jan 2008) the existing SELinux
postroute code ran for every transform; this was obviously a bug that
had persisted for some time, but considering the very strong desire to
preserve any user/admin visible behavior, I did not fix this when I
moved the old code up into selinux_ip_postroute_compat().  The good
news, is that I didn't carryover this bug into the new egress controls
as the IPsec transform check occurs before the egress controls are
executed.

So, a big NACK on this patch for compatibility reasons.  In order to get
the behavior you are looking for, make sure your policy enables the
"network_peer_controls" policy capability.

> ---
>  security/selinux/hooks.c |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c8d6992..3bf855a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4547,12 +4547,6 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
>  	u8 secmark_active;
>  	u8 peerlbl_active;
>  
> -	/* If any sort of compatibility mode is enabled then handoff processing
> -	 * to the selinux_ip_postroute_compat() function to deal with the
> -	 * special handling.  We do this in an attempt to keep this function
> -	 * as fast and as clean as possible. */
> -	if (!selinux_policycap_netpeer)
> -		return selinux_ip_postroute_compat(skb, ifindex, family);
>  #ifdef CONFIG_XFRM
>  	/* If skb->dst->xfrm is non-NULL then the packet is undergoing an IPsec
>  	 * packet transformation so allow the packet to pass without any checks
> @@ -4563,6 +4557,14 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
>  	if (skb_dst(skb) != NULL && skb_dst(skb)->xfrm != NULL)
>  		return NF_ACCEPT;
>  #endif
> +
> +	/* If any sort of compatibility mode is enabled then handoff processing
> +	 * to the selinux_ip_postroute_compat() function to deal with the
> +	 * special handling.  We do this in an attempt to keep this function
> +	 * as fast and as clean as possible. */
> +	if (!selinux_policycap_netpeer)
> +		return selinux_ip_postroute_compat(skb, ifindex, family);
> +
>  	secmark_active = selinux_secmark_enabled();
>  	peerlbl_active = netlbl_enabled() || selinux_xfrm_enabled();
>  	if (!secmark_active && !peerlbl_active)

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 02/10] selinux: Perform postroute access control checks after IPsec transfomations

[Paul Moore]

On Tuesday, February 22, 2011 6:23:34 AM Steffen Klassert wrote:
> On Wed, Feb 16, 2011 at 02:34:54PM -0500, Paul Moore wrote:
> > Believe it or not, this code you are changing was done that way for a
> > reason: compatibility, bug-for-bug compatibility :)
> 
> As a selinux newbie, I'm well adviced to believe it :)
> 
> > When the new ingress/egress controls were first introduced (check the
> > archives, the patches were merged Jan 2008) the existing SELinux
> > postroute code ran for every transform; this was obviously a bug that
> > had persisted for some time, but considering the very strong desire to
> > preserve any user/admin visible behavior, I did not fix this when I
> > moved the old code up into selinux_ip_postroute_compat().  The good
> > news, is that I didn't carryover this bug into the new egress controls
> > as the IPsec transform check occurs before the egress controls are
> > executed.
> > 
> > So, a big NACK on this patch for compatibility reasons.  In order to get
> > the behavior you are looking for, make sure your policy enables the
> > "network_peer_controls" policy capability.
> 
> I just noticed that because I started with a dummy policy where I had
> network_peer_controls disabled. I can easily live without that patch
> of course.

Ah, that would explain it.  Were you using the dummy policy generated by 
scripts/selinux?  If so, that might be a worthwhile patch to add that policy 
capability to the generated policy.

--
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 02/10] selinux: Perform postroute access control checks after IPsec transfomations

[Steffen Klassert]

On Wed, Feb 23, 2011 at 04:02:54PM -0500, Paul Moore wrote:
> > 
> > I just noticed that because I started with a dummy policy where I had
> > network_peer_controls disabled. I can easily live without that patch
> > of course.
> 
> Ah, that would explain it.  Were you using the dummy policy generated by 
> scripts/selinux?  

Yes, I did.

> If so, that might be a worthwhile patch to add that policy 
> capability to the generated policy.
> 

Indeed, would be nice to have the network_peer_controls enabled
in the generated dummy policy. I'll look at it.

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 03/10] selinux: Remove checks for xfrm transformations from selinux_xfrm_postroute_last

[Paul Moore]

On Mon, 2011-02-14 at 14:18 +0100, Steffen Klassert wrote:
> Postroute access control checks are just performed on the packets final
> way out, so there is no need to check for further transformations
> in the xfrm state bundle.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

For similar reasons as patch 2/10 I have to NACK this patch.  The
selinux_xfrm_postroute_last() function is a holdout from the previous
network access controls.  If you want the new behavior, use a modern
SELinux policy on a modern kernel.

> ---
>  security/selinux/xfrm.c |   15 ---------------
>  1 files changed, 0 insertions(+), 15 deletions(-)
> 
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index 728c57e..62f3b26 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -445,23 +445,8 @@ int selinux_xfrm_sock_rcv_skb(u32 isec_sid, struct sk_buff *skb,
>  int selinux_xfrm_postroute_last(u32 isec_sid, struct sk_buff *skb,
>  					struct common_audit_data *ad, u8 proto)
>  {
> -	struct dst_entry *dst;
>  	int rc = 0;
>  
> -	dst = skb_dst(skb);
> -
> -	if (dst) {
> -		struct dst_entry *dst_test;
> -
> -		for (dst_test = dst; dst_test != NULL;
> -		     dst_test = dst_test->child) {
> -			struct xfrm_state *x = dst_test->xfrm;
> -
> -			if (x && selinux_authorizable_xfrm(x))
> -				goto out;
> -		}
> -	}
> -
>  	switch (proto) {
>  	case IPPROTO_AH:
>  	case IPPROTO_ESP:

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 04/10] selinux: Fix wrong checks for selinux_policycap_netpeer

[Paul Moore]

On Mon, 2011-02-14 at 14:19 +0100, Steffen Klassert wrote:
> selinux_sock_rcv_skb_compat and selinux_ip_postroute_compat are just
> called if selinux_policycap_netpeer is not set. However in these
> functions we check if selinux_policycap_netpeer is set. This leads
> to some dead code and to the fact that selinux_xfrm_postroute_last
> is never executed. This patch removes the dead code and the checks
> for selinux_policycap_netpeer in the compatibility functions.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

Nice cleanup.

Acked-by: Paul Moore <paul.moore@hp.com>

> ---
>  security/selinux/hooks.c |   24 ++++++------------------
>  1 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 3bf855a..2b594de 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4002,7 +4002,6 @@ static int selinux_sock_rcv_skb_compat(struct sock *sk, struct sk_buff *skb,
>  {
>  	int err = 0;
>  	struct sk_security_struct *sksec = sk->sk_security;
> -	u32 peer_sid;
>  	u32 sk_sid = sksec->sid;
>  	struct common_audit_data ad;
>  	char *addrp;
> @@ -4021,20 +4020,10 @@ static int selinux_sock_rcv_skb_compat(struct sock *sk, struct sk_buff *skb,
>  			return err;
>  	}
>  
> -	if (selinux_policycap_netpeer) {
> -		err = selinux_skb_peerlbl_sid(skb, family, &peer_sid);
> -		if (err)
> -			return err;
> -		err = avc_has_perm(sk_sid, peer_sid,
> -				   SECCLASS_PEER, PEER__RECV, &ad);
> -		if (err)
> -			selinux_netlbl_err(skb, err, 0);
> -	} else {
> -		err = selinux_netlbl_sock_rcv_skb(sksec, skb, family, &ad);
> -		if (err)
> -			return err;
> -		err = selinux_xfrm_sock_rcv_skb(sksec->sid, skb, &ad);
> -	}
> +	err = selinux_netlbl_sock_rcv_skb(sksec, skb, family, &ad);
> +	if (err)
> +		return err;
> +	err = selinux_xfrm_sock_rcv_skb(sksec->sid, skb, &ad);
>  
>  	return err;
>  }
> @@ -4529,9 +4518,8 @@ static unsigned int selinux_ip_postroute_compat(struct sk_buff *skb,
>  				 SECCLASS_PACKET, PACKET__SEND, &ad))
>  			return NF_DROP_ERR(-ECONNREFUSED);
>  
> -	if (selinux_policycap_netpeer)
> -		if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
> -			return NF_DROP_ERR(-ECONNREFUSED);
> +	if (selinux_xfrm_postroute_last(sksec->sid, skb, &ad, proto))
> +		return NF_DROP_ERR(-ECONNREFUSED);
>  
>  	return NF_ACCEPT;
>  }

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 05/10] selinux: selinux_xfrm_decode_session check for socket sid

[Paul Moore]

On Mon, 2011-02-14 at 14:20 +0100, Steffen Klassert wrote:
> selinux_xfrm_decode_session is used to decode the security identifyer
> of the originating flow. So return the sid of the socket instead of
> SECSID_NULL, if we have socket context but no secpath. This is
> needed because ip_xfrm_me_harder (and selinux_xfrm_decode_session)
> is invoked on outbound packets and before the xfrm transformations,
> therefore we have no secpath and the sid of a labeled IPsec SA usually
> does not match SECSID_NULL.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

Granted, it has been some time since I've looked at the labeled IPsec
code in some detail so I might be a little off here, but is it possible
to move the xfrm_decode LSM hook to an area on the outbound processing
where we do have a valid secpath?  If not, I'd rather see us split this
hook so that we preserve the existing xfrm_decode_session() hook for
input (I believe it is also used for input, yes?) and create a new hook
which only grabs the sksec's label on output (preferably named so that
it is clear this is the socket's label and not the SA's label).

> ---
>  security/selinux/xfrm.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index 62f3b26..e990c39 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -160,6 +160,7 @@ int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *
>  int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall)
>  {
>  	struct sec_path *sp;
> +	struct sock *sk;
>  
>  	*sid = SECSID_NULL;
>  
> @@ -167,6 +168,8 @@ int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall)
>  		return 0;
>  
>  	sp = skb->sp;
> +	sk = skb->sk;
> +
>  	if (sp) {
>  		int i, sid_set = 0;
>  
> @@ -185,6 +188,9 @@ int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall)
>  					return -EINVAL;
>  			}
>  		}
> +	} else if (sk) {
> +		struct sk_security_struct *sksec = sk->sk_security;
> +		*sid = sksec->sid;
>  	}
>  
>  	return 0;

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 05/10] selinux: selinux_xfrm_decode_session check for socket sid

[Paul Moore]

On Tuesday, February 22, 2011 7:11:43 AM Steffen Klassert wrote:
> On Wed, Feb 16, 2011 at 03:11:25PM -0500, Paul Moore wrote:
> > On Mon, 2011-02-14 at 14:20 +0100, Steffen Klassert wrote:
> > > selinux_xfrm_decode_session is used to decode the security identifyer
> > > of the originating flow. So return the sid of the socket instead of
> > > SECSID_NULL, if we have socket context but no secpath. This is
> > > needed because ip_xfrm_me_harder (and selinux_xfrm_decode_session)
> > > is invoked on outbound packets and before the xfrm transformations,
> > > therefore we have no secpath and the sid of a labeled IPsec SA usually
> > > does not match SECSID_NULL.
> > > 
> > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > 
> > Granted, it has been some time since I've looked at the labeled IPsec
> > code in some detail so I might be a little off here, but is it possible
> > to move the xfrm_decode LSM hook to an area on the outbound processing
> > where we do have a valid secpath?
> 
> No, we don't have a secpath for outbound packets. The secpath is used to
> do a inbound policy check, check the used SA against their selectors etc.
> With these checks we ensure that the packet is transformed back to it's
> native form in the right way. That's not needed on outbound packets,
> so we don't record a secpath in this case. The lack of a outbound
> secpath was also the problem I mentioned in the introduction mail
> of the patchset.

Be warned: most of this reply is just me tossing out ideas ...

Okay, so basically selinux_xfrm_decode_session() is a total waste of time in 
the output path, yes?  I'm looking at the current code and it does nothing if 
the sec_path is NULL, so I'm wondering why we even keep it around for the 
outbound path.

> > If not, I'd rather see us split this
> > hook so that we preserve the existing xfrm_decode_session() hook for
> > input (I believe it is also used for input, yes?) and create a new hook
> > which only grabs the sksec's label on output (preferably named so that
> > it is clear this is the socket's label and not the SA's label).
> 
> I think that's not possible too. The security_xfrm_decode_session()
> hook is used from within xfrm_decode_session(). This function
> is used in codepaths that are used for both, inbound and outbound
> processing (xfrm_lookup, xfrm_policy_check etc.).

This makes me wonder if the LSM hook is even in the right place.

> The xfrm_decode_session() function simply fills a 'struct flowi' with
> the informations of the originating flow whenever these informations
> are needed, selinux_xfrm_decode_session just adds the sid the
> that struct.
> 
> In the case we are constructing a 'struct flowi' within the outbound
> packet path, we use selinux_sk_getsecid() which just adds the
> sksec label from the socket if we have socket context.
> 
> So with this patch we would use the secpath's sid on inbound
> packets and the sksec label from the socket if we are on
> oubound packet processing with socket context, when we construct
> a 'struct flowi' from xfrm_decode_session().

I _really_ think we need to split the inbound and outbound handling for xfrm.  
I suppose we can always fall back to selecting the right path based on the 
presence of a sec_path pointer, but I would rather see us get rid of an 
unnecessary conditional.

The inbound handling can stick with the logic in selinux_xfrm_decode_session() 
but the outbound handling should follow similar logic to how we determine the 
network peer label in selinux_ip_postroute().

 1. If we have a valid sk_security_stuct, use it.
 2. If we are forwarding the packet, call selinux_skb_peerlbl_sid().
 3. If all else fails, use SECINITSID_KERNEL.

--
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 05/10] selinux: selinux_xfrm_decode_session check for socket sid

[Joy Latten]

On Wed, 2011-02-23 at 16:16 -0500, Paul Moore wrote:
> On Tuesday, February 22, 2011 7:11:43 AM Steffen Klassert wrote:
> > On Wed, Feb 16, 2011 at 03:11:25PM -0500, Paul Moore wrote:
> > > On Mon, 2011-02-14 at 14:20 +0100, Steffen Klassert wrote:
> > > > selinux_xfrm_decode_session is used to decode the security identifyer
> > > > of the originating flow. So return the sid of the socket instead of
> > > > SECSID_NULL, if we have socket context but no secpath. This is
> > > > needed because ip_xfrm_me_harder (and selinux_xfrm_decode_session)
> > > > is invoked on outbound packets and before the xfrm transformations,
> > > > therefore we have no secpath and the sid of a labeled IPsec SA usually
> > > > does not match SECSID_NULL.
> > > > 
> > > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > > 
> > > Granted, it has been some time since I've looked at the labeled IPsec
> > > code in some detail so I might be a little off here, but is it possible
> > > to move the xfrm_decode LSM hook to an area on the outbound processing
> > > where we do have a valid secpath?
> > 
> > No, we don't have a secpath for outbound packets. The secpath is used to
> > do a inbound policy check, check the used SA against their selectors etc.
> > With these checks we ensure that the packet is transformed back to it's
> > native form in the right way. That's not needed on outbound packets,
> > so we don't record a secpath in this case. The lack of a outbound
> > secpath was also the problem I mentioned in the introduction mail
> > of the patchset.
> 
> Be warned: most of this reply is just me tossing out ideas ...
> 
> Okay, so basically selinux_xfrm_decode_session() is a total waste of time in 
> the output path, yes?  I'm looking at the current code and it does nothing if 
> the sec_path is NULL, so I'm wondering why we even keep it around for the 
> outbound path.
> 
> > > If not, I'd rather see us split this
> > > hook so that we preserve the existing xfrm_decode_session() hook for
> > > input (I believe it is also used for input, yes?) and create a new hook
> > > which only grabs the sksec's label on output (preferably named so that
> > > it is clear this is the socket's label and not the SA's label).
> > 
> > I think that's not possible too. The security_xfrm_decode_session()
> > hook is used from within xfrm_decode_session(). This function
> > is used in codepaths that are used for both, inbound and outbound
> > processing (xfrm_lookup, xfrm_policy_check etc.).
> 
> This makes me wonder if the LSM hook is even in the right place.

I am unable to find the original email to get a full understanding of
the context of this particular patch so am responding via Paul's email.
If my comments seem incorrect due to lack of context... please let me
know.

I believe xfrm_decode_session is for inbound processing.
I could not readily find anything suggesting that xfrm_lookup()
results in __xfrm_decode_session() getting called. If I have missed
it, please let me know. I was looking at kernel code for 2.6.35.7.

The only exception I am readily aware of, is made for sending 
icmp error messages. 
RFC 4301, section 6.2 states special processing for icmp 
error messages. This special processing results in there being 
cases where Linux kernel will need to call
xfrm_decode_session_reverse(), to reverse flow's info and then call
xfrm_lookup() to get an SA with this "reversed" info, when
sending an icmp error message. Please see rfc 4301, section 6.2 if 
more info is required.

I am thinking that in this particular icmp case, when
security_xfrm_decode_session() is called, sec_path in the skb will
be empty, so nothing really happens. icmp_send then calls xfrm_lookup()
with the "reversed" info and regular outbound labeled ipsec processing
occurs with the reversed flow info. 

Please let me know if I have misunderstood context of discussion.

regards, 
Joy



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 05/10] selinux: selinux_xfrm_decode_session check for socket sid

[Steffen Klassert]

On Fri, Feb 25, 2011 at 01:21:05PM -0600, Joy Latten wrote:
> > > 
> > > I think that's not possible too. The security_xfrm_decode_session()
> > > hook is used from within xfrm_decode_session(). This function
> > > is used in codepaths that are used for both, inbound and outbound
> > > processing (xfrm_lookup, xfrm_policy_check etc.).
> > 
> > This makes me wonder if the LSM hook is even in the right place.
> 
> I am unable to find the original email to get a full understanding of
> the context of this particular patch so am responding via Paul's email.
> If my comments seem incorrect due to lack of context... please let me
> know.
> 
> I believe xfrm_decode_session is for inbound processing.
> I could not readily find anything suggesting that xfrm_lookup()
> results in __xfrm_decode_session() getting called. If I have missed
> it, please let me know. I was looking at kernel code for 2.6.35.7.

Well, xfrm_decode_session() called in the forwarding path from
__xfrm_route_forward() to construct the flow that is passed to
xfrm_lookup().

Also it is called from the netfilter functions ip_route_me_harder()
on output, and ip_xfrm_me_harder() from the local out and the
postrouting hook.

Further, security_xfrm_decode_session() is called via selinux_skb_xfrm_sid()
from selinux_skb_peerlbl_sid() which is called from input, output and
forwaring codepaths.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 05/10] selinux: selinux_xfrm_decode_session check for socket sid

[Steffen Klassert]

On Wed, Feb 23, 2011 at 04:16:38PM -0500, Paul Moore wrote:
> > 
> > No, we don't have a secpath for outbound packets. The secpath is used to
> > do a inbound policy check, check the used SA against their selectors etc.
> > With these checks we ensure that the packet is transformed back to it's
> > native form in the right way. That's not needed on outbound packets,
> > so we don't record a secpath in this case. The lack of a outbound
> > secpath was also the problem I mentioned in the introduction mail
> > of the patchset.
> 
> Be warned: most of this reply is just me tossing out ideas ...
> 
> Okay, so basically selinux_xfrm_decode_session() is a total waste of time in 
> the output path, yes?  

Right.

> > 
> > I think that's not possible too. The security_xfrm_decode_session()
> > hook is used from within xfrm_decode_session(). This function
> > is used in codepaths that are used for both, inbound and outbound
> > processing (xfrm_lookup, xfrm_policy_check etc.).
> 
> This makes me wonder if the LSM hook is even in the right place.
> 

As security_xfrm_decode_session() should add the sid of the originating
flow, I think it was was quite reasonable to place it to the function
that constructs the flow. 

> 
> I _really_ think we need to split the inbound and outbound handling for xfrm.  
> I suppose we can always fall back to selecting the right path based on the 
> presence of a sec_path pointer, but I would rather see us get rid of an 
> unnecessary conditional.

This would require some structural changes in the core IPsec code too.
At first glance I don't see a good way to do this. Any suggestions?

> 
> The inbound handling can stick with the logic in selinux_xfrm_decode_session() 
> but the outbound handling should follow similar logic to how we determine the 
> network peer label in selinux_ip_postroute().
> 
>  1. If we have a valid sk_security_stuct, use it.
>  2. If we are forwarding the packet, call selinux_skb_peerlbl_sid().
>  3. If all else fails, use SECINITSID_KERNEL.
> 

Sounds reasonable for outbound packets. But we have to be carefull
on packet forwarding. selinux_skb_peerlbl_sid() gets the xfrm sid
from selinux_xfrm_decode_session(). So if we receive a IPsec transformed
packet and forward it, transformed back, we have a valid secpath and we
use the sid of the SA as the peer label. If we receive a plain IP packet
and forward it, IPsec transformed, we have no secpath. So we would stick
with SECSID_NULL, even though it might be transformed with a labeled SA.
I think the sid of the SA should be used as the peer label in this case
too.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 06/10] selinux: Fix packet forwarding checks on postrouting

[Paul Moore]

On Mon, 2011-02-14 at 14:20 +0100, Steffen Klassert wrote:
> The IPSKB_FORWARDED and IP6SKB_FORWARDED flags are used only in the
> multicast forwarding case to indicate that a packet looped back after
> forward. So these flags are not a good indicator for packet forwarding.
> A better indicator is the incoming interface. If we have no socket context,
> but an incoming interface and we see the packet in the ip postroute hook,
> the packet is going to be forwarded.
> 
> With this patch we use the incoming interface as an indicator on packet
> forwarding.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

Nice fix.  I could have sworn that IP{6}SKB_FORWARDED was more universal
when this code was written ... but then again, I'm easily confused :)

Acked-by: Paul Moore <paul.moore@hp.com>

> ---
>  security/selinux/hooks.c |   23 +++++------------------
>  1 files changed, 5 insertions(+), 18 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2b594de..1aeae26 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4564,27 +4564,14 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
>  	 * from the sending socket, otherwise use the kernel's sid */
>  	sk = skb->sk;
>  	if (sk == NULL) {
> -		switch (family) {
> -		case PF_INET:
> -			if (IPCB(skb)->flags & IPSKB_FORWARDED)
> -				secmark_perm = PACKET__FORWARD_OUT;
> -			else
> -				secmark_perm = PACKET__SEND;
> -			break;
> -		case PF_INET6:
> -			if (IP6CB(skb)->flags & IP6SKB_FORWARDED)
> -				secmark_perm = PACKET__FORWARD_OUT;
> -			else
> -				secmark_perm = PACKET__SEND;
> -			break;
> -		default:
> -			return NF_DROP_ERR(-ECONNREFUSED);
> -		}
> -		if (secmark_perm == PACKET__FORWARD_OUT) {
> +		if (skb->skb_iif) {
> +			secmark_perm = PACKET__FORWARD_OUT;
>  			if (selinux_skb_peerlbl_sid(skb, family, &peer_sid))
>  				return NF_DROP;
> -		} else
> +		} else {
> +			secmark_perm = PACKET__SEND;
>  			peer_sid = SECINITSID_KERNEL;
> +		}
>  	} else {
>  		struct sk_security_struct *sksec = sk->sk_security;
>  		peer_sid = sksec->sid;

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 07/10] selinux: Check receiving against sending interface on packet forwarding

[Paul Moore]

On Mon, 2011-02-14 at 14:21 +0100, Steffen Klassert wrote:
> As it is, it is not possible to check in the forwarding and
> the postrouting hook whether a packet that is received via some
> network interface is allowed to be forwarded via some other network
> interface. With this patch we decode the security identifier on
> selinux_xfrm_decode_session to the sid of the incoming interface,
> if we have neither a secpath nor socket context, so this check is
> possible now. Also set the sid to SECINITSID_KERNEL if we have none
> of secpath, socket context and incoming interface as the packet
> must be kernel generated in this case.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

If you want to control which interface is allowed to forward to which
other interface, you should use good ol' iptables, not SELinux.

The SELinux peer label network access controls allow you to restrict the
following:

* Which peers are allowed to enter the system via specific networks and
interfaces
* Which peers are allowed to forward traffic matching a specific inbound
iptables rule (requires Secmark configuration)
* Which peers are allowed to forward traffic matching a specific
outbound iptables rule (requires Secmark configuration)
* Which peers are allowed to leave the system via specific networks and
interfaces

> ---
>  security/selinux/xfrm.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index e990c39..17ad37b 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -51,6 +51,7 @@
>  #include "avc.h"
>  #include "objsec.h"
>  #include "xfrm.h"
> +#include "netif.h"
>  
>  /* Labeled XFRM instance counter */
>  atomic_t selinux_xfrm_refcount = ATOMIC_INIT(0);
> @@ -162,7 +163,7 @@ int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall)
>  	struct sec_path *sp;
>  	struct sock *sk;
>  
> -	*sid = SECSID_NULL;
> +	*sid = SECINITSID_KERNEL;
>  
>  	if (skb == NULL)
>  		return 0;
> @@ -188,11 +189,20 @@ int selinux_xfrm_decode_session(struct sk_buff *skb, u32 *sid, int ckall)
>  					return -EINVAL;
>  			}
>  		}
> -	} else if (sk) {
> +
> +		return 0;
> +	}
> +
> +	if (sk) {
>  		struct sk_security_struct *sksec = sk->sk_security;
>  		*sid = sksec->sid;
> +
> +		return 0;
>  	}
>  
> +	if (selinux_policycap_netpeer && skb->skb_iif)
> +		return sel_netif_sid(skb->skb_iif, sid);
> +
>  	return 0;
>  }
>  

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 07/10] selinux: Check receiving against sending interface on packet forwarding

[Paul Moore]

On Tuesday, February 22, 2011 8:04:09 AM Steffen Klassert wrote:
> On Wed, Feb 16, 2011 at 03:32:40PM -0500, Paul Moore wrote:
> > On Mon, 2011-02-14 at 14:21 +0100, Steffen Klassert wrote:
> > > As it is, it is not possible to check in the forwarding and
> > > the postrouting hook whether a packet that is received via some
> > > network interface is allowed to be forwarded via some other network
> > > interface. With this patch we decode the security identifier on
> > > selinux_xfrm_decode_session to the sid of the incoming interface,
> > > if we have neither a secpath nor socket context, so this check is
> > > possible now. Also set the sid to SECINITSID_KERNEL if we have none
> > > of secpath, socket context and incoming interface as the packet
> > > must be kernel generated in this case.
> > > 
> > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > 
> > If you want to control which interface is allowed to forward to which
> > other interface, you should use good ol' iptables, not SELinux.
> 
> Well, I think the problem is that we can't use labled SA's on
> packet forwarding.
> 
> Consider the following: As it is, the sid of the labeled SA and the sid of
> the flow must be identical. Say we receive a plain IP packet that should be
> forwarded ...

For this argument I'm assuming "plain IP packet" == "IP packet without a peer 
label associated with it".

> ... IPsec transformed with a labeled SA.

Okay, I think that is our disconnect right there.  You are trying to send an 
unlabeled packet (peer label set to "unlabeled_t") over a labeled SA (peer 
label set to "foo_t") without any explicit relabel operation by the 
administrator.  This is a Very Bad Thing.

> Now __xfrm_route_forward() decodes the sid of the flow with
> selinux_xfrm_decode_session(). This packet has neither a secpath nor socket
> conext. So the sid of the flow is decoded to SECSID_NULL.

I suppose we probably should set the flow's label in this case to 
SECINITSID_UNLABELED instead of SECSID_NULL, that would be more consistent ... 
although we would probably need to make sure we don't break anything in 
selinux_xfrm_state_pol_flow_match().

> Then selinux_xfrm_state_pol_flow_match() enforces the sid of SA and flow to
> be identical. This in turn means, that the label of the SA must be
> SECSID_NULL.

Which makes sense to me.  If you are forwarding unlabeled packets across an 
IPsec gateway, the IPsec gateway should establish SAs which are also 
unlabeled.

> So I think we have to chane something if we want to use labeled SAs on
> packet forwarding, or do I miss something here?

I think so, look below.

> With this patch we would decode the sid of the flow to the sid of the
> receiving interface, so the used SA could have the same sid as the
> receiving interface.

I think the problem is that you believe the network interface's label becomes 
the peer label of unlabeled packets, that is not the case.  If you want to 
provide a network peer label to unlabeled packets you need to use NetLabel's 
fallback labeling mechanism which applies peer labels to what would otherwise 
be unlabeled packets (see an example at the link below).

I haven't (or can't remember) testing this in the case of labeled IPsec 
gateway connected to unlabeled, single label networks but I have tested it 
against a CIPSO gateway connected to unlabeled, single label networks.  If it 
doesn't work for you in the labeled IPsec case, this would be a bug and 
something we would need to address.

* http://paulmoore.livejournal.com/1758.html

--
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 07/10] selinux: Check receiving against sending interface on packet forwarding

[Steffen Klassert]

On Wed, Feb 23, 2011 at 04:34:59PM -0500, Paul Moore wrote:
> 
> > Now __xfrm_route_forward() decodes the sid of the flow with
> > selinux_xfrm_decode_session(). This packet has neither a secpath nor socket
> > conext. So the sid of the flow is decoded to SECSID_NULL.
> 
> I suppose we probably should set the flow's label in this case to 
> SECINITSID_UNLABELED instead of SECSID_NULL, that would be more consistent ... 
> although we would probably need to make sure we don't break anything in 
> selinux_xfrm_state_pol_flow_match().

I think using SECINITSID_UNLABELED instead of SECSID_NULL would break the
netlabel fallback labeling. security_net_peersid_resolve() requires
SECSID_NULL on unlabeled packets.

> 
> I think the problem is that you believe the network interface's label becomes 
> the peer label of unlabeled packets, that is not the case.  If you want to 
> provide a network peer label to unlabeled packets you need to use NetLabel's 
> fallback labeling mechanism which applies peer labels to what would otherwise 
> be unlabeled packets (see an example at the link below).

Ok, I've missed the possibility to relabel unlabeled packets with netlabel.
Knowing about this possibility makes many things clear, thanks for pointing
to it.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 08/10] selinux: Fix handling of kernel generated packets on labeled IPsec

[Paul Moore]

On Mon, 2011-02-14 at 14:21 +0100, Steffen Klassert wrote:
> As it is, we require the security context of a labeled SA to be the
> same as the security context of the originating flow. On a nontrivial
> selinux policy the security context of the flow of kernel generated
> packets, like echo reply packets, are usually not equal to the
> security context of the labeled SA. So it is not possible to send
> such packets in this case. The fact that the security context of a
> labeled SA must be the same as the security context of the originating
> flow can also lead to problems if an outgoing SA is used for packet
> forwarding and for locally generated traffic.
> 
> This patch fixes this by removing this equality check. Instead we
> ask the access vector cache if the security context of the SA is
> compatible to the security context of the IPsec policy. This
> partially reverts commit 67f83cbf081a70426ff667e8d14f94e13ed3bdca
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

I think understand the problem you're running up against, and if I'm
understanding it correctly it is an inherent problem with the labeled
IPsec design - not something that we can fix.

The core issue is that with labeled IPsec the packets themselves are not
labeled, the SAs are labeled instead.  This means that in order to
accurately convey peer labels over the network you need to ensure that
the flow's label matches the SA label; this is why there is a direct SID
to SID comparison in the code.  Failing to match a flow to the
associated SA will result in data being mis/relabeled which is a big
no-no.

> ---
>  security/selinux/xfrm.c |   17 +++++++----------
>  1 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/security/selinux/xfrm.c b/security/selinux/xfrm.c
> index 17ad37b..05e1c1c 100644
> --- a/security/selinux/xfrm.c
> +++ b/security/selinux/xfrm.c
> @@ -116,6 +116,7 @@ int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *
>  			struct flowi *fl)
>  {
>  	u32 state_sid;
> +	u32 pol_sid;
>  	int rc;
>  
>  	if (!xp->security)
> @@ -135,20 +136,16 @@ int selinux_xfrm_state_pol_flow_match(struct xfrm_state *x, struct xfrm_policy *
>  				return 0;
>  
>  	state_sid = x->security->ctx_sid;
> +	pol_sid = xp->security->ctx_sid;
>  
> -	if (fl->secid != state_sid)
> +	rc = avc_has_perm(state_sid, pol_sid, SECCLASS_ASSOCIATION,
> +			  ASSOCIATION__POLMATCH, NULL)? 0:1;
> +
> +	if (!rc)
>  		return 0;
>  
>  	rc = avc_has_perm(fl->secid, state_sid, SECCLASS_ASSOCIATION,
> -			  ASSOCIATION__SENDTO,
> -			  NULL)? 0:1;
> -
> -	/*
> -	 * We don't need a separate SA Vs. policy polmatch check
> -	 * since the SA is now of the same label as the flow and
> -	 * a flow Vs. policy polmatch check had already happened
> -	 * in selinux_xfrm_policy_lookup() above.
> -	 */
> +			  ASSOCIATION__SENDTO, NULL)? 0:1;
>  
>  	return rc;
>  }

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 08/10] selinux: Fix handling of kernel generated packets on labeled IPsec

[Paul Moore]

On Tuesday, February 22, 2011 8:31:50 AM Steffen Klassert wrote:
> On Wed, Feb 16, 2011 at 03:57:27PM -0500, Paul Moore wrote:
> > On Mon, 2011-02-14 at 14:21 +0100, Steffen Klassert wrote:
> > > As it is, we require the security context of a labeled SA to be the
> > > same as the security context of the originating flow. On a nontrivial
> > > selinux policy the security context of the flow of kernel generated
> > > packets, like echo reply packets, are usually not equal to the
> > > security context of the labeled SA. So it is not possible to send
> > > such packets in this case. The fact that the security context of a
> > > labeled SA must be the same as the security context of the originating
> > > flow can also lead to problems if an outgoing SA is used for packet
> > > forwarding and for locally generated traffic.
> > > 
> > > This patch fixes this by removing this equality check. Instead we
> > > ask the access vector cache if the security context of the SA is
> > > compatible to the security context of the IPsec policy. This
> > > partially reverts commit 67f83cbf081a70426ff667e8d14f94e13ed3bdca
> > > 
> > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > 
> > I think understand the problem you're running up against, and if I'm
> > understanding it correctly it is an inherent problem with the labeled
> > IPsec design - not something that we can fix.
> 
> Hm, I think we need to get this to work. Echo reply packets are just
> one thing. Also other icmp messages can't be send. For example path mtu
> discovery can't work with labeled IPsec. And have you tried to use
> labeled IPsec on IPv6 networks? Kernel generated icmp messages are
> quite important here.

Yes, several years ago I worked on the IPsec stack for a commercial UNIX OS 
and we had to basically allow all IPv6/ICMP in the SPD so that router/neighbor 
discovery would work correctly.  I believe that the specifications have now 
evolved to allow type/code which would make this a bit easier now.

Why not just create a SPD rule that would send these ICMP packets over a 
traditional, unlabeled SA?  Am I missing something that would require them to 
be sent over a labeled SA?

> > The core issue is that with labeled IPsec the packets themselves are not
> > labeled, the SAs are labeled instead.  This means that in order to
> > accurately convey peer labels over the network you need to ensure that
> > the flow's label matches the SA label; this is why there is a direct SID
> > to SID comparison in the code.  Failing to match a flow to the
> > associated SA will result in data being mis/relabeled which is a big
> > no-no.
> 
> I still don't understand why this is necessary, would'nt it be also
> ok to enforce this with a policy rule? I have to think a bit longer
> about that...

Think about it some more and let me know if you are still having problems.

It is absolutely critical that the SA's label matches the peer's label 
_exactly_.  Once you understand how labeled IPsec conveys peer labels across 
the network, you will understand why this is so important.

--
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 08/10] selinux: Fix handling of kernel generated packets on labeled IPsec

[Joy Latten]

On Wed, 2011-02-23 at 16:45 -0500, Paul Moore wrote:
> On Tuesday, February 22, 2011 8:31:50 AM Steffen Klassert wrote:
> > On Wed, Feb 16, 2011 at 03:57:27PM -0500, Paul Moore wrote:
> > > On Mon, 2011-02-14 at 14:21 +0100, Steffen Klassert wrote:
> > > > As it is, we require the security context of a labeled SA to be the
> > > > same as the security context of the originating flow. On a nontrivial
> > > > selinux policy the security context of the flow of kernel generated
> > > > packets, like echo reply packets, are usually not equal to the
> > > > security context of the labeled SA. So it is not possible to send
> > > > such packets in this case. The fact that the security context of a
> > > > labeled SA must be the same as the security context of the originating
> > > > flow can also lead to problems if an outgoing SA is used for packet
> > > > forwarding and for locally generated traffic.

hmmm... i am thinking that if the security context of the flow does
not match the security context of the SA, then that means an SA pair
needs to have been negotiated for that outgoing pkt(s). Labeled ipsec
design calls for SApair-per-security-context for a traffic stream, 
thus there may be more than one SA-pair per traffic stream. As Paul
stated, this is inherent in design.

selinux_xfrm_policy_lookup does check that flow has permission to use
the spd entry. 

> > > > 
> > > > This patch fixes this by removing this equality check. Instead we
> > > > ask the access vector cache if the security context of the SA is
> > > > compatible to the security context of the IPsec policy. This
> > > > partially reverts commit 67f83cbf081a70426ff667e8d14f94e13ed3bdca
> > > > 
> > > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > > 
> > > I think understand the problem you're running up against, and if I'm
> > > understanding it correctly it is an inherent problem with the labeled
> > > IPsec design - not something that we can fix.
> > 
> > Hm, I think we need to get this to work. Echo reply packets are just
> > one thing. Also other icmp messages can't be send. For example path mtu
> > discovery can't work with labeled IPsec. And have you tried to use
> > labeled IPsec on IPv6 networks? Kernel generated icmp messages are
> > quite important here.
> 
> Yes, several years ago I worked on the IPsec stack for a commercial UNIX OS 
> and we had to basically allow all IPv6/ICMP in the SPD so that router/neighbor 
> discovery would work correctly.  I believe that the specifications have now 
> evolved to allow type/code which would make this a bit easier now.
> 
> Why not just create a SPD rule that would send these ICMP packets over a 
> traditional, unlabeled SA?  Am I missing something that would require them to 
> be sent over a labeled SA?
> 
> > > The core issue is that with labeled IPsec the packets themselves are not
> > > labeled, the SAs are labeled instead.  This means that in order to
> > > accurately convey peer labels over the network you need to ensure that
> > > the flow's label matches the SA label; this is why there is a direct SID
> > > to SID comparison in the code.  Failing to match a flow to the
> > > associated SA will result in data being mis/relabeled which is a big
> > > no-no.
> > 
> > I still don't understand why this is necessary, would'nt it be also
> > ok to enforce this with a policy rule? I have to think a bit longer
> > about that...
> 
> Think about it some more and let me know if you are still having problems.
> 
> It is absolutely critical that the SA's label matches the peer's label 
> _exactly_.  Once you understand how labeled IPsec conveys peer labels across 
> the network, you will understand why this is so important.
> 

I agree with Paul.

regards,
Joy



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 08/10] selinux: Fix handling of kernel generated packets on labeled IPsec

[Steffen Klassert]

On Wed, Feb 23, 2011 at 04:45:57PM -0500, Paul Moore wrote:
> > 
> > Hm, I think we need to get this to work. Echo reply packets are just
> > one thing. Also other icmp messages can't be send. For example path mtu
> > discovery can't work with labeled IPsec. And have you tried to use
> > labeled IPsec on IPv6 networks? Kernel generated icmp messages are
> > quite important here.
> 
> Yes, several years ago I worked on the IPsec stack for a commercial UNIX OS 
> and we had to basically allow all IPv6/ICMP in the SPD so that router/neighbor 
> discovery would work correctly.  I believe that the specifications have now 
> evolved to allow type/code which would make this a bit easier now.
> 
> Why not just create a SPD rule that would send these ICMP packets over a 
> traditional, unlabeled SA?  Am I missing something that would require them to 
> be sent over a labeled SA?
> 

Might be possible. But I thought about this from a multi level security
point of view. Such packets should be send back over an SA with the same
confidential level as the receiving SA. Different confidential levels might
require different crypto algorithems etc.

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 08/10] selinux: Fix handling of kernel generated packets on labeled IPsec

[Paul Moore]

On Monday, February 28, 2011 5:33:41 AM Steffen Klassert wrote:
> On Wed, Feb 23, 2011 at 04:45:57PM -0500, Paul Moore wrote:
> > > Hm, I think we need to get this to work. Echo reply packets are just
> > > one thing. Also other icmp messages can't be send. For example path mtu
> > > discovery can't work with labeled IPsec. And have you tried to use
> > > labeled IPsec on IPv6 networks? Kernel generated icmp messages are
> > > quite important here.
> > 
> > Yes, several years ago I worked on the IPsec stack for a commercial UNIX
> > OS and we had to basically allow all IPv6/ICMP in the SPD so that
> > router/neighbor discovery would work correctly.  I believe that the
> > specifications have now evolved to allow type/code which would make this
> > a bit easier now.
> > 
> > Why not just create a SPD rule that would send these ICMP packets over a
> > traditional, unlabeled SA?  Am I missing something that would require
> > them to be sent over a labeled SA?
> 
> Might be possible. But I thought about this from a multi level security
> point of view. Such packets should be send back over an SA with the same
> confidential level as the receiving SA. Different confidential levels might
> require different crypto algorithems etc.

I think you missed the point, send both the request and the response over an 
unlabeled SA.  This shouldn't cause a security issue around data import/export 
into and out of the system as there shouldn't be any user data in these ICMP 
messages, just system management data.

Also, by default, SELinux treats packets arriving over unlabeled SAs as 
unlabeled_t:SystemHigh so assuming you aren't normally running user processes 
at SystemHigh you should be protected in the case that something goes wrong 
with your IPsec configuration.

--
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 10/10] selinux: Perform xfrm checks for unlabeled access in any case

[Paul Moore]

On Mon, 2011-02-14 at 14:23 +0100, Steffen Klassert wrote:
> We do the checks for unlabeled access with selinux_xfrm_postroute_last
> and selinux_xfrm_sock_rcv_skb just in compatibility mode.
> However these checks are required in any case as we have to ensure
> that a packet is allowed to be send to (or received from) an unlabeled
> destination if we have no security association configured.
> 
> This patch fixes this by calling selinux_xfrm_postroute_last and
> selinux_xfrm_sock_rcv_skb in any case.
> 
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>

Big NACK on this patch.

While it may seem odd to bail out on the labeled network access checks,
we should only do this when the admin has not configured any of the
labeled networking bits in the kernel (labeled IPsec, NetLabel,
Secmark).  The idea is that if the admin hasn't configured anything, all
you are going to end up checking is if unlabeled packets can be sent up,
down and around the network stack.  Not a very interesting access
control, and when you consider the policy headache and performance
impact it was causing we decided that dynamic access controls would be
okay for networking.

All you have to do to enable the dynamic access controls is configure
the system and viola, you get back all your unlabeled_t AVCs.

> ---
>  security/selinux/hooks.c |   12 +++++++-----
>  1 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 1aeae26..85c7667 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4055,8 +4055,6 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  
>  	secmark_active = selinux_secmark_enabled();
>  	peerlbl_active = netlbl_enabled() || selinux_xfrm_enabled();
> -	if (!secmark_active && !peerlbl_active)
> -		return 0;
>  
>  	COMMON_AUDIT_DATA_INIT(&ad, NET);
>  	ad.u.net.netif = skb->skb_iif;
> @@ -4090,6 +4088,8 @@ static int selinux_socket_sock_rcv_skb(struct sock *sk, struct sk_buff *skb)
>  			return err;
>  	}
>  
> +	err = selinux_xfrm_sock_rcv_skb(sk_sid, skb, &ad);
> +
>  	return err;
>  }
>  
> @@ -4532,6 +4532,7 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
>  	struct sock *sk;
>  	struct common_audit_data ad;
>  	char *addrp;
> +	u8 proto;
>  	u8 secmark_active;
>  	u8 peerlbl_active;
>  
> @@ -4555,8 +4556,6 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
>  
>  	secmark_active = selinux_secmark_enabled();
>  	peerlbl_active = netlbl_enabled() || selinux_xfrm_enabled();
> -	if (!secmark_active && !peerlbl_active)
> -		return NF_ACCEPT;
>  
>  	/* if the packet is being forwarded then get the peer label from the
>  	 * packet itself; otherwise check to see if it is from a local
> @@ -4581,7 +4580,7 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
>  	COMMON_AUDIT_DATA_INIT(&ad, NET);
>  	ad.u.net.netif = ifindex;
>  	ad.u.net.family = family;
> -	if (selinux_parse_skb(skb, &ad, &addrp, 0, NULL))
> +	if (selinux_parse_skb(skb, &ad, &addrp, 0, &proto))
>  		return NF_DROP;
>  
>  	if (secmark_active)
> @@ -4606,6 +4605,9 @@ static unsigned int selinux_ip_postroute(struct sk_buff *skb, int ifindex,
>  			return NF_DROP_ERR(-ECONNREFUSED);
>  	}
>  
> +	if (selinux_xfrm_postroute_last(peer_sid, skb, &ad, proto))
> +		return NF_DROP_ERR(-ECONNREFUSED);
> +
>  	return NF_ACCEPT;
>  }
>  

-- 
paul moore
linux @ hp



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 10/10] selinux: Perform xfrm checks for unlabeled access in any case

[Paul Moore]

On Tuesday, February 22, 2011 8:52:17 AM Steffen Klassert wrote:
> On Wed, Feb 16, 2011 at 04:08:18PM -0500, Paul Moore wrote:
> > On Mon, 2011-02-14 at 14:23 +0100, Steffen Klassert wrote:
> > > We do the checks for unlabeled access with selinux_xfrm_postroute_last
> > > and selinux_xfrm_sock_rcv_skb just in compatibility mode.
> > > However these checks are required in any case as we have to ensure
> > > that a packet is allowed to be send to (or received from) an unlabeled
> > > destination if we have no security association configured.
> > > 
> > > This patch fixes this by calling selinux_xfrm_postroute_last and
> > > selinux_xfrm_sock_rcv_skb in any case.
> > > 
> > > Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> > 
> > Big NACK on this patch.
> > 
> > While it may seem odd to bail out on the labeled network access checks,
> > we should only do this when the admin has not configured any of the
> > labeled networking bits in the kernel (labeled IPsec, NetLabel,
> > Secmark).  The idea is that if the admin hasn't configured anything, all
> > you are going to end up checking is if unlabeled packets can be sent up,
> > down and around the network stack.  Not a very interesting access
> > control, and when you consider the policy headache and performance
> > impact it was causing we decided that dynamic access controls would be
> > okay for networking.
> 
> If we want to keep that behaviour, we should change the Kconfig help
> of labeled IPsec at least, there one can find:
> 
> Non-IPSec communications are designated as unlabelled, and only sockets
> authorized to communicate unlabelled data can send without using IPSec.
> 
> What is simply not the case, as far as I can see.

Here is the full text of CONFIG_SECURITY_NETWORK_XFRM for those of you 
following along at home:

          This enables the XFRM (IPSec) networking security hooks.
          If enabled, a security module can use these hooks to
          implement per-packet access controls based on labels
          derived from IPSec policy.  Non-IPSec communications are
          designated as unlabelled, and only sockets authorized
          to communicate unlabelled data can send without using
          IPSec.
          If you are unsure how to answer this question, answer N.

What do you suggest?  If you're going to complain about help text you have to 
offer some suggestions, that's the rule :)

> > All you have to do to enable the dynamic access controls is configure
> > the system and viola, you get back all your unlabeled_t AVCs.
> 
> If I understand it correct, the dynamic access controls are just active
> if a labeled SA/policy is inserted to the kernel.

... or Secmark rules, or NetLabel configuration.

> So if no labeled SA/policy is loaded, we might send confidential data
> untransformed over the network.

If you haven't configured any of the SELinux network access controls, meaning 
_all_ data flowing into and out of the system via the network is considered 
to be unlabeled_t:SystemHigh, then yes, confidential and every other type of 
data can be sent out the network.

Ask yourself this question: why would an admin, running SELinux, who cares 
about restricting what data can be sent over the network not configure any of 
SELinux's network access controls?  It just doesn't make sense ...

> Even though, we could have a selinux policy rule that enforces the usage of
> a certain labeled SA. So for example if the key daemon does not start up
> for some reason, we have no labeled SA and the traffic leaves the system
> untransformed. That's what I wanted to avoid.

This will not happen, or rather it should not happen if everything works the 
way it should.

In this case you may not have an automatically generated SAs with labels, but 
you will have SPD rules with labels so the network access controls will be 
enabled (if not, we have a bug).  See how selinux_xfrm_refcount is managed in 
selinux/xfrm.c to see what I mean.

--
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 10/10] selinux: Perform xfrm checks for unlabeled access in any case

[Steffen Klassert]

On Wed, Feb 23, 2011 at 04:59:17PM -0500, Paul Moore wrote:
> > 
> > If we want to keep that behaviour, we should change the Kconfig help
> > of labeled IPsec at least, there one can find:
> > 
> > Non-IPSec communications are designated as unlabelled, and only sockets
> > authorized to communicate unlabelled data can send without using IPSec.
> > 
> > What is simply not the case, as far as I can see.
> 
> Here is the full text of CONFIG_SECURITY_NETWORK_XFRM for those of you 
> following along at home:
> 
>           This enables the XFRM (IPSec) networking security hooks.
>           If enabled, a security module can use these hooks to
>           implement per-packet access controls based on labels
>           derived from IPSec policy.  Non-IPSec communications are
>           designated as unlabelled, and only sockets authorized
>           to communicate unlabelled data can send without using
>           IPSec.
>           If you are unsure how to answer this question, answer N.
> 
> What do you suggest?  If you're going to complain about help text you have to 
> offer some suggestions, that's the rule :)
> 

Yeah, I know about the rules. Right now I've tried to change the code to
fit better to the help text. If this does not work out, I still can try
to do it the oher way arround :)

> 
> If you haven't configured any of the SELinux network access controls, meaning 
> _all_ data flowing into and out of the system via the network is considered 
> to be unlabeled_t:SystemHigh, then yes, confidential and every other type of 
> data can be sent out the network.
> 
> Ask yourself this question: why would an admin, running SELinux, who cares 
> about restricting what data can be sent over the network not configure any of 
> SELinux's network access controls?  It just doesn't make sense ...
> 
> > Even though, we could have a selinux policy rule that enforces the usage of
> > a certain labeled SA. So for example if the key daemon does not start up
> > for some reason, we have no labeled SA and the traffic leaves the system
> > untransformed. That's what I wanted to avoid.
> 
> This will not happen, or rather it should not happen if everything works the 
> way it should.
> 

Yes, if everything works the way it should we are fine and we would not even
need to use selinux, but in real live bugs happen.

Usually I have to answer questions like:
Given there is a bug in subsystem xyz, show that we still on the save
side. And depending on the confidential level I have to show several lines
of defense.


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 10/10] selinux: Perform xfrm checks for unlabeled access in any case

[Paul Moore]

On Monday, February 28, 2011 6:34:53 AM Steffen Klassert wrote:
> On Wed, Feb 23, 2011 at 04:59:17PM -0500, Paul Moore wrote:
> > > If we want to keep that behaviour, we should change the Kconfig help
> > > of labeled IPsec at least, there one can find:
> > > 
> > > Non-IPSec communications are designated as unlabelled, and only sockets
> > > authorized to communicate unlabelled data can send without using IPSec.
> > > 
> > > What is simply not the case, as far as I can see.
> > 
> > Here is the full text of CONFIG_SECURITY_NETWORK_XFRM for those of you
> > 
> > following along at home:
> >           This enables the XFRM (IPSec) networking security hooks.
> >           If enabled, a security module can use these hooks to
> >           implement per-packet access controls based on labels
> >           derived from IPSec policy.  Non-IPSec communications are
> >           designated as unlabelled, and only sockets authorized
> >           to communicate unlabelled data can send without using
> >           IPSec.
> >           If you are unsure how to answer this question, answer N.
> > 
> > What do you suggest?  If you're going to complain about help text you
> > have to offer some suggestions, that's the rule :)
> 
> Yeah, I know about the rules. Right now I've tried to change the code to
> fit better to the help text. If this does not work out, I still can try
> to do it the oher way arround :)
> 
> > If you haven't configured any of the SELinux network access controls,
> > meaning _all_ data flowing into and out of the system via the network is
> > considered to be unlabeled_t:SystemHigh, then yes, confidential and
> > every other type of data can be sent out the network.
> > 
> > Ask yourself this question: why would an admin, running SELinux, who
> > cares about restricting what data can be sent over the network not
> > configure any of SELinux's network access controls?  It just doesn't
> > make sense ...
> > 
> > > Even though, we could have a selinux policy rule that enforces the
> > > usage of a certain labeled SA. So for example if the key daemon does
> > > not start up for some reason, we have no labeled SA and the traffic
> > > leaves the system untransformed. That's what I wanted to avoid.
> > 
> > This will not happen, or rather it should not happen if everything works
> > the way it should.
> 
> Yes, if everything works the way it should we are fine and we would not
> even need to use selinux, but in real live bugs happen.
> 
> Usually I have to answer questions like:
> Given there is a bug in subsystem xyz, show that we still on the save
> side. And depending on the confidential level I have to show several lines
> of defense.

Just keep in mind that the kernel operates as one big memory space; if a bug 
inside the kernel is exploited, it is hard to make any guarantees about the 
kernel's security properties at that point.

--
paul moore
linux @ hp

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH 09/10] selinux: xfrm - notify users on dropped packets

[James Morris]

On Wed, 16 Feb 2011, Paul Moore wrote:

> >  	if (!xp->security)
> > -		if (x->security)
> > -			/* unlabeled policy and labeled SA can't match */
> > +		if (x->security) {
> > +			if (net_ratelimit())
> > +				printk("selinux: unlabeled policy and labeled"
> > +				       "SA can't match!\n");

I don't think these paths have been tested.  Think how the above will look 
when printed.  Same for the rest in the patch.

Please get rid of the exclamation marks.


- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.