[PATCH 0/2] Dump the sock's security context

[PATCH 0/2] Dump the sock's security context

[rongqing.li]

-------
    Any review would be much appreciated.
 
Comments:
--------
    Add a netlink attribute INET_DIAG_SECCTX
    
    Add a new netlink attribute INET_DIAG_SECCTX to dump the security
    context of TCP sockets.
    
    The element sk_security of struct sock represents the socket
    security context ID, which is inherited from the parent process
    when the socket is created.
    
    but when SELinux type_transition rule is applied to socket, or
    application sets /proc/xxx/attr/createsock, the socket security
    context would be different from the creating process. For these
    conditions, the "netstat -Z" would return wrong value, since
    "netstat -Z" only returns the process security context as socket
    process security.


The application to verify the netlink new attribute.
------
See attached file

test:
--------
1. Enable SELinux when compile and startup .
	root@qemu-host:/root> ./printsocketsec
	 inode:7141 system_u:system_r:rpcbind_t:s0 
	 inode:7136 system_u:system_r:rpcbind_t:s0 
	 inode:7604 system_u:system_r:initrc_t:s0 
	 inode:7227 system_u:system_r:rpcd_t:s0 
	 inode:7471 system_u:system_r:sshd_t:s0-s0:c0.c1023 
	 inode:7469 system_u:system_r:sshd_t:s0-s0:c0.c1023 
	 inode:7552 system_u:system_r:sendmail_t:s0 
	 inode:7348 system_u:system_r:initrc_t:s0 
	 inode:7553 system_u:system_r:sendmail_t:s0 
	root@qemu-host:/root> 

2. Disable SELinux when startup.
	root@qemu-host:/root> ./printsocketsec 
	inode:3221 
	inode:2942 
	inode:2861 
	inode:3256 
	inode:3156 
	inode:3220 
	inode:3060
	root@qemu-host:/root>

3. Disable SELinux when compile and startup
	root@qemu-host:/root> ./printsocketsec 
	inode:3221 
	inode:2942 
	inode:2861 
	inode:3256 
	inode:3156 
	inode:3220 
	inode:3060
	root@qemu-host:/root>

[PATCH 1/2] Define security_sk_getsecctx

[rongqing.li]

From: Roy.Li <rongqing.li@windriver.com>

Define security_sk_getsecctx to return the security
context of a sock.

Signed-off-by: Roy.Li <rongqing.li@windriver.com>
---
 include/linux/security.h |   13 +++++++++++++
 security/capability.c    |    6 ++++++
 security/security.c      |    6 ++++++
 security/selinux/hooks.c |    9 +++++++++
 4 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index ebd2a53..6bb8e0c 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -959,6 +959,12 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  * @sk_getsecid:
  *	Retrieve the LSM-specific secid for the sock to enable caching of network
  *	authorizations.
+ * @sk_getsecctx:
+ *	Returns a string containing sock security context information
+ *	@sk whom we wish to get the security context.
+ *	@ctx is the address of the pointer to where to place the allocated
+ *	security context.
+ *	@ctxlen points to the value of the length of the security context.
  * @sock_graft:
  *	Sets the socket's isec sid to the sock's sid.
  * @inet_conn_request:
@@ -1600,6 +1606,7 @@ struct security_operations {
 	void (*sk_free_security) (struct sock *sk);
 	void (*sk_clone_security) (const struct sock *sk, struct sock *newsk);
 	void (*sk_getsecid) (struct sock *sk, u32 *secid);
+	int (*sk_getsecctx) (struct sock *sk, void **ctx, u32 *ctxlen);
 	void (*sock_graft) (struct sock *sk, struct socket *parent);
 	int (*inet_conn_request) (struct sock *sk, struct sk_buff *skb,
 				  struct request_sock *req);
@@ -2574,6 +2581,7 @@ void security_secmark_refcount_dec(void);
 int security_tun_dev_create(void);
 void security_tun_dev_post_create(struct sock *sk);
 int security_tun_dev_attach(struct sock *sk);
+int security_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen);
 
 #else	/* CONFIG_SECURITY_NETWORK */
 static inline int security_unix_stream_connect(struct sock *sock,
@@ -2751,6 +2759,11 @@ static inline int security_tun_dev_attach(struct sock *sk)
 {
 	return 0;
 }
+
+static int security_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen)
+{
+	return -EOPNOTSUPP;
+}
 #endif	/* CONFIG_SECURITY_NETWORK */
 
 #ifdef CONFIG_SECURITY_NETWORK_XFRM
diff --git a/security/capability.c b/security/capability.c
index 2984ea4..89256a6 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -664,6 +664,11 @@ static void cap_sk_getsecid(struct sock *sk, u32 *secid)
 {
 }
 
+static int cap_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen)
+{
+	return 0;
+}
+
 static void cap_sock_graft(struct sock *sk, struct socket *parent)
 {
 }
@@ -1032,6 +1037,7 @@ void __init security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, sk_free_security);
 	set_to_cap_if_null(ops, sk_clone_security);
 	set_to_cap_if_null(ops, sk_getsecid);
+	set_to_cap_if_null(ops, sk_getsecctx);
 	set_to_cap_if_null(ops, sock_graft);
 	set_to_cap_if_null(ops, inet_conn_request);
 	set_to_cap_if_null(ops, inet_csk_clone);
diff --git a/security/security.c b/security/security.c
index 0e4fccf..a939f5c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -757,6 +757,12 @@ void security_task_getsecid(struct task_struct *p, u32 *secid)
 }
 EXPORT_SYMBOL(security_task_getsecid);
 
+int security_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen)
+{
+	return security_ops->sk_getsecctx(sk, ctx, ctxlen);
+}
+EXPORT_SYMBOL(security_sk_getsecctx);
+
 int security_task_setnice(struct task_struct *p, int nice)
 {
 	return security_ops->task_setnice(p, nice);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 266a229..6e96f01 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4284,6 +4284,14 @@ static void selinux_sk_getsecid(struct sock *sk, u32 *secid)
 	}
 }
 
+static int selinux_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen)
+{
+	u32 secid;
+
+	selinux_sk_getsecid(sk, &secid);
+	return security_sid_to_context(secid, ctx, ctxlen);
+}
+
 static void selinux_sock_graft(struct sock *sk, struct socket *parent)
 {
 	struct inode_security_struct *isec = SOCK_INODE(parent)->i_security;
@@ -5613,6 +5621,7 @@ static struct security_operations selinux_ops = {
 	.sk_free_security =		selinux_sk_free_security,
 	.sk_clone_security =		selinux_sk_clone_security,
 	.sk_getsecid =			selinux_sk_getsecid,
+	.sk_getsecctx =                 selinux_sk_getsecctx,
 	.sock_graft =			selinux_sock_graft,
 	.inet_conn_request =		selinux_inet_conn_request,
 	.inet_csk_clone =		selinux_inet_csk_clone,
-- 
1.7.1

Re: [PATCH 1/2] Define security_sk_getsecctx

[Casey Schaufler]

On 8/31/2011 1:36 AM, rongqing.li@windriver.com wrote:
> From: Roy.Li <rongqing.li@windriver.com>
>
> Define security_sk_getsecctx to return the security
> context of a sock.

So, what is the intended use of the information
coming from this hook? If I wanted to write the
Smack hook, which of the "contexts" would I want
to return? There are potentially three. If I know
what the caller is looking for, I can (hopefully)
select the correct information.

> Signed-off-by: Roy.Li <rongqing.li@windriver.com>
> ---
>  include/linux/security.h |   13 +++++++++++++
>  security/capability.c    |    6 ++++++
>  security/security.c      |    6 ++++++
>  security/selinux/hooks.c |    9 +++++++++
>  4 files changed, 34 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ebd2a53..6bb8e0c 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -959,6 +959,12 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   * @sk_getsecid:
>   *	Retrieve the LSM-specific secid for the sock to enable caching of network
>   *	authorizations.
> + * @sk_getsecctx:
> + *	Returns a string containing sock security context information
> + *	@sk whom we wish to get the security context.
> + *	@ctx is the address of the pointer to where to place the allocated
> + *	security context.
> + *	@ctxlen points to the value of the length of the security context.
>   * @sock_graft:
>   *	Sets the socket's isec sid to the sock's sid.
>   * @inet_conn_request:
> @@ -1600,6 +1606,7 @@ struct security_operations {
>  	void (*sk_free_security) (struct sock *sk);
>  	void (*sk_clone_security) (const struct sock *sk, struct sock *newsk);
>  	void (*sk_getsecid) (struct sock *sk, u32 *secid);
> +	int (*sk_getsecctx) (struct sock *sk, void **ctx, u32 *ctxlen);
>  	void (*sock_graft) (struct sock *sk, struct socket *parent);
>  	int (*inet_conn_request) (struct sock *sk, struct sk_buff *skb,
>  				  struct request_sock *req);
> @@ -2574,6 +2581,7 @@ void security_secmark_refcount_dec(void);
>  int security_tun_dev_create(void);
>  void security_tun_dev_post_create(struct sock *sk);
>  int security_tun_dev_attach(struct sock *sk);
> +int security_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen);
>  
>  #else	/* CONFIG_SECURITY_NETWORK */
>  static inline int security_unix_stream_connect(struct sock *sock,
> @@ -2751,6 +2759,11 @@ static inline int security_tun_dev_attach(struct sock *sk)
>  {
>  	return 0;
>  }
> +
> +static int security_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen)
> +{
> +	return -EOPNOTSUPP;
> +}
>  #endif	/* CONFIG_SECURITY_NETWORK */
>  
>  #ifdef CONFIG_SECURITY_NETWORK_XFRM
> diff --git a/security/capability.c b/security/capability.c
> index 2984ea4..89256a6 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -664,6 +664,11 @@ static void cap_sk_getsecid(struct sock *sk, u32 *secid)
>  {
>  }
>  
> +static int cap_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen)
> +{
> +	return 0;
> +}
> +
>  static void cap_sock_graft(struct sock *sk, struct socket *parent)
>  {
>  }
> @@ -1032,6 +1037,7 @@ void __init security_fixup_ops(struct security_operations *ops)
>  	set_to_cap_if_null(ops, sk_free_security);
>  	set_to_cap_if_null(ops, sk_clone_security);
>  	set_to_cap_if_null(ops, sk_getsecid);
> +	set_to_cap_if_null(ops, sk_getsecctx);
>  	set_to_cap_if_null(ops, sock_graft);
>  	set_to_cap_if_null(ops, inet_conn_request);
>  	set_to_cap_if_null(ops, inet_csk_clone);
> diff --git a/security/security.c b/security/security.c
> index 0e4fccf..a939f5c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -757,6 +757,12 @@ void security_task_getsecid(struct task_struct *p, u32 *secid)
>  }
>  EXPORT_SYMBOL(security_task_getsecid);
>  
> +int security_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen)
> +{
> +	return security_ops->sk_getsecctx(sk, ctx, ctxlen);
> +}
> +EXPORT_SYMBOL(security_sk_getsecctx);
> +
>  int security_task_setnice(struct task_struct *p, int nice)
>  {
>  	return security_ops->task_setnice(p, nice);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 266a229..6e96f01 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4284,6 +4284,14 @@ static void selinux_sk_getsecid(struct sock *sk, u32 *secid)
>  	}
>  }
>  
> +static int selinux_sk_getsecctx(struct sock *sk, void **ctx, u32 *ctxlen)
> +{
> +	u32 secid;
> +
> +	selinux_sk_getsecid(sk, &secid);
> +	return security_sid_to_context(secid, ctx, ctxlen);
> +}
> +
>  static void selinux_sock_graft(struct sock *sk, struct socket *parent)
>  {
>  	struct inode_security_struct *isec = SOCK_INODE(parent)->i_security;
> @@ -5613,6 +5621,7 @@ static struct security_operations selinux_ops = {
>  	.sk_free_security =		selinux_sk_free_security,
>  	.sk_clone_security =		selinux_sk_clone_security,
>  	.sk_getsecid =			selinux_sk_getsecid,
> +	.sk_getsecctx =                 selinux_sk_getsecctx,
>  	.sock_graft =			selinux_sock_graft,
>  	.inet_conn_request =		selinux_inet_conn_request,
>  	.inet_csk_clone =		selinux_inet_csk_clone,

Re: [PATCH 1/2] Define security_sk_getsecctx

[Stephen Smalley]

On Wed, 2011-08-31 at 08:43 -0700, Casey Schaufler wrote:
> On 8/31/2011 1:36 AM, rongqing.li@windriver.com wrote:
> > From: Roy.Li <rongqing.li@windriver.com>
> >
> > Define security_sk_getsecctx to return the security
> > context of a sock.
> 
> So, what is the intended use of the information
> coming from this hook? If I wanted to write the
> Smack hook, which of the "contexts" would I want
> to return? There are potentially three. If I know
> what the caller is looking for, I can (hopefully)
> select the correct information.

The initial use case is for netstat -Z so that it can reliably show the
security context of the socket rather than inferring it from the owning
process, which can be inaccurate for security-aware applications.

In your situation, when in != out, which would you rather see in netstat
-Z output?  Alternatively, if you want them both, perhaps you could
combine in and out into a single string that is returned, similar to
what you proposed for handling multiple xattrs with inode_getsecctx()?

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 1/2] Define security_sk_getsecctx

[Casey Schaufler]

On 8/31/2011 11:46 AM, Stephen Smalley wrote:
> On Wed, 2011-08-31 at 08:43 -0700, Casey Schaufler wrote:
>> On 8/31/2011 1:36 AM, rongqing.li@windriver.com wrote:
>>> From: Roy.Li <rongqing.li@windriver.com>
>>>
>>> Define security_sk_getsecctx to return the security
>>> context of a sock.
>> So, what is the intended use of the information
>> coming from this hook? If I wanted to write the
>> Smack hook, which of the "contexts" would I want
>> to return? There are potentially three. If I know
>> what the caller is looking for, I can (hopefully)
>> select the correct information.
> The initial use case is for netstat -Z so that it can reliably show the
> security context of the socket rather than inferring it from the owning
> process, which can be inaccurate for security-aware applications.
>
> In your situation, when in != out, which would you rather see in netstat
> -Z output?  Alternatively, if you want them both, perhaps you could
> combine in and out into a single string that is returned, similar to
> what you proposed for handling multiple xattrs with inode_getsecctx()?
>

If we want to use secctx consistently within the kernel, and
I personally think that is a good idea, I would have to chose
the SMACK64IPIN (label checked on packet delivery) value. Putting
both the SMACK64IPOUT and SMACK64IPIN "contexts" into the secctx
would violate the architectural notion that a secctx is the
textual representation of a value used to make access control
decisions. It would mean that calling security_secctx_to_secid()
with the value returned by security_sk_getsecctx would be invalid.
Note that this is different from the mechanism I had suggested
for handling secctx in the multiple LSM case, as the composed
string would map to a single secid which would in turn map back
to that same composed string. If, on the other hand, what netstat -Z
is out to show is all of the LSM base information about the socket
a compound string might make sense, it just would not be a
secctx, it would be an informational string of some other flavor.

[PATCH 2/2] Add a netlink attribute INET_DIAG_SECCTX

[rongqing.li]

From: Roy.Li <rongqing.li@windriver.com>

Add a new netlink attribute INET_DIAG_SECCTX to dump the security
context of TCP sockets.

The element sk_security of struct sock represents the socket
security context ID, which is inherited from the parent process
when the socket is created.

but when SELinux type_transition rule is applied to socket, or
application sets /proc/xxx/attr/createsock, the socket security
context would be different from the creating process. For these
conditions, the "netstat -Z" will return wrong value, since
"netstat -Z" only returns the process security context as socket
process security.

Signed-off-by: Roy.Li <rongqing.li@windriver.com>
---
 include/linux/inet_diag.h |    3 ++-
 net/ipv4/inet_diag.c      |   38 +++++++++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/include/linux/inet_diag.h b/include/linux/inet_diag.h
index bc8c490..00382b4 100644
--- a/include/linux/inet_diag.h
+++ b/include/linux/inet_diag.h
@@ -97,9 +97,10 @@ enum {
 	INET_DIAG_INFO,
 	INET_DIAG_VEGASINFO,
 	INET_DIAG_CONG,
+	INET_DIAG_SECCTX,
 };
 
-#define INET_DIAG_MAX INET_DIAG_CONG
+#define INET_DIAG_MAX INET_DIAG_SECCTX
 
 
 /* INET_DIAG_MEM */
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 389a2e6..1faf752 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -34,6 +34,8 @@
 
 #include <linux/inet_diag.h>
 
+#define MAX_SECCTX_LEN 128
+
 static const struct inet_diag_handler **inet_diag_table;
 
 struct inet_diag_entry {
@@ -108,6 +110,25 @@ static int inet_csk_diag_fill(struct sock *sk,
 		       icsk->icsk_ca_ops->name);
 	}
 
+	if (ext & (1 << (INET_DIAG_SECCTX - 1))) {
+		u32 ctxlen = 0;
+		void *secctx;
+		int error;
+
+		error = security_sk_getsecctx(sk, &secctx, &ctxlen);
+
+		if (!error && ctxlen) {
+			if (ctxlen < MAX_SECCTX_LEN) {
+				strcpy(INET_DIAG_PUT(skb, INET_DIAG_SECCTX,
+					ctxlen + 1), secctx);
+			} else {
+				strcpy(INET_DIAG_PUT(skb, INET_DIAG_SECCTX,
+					2), "-");
+			}
+			security_release_secctx(secctx, ctxlen);
+		}
+	}
+
 	r->idiag_family = sk->sk_family;
 	r->idiag_state = sk->sk_state;
 	r->idiag_timer = 0;
@@ -246,7 +267,7 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff *skb,
 static int inet_diag_get_exact(struct sk_buff *in_skb,
 			       const struct nlmsghdr *nlh)
 {
-	int err;
+	int err, len;
 	struct sock *sk;
 	struct inet_diag_req *req = NLMSG_DATA(nlh);
 	struct sk_buff *rep;
@@ -293,10 +314,17 @@ static int inet_diag_get_exact(struct sk_buff *in_skb,
 		goto out;
 
 	err = -ENOMEM;
-	rep = alloc_skb(NLMSG_SPACE((sizeof(struct inet_diag_msg) +
-				     sizeof(struct inet_diag_meminfo) +
-				     handler->idiag_info_size + 64)),
-			GFP_KERNEL);
+	len = sizeof(struct inet_diag_msg) + 64;
+
+	len += (req->idiag_ext & (1 << (INET_DIAG_MEMINFO - 1))) ?
+		sizeof(struct inet_diag_meminfo) : 0;
+	len += (req->idiag_ext & (1 << (INET_DIAG_INFO - 1))) ?
+		handler->idiag_info_size : 0;
+	len += (req->idiag_ext & (1 << (INET_DIAG_SECCTX - 1))) ?
+		MAX_SECCTX_LEN : 0;
+
+	rep = alloc_skb(NLMSG_SPACE(len), GFP_KERNEL);
+
 	if (!rep)
 		goto out;
 
-- 
1.7.1

Re: [PATCH 2/2] Add a netlink attribute INET_DIAG_SECCTX

[Stephen Smalley]

On Wed, 2011-08-31 at 16:36 +0800, rongqing.li@windriver.com wrote:
> From: Roy.Li <rongqing.li@windriver.com>
> 
> Add a new netlink attribute INET_DIAG_SECCTX to dump the security
> context of TCP sockets.
> 
> The element sk_security of struct sock represents the socket
> security context ID, which is inherited from the parent process
> when the socket is created.
> 
> but when SELinux type_transition rule is applied to socket, or
> application sets /proc/xxx/attr/createsock, the socket security
> context would be different from the creating process. For these
> conditions, the "netstat -Z" will return wrong value, since
> "netstat -Z" only returns the process security context as socket
> process security.
> 
> Signed-off-by: Roy.Li <rongqing.li@windriver.com>
> ---
>  include/linux/inet_diag.h |    3 ++-
>  net/ipv4/inet_diag.c      |   38 +++++++++++++++++++++++++++++++++-----
>  2 files changed, 35 insertions(+), 6 deletions(-)

> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> index 389a2e6..1faf752 100644
> --- a/net/ipv4/inet_diag.c
> +++ b/net/ipv4/inet_diag.c
> @@ -34,6 +34,8 @@
>  
>  #include <linux/inet_diag.h>
>  
> +#define MAX_SECCTX_LEN 128

We don't impose such a (low) limit on other interfaces for reporting
security contexts.  Can you just size the buffer appropriately for the
actual secctx length?

-- 
Stephen Smalley
National Security Agency

Re: [PATCH 2/2] Add a netlink attribute INET_DIAG_SECCTX

[Paul Moore]

On Wednesday, August 31, 2011 04:36:17 PM rongqing.li@windriver.com wrote:
> From: Roy.Li <rongqing.li@windriver.com>
> 
> Add a new netlink attribute INET_DIAG_SECCTX to dump the security
> context of TCP sockets.

You'll have to forgive me, I'm not familiar with the netlink code used by 
netstat and friends, but is there anyway to report back the security context 
of UDP sockets?  Or does the code below handle that already?

In general, AF_INET and AF_INET6 sockets, regardless of any upper level 
protocols, have security contexts associated with them and it would be nice to 
see them in netstat.

> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> index 389a2e6..1faf752 100644
> --- a/net/ipv4/inet_diag.c
> +++ b/net/ipv4/inet_diag.c
> @@ -34,6 +34,8 @@
> 
>  #include <linux/inet_diag.h>
> 
> +#define MAX_SECCTX_LEN 128

I'll echo Stephen's concerns that this is too small.  A MCS/MLS system with a 
moderate number of categories could bump into this limit without too much 
difficulty.

>  struct inet_diag_entry {
> @@ -108,6 +110,25 @@ static int inet_csk_diag_fill(struct sock *sk,
>  		       icsk->icsk_ca_ops->name);
>  	}
> 
> +	if (ext & (1 << (INET_DIAG_SECCTX - 1))) {
> +		u32 ctxlen = 0;
> +		void *secctx;
> +		int error;
> +
> +		error = security_sk_getsecctx(sk, &secctx, &ctxlen);
> +
> +		if (!error && ctxlen) {
> +			if (ctxlen < MAX_SECCTX_LEN) {
> +				strcpy(INET_DIAG_PUT(skb, INET_DIAG_SECCTX,
> +					ctxlen + 1), secctx);
> +			} else {
> +				strcpy(INET_DIAG_PUT(skb, INET_DIAG_SECCTX,
> +					2), "-");

Is the "-" string a special value already interpreted by the userspace tools?  
If not, you might consider using a string that would indicate an out-of-space 
condition occurred; at first glance I thought the "-" string indicated no 
context.

> +			}
> +			security_release_secctx(secctx, ctxlen);
> +		}
> +	}
> +
>  	r->idiag_family = sk->sk_family;
>  	r->idiag_state = sk->sk_state;
>  	r->idiag_timer = 0;
> @@ -246,7 +267,7 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff
> *skb, static int inet_diag_get_exact(struct sk_buff *in_skb,
>  			       const struct nlmsghdr *nlh)
>  {
> -	int err;
> +	int err, len;
>  	struct sock *sk;
>  	struct inet_diag_req *req = NLMSG_DATA(nlh);
>  	struct sk_buff *rep;
> @@ -293,10 +314,17 @@ static int inet_diag_get_exact(struct sk_buff *in_skb,
> goto out;
> 
>  	err = -ENOMEM;
> -	rep = alloc_skb(NLMSG_SPACE((sizeof(struct inet_diag_msg) +
> -				     sizeof(struct inet_diag_meminfo) +
> -				     handler->idiag_info_size + 64)),
> -			GFP_KERNEL);
> +	len = sizeof(struct inet_diag_msg) + 64;
> +
> +	len += (req->idiag_ext & (1 << (INET_DIAG_MEMINFO - 1))) ?
> +		sizeof(struct inet_diag_meminfo) : 0;
> +	len += (req->idiag_ext & (1 << (INET_DIAG_INFO - 1))) ?
> +		handler->idiag_info_size : 0;
> +	len += (req->idiag_ext & (1 << (INET_DIAG_SECCTX - 1))) ?
> +		MAX_SECCTX_LEN : 0;
> +
> +	rep = alloc_skb(NLMSG_SPACE(len), GFP_KERNEL);

How much of a problem would it be if you just allocated an entire page (or 4k 
in the case of huge pages) and used that?  Is memory usage a concern here?

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2] Add a netlink attribute INET_DIAG_SECCTX

[Rongqing Li]

On 09/01/2011 05:18 AM, Paul Moore wrote:
> On Wednesday, August 31, 2011 04:36:17 PM rongqing.li@windriver.com wrote:
>> From: Roy.Li<rongqing.li@windriver.com>
>>
>> Add a new netlink attribute INET_DIAG_SECCTX to dump the security
>> context of TCP sockets.
>
> You'll have to forgive me, I'm not familiar with the netlink code used by
> netstat and friends, but is there anyway to report back the security context
> of UDP sockets?  Or does the code below handle that already?
>
> In general, AF_INET and AF_INET6 sockets, regardless of any upper level
> protocols, have security contexts associated with them and it would be nice to
> see them in netstat.
>

Yes, this is real concern, If the dumping tcp security context can be 
accepted
by netdev, I am planning to implement it for ipv4 udp socket, unix socket.
then ipv6..

>> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
>> index 389a2e6..1faf752 100644
>> --- a/net/ipv4/inet_diag.c
>> +++ b/net/ipv4/inet_diag.c
>> @@ -34,6 +34,8 @@
>>
>>   #include<linux/inet_diag.h>
>>
>> +#define MAX_SECCTX_LEN 128
>
> I'll echo Stephen's concerns that this is too small.  A MCS/MLS system with a
> moderate number of categories could bump into this limit without too much
> difficulty.
>

I will reconsider this as Stephen's suggestion, just size the buffer 
appropriately
for the actual secctx length, so that, your next question will be fixed 
since we
have enough memory to place the security context.

>>   struct inet_diag_entry {
>> @@ -108,6 +110,25 @@ static int inet_csk_diag_fill(struct sock *sk,
>>   		       icsk->icsk_ca_ops->name);
>>   	}
>>
>> +	if (ext&  (1<<  (INET_DIAG_SECCTX - 1))) {
>> +		u32 ctxlen = 0;
>> +		void *secctx;
>> +		int error;
>> +
>> +		error = security_sk_getsecctx(sk,&secctx,&ctxlen);
>> +
>> +		if (!error&&  ctxlen) {
>> +			if (ctxlen<  MAX_SECCTX_LEN) {
>> +				strcpy(INET_DIAG_PUT(skb, INET_DIAG_SECCTX,
>> +					ctxlen + 1), secctx);
>> +			} else {
>> +				strcpy(INET_DIAG_PUT(skb, INET_DIAG_SECCTX,
>> +					2), "-");
>
> Is the "-" string a special value already interpreted by the userspace tools?
> If not, you might consider using a string that would indicate an out-of-space
> condition occurred; at first glance I thought the "-" string indicated no
> context.
>
>> +			}
>> +			security_release_secctx(secctx, ctxlen);
>> +		}
>> +	}
>> +
>>   	r->idiag_family = sk->sk_family;
>>   	r->idiag_state = sk->sk_state;
>>   	r->idiag_timer = 0;
>> @@ -246,7 +267,7 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff
>> *skb, static int inet_diag_get_exact(struct sk_buff *in_skb,
>>   			       const struct nlmsghdr *nlh)
>>   {
>> -	int err;
>> +	int err, len;
>>   	struct sock *sk;
>>   	struct inet_diag_req *req = NLMSG_DATA(nlh);
>>   	struct sk_buff *rep;
>> @@ -293,10 +314,17 @@ static int inet_diag_get_exact(struct sk_buff *in_skb,
>> goto out;
>>
>>   	err = -ENOMEM;
>> -	rep = alloc_skb(NLMSG_SPACE((sizeof(struct inet_diag_msg) +
>> -				     sizeof(struct inet_diag_meminfo) +
>> -				     handler->idiag_info_size + 64)),
>> -			GFP_KERNEL);
>> +	len = sizeof(struct inet_diag_msg) + 64;
>> +
>> +	len += (req->idiag_ext&  (1<<  (INET_DIAG_MEMINFO - 1))) ?
>> +		sizeof(struct inet_diag_meminfo) : 0;
>> +	len += (req->idiag_ext&  (1<<  (INET_DIAG_INFO - 1))) ?
>> +		handler->idiag_info_size : 0;
>> +	len += (req->idiag_ext&  (1<<  (INET_DIAG_SECCTX - 1))) ?
>> +		MAX_SECCTX_LEN : 0;
>> +
>> +	rep = alloc_skb(NLMSG_SPACE(len), GFP_KERNEL);
>
> How much of a problem would it be if you just allocated an entire page (or 4k
> in the case of huge pages) and used that?  Is memory usage a concern here?
>

The memory usage is main concern, or else the 4k page is good idea.
Other side is that this function is few called, so the 4k maybe acceptable.



-- 
Best Reagrds,
Roy | RongQing Li

Re: [PATCH 2/2] Add a netlink attribute INET_DIAG_SECCTX

[Paul Moore]

On Thursday, September 01, 2011 05:33:07 PM Rongqing Li wrote:
> On 09/01/2011 05:18 AM, Paul Moore wrote:
> > On Wednesday, August 31, 2011 04:36:17 PM rongqing.li@windriver.com wrote:
> >> From: Roy.Li<rongqing.li@windriver.com>
> >> 
> >> Add a new netlink attribute INET_DIAG_SECCTX to dump the security
> >> context of TCP sockets.
> > 
> > You'll have to forgive me, I'm not familiar with the netlink code used
> > by
> > netstat and friends, but is there anyway to report back the security
> > context of UDP sockets?  Or does the code below handle that already?
> > 
> > In general, AF_INET and AF_INET6 sockets, regardless of any upper level
> > protocols, have security contexts associated with them and it would be
> > nice to see them in netstat.
> 
> Yes, this is real concern, If the dumping tcp security context can be
> accepted by netdev, I am planning to implement it for ipv4 udp socket, unix
> socket. then ipv6..

Great, I'm glad to hear you're planning on implementing this for more than 
just TCP.

I understand your desire to have the basic idea accepted with only TCP 
implemented - and that is fine with me - but I would like to see support for 
all of the protocols merged at the same time.  In other words, seeking the 
basic ACKs for TCP from the davem, et al is okay but I'd like to defer merging 
TCP support until you have everything implemented and ready to be merged.

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2] Add a netlink attribute INET_DIAG_SECCTX

[Rongqing Li]

On 09/01/2011 08:28 PM, Paul Moore wrote:
> On Thursday, September 01, 2011 05:33:07 PM Rongqing Li wrote:
>> On 09/01/2011 05:18 AM, Paul Moore wrote:
>>> On Wednesday, August 31, 2011 04:36:17 PM rongqing.li@windriver.com wrote:
>>>> From: Roy.Li<rongqing.li@windriver.com>
>>>>
>>>> Add a new netlink attribute INET_DIAG_SECCTX to dump the security
>>>> context of TCP sockets.
>>>
>>> You'll have to forgive me, I'm not familiar with the netlink code used
>>> by
>>> netstat and friends, but is there anyway to report back the security
>>> context of UDP sockets?  Or does the code below handle that already?
>>>
>>> In general, AF_INET and AF_INET6 sockets, regardless of any upper level
>>> protocols, have security contexts associated with them and it would be
>>> nice to see them in netstat.
>>
>> Yes, this is real concern, If the dumping tcp security context can be
>> accepted by netdev, I am planning to implement it for ipv4 udp socket, unix
>> socket. then ipv6..
>
> Great, I'm glad to hear you're planning on implementing this for more than
> just TCP.
>
> I understand your desire to have the basic idea accepted with only TCP
> implemented - and that is fine with me - but I would like to see support for
> all of the protocols merged at the same time.  In other words, seeking the
> basic ACKs for TCP from the davem, et al is okay but I'd like to defer merging
> TCP support until you have everything implemented and ready to be merged.
>

Ok, I will try

-- 
Best Reagrds,
Roy | RongQing Li

Re: [PATCH 0/2] Dump the sock's security context

[Rongqing Li]

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

On 08/31/2011 04:36 PM, rongqing.li@windriver.com wrote:
> -------
>      Any review would be much appreciated.
>
> Comments:
> --------
>      Add a netlink attribute INET_DIAG_SECCTX
>
>      Add a new netlink attribute INET_DIAG_SECCTX to dump the security
>      context of TCP sockets.
>
>      The element sk_security of struct sock represents the socket
>      security context ID, which is inherited from the parent process
>      when the socket is created.
>
>      but when SELinux type_transition rule is applied to socket, or
>      application sets /proc/xxx/attr/createsock, the socket security
>      context would be different from the creating process. For these
>      conditions, the "netstat -Z" would return wrong value, since
>      "netstat -Z" only returns the process security context as socket
>      process security.
>
>
> The application to verify the netlink new attribute.
> ------
> See attached file
>
> test:
> --------
> 1. Enable SELinux when compile and startup .
> 	root@qemu-host:/root>  ./printsocketsec
> 	 inode:7141 system_u:system_r:rpcbind_t:s0
> 	 inode:7136 system_u:system_r:rpcbind_t:s0
> 	 inode:7604 system_u:system_r:initrc_t:s0
> 	 inode:7227 system_u:system_r:rpcd_t:s0
> 	 inode:7471 system_u:system_r:sshd_t:s0-s0:c0.c1023
> 	 inode:7469 system_u:system_r:sshd_t:s0-s0:c0.c1023
> 	 inode:7552 system_u:system_r:sendmail_t:s0
> 	 inode:7348 system_u:system_r:initrc_t:s0
> 	 inode:7553 system_u:system_r:sendmail_t:s0
> 	root@qemu-host:/root>
>
> 2. Disable SELinux when startup.
> 	root@qemu-host:/root>  ./printsocketsec
> 	inode:3221
> 	inode:2942
> 	inode:2861
> 	inode:3256
> 	inode:3156
> 	inode:3220
> 	inode:3060
> 	root@qemu-host:/root>
>
> 3. Disable SELinux when compile and startup
> 	root@qemu-host:/root>  ./printsocketsec
> 	inode:3221
> 	inode:2942
> 	inode:2861
> 	inode:3256
> 	inode:3156
> 	inode:3220
> 	inode:3060
> 	root@qemu-host:/root>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
Best Reagrds,
Roy | RongQing Li

[-- Attachment #2: printsocketsec.c --]
[-- Type: text/x-csrc, Size: 2876 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <errno.h>

#include "libnetlink.h"

#include <netinet/tcp.h>
#include <linux/inet_diag.h>

enum {
        SS_UNKNOWN,
        SS_ESTABLISHED,
        SS_SYN_SENT,
        SS_SYN_RECV,
        SS_FIN_WAIT1,
        SS_FIN_WAIT2,
        SS_TIME_WAIT,
        SS_CLOSE,
        SS_CLOSE_WAIT,
        SS_LAST_ACK,
        SS_LISTEN,
        SS_CLOSING,
        SS_MAX
};

#define SS_ALL ((1<<SS_MAX)-1)

/*The INET_DIAG_SECCTX should be defined in inet_diag.h at last,
To simply the test, I define it locally*/
#define INET_DIAG_SECCTX (INET_DIAG_CONG+1)
#define LOCAL_MAX INET_DIAG_SECCTX+1


void tcp_show_info(const struct nlmsghdr *nlh, struct inet_diag_msg *r)
{

	struct rtattr * tb[ LOCAL_MAX + 1];

	printf(" inode:%u", r->idiag_inode);

	parse_rtattr(tb, LOCAL_MAX, (struct rtattr*)(r+1),
		     nlh->nlmsg_len - NLMSG_LENGTH(sizeof(*r)));


	if (tb[INET_DIAG_SECCTX])
		printf(" %s", (char *) RTA_DATA(tb[INET_DIAG_SECCTX]));
	printf("\n");
}

static int tcp_show_netlink( int socktype)
{
	int fd;
	struct sockaddr_nl nladdr;
	struct {
		struct nlmsghdr nlh;
		struct inet_diag_req r;
	} req;

	struct msghdr msg;
	struct rtattr rta;
	char	buf[8192];
	struct iovec iov[3];

	if ((fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_INET_DIAG)) < 0)
		return -1;

	memset(&nladdr, 0, sizeof(nladdr));
	nladdr.nl_family = AF_NETLINK;

	req.nlh.nlmsg_len = sizeof(req);
	req.nlh.nlmsg_type = socktype;
	req.nlh.nlmsg_flags = NLM_F_ROOT|NLM_F_MATCH|NLM_F_REQUEST;
	req.nlh.nlmsg_pid = 0;
	req.nlh.nlmsg_seq = 123456;
	memset(&req.r, 0, sizeof(req.r));
	req.r.idiag_family = AF_INET;
	req.r.idiag_states = SS_ALL;

	req.r.idiag_ext |= (1<<(INET_DIAG_SECCTX-1));

	iov[0] = (struct iovec){
		.iov_base = &req,
		.iov_len = sizeof(req)
	};

	msg = (struct msghdr) {
		.msg_name = (void*)&nladdr,
		.msg_namelen = sizeof(nladdr),
		.msg_iov = iov,
		.msg_iovlen = 1,
	};

	if (sendmsg(fd, &msg, 0) < 0)
		return -1;

	iov[0] = (struct iovec){
		.iov_base = buf,
		.iov_len = sizeof(buf)
	};

	while (1) {
		int status;
		struct nlmsghdr *h;

		msg = (struct msghdr) {
			(void*)&nladdr, sizeof(nladdr),
			iov,	1,
			NULL,	0,
			0
		};

		status = recvmsg(fd, &msg, 0);

		if (status < 0) {
			if (errno == EINTR)
				continue;
			perror("OVERRUN");
			continue;
		}
		if (status == 0) {
			fprintf(stderr, "EOF on netlink\n");
			return 0;
		}

		h = (struct nlmsghdr*)buf;
		while (NLMSG_OK(h, status)) {
			struct inet_diag_msg *r = NLMSG_DATA(h);

			if (/*h->nlmsg_pid != rth->local.nl_pid ||*/
			    h->nlmsg_seq != 123456)
				goto skip_it;

			if (h->nlmsg_type == NLMSG_DONE)
				return 0;

			if (h->nlmsg_type == NLMSG_ERROR) 
				return 0;

			tcp_show_info(h, r);
skip_it:
			h = NLMSG_NEXT(h, status);
		}
	}
	return 0;
}
void main()
{
	tcp_show_netlink( TCPDIAG_GETSOCK);
}