Separate socket SID than its creator

Separate socket SID than its creator

[HarryCiao]

[-- Attachment #1.1: Type: text/plain, Size: 1186 bytes --]


Hello Stephen, James and Eric,

The attached is the v1 patches to compute socket SID on creation. 

The propagation of the return code of security_transition_sid is a good practice and actually helps identifying the problem that the socket creator's role also needs to be able to type the socket role. 

The class number(orig_tclass) used by SELinux kernel driver is checked against SECCLASS_XXX macros and if it's any one of those 22 socket classes both security_compute_sid and mls_comput_sid will preserve the scontext's role and MLS attribute as what they have been done for policydb.policy_class. (we are not checking access vectors so we don't have to concern about the unmapped, real class number allocated by checkpolicy)

I've also come up with a debug patch to make use of the security_sid_to_context which helps to reveal that the unix_dgram_socket object created by syslogd differs only in type from its creator and shares the same user, role and MLS attribute. The relevant changes to logging.pp used for debugging is also attached.

Please kindly help commenting and suggesting how I could further test these two patches.

Many thanks!

Best regards,
Harry


 		 	   		  

[-- Attachment #1.2: Type: text/html, Size: 1426 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v1-0001-Compute-SID-for-the-newly-created-socket.patch --]
[-- Type: text/x-patch, Size: 2441 bytes --]

From 7fa2e0f6a6b537c5ff9f5586af35f3221db75de5 Mon Sep 17 00:00:00 2001
From: Harry Ciao <qingtao.cao@windriver.com>
Date: Thu, 24 Feb 2011 13:10:13 +0800
Subject: [v1 PATCH 1/2] Compute SID for the newly created socket.

The security context for the newly created socket shares the same
user, role and MLS attribute as its creator but may have a different
type, which could be specified by a type_transition rule in the relevant
policy package.

Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
---
 security/selinux/hooks.c |   26 ++++++++++++++++++++------
 1 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c8d6992..f086541 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3644,9 +3644,15 @@ static int selinux_skb_peerlbl_sid(struct sk_buff *skb, u16 family, u32 *sid)
 
 /* socket security operations */
 
-static u32 socket_sockcreate_sid(const struct task_security_struct *tsec)
+static int socket_sockcreate_sid(const struct task_security_struct *tsec,
+				 u16 secclass, u32 *socksid)
 {
-	return tsec->sockcreate_sid ? : tsec->sid;
+	if (tsec->sockcreate_sid > SECSID_NULL) {
+		*socksid = tsec->sockcreate_sid;
+		return 0;
+	}
+
+	return security_transition_sid(tsec->sid, tsec->sid, secclass, socksid);
 }
 
 static int sock_has_perm(struct task_struct *task, struct sock *sk, u32 perms)
@@ -3670,12 +3676,16 @@ static int selinux_socket_create(int family, int type,
 	const struct task_security_struct *tsec = current_security();
 	u32 newsid;
 	u16 secclass;
+	int rc;
 
 	if (kern)
 		return 0;
 
-	newsid = socket_sockcreate_sid(tsec);
 	secclass = socket_type_to_security_class(family, type, protocol);
+	rc = socket_sockcreate_sid(tsec, secclass, &newsid);
+	if (rc)
+		return rc;
+
 	return avc_has_perm(tsec->sid, newsid, secclass, SOCKET__CREATE, NULL);
 }
 
@@ -3687,12 +3697,16 @@ static int selinux_socket_post_create(struct socket *sock, int family,
 	struct sk_security_struct *sksec;
 	int err = 0;
 
+	isec->sclass = socket_type_to_security_class(family, type, protocol);
+
 	if (kern)
 		isec->sid = SECINITSID_KERNEL;
-	else
-		isec->sid = socket_sockcreate_sid(tsec);
+	else {
+		err = socket_sockcreate_sid(tsec, isec->sclass, &(isec->sid));
+		if (err)
+			return err;
+	}
 
-	isec->sclass = socket_type_to_security_class(family, type, protocol);
 	isec->initialized = 1;
 
 	if (sock->sk) {
-- 
1.7.0.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: v1-0002-Socket-retains-creator-s-user-role-and-MLS-attribute.patch --]
[-- Type: text/x-patch, Size: 4241 bytes --]

From 2feb08d2c34cf61daebe56e9c9374059a9c832f2 Mon Sep 17 00:00:00 2001
From: Harry Ciao <qingtao.cao@windriver.com>
Date: Sun, 27 Feb 2011 16:12:04 +0800
Subject: [v1 PATCH 2/2] Socket retains creator's user, role and MLS attribute.

The socket SID would be computed on creation and no longer inherit
its creator's SID by default. Socket may have a different type but
needs to retain the creator's role and MLS attribute in order not
to break labeled networking and network access control.

Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
---
 security/selinux/ss/mls.c      |    5 ++-
 security/selinux/ss/mls.h      |    3 +-
 security/selinux/ss/services.c |   42 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 1ef8e4e..e961742 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -512,7 +512,8 @@ int mls_compute_sid(struct context *scontext,
 		    struct context *tcontext,
 		    u16 tclass,
 		    u32 specified,
-		    struct context *newcontext)
+		    struct context *newcontext,
+		    bool sock)
 {
 	struct range_trans rtr;
 	struct mls_range *r;
@@ -531,7 +532,7 @@ int mls_compute_sid(struct context *scontext,
 			return mls_range_set(newcontext, r);
 		/* Fallthrough */
 	case AVTAB_CHANGE:
-		if (tclass == policydb.process_class)
+		if ((tclass == policydb.process_class) || (sock == true))
 			/* Use the process MLS attributes. */
 			return mls_context_cpy(newcontext, scontext);
 		else
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index cd91526..037bf9d 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -49,7 +49,8 @@ int mls_compute_sid(struct context *scontext,
 		    struct context *tcontext,
 		    u16 tclass,
 		    u32 specified,
-		    struct context *newcontext);
+		    struct context *newcontext,
+		    bool sock);
 
 int mls_setup_user_range(struct context *fromcon, struct user_datum *user,
 			 struct context *usercon);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index a03cfaf..1adee26 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1343,6 +1343,42 @@ out:
 	return -EACCES;
 }
 
+static bool security_is_socket_class(u16 orig_tclass)
+{
+	bool sock = false;
+
+	switch (orig_tclass) {
+	case SECCLASS_SOCKET:
+	case SECCLASS_TCP_SOCKET:
+	case SECCLASS_UDP_SOCKET:
+	case SECCLASS_RAWIP_SOCKET:
+	case SECCLASS_NETLINK_SOCKET:
+	case SECCLASS_PACKET_SOCKET:
+	case SECCLASS_KEY_SOCKET:
+	case SECCLASS_UNIX_STREAM_SOCKET:
+	case SECCLASS_UNIX_DGRAM_SOCKET:
+	case SECCLASS_NETLINK_ROUTE_SOCKET:
+	case SECCLASS_NETLINK_FIREWALL_SOCKET:
+	case SECCLASS_NETLINK_TCPDIAG_SOCKET:
+	case SECCLASS_NETLINK_NFLOG_SOCKET:
+	case SECCLASS_NETLINK_XFRM_SOCKET:
+	case SECCLASS_NETLINK_SELINUX_SOCKET:
+	case SECCLASS_NETLINK_AUDIT_SOCKET:
+	case SECCLASS_NETLINK_IP6FW_SOCKET:
+	case SECCLASS_NETLINK_DNRT_SOCKET:
+	case SECCLASS_NETLINK_KOBJECT_UEVENT_SOCKET:
+	case SECCLASS_APPLETALK_SOCKET:
+	case SECCLASS_DCCP_SOCKET:
+	case SECCLASS_TUN_SOCKET:
+		sock = true;
+		break;
+	default:
+		break;
+	}
+
+	return sock;
+}
+
 static int security_compute_sid(u32 ssid,
 				u32 tsid,
 				u16 orig_tclass,
@@ -1357,6 +1393,7 @@ static int security_compute_sid(u32 ssid,
 	struct avtab_node *node;
 	u16 tclass;
 	int rc = 0;
+	bool sock = security_is_socket_class(orig_tclass);
 
 	if (!ss_initialized) {
 		switch (orig_tclass) {
@@ -1408,7 +1445,7 @@ static int security_compute_sid(u32 ssid,
 	}
 
 	/* Set the role and type to default values. */
-	if (tclass == policydb.process_class) {
+	if ((tclass == policydb.process_class) || (sock == true)) {
 		/* Use the current role and type of process. */
 		newcontext.role = scontext->role;
 		newcontext.type = scontext->type;
@@ -1460,7 +1497,8 @@ static int security_compute_sid(u32 ssid,
 
 	/* Set the MLS attributes.
 	   This is done last because it may allocate memory. */
-	rc = mls_compute_sid(scontext, tcontext, tclass, specified, &newcontext);
+	rc = mls_compute_sid(scontext, tcontext, tclass, specified,
+			     &newcontext, sock);
 	if (rc)
 		goto out_unlock;
 
-- 
1.7.0.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: Debug-separate-socket-SID.patch --]
[-- Type: text/x-patch, Size: 2419 bytes --]

From 045c025ed43ddb41a2f94aa6687ffd26a2e3529b Mon Sep 17 00:00:00 2001
From: Harry Ciao <qingtao.cao@windriver.com>
Date: Sun, 27 Feb 2011 16:23:18 +0800
Subject: [PATCH 1/1] Debug separate socket SID.

With the help of this debug patch we can see that the socket created
by syslogd_t shares the same user, role and MLS attribute but has a
separate type:

[root/sysadm_r/s0@~]# getenforce
Enforcing
[root/sysadm_r/s0@~]# dmesg | tail -n 13
Pid: 397, comm: syslogd Not tainted 2.6.34.8 #9
Call Trace:
 [<c1247042>] socket_sockcreate_sid+0x52/0xc0
 [<c1247143>] selinux_socket_post_create+0x93/0x180
 [<c1240010>] security_socket_post_create+0x20/0x30
 [<c143bca2>] __sock_create+0x252/0x3d0
 [<c1072234>] ? audit_syscall_exit+0x2d4/0x300
 [<c143be87>] sock_create+0x37/0x50
 [<c143c0c6>] sys_socket+0x56/0xe0
 [<c143d27f>] sys_socketcall+0x8f/0x2f0
 [<c1525391>] system_call_done+0x0/0x4
tsec->sid: a9, context: system_u:system_r:syslogd_t:s15:c0.c1023
socksid: ad, context: system_u:system_r:syslogd_s_t:s15:c0.c1023
[root/sysadm_r/s0@~]#

Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
---
 security/selinux/hooks.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f086541..8527a57 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3644,15 +3644,36 @@ static int selinux_skb_peerlbl_sid(struct sk_buff *skb, u16 family, u32 *sid)
 
 /* socket security operations */
 
+void dump_stack(void);
+
 static int socket_sockcreate_sid(const struct task_security_struct *tsec,
 				 u16 secclass, u32 *socksid)
 {
+	int rc;
+	char *scontext;
+	u32 len;
+
 	if (tsec->sockcreate_sid > SECSID_NULL) {
 		*socksid = tsec->sockcreate_sid;
 		return 0;
 	}
 
-	return security_transition_sid(tsec->sid, tsec->sid, secclass, socksid);
+	rc = security_transition_sid(tsec->sid, tsec->sid, secclass, socksid);
+
+	if (rc == 0 && *socksid != tsec->sid) {
+		dump_stack();
+		security_sid_to_context(tsec->sid, &scontext, &len);
+		printk(KERN_INFO "tsec->sid: %x, context: %s\n",
+				  tsec->sid, scontext);
+		kfree(scontext);
+
+		security_sid_to_context(*socksid, &scontext, &len);
+		printk(KERN_INFO "socksid: %x, context: %s\n",
+				 *socksid, scontext);
+		kfree(scontext);
+	}
+
+	return rc;
 }
 
 static int sock_has_perm(struct task_struct *task, struct sock *sk, u32 perms)
-- 
1.7.0.4


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: Specify-a-separate-socket-type-for-syslogd_t.patch --]
[-- Type: text/x-patch, Size: 3663 bytes --]

From 44dd4c1ef2042ba6d5eb014b6c9826a07474953e Mon Sep 17 00:00:00 2001
From: Harry Ciao <qingtao.cao@windriver.com>
Date: Thu, 24 Feb 2011 16:23:42 +0800
Subject: [PATCH 1/1] Specify a separate socket type for syslogd_t.

Use a type_transition rule to specify a separate type for unix_dgram_socket
object created by syslogd_t, so that the socket type alone could be added
to the mlstrustedobject attrbute to avoid below error message:

type=1400 audit(1298535101.654:868): avc:  denied  { sendto } for  pid=385 comm="klogd" path="/dev/log" scontext=system_u:object_r:klogd_t:s0 tcontext=system_u:object_r:syslogd_t:s15:c0.c1023 tclass=unix_dgram_socket

This helps to avoid adding syslogd_t to this attribute which also is the
label for all syslogd's procfs contents.

BTW, in SELinux kernel driver the security_transition_sid() should be
called to query above type_transition rule for the newly created socket,
which will retain the same user, role and MLS attribute as its creator.

Signed-off-by: Harry Ciao <qingtao.cao@windriver.com>
---
 policy/modules/system/logging.if |    4 ++--
 policy/modules/system/logging.te |   14 ++++++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/policy/modules/system/logging.if b/policy/modules/system/logging.if
index c7cfb62..92582b0 100644
--- a/policy/modules/system/logging.if
+++ b/policy/modules/system/logging.if
@@ -525,14 +525,14 @@ interface(`logging_log_filetrans',`
 #
 interface(`logging_send_syslog_msg',`
 	gen_require(`
-		type syslogd_t, devlog_t;
+		type syslogd_t, syslogd_s_t, devlog_t;
 	')
 
 	allow $1 devlog_t:lnk_file read_lnk_file_perms;
 	allow $1 devlog_t:sock_file write_sock_file_perms;
 
 	# the type of socket depends on the syslog daemon
-	allow $1 syslogd_t:unix_dgram_socket sendto;
+	allow $1 syslogd_s_t:unix_dgram_socket sendto;
 	allow $1 syslogd_t:unix_stream_socket connectto;
 	allow $1 self:unix_dgram_socket create_socket_perms;
 	allow $1 self:unix_stream_socket create_socket_perms;
diff --git a/policy/modules/system/logging.te b/policy/modules/system/logging.te
index 9b5a9ed..5bdedcb 100644
--- a/policy/modules/system/logging.te
+++ b/policy/modules/system/logging.te
@@ -61,6 +61,13 @@ type syslogd_t;
 type syslogd_exec_t;
 init_daemon_domain(syslogd_t, syslogd_exec_t)
 
+# PF_UNIX socket created by syslogd.
+# Any socket will retain the same user, role and MLS attribute as
+# its creator, thus the creator's role needs to type the socket type.
+type syslogd_s_t;
+role system_r types syslogd_s_t;
+mls_trusted_object(syslogd_s_t)
+
 type syslogd_initrc_exec_t;
 init_script_file(syslogd_initrc_exec_t)
 
@@ -344,15 +351,18 @@ dontaudit syslogd_t self:capability sys_tty_config;
 # setrlimit for syslog-ng
 allow syslogd_t self:process { signal_perms setpgid setrlimit };
 # receive messages to be logged
-allow syslogd_t self:unix_dgram_socket create_socket_perms;
+allow syslogd_t syslogd_s_t:unix_dgram_socket create_socket_perms;
 allow syslogd_t self:unix_stream_socket create_stream_socket_perms;
-allow syslogd_t self:unix_dgram_socket sendto;
+allow syslogd_t syslogd_s_t:unix_dgram_socket sendto;
 allow syslogd_t self:fifo_file rw_fifo_file_perms;
 allow syslogd_t self:udp_socket create_socket_perms;
 allow syslogd_t self:tcp_socket create_stream_socket_perms;
 
 allow syslogd_t syslog_conf_t:file read_file_perms;
 
+# PF_UNIX dgram socket created by syslogd_t labeled as syslogd_s_t
+type_transition syslogd_t syslogd_t:unix_dgram_socket syslogd_s_t;
+
 # Create and bind to /dev/log or /var/run/log.
 allow syslogd_t devlog_t:sock_file manage_sock_file_perms;
 files_pid_filetrans(syslogd_t, devlog_t, sock_file)
-- 
1.7.0.4

Re: Separate socket SID than its creator

[Stephen Smalley]

On Sun, 2011-02-27 at 10:14 +0000, HarryCiao wrote:
> Hello Stephen, James and Eric,
> 
> The attached is the v1 patches to compute socket SID on creation. 
> 
> The propagation of the return code of security_transition_sid is a
> good practice and actually helps identifying the problem that the
> socket creator's role also needs to be able to type the socket role. 
> 
> The class number(orig_tclass) used by SELinux kernel driver is checked
> against SECCLASS_XXX macros and if it's any one of those 22 socket
> classes both security_compute_sid and mls_comput_sid will preserve the
> scontext's role and MLS attribute as what they have been done for
> policydb.policy_class. (we are not checking access vectors so we don't
> have to concern about the unmapped, real class number allocated by
> checkpolicy)
> 
> I've also come up with a debug patch to make use of the
> security_sid_to_context which helps to reveal that the
> unix_dgram_socket object created by syslogd differs only in type from
> its creator and shares the same user, role and MLS at! tribute. The
> relevant changes to logging.pp used for debugging is also attached.
> 
> Please kindly help commenting and suggesting how I could further test
> these two patches.

Kernel patches need to be posted one patch per message, inline, as per
SubmittingPatches.  http://userweb.kernel.org/~akpm/stuff/tpp.txt is
also a good resource on preparing kernel patches.

The order of your patches needs to be reversed so that there is no
change in behavior and everything still works after each patch is
applied - this is important for git bisect.

I'm a bit concerned about the maintainability of the socket class list
in security_is_socket_class().  We need to do something to ensure that
if someone later adds a new socket security class to the kernel, they
know to update this list.  At least a comment in classmap.h; ideally
some kind of compile-time check (similar to the #error case in hooks.c
to catch when someone adds more than 64 capabilities) to detect when
someone adds a socket class without updating the list there.  Or maybe
security_is_socket_class() needs to be auto-generated from classmap.h by
genheaders?

Since you are checking against the kernel values rather than the policy
values, you need to make sure you only apply this check when called from
the kernel (if kern == true).  

-- 
Stephen Smalley
National Security Agency


--
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: Separate socket SID than its creator

[HarryCiao]

[-- Attachment #1: Type: text/plain, Size: 3888 bytes --]


Hello Stephen,

> Subject: Re: Separate socket SID than its creator
> From: sds@tycho.nsa.gov
> To: harrytaurus2002@hotmail.com
> CC: jmorris@namei.org; eparis@parisplace.org; selinux@tycho.nsa.gov
> Date: Mon, 28 Feb 2011 08:52:38 -0500
> 
> On Sun, 2011-02-27 at 10:14 +0000, HarryCiao wrote:
> > Hello Stephen, James and Eric,
> > 
> > The attached is the v1 patches to compute socket SID on creation. 
> > 
> > The propagation of the return code of security_transition_sid is a
> > good practice and actually helps identifying the problem that the
> > socket creator's role also needs to be able to type the socket role. 
> > 
> > The class number(orig_tclass) used by SELinux kernel driver is checked
> > against SECCLASS_XXX macros and if it's any one of those 22 socket
> > classes both security_compute_sid and mls_comput_sid will preserve the
> > scontext's role and MLS attribute as what they have been done for
> > policydb.policy_class. (we are not checking access vectors so we don't
> > have to concern about the unmapped, real class number allocated by
> > checkpolicy)
> > 
> > I've also come up with a debug patch to make use of the
> > security_sid_to_context which helps to reveal that the
> > unix_dgram_socket object created by syslogd differs only in type from
> > its creator and shares the same user, role and MLS at! tribute. The
> > relevant changes to logging.pp used for debugging is also attached.
> > 
> > Please kindly help commenting and suggesting how I could further test
> > these two patches.
> 
> Kernel patches need to be posted one patch per message, inline, as per
> SubmittingPatches.  http://userweb.kernel.org/~akpm/stuff/tpp.txt is
> also a good resource on preparing kernel patches.
> 

Ok, I will try to send patches to selinux mailing list by git-send-emails with one patch per message inline.

> The order of your patches needs to be reversed so that there is no
> change in behavior and everything still works after each patch is
> applied - this is important for git bisect.

Understand, will do.

> 
> I'm a bit concerned about the maintainability of the socket class list
> in security_is_socket_class().  We need to do something to ensure that
> if someone later adds a new socket security class to the kernel, they
> know to update this list.  At least a comment in classmap.h; ideally
> some kind of compile-time check (similar to the #error case in hooks.c
> to catch when someone adds more than 64 capabilities) to detect when
> someone adds a socket class without updating the list there.  Or maybe
> security_is_socket_class() needs to be auto-generated from classmap.h by
> genheaders?
> 

Good idea!

Actually it won't take much effort to have the genheaders auto-generate the security_is_socket_class() function by traversing the secclass_map[] array and collecting all socket classes' name into the 'case' branch. I will send it out in v2 patches.


> Since you are checking against the kernel values rather than the policy
> values, you need to make sure you only apply this check when called from
> the kernel (if kern == true).  
> 

Yeah, since the security_is_socket_class() function checks class kernel value against SECCLASS_XXX macros we should also mapping the class policy value to the relevant kernel value when the security_compue_sid() is called with kern == false, say by compute_create program, which should compute the same SID for socket as in kernel. For this purpose the index for an current_mapping[] entry should be returned if its value is the same as the class policy value.

Thanks!

Best regards,
Harry


> -- 
> Stephen Smalley
> National Security Agency
> 
> 
> --
> 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.
 		 	   		  

[-- Attachment #2: Type: text/html, Size: 4604 bytes --]