[PATCH BUGFIX -rc4] Smack: Respect 'unlabeled' netlabel mode

[PATCH BUGFIX -rc4] Smack: Respect 'unlabeled' netlabel mode

[Ahmed S. Darwish]

Hi all,

In case of Smack 'unlabeled' netlabel option, Smack passes a _zero_
initialized 'secattr' to label a packet/sock. This causes an 
[unfound domain label error]/-ENOENT by netlbl_sock_setattr().
Above Netlabel failure leads to Smack socket hooks failure causing 
an always-on socket() -EPERM error.

Such packets should have a netlabel domain agreed with netlabel to 
represent unlabeled packets. Fortunately Smack net ambient label 
packets are agreed with netlabel to be treated as unlabeled packets. 

Treat all packets coming out from a 'unlabeled' Smack system as
coming from the smack net ambient label.

Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index b5c8f92..03735f4 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1292,6 +1292,8 @@ static void smack_to_secattr(char *smack, struct netlbl_lsm_secattr *nlsp)
 		}
 		break;
 	default:
+		nlsp->domain = kstrdup(smack_net_ambient, GFP_ATOMIC);
+		nlsp->flags = NETLBL_SECATTR_DOMAIN;
 		break;
 	}
 }

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH BUGFIX -rc4] Smack: Respect 'unlabeled' netlabel mode

[Casey Schaufler]

--- "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:

> Hi all,
> 
> In case of Smack 'unlabeled' netlabel option, Smack passes a _zero_
> initialized 'secattr' to label a packet/sock. This causes an 
> [unfound domain label error]/-ENOENT by netlbl_sock_setattr().
> Above Netlabel failure leads to Smack socket hooks failure causing 
> an always-on socket() -EPERM error.
> 
> Such packets should have a netlabel domain agreed with netlabel to 
> represent unlabeled packets. Fortunately Smack net ambient label 
> packets are agreed with netlabel to be treated as unlabeled packets. 
> 
> Treat all packets coming out from a 'unlabeled' Smack system as
> coming from the smack net ambient label.

To date the behavior of a Smack system running with nltype
unlabeled has been carefully undefined. The way you're defining
it will result in a system in which only processes running with
the ambient label will be able to use sockets, unless I'm reading
the code incorrectly. This seems like "correct" behavior, but
I don't think it is what those who've tried it would expect.


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH BUGFIX -rc4] Smack: Respect 'unlabeled' netlabel mode

[Ahmed S. Darwish]

Hi Casey,

On Fri, May 30, 2008 at 04:10:37PM -0700, Casey Schaufler wrote:
> 
> 
> To date the behavior of a Smack system running with nltype
> unlabeled has been carefully undefined. 
>

In the early days (before the 'Smack: unlabeled outgoing ambient packets'
patch - 4bc87e62), I used '$ echo unlabeled > /smack/nltype' in my startup 
scripts to avoid sending cipso-affected packets. When I upgraded this 
machine's kernel, I faced the -EPERM problem mentiond above. 

> The way you're defining
> it will result in a system in which only processes running with
> the ambient label will be able to use sockets, unless I'm reading
> the code incorrectly. 

I've tried to see the relation but failed, any help?

I'm noticing the opposite though, without defining nltype=unlabeled, 
we're forcing every smack-labeled process to send cipso-affected 
packets (and usually no machine around understands cipso).

_Assuming_ the concept is accepted, depending on the ambient label
may actually lead to a race condition though:

- A packet is set with the ambient label domain
- Ambient label changes
  - old ambient-label netlabel domain is deleted
  - new ambient-label is set
  - new ambient-label netlabel domain is created
- call netlabel_sock_setattr(), uses the old ambient label, leads
  to the -EPERM problem.
  -- Rare, but can happen

There are two possible solutions in my mind:

- Using a predefined netlabel domain to denote to unlabeled packets.
  Defect: May collide with a user chosen label and used to break security.
  Solution: Use a domain name that can't become a label (Hackery ?)

- I've tried first to use what was done before the 'Smack: unlabeled outgoing 
  ambient packets' patch, which honored nltype=unlabeled, but ignored netlabel
  completely:
  i.e.

  int rc = 0;
  if (secattr.flags != NETLBL_SECATTR_NONE)
       rc = netlbl_sock_setattr(sk, &secattr);
  return rc

  Paul, would this be right from a netlabel perspective ?

-- 
Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH BUGFIX -rc4] Smack: Respect 'unlabeled' netlabel mode

[Casey Schaufler]

--- "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:

> Hi Casey,
> 
> On Fri, May 30, 2008 at 04:10:37PM -0700, Casey Schaufler wrote:
> > 
> > 
> > To date the behavior of a Smack system running with nltype
> > unlabeled has been carefully undefined. 
> >
> 
> In the early days (before the 'Smack: unlabeled outgoing ambient packets'
> patch - 4bc87e62), I used '$ echo unlabeled > /smack/nltype' in my startup 
> scripts to avoid sending cipso-affected packets. When I upgraded this 
> machine's kernel, I faced the -EPERM problem mentiond above. 

Ok. I haven't done much work with nltype != CIPSO since that change.
Since that change the behavior with nltype == CIPSO appears to meet
most people's needs because:
   - there's a way to talk to non-cipso systems (use the ambient label)
   - you can talk multi-label to cipso aware systems (including yourself)

> > The way you're defining
> > it will result in a system in which only processes running with
> > the ambient label will be able to use sockets, unless I'm reading
> > the code incorrectly. 
> 
> I've tried to see the relation but failed, any help?

Unlabeled packets have to all be treated as having the same label.
That's what the ambient label is for. If you turn off cipso all
packets must be treated as if they came from ambient labeled processes.
If a process is running at some other label, and there is no rule
to allow the ambient label subject to write to that process' label
the packet can't be delivered. Thus, only ambient label processes
will be able to use sockets.

> I'm noticing the opposite though, without defining nltype=unlabeled, 
> we're forcing every smack-labeled process to send cipso-affected 
> packets (and usually no machine around understands cipso).

Except at the ambient label, which sends packets unlabeled.
Unlabeled packets get the ambient label and are delivered only
to sockets that an ambient label subject can write to.
This is the desired behavior. We don't want to deliver unlabeled
packets to sockets that can't be writen to by ambient labeled subjects.

> _Assuming_ the concept is accepted, depending on the ambient label
> may actually lead to a race condition though:
> 
> - A packet is set with the ambient label domain
> - Ambient label changes
>   - old ambient-label netlabel domain is deleted
>   - new ambient-label is set
>   - new ambient-label netlabel domain is created
> - call netlabel_sock_setattr(), uses the old ambient label, leads
>   to the -EPERM problem.
>   -- Rare, but can happen

I would consider additional restrictions on changing the
ambient label and nltype. They should not be changing on
a running system once it gets going.


> There are two possible solutions in my mind:
> 
> - Using a predefined netlabel domain to denote to unlabeled packets.
>   Defect: May collide with a user chosen label and used to break security.
>   Solution: Use a domain name that can't become a label (Hackery ?)
> 
> - I've tried first to use what was done before the 'Smack: unlabeled outgoing
> 
>   ambient packets' patch, which honored nltype=unlabeled, but ignored
> netlabel
>   completely:
>   i.e.
> 
>   int rc = 0;
>   if (secattr.flags != NETLBL_SECATTR_NONE)
>        rc = netlbl_sock_setattr(sk, &secattr);
>   return rc
> 
>   Paul, would this be right from a netlabel perspective ?

It might be that the right thing to do is remove nltype unlabeled.
It's pretty pointless with the cipso nltype dealing with unlabeled
packets by treating them as ambient.


Casey Schaufler
casey@schaufler-ca.com

Re: [PATCH BUGFIX -rc4] Smack: Respect 'unlabeled' netlabel mode

[Paul Moore]

Sorry I'm late to the party ...

On Friday 30 May 2008 8:58:26 pm Ahmed S. Darwish wrote:
> There are two possible solutions in my mind:
>
> - Using a predefined netlabel domain to denote to unlabeled packets.
>   Defect: May collide with a user chosen label and used to break
> security. Solution: Use a domain name that can't become a label
> (Hackery ?)

>From my understanding of Smack that is what the ambient label does 
currently.  Does this not work correctly for you?

> - I've tried first to use what was done before the 'Smack: unlabeled
> outgoing ambient packets' patch, which honored nltype=unlabeled, but
> ignored netlabel completely:
>   i.e.
>
>   int rc = 0;
>   if (secattr.flags != NETLBL_SECATTR_NONE)
>        rc = netlbl_sock_setattr(sk, &secattr);
>   return rc
>
>   Paul, would this be right from a netlabel perspective ?

Well, what are you trying to do (it isn't clear to me from the code 
snippet above)?  The netlbl_sock_setattr() function looks at the 
secattr->domain field and uses the value their to lookup the desired 
labeling protocol (currently either CIPSO or unlabeled) and then the 
NetLabel subsystem passes the socket and the secattr information onto 
the specific protocol handler where the secattr->attr information is 
used to assign on-the-wire labels to the socket.

-- 
paul moore
linux @ hp

[PATCH BUGFIX -v2 -rc4] Smack: Respect 'unlabeled' netlabel mode

[Ahmed S. Darwish]

Hi!,

[ Sorry, Fix: Acquire smack_ambient_lock before reading smack_net_ambient ]

-->

In case of Smack 'unlabeled' netlabel option, Smack passes a _zero_
initialized 'secattr' to label a packet/sock. This causes an 
[unfound domain label error]/-ENOENT by netlbl_sock_setattr().
Above Netlabel failure leads to Smack socket hooks failure causing 
an always-on socket() -EPERM error.

Such packets should have a netlabel domain agreed with netlabel to 
represent unlabeled packets. Fortunately Smack net ambient label 
packets are agreed with netlabel to be treated as unlabeled packets. 

Treat all packets coming out from a 'unlabeled' Smack system as
coming from the smack net ambient label.

Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
---

diff --git a/security/smack/smack.h b/security/smack/smack.h
index 4a4477f..81b94ba 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -16,6 +16,7 @@
 #include <linux/capability.h>
 #include <linux/spinlock.h>
 #include <linux/security.h>
+#include <linux/mutex.h>
 #include <net/netlabel.h>
 
 /*
@@ -178,6 +179,7 @@ u32 smack_to_secid(const char *);
 extern int smack_cipso_direct;
 extern int smack_net_nltype;
 extern char *smack_net_ambient;
+extern struct mutex smack_ambient_lock;
 
 extern struct smack_known *smack_known;
 extern struct smack_known smack_known_floor;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index b5c8f92..0d9c7dc 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1292,6 +1292,11 @@ static void smack_to_secattr(char *smack, struct netlbl_lsm_secattr *nlsp)
 		}
 		break;
 	default:
+		mutex_lock(&smack_ambient_lock);
+		nlsp->domain = kstrdup(smack_net_ambient, GFP_ATOMIC);
+		mutex_unlock(&smack_ambient_lock);
+
+		nlsp->flags = NETLBL_SECATTR_DOMAIN;
 		break;
 	}
 }
diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index 271a835..cc357dc 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -46,7 +46,7 @@ enum smk_inos {
  */
 static DEFINE_MUTEX(smack_list_lock);
 static DEFINE_MUTEX(smack_cipso_lock);
-static DEFINE_MUTEX(smack_ambient_lock);
+DEFINE_MUTEX(smack_ambient_lock);
 
 /*
  * This is the "ambient" label for network traffic.

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH BUGFIX -v2 -rc4] Smack: Respect 'unlabeled' netlabel mode

[Tetsuo Handa]

Hello.

Ahmed S. Darwish wrote:
>  	default:
> +		mutex_lock(&smack_ambient_lock);
> +		nlsp->domain = kstrdup(smack_net_ambient, GFP_ATOMIC);
> +		mutex_unlock(&smack_ambient_lock);
> +
> +		nlsp->flags = NETLBL_SECATTR_DOMAIN;
>  		break;

Getting a mutex lock might sleep, so why not use GFP_KERNEL ?

Re: [PATCH BUGFIX -v2 -rc4] Smack: Respect 'unlabeled' netlabel mode

[Andrew Morton]

On Sat, 31 May 2008 02:57:51 +0300
"Ahmed S. Darwish" <darwish.07@gmail.com> wrote:

> +		mutex_lock(&smack_ambient_lock);
> +		nlsp->domain = kstrdup(smack_net_ambient, GFP_ATOMIC);
> +		mutex_unlock(&smack_ambient_lock);

no no no no no.  And no.

GFP_ATOMIC is *unreliable*.  Using it in a "security" feature is a bug
- if it fails, the feature isn't secure any more.

Failing to check the kmalloc() return value might be a bug.

If we _need_ GFP_ATOMIC here then taking a mutex in a cannot-sleep
context is a bug.

The patch adds a kmalloc but doesn't add a kfree.  Is it leaky?

Finally, why is there a need to take a lock around a single store
instruction?

Re: [PATCH BUGFIX -v2 -rc4] Smack: Respect 'unlabeled' netlabel mode

[Ahmed S. Darwish]

On Fri, May 30, 2008 at 04:25:00PM -0700, Andrew Morton wrote:
> On Sat, 31 May 2008 02:57:51 +0300
> "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:
> 
> > +		mutex_lock(&smack_ambient_lock);
> > +		nlsp->domain = kstrdup(smack_net_ambient, GFP_ATOMIC);
> > +		mutex_unlock(&smack_ambient_lock);
> 
> no no no no no.  And no.
> 
> GFP_ATOMIC is *unreliable*.  Using it in a "security" feature is a bug
> - if it fails, the feature isn't secure any more.
> 
> Failing to check the kmalloc() return value might be a bug.
> 
> If we _need_ GFP_ATOMIC here then taking a mutex in a cannot-sleep
> context is a bug.
> 
> The patch adds a kmalloc but doesn't add a kfree.  Is it leaky?
> 
> Finally, why is there a need to take a lock around a single store
> instruction?

Possibly the worst three lines written ever. GFP_ATOMIC line
was cut-and-paste forgetting to change it to GFP_KERNEL and the lock
is already useless. 

-- 

"Better to light a candle, than curse the darkness"

Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com

Re: [PATCH BUGFIX -v2 -rc4] Smack: Respect 'unlabeled' netlabel mode

[Casey Schaufler]

--- "Ahmed S. Darwish" <darwish.07@gmail.com> wrote:

> Hi!,
> 
> [ Sorry, Fix: Acquire smack_ambient_lock before reading smack_net_ambient ]
> 
> -->
> 
> In case of Smack 'unlabeled' netlabel option, Smack passes a _zero_
> initialized 'secattr' to label a packet/sock. This causes an 
> [unfound domain label error]/-ENOENT by netlbl_sock_setattr().
> Above Netlabel failure leads to Smack socket hooks failure causing 
> an always-on socket() -EPERM error.
> 
> Such packets should have a netlabel domain agreed with netlabel to 
> represent unlabeled packets. Fortunately Smack net ambient label 
> packets are agreed with netlabel to be treated as unlabeled packets. 
> 
> Treat all packets coming out from a 'unlabeled' Smack system as
> coming from the smack net ambient label.
> 
> Signed-off-by: Ahmed S. Darwish <darwish.07@gmail.com>
> ---
> 
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 4a4477f..81b94ba 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -16,6 +16,7 @@
>  #include <linux/capability.h>
>  #include <linux/spinlock.h>
>  #include <linux/security.h>
> +#include <linux/mutex.h>
>  #include <net/netlabel.h>
>  
>  /*
> @@ -178,6 +179,7 @@ u32 smack_to_secid(const char *);
>  extern int smack_cipso_direct;
>  extern int smack_net_nltype;
>  extern char *smack_net_ambient;
> +extern struct mutex smack_ambient_lock;
>  
>  extern struct smack_known *smack_known;
>  extern struct smack_known smack_known_floor;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index b5c8f92..0d9c7dc 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -1292,6 +1292,11 @@ static void smack_to_secattr(char *smack, struct
> netlbl_lsm_secattr *nlsp)
>  		}
>  		break;
>  	default:
> +		mutex_lock(&smack_ambient_lock);
> +		nlsp->domain = kstrdup(smack_net_ambient, GFP_ATOMIC);
> +		mutex_unlock(&smack_ambient_lock);
> +
> +		nlsp->flags = NETLBL_SECATTR_DOMAIN;
>  		break;
>  	}
>  }

This is truely awful. I suggest that instead of doing this
locking you disallow changes to the ambient label if the
nltype is not a well handled type, that is, CIPSO. The overhead
you're introducing to handle a case that will cause the system
to be pretty well useless (if ambient isn't the floor label
your system isn't going to work very well) seems ill advised.

> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index 271a835..cc357dc 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -46,7 +46,7 @@ enum smk_inos {
>   */
>  static DEFINE_MUTEX(smack_list_lock);
>  static DEFINE_MUTEX(smack_cipso_lock);
> -static DEFINE_MUTEX(smack_ambient_lock);
> +DEFINE_MUTEX(smack_ambient_lock);
>  
>  /*
>   * This is the "ambient" label for network traffic.
> 
> -- 
> 
> "Better to light a candle, than curse the darkness"
> 
> Ahmed S. Darwish
> Homepage: http://darwish.07.googlepages.com
> Blog: http://darwish-07.blogspot.com
> 
> 


Casey Schaufler
casey@schaufler-ca.com