Smack: Respect 'unlabeled' netlabel mode

PATCH In review Ahmed S. Darwish 5 participants 10 messages May 30, 2008 – May 31, 2008

Subsystems (4)

Networking Network drivers Security (LSM) Smack security module

lore

Share & tools

lore

Ahmed S. Darwish

author

2008-05-30 22:40 UTC #0 lore raw

PATCH 1/1 Smack: Respect 'unlabeled' netlabel mode

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 <[email protected]>

---

security/smack/smack_lsm.c +2 -0

index b5c8f92..03735f4 100644
@@ -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;
 	}
 }

Signature · 5 lines

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

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

Ahmed S. Darwish

author

2008-05-30 23:02 UTC #1 lore raw

PATCH 1/1 Smack: Respect 'unlabeled' netlabel mode

Hi!,

[ Sorry, Fix: Acquire smack_ambient_lock before reading smack_net_ambient ]

-->

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

Signed-off-by Ahmed S. Darwish <[email protected]>

---

security/smack/smack.h +2 -0

index 4a4477f..81b94ba 100644
@@ -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;

security/smack/smack_lsm.c +5 -0

index b5c8f92..0d9c7dc 100644
@@ -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;
 	}
 }

security/smack/smackfs.c +1 -1

index 271a835..cc357dc 100644
@@ -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.

Signature · 5 lines

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

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

Tetsuo Handa

2008-05-30 23:10 UTC #2 lore raw

Hello.

security/smack/smack_lsm.c static void smack_to_secattr(char *smack, struct netlbl_lsm_secattr *nlsp) · line 1300

Quoted 7 lines · Ahmed S. Darwish

security/smack/smack_lsm.c +5 -0

 	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 ?

Andrew Morton

2008-05-30 23:25 UTC #3 lore raw

In reply to Ahmed S. Darwish

security/smack/smack_lsm.c static void smack_to_secattr(char *smack, struct netlbl_lsm_secattr *nlsp) · line 1297

Quoted 3 lines · On Sat, 31 May 2008 02:57:51 +0300 "Ahmed S. Darwish" <[email protected]>…

security/smack/smack_lsm.c +3 -0

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

Ahmed S. Darwish

author

2008-05-31 00:17 UTC #4 lore raw

Quoted 14 lines · Andrew Morton

security/smack/smack_lsm.c static void smack_to_secattr(char *smack, struct netlbl_lsm_secattr *nlsp) · line 1297
Quoted 3 lines · On Sat, 31 May 2008 02:57:51 +0300 "Ahmed S. Darwish" <[email protected]>…
security/smack/smack_lsm.c +3 -0
+		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.

Quoted 7 lines

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.

Signature · 5 lines

"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

maintainer

2008-05-30 23:45 UTC #5 lore raw

In reply to Ahmed S. Darwish

Quoted 58 lines · --- "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 <[email protected]>
---
security/smack/smack.h +2 -0
index 4a4477f..81b94ba 100644
@@ -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;
security/smack/smack_lsm.c +5 -0
index b5c8f92..0d9c7dc 100644
@@ -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;
 	}

Quoted 1 line

}

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.

Quoted 23 lines

security/smack/smackfs.c +1 -1

index 271a835..cc357dc 100644
@@ -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.

Signature · 7 lines

-- 

"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
[email protected]

Casey Schaufler

maintainer

2008-05-30 23:10 UTC #6 lore raw

In reply to Ahmed S. Darwish

Quoted 8 lines · --- "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.

Quoted 6 lines

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
[email protected]

Ahmed S. Darwish

author

2008-05-31 00:05 UTC #7 lore raw

Hi Casey,

Quoted 5 lines · Casey Schaufler

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.

Quoted 4 lines

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 ?

Signature · 3 lines

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

Casey Schaufler

maintainer

2008-05-31 00:37 UTC #8 lore raw

Quoted 5 lines · --- "Ahmed S. Darwish

Hi Casey,
Quoted 2 lines · Casey Schaufler

Quoted 8 lines

Quoted 3 lines
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)

Quoted 6 lines

Quoted 4 lines
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.

Quoted 3 lines

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.

Quoted 11 lines

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

Quoted 13 lines

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.

Quoted 6 lines

  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
[email protected]

Paul Moore

2008-05-31 13:08 UTC #9 lore raw

In reply to Ahmed S. Darwish

Sorry I'm late to the party ...

Quoted 8 lines · Ahmed S. Darwish

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?

Quoted 11 lines

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

Signature · 2 lines

paul moore
linux @ hp