libselinux patch

libselinux patch

[Daniel J Walsh]

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

This patch initializes all swig parameters to prevent possible crashes 
in python.

Also changes is_selinux_enabled to work even if  signaled and not have 
short reads.



[-- Attachment #2: libselinux-rhat.patch --]
[-- Type: text/x-patch, Size: 5658 bytes --]

--- libselinux-2.0.1/src/selinuxswig_wrap.c.rhat	2007-02-20 09:25:03.000000000 -0500
+++ libselinux-2.0.1/src/selinuxswig_wrap.c	2007-02-20 09:25:14.000000000 -0500
@@ -2832,7 +2832,7 @@
   security_context_t *arg1 = (security_context_t *) 0 ;
   int result;
   security_context_t temp1 = NULL ;
-  char *temp10 ;
+  char *temp10 = NULL ;
   
   {
     arg1 = &temp1;
@@ -2879,7 +2879,7 @@
   int val1 ;
   int ecode1 = 0 ;
   security_context_t temp2 = NULL ;
-  char *temp20 ;
+  char *temp20 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
@@ -2911,7 +2911,7 @@
   security_context_t *arg1 = (security_context_t *) 0 ;
   int result;
   security_context_t temp1 = NULL ;
-  char *temp10 ;
+  char *temp10 = NULL ;
   
   {
     arg1 = &temp1;
@@ -2937,7 +2937,7 @@
   security_context_t *arg1 = (security_context_t *) 0 ;
   int result;
   security_context_t temp1 = NULL ;
-  char *temp10 ;
+  char *temp10 = NULL ;
   
   {
     arg1 = &temp1;
@@ -2981,7 +2981,7 @@
   security_context_t *arg1 = (security_context_t *) 0 ;
   int result;
   security_context_t temp1 = NULL ;
-  char *temp10 ;
+  char *temp10 = NULL ;
   
   {
     arg1 = &temp1;
@@ -3025,7 +3025,7 @@
   security_context_t *arg1 = (security_context_t *) 0 ;
   int result;
   security_context_t temp1 = NULL ;
-  char *temp10 ;
+  char *temp10 = NULL ;
   
   {
     arg1 = &temp1;
@@ -3069,7 +3069,7 @@
   security_context_t *arg1 = (security_context_t *) 0 ;
   int result;
   security_context_t temp1 = NULL ;
-  char *temp10 ;
+  char *temp10 = NULL ;
   
   {
     arg1 = &temp1;
@@ -3117,7 +3117,7 @@
   char *buf1 = 0 ;
   int alloc1 = 0 ;
   security_context_t temp2 = NULL ;
-  char *temp20 ;
+  char *temp20 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
@@ -3155,7 +3155,7 @@
   char *buf1 = 0 ;
   int alloc1 = 0 ;
   security_context_t temp2 = NULL ;
-  char *temp20 ;
+  char *temp20 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
@@ -3192,7 +3192,7 @@
   int val1 ;
   int ecode1 = 0 ;
   security_context_t temp2 = NULL ;
-  char *temp20 ;
+  char *temp20 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
@@ -3314,7 +3314,7 @@
   int val1 ;
   int ecode1 = 0 ;
   security_context_t temp2 = NULL ;
-  char *temp20 ;
+  char *temp20 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
@@ -3475,7 +3475,7 @@
   security_context_t *arg2 = (security_context_t *) 0 ;
   int result;
   security_context_t temp2 = NULL ;
-  char *temp20 ;
+  char *temp20 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
@@ -3735,7 +3735,7 @@
   unsigned int val2 ;
   int ecode2 = 0 ;
   security_context_t temp3 = NULL ;
-  char *temp30 ;
+  char *temp30 = NULL ;
   PyObject * obj0 = 0 ;
   PyObject * obj1 = 0 ;
   
@@ -3779,7 +3779,7 @@
   char *buf1 = 0 ;
   int alloc1 = 0 ;
   security_context_t temp2 = NULL ;
-  char *temp20 ;
+  char *temp20 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
@@ -4213,7 +4213,7 @@
   char *buf1 = 0 ;
   int alloc1 = 0 ;
   security_context_t temp2 = NULL ;
-  char *temp20 ;
+  char *temp20 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
@@ -4251,7 +4251,7 @@
   char *buf1 = 0 ;
   int alloc1 = 0 ;
   security_context_t temp2 = NULL ;
-  char *temp20 ;
+  char *temp20 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
@@ -4284,7 +4284,7 @@
   PyObject *resultobj = 0;
   char **arg1 = (char **) 0 ;
   int result;
-  char *temp1 ;
+  char *temp1 = NULL ;
   
   {
     arg1 = &temp1;
@@ -4310,8 +4310,8 @@
   int res1 ;
   char *buf1 = 0 ;
   int alloc1 = 0 ;
-  char *temp2 ;
-  char *temp3 ;
+  char *temp2 = NULL ;
+  char *temp3 = NULL ;
   PyObject * obj0 = 0 ;
   
   {
--- libselinux-2.0.1/src/enabled.c.rhat	2007-02-20 09:25:03.000000000 -0500
+++ libselinux-2.0.1/src/enabled.c	2007-02-20 09:27:53.000000000 -0500
@@ -8,10 +8,44 @@
 #include <stdio.h>
 #include "policy.h"
 
+static int readFD (int fd, char **buf)
+{
+    char *p;
+    size_t size = 16384;
+    int s, filesize;
+
+    *buf = calloc (16384, sizeof (char));
+    if (*buf == 0)
+        return -1;
+
+    filesize = 0;
+    do {
+        p = &(*buf) [filesize];
+	do {
+		s = read (fd, p, 16384);
+	} while (s < 0 && errno == EINTR);
+        if (s < 0)
+            break;
+        filesize += s;
+        /* only exit for empty reads */
+        if (s == 0)
+            break;
+        size += s;
+        *buf = realloc (*buf, size);
+    } while (1);
+
+    if (filesize == 0 && s < 0) {
+        free (*buf);
+        *buf = NULL;
+        return -1;
+    }
+
+    return filesize;
+}
+
 int is_selinux_enabled(void)
 {
-	char *buf;
-	size_t size;
+	char *buf = NULL;
 	int fd;
 	ssize_t ret;
 	int enabled = 0;
@@ -21,19 +55,11 @@
 	if (fd < 0)
 		return -1;
 
-	size = selinux_page_size;
-	buf = malloc(size);
-	if (!buf) {
-		enabled = -1;
-		goto out;
-	}
-
-	memset(buf, 0, size);
-
-	ret = read(fd, buf, size - 1);
+        ret = readFD(fd, &buf);
+        close(fd);
 	if (ret < 0) {
 		enabled = -1;
-		goto out2;
+		goto out;
 	}
 
 	if (!strstr(buf, "selinuxfs"))
@@ -49,7 +75,6 @@
       out2:
 	free(buf);
       out:
-	close(fd);
 	return enabled;
 }
 
--- libselinux-2.0.1/src/selinuxswig.i.rhat	2007-02-20 09:25:03.000000000 -0500
+++ libselinux-2.0.1/src/selinuxswig.i	2007-02-20 09:25:14.000000000 -0500
@@ -28,7 +28,7 @@
 %typemap(in, numinputs=0) security_context_t *(security_context_t temp=NULL) {
 	$1 = &temp;
 }
-%typemap(argout) security_context_t * (char *temp) {
+%typemap(argout) security_context_t * (char *temp=NULL) {
 	if (*$1)
 		temp = *$1;
 	else
@@ -126,7 +126,7 @@
 extern int selinux_raw_to_trans_context(char *raw, 
 					security_context_t *transp);
 
-%typemap(in, numinputs=0) char **(char *temp) {
+%typemap(in, numinputs=0) char **(char *temp=NULL) {
 	$1 = &temp;
 }
 

Re: libselinux patch

[Steve G]

>+static int readFD (int fd, char **buf)
>+{
>+    char *p;
>+    size_t size = 16384;

Why this number? Should it be a define somewhere or based off a kernel parameter?

+    int s, filesize;
+
+    *buf = calloc (16384, sizeof (char));

Why calloc? Its not as efficient as plain old malloc.

+    if (*buf == 0)
+        return -1;

0 or NULL? There's a difference on 64 bit machines.

+
+    filesize = 0;
+    do {
+        p = &(*buf) [filesize];
+	do {
+		s = read (fd, p, 16384);
+	} while (s < 0 && errno == EINTR);
+        if (s < 0)
+            break;
+        filesize += s;
+        /* only exit for empty reads */
+        if (s == 0)
+            break;
+        size += s;
+        *buf = realloc (*buf, size);

So, if you get a short read, why are we playing with memory? This is inefficiant
for the _normal_ case. The normal case is that you get about 400 bytes for a
succesful read. I'd say you are better off adjusting the read size on the fly and
only realloc when you have less that 512 bytes left in the buffer so that there
is some hysteresis.

+    } while (1);
+
+    if (filesize == 0 && s < 0) {
+        free (*buf);
+        *buf = NULL;
+        return -1;
+    }
+
+    return filesize;
+}
+



 
____________________________________________________________________________________
Don't pick lemons.
See all the new 2007 cars at Yahoo! Autos.
http://autos.yahoo.com/new_cars.html 

--
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: libselinux patch

[Stephen Smalley]

On Tue, 2007-02-20 at 07:05 -0800, Steve G wrote:
> >+static int readFD (int fd, char **buf)
> >+{
> >+    char *p;
> >+    size_t size = 16384;
> 
> Why this number? Should it be a define somewhere or based off a kernel parameter?
> 
> +    int s, filesize;
> +
> +    *buf = calloc (16384, sizeof (char));
> 
> Why calloc? Its not as efficient as plain old malloc.
> 
> +    if (*buf == 0)
> +        return -1;
> 
> 0 or NULL? There's a difference on 64 bit machines.
> 
> +
> +    filesize = 0;
> +    do {
> +        p = &(*buf) [filesize];
> +	do {
> +		s = read (fd, p, 16384);
> +	} while (s < 0 && errno == EINTR);
> +        if (s < 0)
> +            break;
> +        filesize += s;
> +        /* only exit for empty reads */
> +        if (s == 0)
> +            break;
> +        size += s;
> +        *buf = realloc (*buf, size);
> 
> So, if you get a short read, why are we playing with memory? This is inefficiant
> for the _normal_ case. The normal case is that you get about 400 bytes for a
> succesful read. I'd say you are better off adjusting the read size on the fly and
> only realloc when you have less that 512 bytes left in the buffer so that there
> is some hysteresis.
> 
> +    } while (1);
> +
> +    if (filesize == 0 && s < 0) {
> +        free (*buf);
> +        *buf = NULL;
> +        return -1;
> +    }
> +
> +    return filesize;
> +}
> +

I also have to wonder whether we'd be better off just switching from
direct open+read of /proc/filesystems to just using fopen+getline,
thereby automatically picking up glibc's internal support of dynamic
buffer allocation and EINTR handling.  Just read the file line-by-line
and bail out if we find selinuxfs or hit the end of file.

-- 
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: libselinux patch

[Steve G]

>I also have to wonder whether we'd be better off just switching from
>direct open+read of /proc/filesystems to just using fopen+getline,
>thereby automatically picking up glibc's internal support of dynamic
>buffer allocation and EINTR handling.  Just read the file line-by-line
>and bail out if we find selinuxfs or hit the end of file.

There might be a more robust, higher performance way of doing this. Is there ever
a situation where the selinux filesystem is _not_ "/selinux" ? If there never is
a case where this happens, then we could change over to statfs and check for
SELINUX_MAGIC in the f_type field of the structure. Its only 1 syscall instead of
several.

-Steve


 
____________________________________________________________________________________
Be a PS3 game guru.
Get your game face on with the latest PS3 news and previews at Yahoo! Games.
http://videogames.yahoo.com/platform?platform=120121

--
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: libselinux patch

[Stephen Smalley]

On Wed, 2007-02-21 at 05:12 -0800, Steve G wrote:
> >I also have to wonder whether we'd be better off just switching from
> >direct open+read of /proc/filesystems to just using fopen+getline,
> >thereby automatically picking up glibc's internal support of dynamic
> >buffer allocation and EINTR handling.  Just read the file line-by-line
> >and bail out if we find selinuxfs or hit the end of file.
> 
> There might be a more robust, higher performance way of doing this. Is there ever
> a situation where the selinux filesystem is _not_ "/selinux" ? If there never is
> a case where this happens, then we could change over to statfs and check for
> SELINUX_MAGIC in the f_type field of the structure. Its only 1 syscall instead of
> several.

At present, libselinux dynamically determines where selinuxfs is mounted
via /proc/mounts in init_selinuxmnt() and uses that path for all calls
that operate on selinuxfs.  Also, statfs'ing /selinux requires selinuxfs
to already be mounted before you can call is_selinux_enabled(), whereas
checking /proc/filesystems only requires that the selinux kernel code
have registered selinuxfs as a filesystem (much earlier) and that proc
be mounted.

-- 
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: libselinux patch

[Steve G]

>At present, libselinux dynamically determines where selinuxfs is mounted
>via /proc/mounts in init_selinuxmnt() and uses that path for all calls
>that operate on selinuxfs.

I guess that path could be used for statfs?

>Also, statfs'ing /selinux requires selinuxfs to already be mounted before
>you can call is_selinux_enabled(),

Is selinux really enabled if its not available for use (mounted)?

-Steve


 
____________________________________________________________________________________
It's here! Your new message!  
Get new email alerts with the free Yahoo! Toolbar.
http://tools.search.yahoo.com/toolbar/features/mail/

--
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: libselinux patch

[Stephen Smalley]

On Wed, 2007-02-21 at 05:37 -0800, Steve G wrote:
> >At present, libselinux dynamically determines where selinuxfs is mounted
> >via /proc/mounts in init_selinuxmnt() and uses that path for all calls
> >that operate on selinuxfs.
> 
> I guess that path could be used for statfs?

That still leaves you with a dependency on successfully
reading /proc/mounts and allocating memory, so no different than
reading /proc/filesystems as far as robustness goes.  It only saves you
time for that portion of is_selinux_enabled(), and is_selinux_enabled()
also does a getcon() call (a hack) to check whether a policy has been
loaded.  is_selinux_enabled() should never be on a critical path (should
always be called during startup by a program and saved for later use in
a variable).

> >Also, statfs'ing /selinux requires selinuxfs to already be mounted before
> >you can call is_selinux_enabled(),
> 
> Is selinux really enabled if its not available for use (mounted)?

Depends on what "enabled" means.  If it just means enabled in the
kernel, then checking /proc/filesystems for selinuxfs is the right test.
Given that we also have it check for a loaded policy, and one has to
mount selinuxfs to load policy, I suppose it might not make any
difference.

-- 
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: libselinux patch

[Steve G]

>> >At present, libselinux dynamically determines where selinuxfs is mounted
>> >via /proc/mounts in init_selinuxmnt() and uses that path for all calls
>> >that operate on selinuxfs.
>> 
>> I guess that path could be used for statfs?
>
>That still leaves you with a dependency on successfully
>reading /proc/mounts and allocating memory, so no different than
>reading /proc/filesystems as far as robustness goes.

Actually, you could guess "/selinux" and drop back to dynamically determining if
that failed. that would get that call down to 1 syscall, too. I'd say 99.99% of
the time it would be the right guess. Only at boot, shutdown, or a strange chroot
config would there be a possibility of something different.

>is_selinux_enabled() should never be on a critical path (should
>always be called during startup by a program and saved for later
>use in a variable).

Every selinux aware app checks it and it could short circuit seeveral syscalls to
get the right answer.

-Steve


 
____________________________________________________________________________________
TV dinner still cooling? 
Check out "Tonight's Picks" on Yahoo! TV.
http://tv.yahoo.com/

--
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: libselinux patch

[Stefanos Harhalakis]

On Wednesday 21 February 2007 15:37, Steve G wrote:
> >At present, libselinux dynamically determines where selinuxfs is mounted
> >via /proc/mounts in init_selinuxmnt() and uses that path for all calls
> >that operate on selinuxfs.
>
> I guess that path could be used for statfs?
>
> >Also, statfs'ing /selinux requires selinuxfs to already be mounted before
> >you can call is_selinux_enabled(),
>
> Is selinux really enabled if its not available for use (mounted)?

  When working under chroot(), /proc may be available but /selinux may not be. 
In that case /proc/mounts will list /selinux but you will not be able to 
stat() it.

<<V13>>

--
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: libselinux patch

[Stephen Smalley]

On Tue, 2007-02-20 at 09:31 -0500, Daniel J Walsh wrote:
> This patch initializes all swig parameters to prevent possible crashes 
> in python.

I merged this part.

> Also changes is_selinux_enabled to work even if  signaled and not have 
> short reads.

But not this one.  Still working out the best solution for
is_selinux_enabled, as per the ongoing discussion on the list.

-- 
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: libselinux patch

[Steve G]

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

>Actually, you could guess "/selinux" and drop back to dynamically determining if
>that failed.

This approach works and saves about 7 syscalls. This is the diff of running
selinuxenabled with and without the attached patch.

 close(3)                                = 0
 munmap(0x2aaaaaaac000, 4096)            = 0
-open("/proc/mounts", O_RDONLY)          = 3
-fstat(3, {st_mode=S_IFREG|0444, st_size=0, ...}) = 0
-mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x2aaaaaaac000
-read(3, "rootfs / rootfs rw 0 0\n/dev/root"..., 4096) = 673
-close(3)                                = 0
-munmap(0x2aaaaaaac000, 4096)            = 0
+statfs("/selinux", {f_type=0xf97cff8c, f_bsize=4096, f_blocks=0, f_bfree=0,
f_bavail=0, f_files=0, f_ffree=0, f_fsid={0, 0}, f_namelen=255, f_frsize=4096}) =
0
 open("/selinux/mls", O_RDONLY)          = 3
 read(3, "1", 19)                        = 1
@@ -71,12 +66,10 @@
 readv(3, [{"\0", 1}], 1)                = 1
 close(3)                                = 0
-open("/proc/filesystems", O_RDONLY)     = 3
-read(3, "nodev\tsysfs\nnodev\trootfs\nnodev\tb"..., 4095) = 311
-close(3)                                = 0
+statfs("/selinux", {f_type=0xf97cff8c, f_bsize=4096, f_blocks=0, f_bfree=0,
f_bavail=0, f_files=0, f_ffree=0, f_fsid={0, 0}, f_namelen=255, f_frsize=4096}) =
0
 gettid()                                = 14365
 open("/proc/self/task/14365/attr/current", O_RDONLY) = 3

This improves performance, falls back to the old method when the guess is wrong,
and checks that /selinux really is an selinuxfs.

While reading through the is_enabled code, I realized something. Right before the
call to getcon_raw(), we decide that its enabled. If the getcon_raw() fails, we
still consider it enabled. Only if the call return success do we do a test and
consider it disabled. I don't know if that's good or bad, but I copied the
behavior. Seems suspicious to me.

Signed-off-by: Steve Grubb <linux_4ever@yahoo.com>


 
____________________________________________________________________________________
Bored stiff? Loosen up... 
Download and play hundreds of games for free on Yahoo! Games.
http://games.yahoo.com/games/front

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 3974351640-libselinux-2.0.0-enabled.patch --]
[-- Type: text/x-patch; name="libselinux-2.0.0-enabled.patch", Size: 2315 bytes --]

diff -urp libselinux-2.0.0.orig/include/selinux/selinux.h libselinux-2.0.0/include/selinux/selinux.h
--- libselinux-2.0.0.orig/include/selinux/selinux.h	2007-02-19 20:57:53.000000000 -0500
+++ libselinux-2.0.0/include/selinux/selinux.h	2007-02-21 13:06:13.000000000 -0500
@@ -8,6 +8,8 @@
 extern "C" {
 #endif
 
+#define SELINUX_MAGIC 0xf97cff8c
+
 /* Return 1 if we are running on a SELinux kernel, or 0 if not or -1 if we get an error. */
 	extern int is_selinux_enabled(void);
 /* Return 1 if we are running on a SELinux MLS kernel, or 0 otherwise. */
diff -urp libselinux-2.0.0.orig/src/enabled.c libselinux-2.0.0/src/enabled.c
--- libselinux-2.0.0.orig/src/enabled.c	2007-02-19 20:57:53.000000000 -0500
+++ libselinux-2.0.0/src/enabled.c	2007-02-21 13:08:24.000000000 -0500
@@ -6,16 +6,34 @@
 #include <errno.h>
 #include <limits.h>
 #include <stdio.h>
+#include <sys/vfs.h>
+
 #include "policy.h"
 
 int is_selinux_enabled(void)
 {
 	char *buf;
 	size_t size;
-	int fd;
+	int fd, rc;
 	ssize_t ret;
 	int enabled = 0;
 	security_context_t con;
+	struct statfs sfbuf;
+
+	do { 
+		rc = statfs("/selinux", &sfbuf);
+	} while(rc < 0 && errno == EINTR);
+	if (rc == 0) {
+		if ((u_int32_t)sfbuf.f_type == (u_int32_t)SELINUX_MAGIC) {
+			enabled = 1;
+			if (getcon_raw(&con) == 0) {
+				if (!strcmp(con, "kernel"))
+					enabled = 0;
+				freecon(con);
+			}
+                        return enabled;
+                }
+        }
 
 	fd = open("/proc/filesystems", O_RDONLY);
 	if (fd < 0)
diff -urp libselinux-2.0.0.orig/src/init.c libselinux-2.0.0/src/init.c
--- libselinux-2.0.0.orig/src/init.c	2007-02-19 20:57:53.000000000 -0500
+++ libselinux-2.0.0/src/init.c	2007-02-21 13:09:39.000000000 -0500
@@ -6,7 +6,7 @@
 #include <ctype.h>
 #include <stdio.h>
 #include <dlfcn.h>
-#include <unistd.h>
+#include <sys/vfs.h>
 
 #include "dso.h"
 #include "policy.h"
@@ -21,10 +21,22 @@ static void init_selinuxmnt(void)
 	char *buf, *bufp, *p;
 	size_t size;
 	FILE *fp;
+	struct statfs sfbuf;
+	int rc;
 
 	if (selinux_mnt)
 		return;
 
+	do {
+		rc = statfs("/selinux", &sfbuf);
+	} while (rc < 0 && errno == EINTR);
+	if (rc == 0) {
+		if ((u_int32_t)sfbuf.f_type == (u_int32_t)SELINUX_MAGIC) {
+			selinux_mnt = strdup("/selinux");
+			return;
+		}
+	}
+
 	fp = fopen("/proc/mounts", "r");
 	if (!fp)
 		return;

Re: libselinux patch

[Stephen Smalley]

On Wed, 2007-02-21 at 10:26 -0800, Steve G wrote:
> >Actually, you could guess "/selinux" and drop back to dynamically determining if
> >that failed.
> 
> This approach works and saves about 7 syscalls. This is the diff of running
> selinuxenabled with and without the attached patch.
<snip>
> This improves performance, falls back to the old method when the guess is wrong,
> and checks that /selinux really is an selinuxfs.
> 
> While reading through the is_enabled code, I realized something. Right before the
> call to getcon_raw(), we decide that its enabled. If the getcon_raw() fails, we
> still consider it enabled. Only if the call return success do we do a test and
> consider it disabled. I don't know if that's good or bad, but I copied the
> behavior. Seems suspicious to me.

That was intentional; likely should add a comment.  The presence of
selinuxfs in /proc/filesystems is a strong indicator that SELinux is
enabled; the getcon test was added later to detect no-policy-loaded, but
could fail for other reasons and I didn't want a failure there to lead
to a selinux-disabled status (particularly given that programs today
usually only check for is_selinux_enabled() > 0).  The only case where a
SELinux system today should have selinuxfs in /proc/filesystems but
no-policy-loaded is if SELINUX=permissive and init wasn't able to load
policy for some reason (e.g. no policy or corrupted policy file); if
SELINUX=enforcing, then init should halt if it can't load policy, and if
SELINUX=disabled, init will use /selinux/disable to turn off SELinux,
which unregisters selinuxfs too.

Patch looks basically sane; a couple of minor points:
- Let's move the existing SELINUXMNT definition used by
src/load_policy.c for mounting selinuxfs for initial policy load to a
private header (e.g. src/policy.h) and use it in this code rather than
repeating the "/selinux" string each time.
- SELINUX_MAGIC doesn't need to be exposed outside of the library, so
I'd put it into src/policy.h too instead of a public header.
- Why u_int32_t instead of just long (per the statfs man page)?  And if
we need to use a fixed size type, we should use C99 style i.e. uint32_t
going forward - a recently merged patch converted all occurrences in
libselinux over to C99 style.

-- 
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: libselinux patch

[Steve G]

>That was intentional; likely should add a comment.

OK, explanation makes sense. I'll add a comment to the patch.

>Patch looks basically sane;

Something else I was thinking about. When the library starts up, it checks for
the mounts then it checks for enabled. Would there be any situation where an
selinuxfs would be mounted and selinux is considered disabled? If not, we can
short circuit the redundant check for /selinux and assume that if selinux_mnt !=
NULL then selinux is enabled. Does this make sense?

>- Let's move the existing SELINUXMNT definition used by
>src/load_policy.c for mounting selinuxfs for initial policy load to a
>private header (e.g. src/policy.h) and use it in this code rather than
>repeating the "/selinux" string each time.

Sure.

>- SELINUX_MAGIC doesn't need to be exposed outside of the library, so
>I'd put it into src/policy.h too instead of a public header.

I have a feeling that people want this. Almost all filesystems have the magic
exposed or well known. SELINUX_MAGIC isn't even listed in statfs man page while
most other filesystems are. If you search /usr/include for SELINUX_MAGIC, you'll
find it defined in:

/usr/include/sepol/policydb/flask_types.h

So, why not have a definition in libselinux and let other subsystems use it?

>- Why u_int32_t instead of just long (per the statfs man page)?

SELINUX_MAGIC is 0xf97cff8c and the upper bit is 1, so it gets sign extended in
64 bit kernels. Then the direct comparison fails. No other MAGIC has a 1 in the
upper bit so selinux is unique in that way. Try this little program and you'll
see:


#include <stdio.h>
#include <sys/vfs.h>
#include <sys/types.h>
#include <dirent.h>
#include <unistd.h>
#include <errno.h>
#include <string.h>
#include <fcntl.h>
#include <sys/stat.h>

int main(void)
{
        DIR *d;
        struct dirent *e;

        d = opendir("/");
        while ((e = readdir(d))) {
                char path[PATH_MAX];
                struct stat buf;

                if (e->d_name[0] == '.')
                        continue;

                snprintf(path, sizeof(path), "/%s", e->d_name);
                stat(path, &buf);
                if (S_ISDIR(buf.st_mode)) {
                        struct statfs sfbuf;
                        statfs(path, &sfbuf);
                        printf("%s: 0x%lx\n", path, sfbuf.f_type);
                }
        }
        closedir(d);
        return 0;
}


>And if we need to use a fixed size type, we should use C99 style i.e. uint32_t
>going forward - a recently merged patch converted all occurrences in libselinux
>over to C99 style.

Sure. I discovered this after making the patch and was looking for a built-in
type so I wouldn't have to re-diff the patch. Long term we should probably fix
the sign-extension if possible.

-Steve


 
____________________________________________________________________________________
Do you Yahoo!?
Everyone is raving about the all-new Yahoo! Mail beta.
http://new.mail.yahoo.com

--
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: libselinux patch

[Stephen Smalley]

On Thu, 2007-02-22 at 05:46 -0800, Steve G wrote:
> >That was intentional; likely should add a comment.
> 
> OK, explanation makes sense. I'll add a comment to the patch.
> 
> >Patch looks basically sane;
> 
> Something else I was thinking about. When the library starts up, it checks for
> the mounts then it checks for enabled. Would there be any situation where an
> selinuxfs would be mounted and selinux is considered disabled? If not, we can
> short circuit the redundant check for /selinux and assume that if selinux_mnt !=
> NULL then selinux is enabled. Does this make sense?

Yes.

> >- SELINUX_MAGIC doesn't need to be exposed outside of the library, so
> >I'd put it into src/policy.h too instead of a public header.
> 
> I have a feeling that people want this. Almost all filesystems have the magic
> exposed or well known. SELINUX_MAGIC isn't even listed in statfs man page while
> most other filesystems are. If you search /usr/include for SELINUX_MAGIC, you'll
> find it defined in:
> 
> /usr/include/sepol/policydb/flask_types.h

There it is used as a magic number in the binary policy file.  Note btw
that sepol/policydb headers aren't part of the shared library interface,
just the static library.

> So, why not have a definition in libselinux and let other subsystems use it?

If a program actually needed the value for some purpose, I'd rather wrap
that in a libselinux interface in some manner rather than directly
expose the value as a fixed definition.   I also think that the
superblock magic number is somewhat meaningless for pseudo filesystems
like selinuxfs, and there isn't any real guarantee of uniqueness.

> >- Why u_int32_t instead of just long (per the statfs man page)?
> 
> SELINUX_MAGIC is 0xf97cff8c and the upper bit is 1, so it gets sign extended in
> 64 bit kernels. Then the direct comparison fails. No other MAGIC has a 1 in the
> upper bit so selinux is unique in that way. Try this little program and you'll
> see:

Hmm...well, (unsigned long) would seem adequate then.

-- 
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: libselinux patch

[James Antill]

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

On Thu, 2007-02-22 at 05:46 -0800, Steve G wrote:
> >
> >- Why u_int32_t instead of just long (per the statfs man page)?
> 
> SELINUX_MAGIC is 0xf97cff8c and the upper bit is 1, so it gets sign extended in
> 64 bit kernels. Then the direct comparison fails.

 Why not just have the define as:

#define SELINUX_MAGIC 0xf97cff8cU

-- 
James Antill - <james.antill@redhat.com>
setsockopt(fd, IPPROTO_TCP, TCP_CONGESTION, ...);
setsockopt(fd, IPPROTO_TCP, TCP_DEFER_ACCEPT, ...);
setsockopt(fd, SOL_SOCKET,  SO_ATTACH_FILTER, ...);


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: libselinux patch

[Steve G]

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

Hi,

OK, I think the attached patch does _everything _ we discussed. It:

- removes 8 syscalls for the normal path
- ensures /selinux is trully an selinuxfs
- drops back to detecting the old way when /selinux is missing
- changes the old way in is_enabled to use fopen for glibc internal retries
- adds retry for EINTR in mls_enabled
- keeps SELINUX_MAGIC private

Signed-off-by: Steve Grubb <linux_4ever@yahoo.com>



 
____________________________________________________________________________________
Cheap talk?
Check out Yahoo! Messenger's low PC-to-Phone call rates.
http://voice.yahoo.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 3974351640-libselinux-2.0.0-enabled.patch --]
[-- Type: text/x-patch; name="libselinux-2.0.0-enabled.patch", Size: 4203 bytes --]

diff -urp libselinux-2.0.0.orig/src/enabled.c libselinux-2.0.0/src/enabled.c
--- libselinux-2.0.0.orig/src/enabled.c	2007-02-19 20:57:53.000000000 -0500
+++ libselinux-2.0.0/src/enabled.c	2007-02-22 10:26:02.000000000 -0500
@@ -10,15 +10,33 @@
 
 int is_selinux_enabled(void)
 {
-	char *buf;
+	char *buf, *bufp;
 	size_t size;
-	int fd;
+	FILE *fp;
 	ssize_t ret;
-	int enabled = 0;
+	int rc, enabled = 0;
 	security_context_t con;
 
-	fd = open("/proc/filesystems", O_RDONLY);
-	if (fd < 0)
+	/* init_selinuxmnt() gets called before this function. We
+ 	 * will assume that if a selinux file system is mounted, then
+ 	 * selinux is enabled. */
+	if (selinux_mnt) {
+
+		/* Since a file system is mounted, we consider selinux
+		 * enabled. If getcon_raw fails, selinux is still enabled.
+		 * We only consider it disabled if no policy is loaded. */
+		enabled = 1;
+		if (getcon_raw(&con) == 0) {
+			if (!strcmp(con, "kernel"))
+				enabled = 0;
+			freecon(con);
+		}
+		return enabled;
+        }
+
+	/* Drop back to detecting it the long way. */
+	fp = fopen("/proc/filesystems", "r");
+	if (!fp)
 		return -1;
 
 	size = selinux_page_size;
@@ -30,17 +48,19 @@ int is_selinux_enabled(void)
 
 	memset(buf, 0, size);
 
-	ret = read(fd, buf, size - 1);
-	if (ret < 0) {
-		enabled = -1;
-		goto out2;
+	while ((bufp = fgets_unlocked(buf, size, fp))) {
+		if (strstr(buf, "selinuxfs")) {
+			enabled = 1;
+			break;
+		}
 	}
 
-	if (!strstr(buf, "selinuxfs"))
+	if (!bufp)
 		goto out2;
 
-	enabled = 1;
-
+	/* Since an selinux file system is available, we consider
+	 * selinux enabled. If getcon_raw fails, selinux is still
+	 * enabled. We only consider it disabled if no policy is loaded. */
 	if (getcon_raw(&con) == 0) {
 		if (!strcmp(con, "kernel"))
 			enabled = 0;
@@ -49,7 +69,7 @@ int is_selinux_enabled(void)
       out2:
 	free(buf);
       out:
-	close(fd);
+	fclose(fp);
 	return enabled;
 }
 
@@ -75,7 +95,9 @@ int is_selinux_mls_enabled(void)
 
 	memset(buf, 0, sizeof buf);
 
-	ret = read(fd, buf, sizeof buf - 1);
+	do {
+		ret = read(fd, buf, sizeof buf - 1);
+	} while (ret < 0 && errno == EINTR);
 	close(fd);
 	if (ret < 0)
 		return enabled;
diff -urp libselinux-2.0.0.orig/src/init.c libselinux-2.0.0/src/init.c
--- libselinux-2.0.0.orig/src/init.c	2007-02-19 20:57:53.000000000 -0500
+++ libselinux-2.0.0/src/init.c	2007-02-22 10:25:04.000000000 -0500
@@ -6,7 +6,8 @@
 #include <ctype.h>
 #include <stdio.h>
 #include <dlfcn.h>
-#include <unistd.h>
+#include <sys/vfs.h>
+#include <stdint.h>
 
 #include "dso.h"
 #include "policy.h"
@@ -21,10 +22,22 @@ static void init_selinuxmnt(void)
 	char *buf, *bufp, *p;
 	size_t size;
 	FILE *fp;
+	struct statfs sfbuf;
+	int rc;
 
 	if (selinux_mnt)
 		return;
 
+	do {
+		rc = statfs(SELINUXMNT, &sfbuf);
+	} while (rc < 0 && errno == EINTR);
+	if (rc == 0) {
+		if ((uint32_t)sfbuf.f_type == (uint32_t)SELINUX_MAGIC) {
+			selinux_mnt = strdup(SELINUXMNT);
+			return;
+		}
+	}
+
 	fp = fopen("/proc/mounts", "r");
 	if (!fp)
 		return;
@@ -62,7 +75,6 @@ static void init_selinuxmnt(void)
       out:
 	fclose(fp);
 	return;
-
 }
 
 static void fini_selinuxmnt(void)
diff -urp libselinux-2.0.0.orig/src/load_policy.c libselinux-2.0.0/src/load_policy.c
--- libselinux-2.0.0.orig/src/load_policy.c	2007-02-19 20:57:53.000000000 -0500
+++ libselinux-2.0.0/src/load_policy.c	2007-02-22 10:23:42.000000000 -0500
@@ -165,7 +165,6 @@ hidden_def(selinux_mkload_policy)
  * We only need the hardcoded definition for the initial mount 
  * required for the initial policy load.
  */
-#define SELINUXMNT "/selinux/"
 int selinux_init_load_policy(int *enforce)
 {
 	int rc = 0, orig_enforce = 0, seconfig = -2, secmdline = -1;
diff -urp libselinux-2.0.0.orig/src/policy.h libselinux-2.0.0/src/policy.h
--- libselinux-2.0.0.orig/src/policy.h	2007-02-19 20:57:53.000000000 -0500
+++ libselinux-2.0.0/src/policy.h	2007-02-22 10:23:42.000000000 -0500
@@ -9,6 +9,12 @@
 /* Initial length guess for getting contexts. */
 #define INITCONTEXTLEN 255
 
+/* selinuxfs magic number */
+#define SELINUX_MAGIC 0xf97cff8c
+
+/* Preferred selinux mount location */
+#define SELINUXMNT "/selinux"
+
 /* selinuxfs mount point */
 extern char *selinux_mnt;
 

Re: libselinux patch

[Stephen Smalley]

On Thu, 2007-02-22 at 07:48 -0800, Steve G wrote:
> Hi,
> 
> OK, I think the attached patch does _everything _ we discussed. It:
> 
> - removes 8 syscalls for the normal path
> - ensures /selinux is trully an selinuxfs
> - drops back to detecting the old way when /selinux is missing
> - changes the old way in is_enabled to use fopen for glibc internal retries
> - adds retry for EINTR in mls_enabled
> - keeps SELINUX_MAGIC private
> 
> Signed-off-by: Steve Grubb <linux_4ever@yahoo.com>

- Can drop the no-longer-used ret and rc local vars.
- Booting with the patched libselinux reveals some permission denials
due to the new attempts to statfs /selinux, e.g. audit2allow after boot
generates:
	allow mount_t security_t : filesystem getattr;
	allow restorecon_t security_t : filesystem getattr;
	allow setrans_t security_t : filesystem getattr;
Hence, we'd need to add that permission to selinux_get_fs_mount() in
refpolicy.  Fortunately, since it just falls back to the old logic if
the statfs fails, it doesn't change behavior so we don't need to
coordinate closely with the policy update.

-- 
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: libselinux patch

[Steve G]

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


OK, I think the attached patch does _everything _ we discussed. It:
 
 - removes 8 syscalls for the normal path
 - ensures /selinux is trully an selinuxfs
 - drops back to detecting the old way when /selinux is missing
 - changes the old way in is_enabled to use fopen & getline for glibc internal
retries
 - adds retry for EINTR in mls_enabled
 - keeps SELINUX_MAGIC private
 
 Signed-off-by: Steve Grubb <linux_4ever@yahoo.com>


 
____________________________________________________________________________________
Now that's room service!  Choose from over 150,000 hotels
in 45,000 destinations on Yahoo! Travel to find your fit.
http://farechase.yahoo.com/promo-generic-14795097

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 3486229055-libselinux-2.0.4-enabled.patch --]
[-- Type: text/x-patch; name="libselinux-2.0.4-enabled.patch", Size: 5094 bytes --]

diff -urp libselinux-2.0.4.orig/src/enabled.c libselinux-2.0.4/src/enabled.c
--- libselinux-2.0.4.orig/src/enabled.c	2007-02-25 14:52:16.000000000 -0500
+++ libselinux-2.0.4/src/enabled.c	2007-02-26 11:18:14.000000000 -0500
@@ -10,46 +10,57 @@
 
 int is_selinux_enabled(void)
 {
-	char *buf;
-	size_t size;
-	int fd;
-	ssize_t ret;
+	char *buf=NULL;
+	FILE *fp;
+	ssize_t num;
+	size_t len;
 	int enabled = 0;
 	security_context_t con;
 
-	fd = open("/proc/filesystems", O_RDONLY);
-	if (fd < 0)
-		return -1;
-
-	size = selinux_page_size;
-	buf = malloc(size);
-	if (!buf) {
-		enabled = -1;
-		goto out;
-	}
+	/* init_selinuxmnt() gets called before this function. We
+ 	 * will assume that if a selinux file system is mounted, then
+ 	 * selinux is enabled. */
+	if (selinux_mnt) {
+
+		/* Since a file system is mounted, we consider selinux
+		 * enabled. If getcon_raw fails, selinux is still enabled.
+		 * We only consider it disabled if no policy is loaded. */
+		enabled = 1;
+		if (getcon_raw(&con) == 0) {
+			if (!strcmp(con, "kernel"))
+				enabled = 0;
+			freecon(con);
+		}
+		return enabled;
+        }
 
-	memset(buf, 0, size);
+	/* Drop back to detecting it the long way. */
+	fp = fopen("/proc/filesystems", "r");
+	if (!fp)
+		return -1;
 
-	ret = read(fd, buf, size - 1);
-	if (ret < 0) {
-		enabled = -1;
-		goto out2;
+	while ((num = getline(&buf, &len, fp)) != -1) {
+		if (strstr(buf, "selinuxfs")) {
+			enabled = 1;
+			break;
+		}
 	}
 
-	if (!strstr(buf, "selinuxfs"))
-		goto out2;
-
-	enabled = 1;
+	if (num < 0)
+		goto out;
 
+	/* Since an selinux file system is available, we consider
+	 * selinux enabled. If getcon_raw fails, selinux is still
+	 * enabled. We only consider it disabled if no policy is loaded. */
 	if (getcon_raw(&con) == 0) {
 		if (!strcmp(con, "kernel"))
 			enabled = 0;
 		freecon(con);
 	}
-      out2:
-	free(buf);
+
       out:
-	close(fd);
+	free(buf);
+	fclose(fp);
 	return enabled;
 }
 
@@ -75,7 +86,9 @@ int is_selinux_mls_enabled(void)
 
 	memset(buf, 0, sizeof buf);
 
-	ret = read(fd, buf, sizeof buf - 1);
+	do {
+		ret = read(fd, buf, sizeof buf - 1);
+	} while (ret < 0 && errno == EINTR);
 	close(fd);
 	if (ret < 0)
 		return enabled;
diff -urp libselinux-2.0.4.orig/src/init.c libselinux-2.0.4/src/init.c
--- libselinux-2.0.4.orig/src/init.c	2007-02-25 14:52:16.000000000 -0500
+++ libselinux-2.0.4/src/init.c	2007-02-26 11:21:02.000000000 -0500
@@ -6,7 +6,8 @@
 #include <ctype.h>
 #include <stdio.h>
 #include <dlfcn.h>
-#include <unistd.h>
+#include <sys/vfs.h>
+#include <stdint.h>
 
 #include "dso.h"
 #include "policy.h"
@@ -18,51 +19,57 @@ int selinux_page_size = 0;
 
 static void init_selinuxmnt(void)
 {
-	char *buf, *bufp, *p;
-	size_t size;
+	char *buf=NULL, *p;
 	FILE *fp;
+	struct statfs sfbuf;
+	int rc;
+	size_t len;
+	ssize_t num;
 
 	if (selinux_mnt)
 		return;
 
+	/* We check to see if the preferred mount point for selinux file
+	 * system has a selinuxfs. */
+	do {
+		rc = statfs(SELINUXMNT, &sfbuf);
+	} while (rc < 0 && errno == EINTR);
+	if (rc == 0) {
+		if ((uint32_t)sfbuf.f_type == (uint32_t)SELINUX_MAGIC) {
+			selinux_mnt = strdup(SELINUXMNT);
+			return;
+		}
+	} 
+
+	/* At this point, the usual spot doesn't have an selinuxfs so
+	 * we look around for it */
 	fp = fopen("/proc/mounts", "r");
 	if (!fp)
 		return;
 
-	size = selinux_page_size;
-
-	buf = malloc(size);
-	if (!buf)
-		goto out;
-
-	memset(buf, 0, size);
-
-	while ((bufp = fgets_unlocked(buf, size, fp))) {
+	while ((num = getline(&buf, &len, fp)) != -1) {
 		char *tmp;
 		p = strchr(buf, ' ');
 		if (!p)
-			goto out2;
+			goto out;
 		p++;
 		tmp = strchr(p, ' ');
 		if (!tmp)
-			goto out2;
+			goto out;
 		if (!strncmp(tmp + 1, "selinuxfs ", 10)) {
 			*tmp = '\0';
 			break;
 		}
 	}
 
-	if (!bufp)
-		goto out2;
+	/* If we found something, dup it */
+	if (num > 0)
+		selinux_mnt = strdup(p);
 
-	selinux_mnt = strdup(p);
-
-      out2:
-	free(buf);
       out:
+	free(buf);
 	fclose(fp);
 	return;
-
 }
 
 static void fini_selinuxmnt(void)
diff -urp libselinux-2.0.4.orig/src/load_policy.c libselinux-2.0.4/src/load_policy.c
--- libselinux-2.0.4.orig/src/load_policy.c	2007-02-25 14:52:16.000000000 -0500
+++ libselinux-2.0.4/src/load_policy.c	2007-02-26 11:18:14.000000000 -0500
@@ -165,7 +165,6 @@ hidden_def(selinux_mkload_policy)
  * We only need the hardcoded definition for the initial mount 
  * required for the initial policy load.
  */
-#define SELINUXMNT "/selinux/"
 int selinux_init_load_policy(int *enforce)
 {
 	int rc = 0, orig_enforce = 0, seconfig = -2, secmdline = -1;
diff -urp libselinux-2.0.4.orig/src/policy.h libselinux-2.0.4/src/policy.h
--- libselinux-2.0.4.orig/src/policy.h	2007-02-25 14:52:16.000000000 -0500
+++ libselinux-2.0.4/src/policy.h	2007-02-26 11:18:14.000000000 -0500
@@ -9,6 +9,12 @@
 /* Initial length guess for getting contexts. */
 #define INITCONTEXTLEN 255
 
+/* selinuxfs magic number */
+#define SELINUX_MAGIC 0xf97cff8c
+
+/* Preferred selinux mount location */
+#define SELINUXMNT "/selinux"
+
 /* selinuxfs mount point */
 extern char *selinux_mnt;
 

Re: libselinux patch

[Stephen Smalley]

On Mon, 2007-02-26 at 08:40 -0800, Steve G wrote:
> OK, I think the attached patch does _everything _ we discussed. It:
>  
>  - removes 8 syscalls for the normal path
>  - ensures /selinux is trully an selinuxfs
>  - drops back to detecting the old way when /selinux is missing
>  - changes the old way in is_enabled to use fopen & getline for glibc internal
> retries
>  - adds retry for EINTR in mls_enabled
>  - keeps SELINUX_MAGIC private
>  
>  Signed-off-by: Steve Grubb <linux_4ever@yahoo.com>

Thanks, applied the revised (with fsetlocking calls) patch on the trunk,
libselinux 2.0.5.  Note to Chris/Dan:  "allow $1 security_t:filesystem
getattr;" should be added to selinux_get_fs_mount() in the policy to
allow the statfs() call introduced by this patch; it won't break
anything if it is missing (since it falls back to the old logic upon
failure), but it should be allowed.

I have not merged this patch onto the stable branch (libselinux 1.x)
since the patch does include a user-visible change (the
security_t:filesystem getattr permission check upon the statfs call),
even though it does fall back gracefully upon failure there.  If
desired, we could merge the rest of the changes that have no
user-visible impact on the stable branch to pick up the use of getline,
EINTR handling, and checking of selinux_mnt by is_selinux_enabled().

-- 
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: libselinux patch

[Christopher J. PeBenito]

On Tue, 2007-02-27 at 10:15 -0500, Stephen Smalley wrote:
> On Mon, 2007-02-26 at 08:40 -0800, Steve G wrote:
> > OK, I think the attached patch does _everything _ we discussed. It:
> >  
> >  - removes 8 syscalls for the normal path
> >  - ensures /selinux is trully an selinuxfs
> >  - drops back to detecting the old way when /selinux is missing
> >  - changes the old way in is_enabled to use fopen & getline for glibc internal
> > retries
> >  - adds retry for EINTR in mls_enabled
> >  - keeps SELINUX_MAGIC private
> >  
> >  Signed-off-by: Steve Grubb <linux_4ever@yahoo.com>
> 
> Thanks, applied the revised (with fsetlocking calls) patch on the trunk,
> libselinux 2.0.5.  Note to Chris/Dan:  "allow $1 security_t:filesystem
> getattr;" should be added to selinux_get_fs_mount() in the policy to
> allow the statfs() call introduced by this patch; it won't break
> anything if it is missing (since it falls back to the old logic upon
> failure), but it should be allowed.

Added to refpolicy trunk, rev 2198.

-- 
Chris PeBenito
Tresys Technology, LLC
(410) 290-1411 x150


--
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: libselinux patch

[Daniel J Walsh]

Stephen Smalley wrote:
> On Thu, 2007-04-05 at 17:00 -0400, Daniel J Walsh wrote:
>   
>> Second pass.
>>
>>
>> plain text document attachment (diff)
>>     
>
>   
>> diff --exclude-from=exclude -N -u -r nsalibselinux/utils/getsebool.c libselinux-2.0.9/utils/getsebool.c
>> --- nsalibselinux/utils/getsebool.c	2006-11-16 17:15:17.000000000 -0500
>> +++ libselinux-2.0.9/utils/getsebool.c	2007-04-05 16:57:51.000000000 -0400
>> @@ -14,7 +14,7 @@
>>  
>>  int main(int argc, char **argv)
>>  {
>> -	int i, rc = 0, active, pending, len = 0, opt;
>> +	int i, get_all = 0, rc = 0, active, pending, len = 0, opt;
>>  	char **names;
>>  
>>  	while ((opt = getopt(argc, argv, "a")) > 0) {
>> @@ -39,6 +39,7 @@
>>  				printf("No booleans\n");
>>  				return 0;
>>  			}
>> +			get_all = 1;
>>  			break;
>>  		default:
>>  			usage(argv[0]);
>> @@ -72,6 +73,8 @@
>>  	for (i = 0; i < len; i++) {
>>  		active = security_get_boolean_active(names[i]);
>>  		if (active < 0) {
>> +			if (! get_all && errno == EACCES) 
>> +				continue;
>>     
>
> Shouldn't that be if (get_all && errno == EACCES)?
> The goal is to only suppress the warnings upon getsebool -a, not the
> other way around.
>
>   
Yes. 
>>  			fprintf(stderr, "Error getting active value for %s\n",
>>  				names[i]);
>>  			rc = -1;
>>     


--
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: libselinux patch

[Stephen Smalley]

On Thu, 2007-04-05 at 17:00 -0400, Daniel J Walsh wrote:
> Second pass.
> 
> 
> plain text document attachment (diff)

> diff --exclude-from=exclude -N -u -r nsalibselinux/utils/getsebool.c libselinux-2.0.9/utils/getsebool.c
> --- nsalibselinux/utils/getsebool.c	2006-11-16 17:15:17.000000000 -0500
> +++ libselinux-2.0.9/utils/getsebool.c	2007-04-05 16:57:51.000000000 -0400
> @@ -14,7 +14,7 @@
>  
>  int main(int argc, char **argv)
>  {
> -	int i, rc = 0, active, pending, len = 0, opt;
> +	int i, get_all = 0, rc = 0, active, pending, len = 0, opt;
>  	char **names;
>  
>  	while ((opt = getopt(argc, argv, "a")) > 0) {
> @@ -39,6 +39,7 @@
>  				printf("No booleans\n");
>  				return 0;
>  			}
> +			get_all = 1;
>  			break;
>  		default:
>  			usage(argv[0]);
> @@ -72,6 +73,8 @@
>  	for (i = 0; i < len; i++) {
>  		active = security_get_boolean_active(names[i]);
>  		if (active < 0) {
> +			if (! get_all && errno == EACCES) 
> +				continue;

Shouldn't that be if (get_all && errno == EACCES)?
The goal is to only suppress the warnings upon getsebool -a, not the
other way around.

>  			fprintf(stderr, "Error getting active value for %s\n",
>  				names[i]);
>  			rc = -1;
-- 
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: libselinux patch

[Daniel J Walsh]

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

Second pass.



[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 2675 bytes --]

diff --exclude-from=exclude -N -u -r nsalibselinux/man/man8/matchpathcon.8 libselinux-2.0.9/man/man8/matchpathcon.8
--- nsalibselinux/man/man8/matchpathcon.8	2007-01-17 11:11:35.000000000 -0500
+++ libselinux-2.0.9/man/man8/matchpathcon.8	2007-04-05 13:20:43.000000000 -0400
@@ -28,4 +28,4 @@
 
 .SH "SEE ALSO"
 .BR selinux "(8), "
-.BR mathpathcon "(3), " 
+.BR matchpathcon "(3), " 
diff --exclude-from=exclude -N -u -r nsalibselinux/src/selinuxswig.i libselinux-2.0.9/src/selinuxswig.i
--- nsalibselinux/src/selinuxswig.i	2007-02-22 08:53:23.000000000 -0500
+++ libselinux-2.0.9/src/selinuxswig.i	2007-04-05 16:47:42.000000000 -0400
@@ -115,9 +115,38 @@
 extern const char *selinux_path(void);
 extern int selinux_check_passwd_access(access_vector_t requested);
 extern int checkPasswdAccess(access_vector_t requested);
+
+// This tells SWIG to treat char ** as a special case
+%typemap(python,in) char ** {
+  /* Check if is a list */
+  if (PyList_Check($input)) {
+    int size = PyList_Size($input);
+    int i = 0;
+    $1 = (char **) malloc((size+1)*sizeof(char *));
+    if ($1 == NULL) {
+	PyErr_SetString(PyExc_MemoryError,"Out of memory");
+	return NULL;
+    }
+    for (i = 0; i < size; i++) {
+      PyObject *o = PyList_GetItem($input,i);
+      if (PyString_Check(o))
+	$1[i] = PyString_AsString(PyList_GetItem($input,i));
+      else {
+	PyErr_SetString(PyExc_TypeError,"list must contain strings");
+	free($1);
+	return NULL;
+      }
+    }
+    $1[i] = 0;
+  } else {
+    PyErr_SetString(PyExc_TypeError,"not a list");
+    return NULL;
+  }
+}
+
 extern int rpm_execcon(unsigned int verified, 
 		       const char *filename, 
-		       char *const argv[], char *const envp[]);
+		       char **, char **);
 
 extern int is_context_customizable (security_context_t scontext);
 
diff --exclude-from=exclude -N -u -r nsalibselinux/utils/getsebool.c libselinux-2.0.9/utils/getsebool.c
--- nsalibselinux/utils/getsebool.c	2006-11-16 17:15:17.000000000 -0500
+++ libselinux-2.0.9/utils/getsebool.c	2007-04-05 16:57:51.000000000 -0400
@@ -14,7 +14,7 @@
 
 int main(int argc, char **argv)
 {
-	int i, rc = 0, active, pending, len = 0, opt;
+	int i, get_all = 0, rc = 0, active, pending, len = 0, opt;
 	char **names;
 
 	while ((opt = getopt(argc, argv, "a")) > 0) {
@@ -39,6 +39,7 @@
 				printf("No booleans\n");
 				return 0;
 			}
+			get_all = 1;
 			break;
 		default:
 			usage(argv[0]);
@@ -72,6 +73,8 @@
 	for (i = 0; i < len; i++) {
 		active = security_get_boolean_active(names[i]);
 		if (active < 0) {
+			if (! get_all && errno == EACCES) 
+				continue;
 			fprintf(stderr, "Error getting active value for %s\n",
 				names[i]);
 			rc = -1;

Re: libselinux patch

[Stephen Smalley]

On Thu, 2007-04-05 at 13:25 -0400, Daniel J Walsh wrote:
> Fixes for rpm_execcon python binding
> 
> Manpage fix
> 
> Fix for erperm on getsebool.
> 
<snip>
> diff --exclude-from=exclude -N -u -r nsalibselinux/src/selinuxswig.i libselinux-2.0.9/src/selinuxswig.i
> --- nsalibselinux/src/selinuxswig.i	2007-02-22 08:53:23.000000000 -0500
> +++ libselinux-2.0.9/src/selinuxswig.i	2007-04-05 11:53:17.000000000 -0400
> @@ -115,9 +115,34 @@
>  extern const char *selinux_path(void);
>  extern int selinux_check_passwd_access(access_vector_t requested);
>  extern int checkPasswdAccess(access_vector_t requested);
> +
> +// This tells SWIG to treat char ** as a special case
> +%typemap(python,in) char ** {
> +  /* Check if is a list */
> +  if (PyList_Check($input)) {
> +    int size = PyList_Size($input);
> +    int i = 0;
> +    $1 = (char **) malloc((size+1)*sizeof(char *));

What happens if the malloc fails?

<snip>
> diff --exclude-from=exclude -N -u -r nsalibselinux/src/selinuxswig_wrap.c libselinux-2.0.9/src/selinuxswig_wrap.c
> --- nsalibselinux/src/selinuxswig_wrap.c	2007-02-22 08:53:23.000000000 -0500
> +++ libselinux-2.0.9/src/selinuxswig_wrap.c	2007-04-05 11:45:04.000000000 -0400

Don't need to send diffs on this file; will regenerate upstream upon
changes to the .i file.

> diff --exclude-from=exclude -N -u -r nsalibselinux/utils/getsebool.c libselinux-2.0.9/utils/getsebool.c
> --- nsalibselinux/utils/getsebool.c	2006-11-16 17:15:17.000000000 -0500
> +++ libselinux-2.0.9/utils/getsebool.c	2007-04-05 10:53:29.000000000 -0400
> @@ -72,17 +72,23 @@
>  	for (i = 0; i < len; i++) {
>  		active = security_get_boolean_active(names[i]);
>  		if (active < 0) {
> -			fprintf(stderr, "Error getting active value for %s\n",
> -				names[i]);
> -			rc = -1;
> -			goto out;
> +			if (errno != EACCES) { 
> +				fprintf(stderr, "Error getting active value for %s\n",
> +					names[i]);
> +				rc = -1;
> +				goto out;
> +			}
> +			continue;

I think we had discussed only suppressing the errors if using getsebool
-a; if the user explicitly specified a boolean name and it isn't
accessible, they would presumably want to see an error message in that
case.

 		}
>  		pending = security_get_boolean_pending(names[i]);
>  		if (pending < 0) {
> -			fprintf(stderr, "Error getting pending value for %s\n",
> -				names[i]);
> -			rc = -1;
> -			goto out;
> +			if (errno != EACCES) { 
> +				fprintf(stderr, "Error getting pending value for %s\n",
> +					names[i]);
> +				rc = -1;
> +				goto out;
> +			}
> +			continue;
>  		}

Not sure we need this one, because a permission denial should't happen
on pending if it didn't already happen on active.

>  		if (pending != active) {
>  			printf("%s --> %s pending: %s\n", names[i],
-- 
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.

libselinux patch

[Daniel J Walsh]

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

Fixes for rpm_execcon python binding

Manpage fix

Fix for erperm on getsebool.



[-- Attachment #2: diff --]
[-- Type: text/plain, Size: 5722 bytes --]

diff --exclude-from=exclude -N -u -r nsalibselinux/man/man8/matchpathcon.8 libselinux-2.0.9/man/man8/matchpathcon.8
--- nsalibselinux/man/man8/matchpathcon.8	2007-01-17 11:11:35.000000000 -0500
+++ libselinux-2.0.9/man/man8/matchpathcon.8	2007-04-05 13:19:29.000000000 -0400
@@ -28,4 +28,4 @@
 
 .SH "SEE ALSO"
 .BR selinux "(8), "
-.BR mathpathcon "(3), " 
+.BR matchpathcon "(3), " 
diff --exclude-from=exclude -N -u -r nsalibselinux/src/selinuxswig.i libselinux-2.0.9/src/selinuxswig.i
--- nsalibselinux/src/selinuxswig.i	2007-02-22 08:53:23.000000000 -0500
+++ libselinux-2.0.9/src/selinuxswig.i	2007-04-05 11:53:17.000000000 -0400
@@ -115,9 +115,34 @@
 extern const char *selinux_path(void);
 extern int selinux_check_passwd_access(access_vector_t requested);
 extern int checkPasswdAccess(access_vector_t requested);
+
+// This tells SWIG to treat char ** as a special case
+%typemap(python,in) char ** {
+  /* Check if is a list */
+  if (PyList_Check($input)) {
+    int size = PyList_Size($input);
+    int i = 0;
+    $1 = (char **) malloc((size+1)*sizeof(char *));
+    for (i = 0; i < size; i++) {
+      PyObject *o = PyList_GetItem($input,i);
+      if (PyString_Check(o))
+	$1[i] = PyString_AsString(PyList_GetItem($input,i));
+      else {
+	PyErr_SetString(PyExc_TypeError,"list must contain strings");
+	free($1);
+	return NULL;
+      }
+    }
+    $1[i] = 0;
+  } else {
+    PyErr_SetString(PyExc_TypeError,"not a list");
+    return NULL;
+  }
+}
+
 extern int rpm_execcon(unsigned int verified, 
 		       const char *filename, 
-		       char *const argv[], char *const envp[]);
+		       char **, char **);
 
 extern int is_context_customizable (security_context_t scontext);
 
diff --exclude-from=exclude -N -u -r nsalibselinux/src/selinuxswig_wrap.c libselinux-2.0.9/src/selinuxswig_wrap.c
--- nsalibselinux/src/selinuxswig_wrap.c	2007-02-22 08:53:23.000000000 -0500
+++ libselinux-2.0.9/src/selinuxswig_wrap.c	2007-04-05 11:45:04.000000000 -0400
@@ -4145,18 +4145,14 @@
   PyObject *resultobj = 0;
   unsigned int arg1 ;
   char *arg2 = (char *) 0 ;
-  char **arg3 ;
-  char **arg4 ;
+  char **arg3 = (char **) 0 ;
+  char **arg4 = (char **) 0 ;
   int result;
   unsigned int val1 ;
   int ecode1 = 0 ;
   int res2 ;
   char *buf2 = 0 ;
   int alloc2 = 0 ;
-  void *argp3 = 0 ;
-  int res3 = 0 ;
-  void *argp4 = 0 ;
-  int res4 = 0 ;
   PyObject * obj0 = 0 ;
   PyObject * obj1 = 0 ;
   PyObject * obj2 = 0 ;
@@ -4173,17 +4169,51 @@
     SWIG_exception_fail(SWIG_ArgError(res2), "in method '" "rpm_execcon" "', argument " "2"" of type '" "char const *""'");
   }
   arg2 = (char *)(buf2);
-  res3 = SWIG_ConvertPtr(obj2, &argp3,SWIGTYPE_p_p_char, 0 |  0 );
-  if (!SWIG_IsOK(res3)) {
-    SWIG_exception_fail(SWIG_ArgError(res3), "in method '" "rpm_execcon" "', argument " "3"" of type '" "char *const []""'"); 
-  } 
-  arg3 = (char **)(argp3);
-  res4 = SWIG_ConvertPtr(obj3, &argp4,SWIGTYPE_p_p_char, 0 |  0 );
-  if (!SWIG_IsOK(res4)) {
-    SWIG_exception_fail(SWIG_ArgError(res4), "in method '" "rpm_execcon" "', argument " "4"" of type '" "char *const []""'"); 
-  } 
-  arg4 = (char **)(argp4);
-  result = (int)rpm_execcon(arg1,(char const *)arg2,(char *const (*))arg3,(char *const (*))arg4);
+  {
+    /* Check if is a list */
+    if (PyList_Check(obj2)) {
+      int size = PyList_Size(obj2);
+      int i = 0;
+      arg3 = (char **) malloc((size+1)*sizeof(char *));
+      for (i = 0; i < size; i++) {
+        PyObject *o = PyList_GetItem(obj2,i);
+        if (PyString_Check(o))
+        arg3[i] = PyString_AsString(PyList_GetItem(obj2,i));
+        else {
+          PyErr_SetString(PyExc_TypeError,"list must contain strings");
+          free(arg3);
+          return NULL;
+        }
+      }
+      arg3[i] = 0;
+    } else {
+      PyErr_SetString(PyExc_TypeError,"not a list");
+      return NULL;
+    }
+  }
+  {
+    /* Check if is a list */
+    if (PyList_Check(obj3)) {
+      int size = PyList_Size(obj3);
+      int i = 0;
+      arg4 = (char **) malloc((size+1)*sizeof(char *));
+      for (i = 0; i < size; i++) {
+        PyObject *o = PyList_GetItem(obj3,i);
+        if (PyString_Check(o))
+        arg4[i] = PyString_AsString(PyList_GetItem(obj3,i));
+        else {
+          PyErr_SetString(PyExc_TypeError,"list must contain strings");
+          free(arg4);
+          return NULL;
+        }
+      }
+      arg4[i] = 0;
+    } else {
+      PyErr_SetString(PyExc_TypeError,"not a list");
+      return NULL;
+    }
+  }
+  result = (int)rpm_execcon(arg1,(char const *)arg2,arg3,arg4);
   resultobj = SWIG_From_int((int)(result));
   if (alloc2 == SWIG_NEWOBJ) free((char*)buf2);
   return resultobj;
diff --exclude-from=exclude -N -u -r nsalibselinux/utils/getsebool.c libselinux-2.0.9/utils/getsebool.c
--- nsalibselinux/utils/getsebool.c	2006-11-16 17:15:17.000000000 -0500
+++ libselinux-2.0.9/utils/getsebool.c	2007-04-05 10:53:29.000000000 -0400
@@ -72,17 +72,23 @@
 	for (i = 0; i < len; i++) {
 		active = security_get_boolean_active(names[i]);
 		if (active < 0) {
-			fprintf(stderr, "Error getting active value for %s\n",
-				names[i]);
-			rc = -1;
-			goto out;
+			if (errno != EACCES) { 
+				fprintf(stderr, "Error getting active value for %s\n",
+					names[i]);
+				rc = -1;
+				goto out;
+			}
+			continue;
 		}
 		pending = security_get_boolean_pending(names[i]);
 		if (pending < 0) {
-			fprintf(stderr, "Error getting pending value for %s\n",
-				names[i]);
-			rc = -1;
-			goto out;
+			if (errno != EACCES) { 
+				fprintf(stderr, "Error getting pending value for %s\n",
+					names[i]);
+				rc = -1;
+				goto out;
+			}
+			continue;
 		}
 		if (pending != active) {
 			printf("%s --> %s pending: %s\n", names[i],