[PATCH] libselinux: Use sestatus if open

[PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
/selinux/status") introduced the selinux_status page mechanism, which
allows for mmap()'ing of selinux status state as a replacement for
avc_netlink.

The mechanism was initially intended for use by userspace object
managers which were calculating access decisions in-tree and did not
rely on the libselinux AVC implementation. In order to properly make use
of sestatus within avc_has_perm, the status mechanism needs to properly
set avc internals during status events; else, avc_enforcing is never
updated upon sestatus changes.

This commit moves the netlink notice logic out into convenience
functions, which are then called by the sestatus code. Since sestatus
uses netlink as a fallback, we can change the avc_netlink_check_nb()
call in avc_has_perm_noaudit to check the status page if it is
available. If it is not, we fall back to

Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
---
 libselinux/src/avc.c          |  2 +-
 libselinux/src/avc_internal.c | 71 ++++++++++++++++++++++-------------
 libselinux/src/avc_internal.h |  4 ++
 libselinux/src/sestatus.c     | 18 +++++++--
 4 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
index b4648b2d..1fceac20 100644
--- a/libselinux/src/avc.c
+++ b/libselinux/src/avc.c
@@ -766,7 +766,7 @@ int avc_has_perm_noaudit(security_id_t ssid,
 		avd_init(avd);
 
 	if (!avc_using_threads && !avc_app_main_loop) {
-		(void)avc_netlink_check_nb();
+		(void)selinux_status_updated();
 	}
 
 	if (!aeref) {
diff --git a/libselinux/src/avc_internal.c b/libselinux/src/avc_internal.c
index 568a3d92..aee01a8a 100644
--- a/libselinux/src/avc_internal.c
+++ b/libselinux/src/avc_internal.c
@@ -53,6 +53,49 @@ int avc_enforcing = 1;
 int avc_setenforce = 0;
 int avc_netlink_trouble = 0;
 
+/* process setenforce events for netlink and sestatus */
+int avc_process_setenforce(int enforcing)
+{
+	int rc = 0;
+
+	avc_log(SELINUX_INFO,
+		"%s:  received setenforce notice (enforcing=%d)\n",
+		avc_prefix, enforcing);
+	if (avc_setenforce)
+		goto out;
+	avc_enforcing = enforcing;
+	if (avc_enforcing && (rc = avc_ss_reset(0)) < 0) {
+		avc_log(SELINUX_ERROR,
+			"%s:  cache reset returned %d (errno %d)\n",
+			avc_prefix, rc, errno);
+		return rc;
+	}
+
+out:
+	return selinux_netlink_setenforce(enforcing);
+}
+
+/* process policyload events for netlink and sestatus */
+int avc_process_policyload(uint32_t seqno)
+{
+	int rc = 0;
+
+	avc_log(SELINUX_INFO,
+		"%s:  received policyload notice (seqno=%u)\n",
+		avc_prefix, seqno);
+	rc = avc_ss_reset(seqno);
+	if (rc < 0) {
+		avc_log(SELINUX_ERROR,
+			"%s:  cache reset returned %d (errno %d)\n",
+			avc_prefix, rc, errno);
+		return rc;
+	}
+
+	selinux_flush_class_cache();
+
+	return selinux_netlink_policyload(seqno);
+}
+
 /* netlink socket code */
 static int fd = -1;
 
@@ -177,20 +220,7 @@ static int avc_netlink_process(void *buf)
 
 	case SELNL_MSG_SETENFORCE:{
 		struct selnl_msg_setenforce *msg = NLMSG_DATA(nlh);
-		msg->val = !!msg->val;
-		avc_log(SELINUX_INFO,
-			"%s:  received setenforce notice (enforcing=%d)\n",
-			avc_prefix, msg->val);
-		if (avc_setenforce)
-			break;
-		avc_enforcing = msg->val;
-		if (avc_enforcing && (rc = avc_ss_reset(0)) < 0) {
-			avc_log(SELINUX_ERROR,
-				"%s:  cache reset returned %d (errno %d)\n",
-				avc_prefix, rc, errno);
-			return rc;
-		}
-		rc = selinux_netlink_setenforce(msg->val);
+		rc = avc_process_setenforce(!!msg->val);
 		if (rc < 0)
 			return rc;
 		break;
@@ -198,18 +228,7 @@ static int avc_netlink_process(void *buf)
 
 	case SELNL_MSG_POLICYLOAD:{
 		struct selnl_msg_policyload *msg = NLMSG_DATA(nlh);
-		avc_log(SELINUX_INFO,
-			"%s:  received policyload notice (seqno=%u)\n",
-			avc_prefix, msg->seqno);
-		rc = avc_ss_reset(msg->seqno);
-		if (rc < 0) {
-			avc_log(SELINUX_ERROR,
-				"%s:  cache reset returned %d (errno %d)\n",
-				avc_prefix, rc, errno);
-			return rc;
-		}
-		selinux_flush_class_cache();
-		rc = selinux_netlink_policyload(msg->seqno);
+		rc = avc_process_policyload(msg->seqno);
 		if (rc < 0)
 			return rc;
 		break;
diff --git a/libselinux/src/avc_internal.h b/libselinux/src/avc_internal.h
index 3f8a6bb1..da67affc 100644
--- a/libselinux/src/avc_internal.h
+++ b/libselinux/src/avc_internal.h
@@ -32,6 +32,10 @@ extern void (*avc_func_get_lock) (void *);
 extern void (*avc_func_release_lock) (void *);
 extern void (*avc_func_free_lock) (void *);
 
+/* selinux status processing for netlink and sestatus */
+extern int avc_process_setenforce(int enforcing);
+extern int avc_process_policyload(uint32_t seqno);
+
 static inline void set_callbacks(const struct avc_memory_callback *mem_cb,
 				 const struct avc_log_callback *log_cb,
 				 const struct avc_thread_callback *thread_cb,
diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
index 86267ff8..4bd2086c 100644
--- a/libselinux/src/sestatus.c
+++ b/libselinux/src/sestatus.c
@@ -39,6 +39,7 @@ struct selinux_status_t
 static struct selinux_status_t *selinux_status = NULL;
 static int			selinux_status_fd;
 static uint32_t			last_seqno;
+static uint32_t			last_policyload;
 
 static uint32_t			fallback_sequence;
 static int			fallback_enforcing;
@@ -116,6 +117,15 @@ int selinux_status_updated(void)
 
 	if (last_seqno != curr_seqno)
 	{
+		if (avc_enforcing != !!selinux_status->enforcing) {
+			if (avc_process_setenforce(!!selinux_status->enforcing) < 0)
+				return -1;
+		}
+		if (last_policyload != selinux_status->policyload) {
+			if (avc_process_policyload(selinux_status->policyload) < 0)
+				return -1;
+			last_policyload = selinux_status->policyload;
+		}
 		last_seqno = curr_seqno;
 		result = 1;
 	}
@@ -131,7 +141,6 @@ int selinux_status_updated(void)
 int selinux_status_getenforce(void)
 {
 	uint32_t	seqno;
-	uint32_t	enforcing;
 
 	if (selinux_status == NULL) {
 		errno = EINVAL;
@@ -149,11 +158,11 @@ int selinux_status_getenforce(void)
 	do {
 		seqno = read_sequence(selinux_status);
 
-		enforcing = selinux_status->enforcing;
+		avc_enforcing = !!selinux_status->enforcing;
 
 	} while (seqno != read_sequence(selinux_status));
 
-	return enforcing ? 1 : 0;
+	return avc_enforcing;
 }
 
 /*
@@ -285,6 +294,9 @@ int selinux_status_open(int fallback)
 	return 0;
 
 error:
+	avc_log(SELINUX_WARNING,
+		"%s: could not open selinux status page: %d (%s)\n",
+		avc_prefix, errno, strerror(errno));
 	/*
 	 * If caller wants fallback routine, we try to provide
 	 * an equivalent functionality using existing netlink
-- 
2.27.0

[PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
/selinux/status") introduced the selinux_status page mechanism, which
allows for mmap()'ing of selinux status state as a replacement for
avc_netlink.

The mechanism was initially intended for use by userspace object
managers which were calculating access decisions in-tree and did not
rely on the libselinux AVC implementation. In order to properly make use
of sestatus within avc_has_perm, the status mechanism needs to properly
set avc internals during status events; else, avc_enforcing is never
updated upon sestatus changes.

This commit moves the netlink notice logic out into convenience
functions, which are then called by the sestatus code. Since sestatus
uses netlink as a fallback, we can change the avc_netlink_check_nb()
call in avc_has_perm_noaudit to check the status page if it is
available. If it is not, we fall back to

Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
---
 libselinux/src/avc.c          |  2 +-
 libselinux/src/avc_internal.c | 71 ++++++++++++++++++++++-------------
 libselinux/src/avc_internal.h |  4 ++
 libselinux/src/sestatus.c     | 18 +++++++--
 4 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
index b4648b2d..1fceac20 100644
--- a/libselinux/src/avc.c
+++ b/libselinux/src/avc.c
@@ -766,7 +766,7 @@ int avc_has_perm_noaudit(security_id_t ssid,
 		avd_init(avd);
 
 	if (!avc_using_threads && !avc_app_main_loop) {
-		(void)avc_netlink_check_nb();
+		(void)selinux_status_updated();
 	}
 
 	if (!aeref) {
diff --git a/libselinux/src/avc_internal.c b/libselinux/src/avc_internal.c
index 568a3d92..aee01a8a 100644
--- a/libselinux/src/avc_internal.c
+++ b/libselinux/src/avc_internal.c
@@ -53,6 +53,49 @@ int avc_enforcing = 1;
 int avc_setenforce = 0;
 int avc_netlink_trouble = 0;
 
+/* process setenforce events for netlink and sestatus */
+int avc_process_setenforce(int enforcing)
+{
+	int rc = 0;
+
+	avc_log(SELINUX_INFO,
+		"%s:  received setenforce notice (enforcing=%d)\n",
+		avc_prefix, enforcing);
+	if (avc_setenforce)
+		goto out;
+	avc_enforcing = enforcing;
+	if (avc_enforcing && (rc = avc_ss_reset(0)) < 0) {
+		avc_log(SELINUX_ERROR,
+			"%s:  cache reset returned %d (errno %d)\n",
+			avc_prefix, rc, errno);
+		return rc;
+	}
+
+out:
+	return selinux_netlink_setenforce(enforcing);
+}
+
+/* process policyload events for netlink and sestatus */
+int avc_process_policyload(uint32_t seqno)
+{
+	int rc = 0;
+
+	avc_log(SELINUX_INFO,
+		"%s:  received policyload notice (seqno=%u)\n",
+		avc_prefix, seqno);
+	rc = avc_ss_reset(seqno);
+	if (rc < 0) {
+		avc_log(SELINUX_ERROR,
+			"%s:  cache reset returned %d (errno %d)\n",
+			avc_prefix, rc, errno);
+		return rc;
+	}
+
+	selinux_flush_class_cache();
+
+	return selinux_netlink_policyload(seqno);
+}
+
 /* netlink socket code */
 static int fd = -1;
 
@@ -177,20 +220,7 @@ static int avc_netlink_process(void *buf)
 
 	case SELNL_MSG_SETENFORCE:{
 		struct selnl_msg_setenforce *msg = NLMSG_DATA(nlh);
-		msg->val = !!msg->val;
-		avc_log(SELINUX_INFO,
-			"%s:  received setenforce notice (enforcing=%d)\n",
-			avc_prefix, msg->val);
-		if (avc_setenforce)
-			break;
-		avc_enforcing = msg->val;
-		if (avc_enforcing && (rc = avc_ss_reset(0)) < 0) {
-			avc_log(SELINUX_ERROR,
-				"%s:  cache reset returned %d (errno %d)\n",
-				avc_prefix, rc, errno);
-			return rc;
-		}
-		rc = selinux_netlink_setenforce(msg->val);
+		rc = avc_process_setenforce(!!msg->val);
 		if (rc < 0)
 			return rc;
 		break;
@@ -198,18 +228,7 @@ static int avc_netlink_process(void *buf)
 
 	case SELNL_MSG_POLICYLOAD:{
 		struct selnl_msg_policyload *msg = NLMSG_DATA(nlh);
-		avc_log(SELINUX_INFO,
-			"%s:  received policyload notice (seqno=%u)\n",
-			avc_prefix, msg->seqno);
-		rc = avc_ss_reset(msg->seqno);
-		if (rc < 0) {
-			avc_log(SELINUX_ERROR,
-				"%s:  cache reset returned %d (errno %d)\n",
-				avc_prefix, rc, errno);
-			return rc;
-		}
-		selinux_flush_class_cache();
-		rc = selinux_netlink_policyload(msg->seqno);
+		rc = avc_process_policyload(msg->seqno);
 		if (rc < 0)
 			return rc;
 		break;
diff --git a/libselinux/src/avc_internal.h b/libselinux/src/avc_internal.h
index 3f8a6bb1..da67affc 100644
--- a/libselinux/src/avc_internal.h
+++ b/libselinux/src/avc_internal.h
@@ -32,6 +32,10 @@ extern void (*avc_func_get_lock) (void *);
 extern void (*avc_func_release_lock) (void *);
 extern void (*avc_func_free_lock) (void *);
 
+/* selinux status processing for netlink and sestatus */
+extern int avc_process_setenforce(int enforcing);
+extern int avc_process_policyload(uint32_t seqno);
+
 static inline void set_callbacks(const struct avc_memory_callback *mem_cb,
 				 const struct avc_log_callback *log_cb,
 				 const struct avc_thread_callback *thread_cb,
diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
index 86267ff8..4bd2086c 100644
--- a/libselinux/src/sestatus.c
+++ b/libselinux/src/sestatus.c
@@ -39,6 +39,7 @@ struct selinux_status_t
 static struct selinux_status_t *selinux_status = NULL;
 static int			selinux_status_fd;
 static uint32_t			last_seqno;
+static uint32_t			last_policyload;
 
 static uint32_t			fallback_sequence;
 static int			fallback_enforcing;
@@ -116,6 +117,15 @@ int selinux_status_updated(void)
 
 	if (last_seqno != curr_seqno)
 	{
+		if (avc_enforcing != !!selinux_status->enforcing) {
+			if (avc_process_setenforce(!!selinux_status->enforcing) < 0)
+				return -1;
+		}
+		if (last_policyload != selinux_status->policyload) {
+			if (avc_process_policyload(selinux_status->policyload) < 0)
+				return -1;
+			last_policyload = selinux_status->policyload;
+		}
 		last_seqno = curr_seqno;
 		result = 1;
 	}
@@ -131,7 +141,6 @@ int selinux_status_updated(void)
 int selinux_status_getenforce(void)
 {
 	uint32_t	seqno;
-	uint32_t	enforcing;
 
 	if (selinux_status == NULL) {
 		errno = EINVAL;
@@ -149,11 +158,11 @@ int selinux_status_getenforce(void)
 	do {
 		seqno = read_sequence(selinux_status);
 
-		enforcing = selinux_status->enforcing;
+		avc_enforcing = !!selinux_status->enforcing;
 
 	} while (seqno != read_sequence(selinux_status));
 
-	return enforcing ? 1 : 0;
+	return avc_enforcing;
 }
 
 /*
@@ -285,6 +294,9 @@ int selinux_status_open(int fallback)
 	return 0;
 
 error:
+	avc_log(SELINUX_WARNING,
+		"%s: could not open selinux status page: %d (%s)\n",
+		avc_prefix, errno, strerror(errno));
 	/*
 	 * If caller wants fallback routine, we try to provide
 	 * an equivalent functionality using existing netlink
-- 
2.27.0

Re: [PATCH] libselinux: Use sestatus if open

[Stephen Smalley]

On Tue, Jul 14, 2020 at 4:35 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> /selinux/status") introduced the selinux_status page mechanism, which
> allows for mmap()'ing of selinux status state as a replacement for
> avc_netlink.
>
> The mechanism was initially intended for use by userspace object
> managers which were calculating access decisions in-tree and did not
> rely on the libselinux AVC implementation. In order to properly make use
> of sestatus within avc_has_perm, the status mechanism needs to properly
> set avc internals during status events; else, avc_enforcing is never
> updated upon sestatus changes.
>
> This commit moves the netlink notice logic out into convenience
> functions, which are then called by the sestatus code. Since sestatus
> uses netlink as a fallback, we can change the avc_netlink_check_nb()
> call in avc_has_perm_noaudit to check the status page if it is
> available. If it is not, we fall back to

Missing word/phrase here.  Also you need to do more than just replace
this one call or selinux_status_updated() will do nothing unless the
application has explicitly done a selinux_status_open() itself, e.g.
avc_netlink_open -> selinux_status_open, avc_netlink_close ->
selinux_status_close, deal with other avc_netlink_* calls including
the multi-threaded case.  Finally, I don't think you need to sanitize
the enforcing value from the kernel; it takes care of that itself
these days and no point in fixing it up for old kernels now.

Re: [PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

On Tue, Jul 14, 2020 at 5:03 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Jul 14, 2020 at 4:35 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > /selinux/status") introduced the selinux_status page mechanism, which
> > allows for mmap()'ing of selinux status state as a replacement for
> > avc_netlink.
> >
> > The mechanism was initially intended for use by userspace object
> > managers which were calculating access decisions in-tree and did not
> > rely on the libselinux AVC implementation. In order to properly make use
> > of sestatus within avc_has_perm, the status mechanism needs to properly
> > set avc internals during status events; else, avc_enforcing is never
> > updated upon sestatus changes.
> >
> > This commit moves the netlink notice logic out into convenience
> > functions, which are then called by the sestatus code. Since sestatus
> > uses netlink as a fallback, we can change the avc_netlink_check_nb()
> > call in avc_has_perm_noaudit to check the status page if it is
> > available. If it is not, we fall back to
>
> Missing word/phrase here.

Oops. I'll go ahead and fix that, thank...

> Also you need to do more than just replace
> this one call or selinux_status_updated() will do nothing unless the
> application has explicitly done a selinux_status_open() itself, e.g.
> avc_netlink_open -> selinux_status_open, avc_netlink_close ->
> selinux_status_close, deal with other avc_netlink_* calls including
> the multi-threaded case.

That was by design. I figured for this version of the patch, we could
just introduce the ability to use sestatus instead of netlink (if
open).

Do you think we should go ahead and completely swap in sestatus? I was
just worried about breaking userspace object managers that are
currently using netlink threads by default, for instance
systemd-dbusd.  I can spend some more time getting those to work with
the status page if you think that's worthwhile.

> Finally, I don't think you need to sanitize
> the enforcing value from the kernel; it takes care of that itself
> these days and no point in fixing it up for old kernels now.

Very good to know, thanks! I'll update in the next version.

-- 
Mike Palmiotto
https://crunchydata.com

Re: [PATCH] libselinux: Use sestatus if open

[Stephen Smalley]

On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Tue, Jul 14, 2020 at 5:03 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 4:35 PM Mike Palmiotto
> > <mike.palmiotto@crunchydata.com> wrote:
> > >
> > > Commit bc2a8f418e3b ("libselinux: add selinux_status_* interfaces for
> > > /selinux/status") introduced the selinux_status page mechanism, which
> > > allows for mmap()'ing of selinux status state as a replacement for
> > > avc_netlink.
> > >
> > > The mechanism was initially intended for use by userspace object
> > > managers which were calculating access decisions in-tree and did not
> > > rely on the libselinux AVC implementation. In order to properly make use
> > > of sestatus within avc_has_perm, the status mechanism needs to properly
> > > set avc internals during status events; else, avc_enforcing is never
> > > updated upon sestatus changes.
> > >
> > > This commit moves the netlink notice logic out into convenience
> > > functions, which are then called by the sestatus code. Since sestatus
> > > uses netlink as a fallback, we can change the avc_netlink_check_nb()
> > > call in avc_has_perm_noaudit to check the status page if it is
> > > available. If it is not, we fall back to
> >
> > Missing word/phrase here.
>
> Oops. I'll go ahead and fix that, thank...
>
> > Also you need to do more than just replace
> > this one call or selinux_status_updated() will do nothing unless the
> > application has explicitly done a selinux_status_open() itself, e.g.
> > avc_netlink_open -> selinux_status_open, avc_netlink_close ->
> > selinux_status_close, deal with other avc_netlink_* calls including
> > the multi-threaded case.
>
> That was by design. I figured for this version of the patch, we could
> just introduce the ability to use sestatus instead of netlink (if
> open).

In its current form, your patch replaces one call in
avc_has_perm_noaudit() to avc_netlink_check_nb() with a call to
selinux_status_updated().  With that change alone, for existing
callers of avc_has_perm_noaudit() that do not themselves call
selinux_status_open(), selinux_status_updated() will find that the
status page is NULL and will return immediately with an error. It
won't fall back to probing the netlink socket.  So you either need to
test for an error return and fall back yourself to
avc_netlink_check_nb(), or change selinux_status_updated() to do that,
or replace the use of avc_netlink_open/loop/close in avc.c with
selinux_status_open/close.  Likewise for selinux_check_access().

> Do you think we should go ahead and completely swap in sestatus? I was
> just worried about breaking userspace object managers that are
> currently using netlink threads by default, for instance
> systemd-dbusd.  I can spend some more time getting those to work with
> the status page if you think that's worthwhile.

I'd be interested in understanding the impact of such a change on
existing userspace object managers.  If we can switch the default
behavior for applications that are not explicitly using
avc_netlink_*() interfaces themselves (e.g. they are only using
selinux_check_access or avc_has_perm), then that would be beneficial
since I think it fully removes the need for a system call on the AVC
cache-hit code path.

> > Finally, I don't think you need to sanitize
> > the enforcing value from the kernel; it takes care of that itself
> > these days and no point in fixing it up for old kernels now.
>
> Very good to know, thanks! I'll update in the next version.

Re: [PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

On Tue, Jul 14, 2020 at 5:35 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 5:03 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
<snip>
>
> In its current form, your patch replaces one call in
> avc_has_perm_noaudit() to avc_netlink_check_nb() with a call to
> selinux_status_updated().  With that change alone, for existing
> callers of avc_has_perm_noaudit() that do not themselves call
> selinux_status_open(), selinux_status_updated() will find that the
> status page is NULL and will return immediately with an error. It
> won't fall back to probing the netlink socket.  So you either need to
> test for an error return and fall back yourself to
> avc_netlink_check_nb(), or change selinux_status_updated() to do that,
> or replace the use of avc_netlink_open/loop/close in avc.c with
> selinux_status_open/close.  Likewise for selinux_check_access().

For some reason I completely glossed over the selinux_status == NULL
case. I guess I didn't see errors when testing dbus because verbose
logging wasn't turned on.

I'll take a look at what it would take to move over sestatus entirely
and if it's too tricky/error-prone, _I think_ it should be fine to
move the selinux_status == NULL check down with the MAP_FAILED case so
they both fall through to avc_netlink_check_nb().

>
> > Do you think we should go ahead and completely swap in sestatus? I was
> > just worried about breaking userspace object managers that are
> > currently using netlink threads by default, for instance
> > systemd-dbusd.  I can spend some more time getting those to work with
> > the status page if you think that's worthwhile.
>
> I'd be interested in understanding the impact of such a change on
> existing userspace object managers.  If we can switch the default
> behavior for applications that are not explicitly using
> avc_netlink_*() interfaces themselves (e.g. they are only using
> selinux_check_access or avc_has_perm), then that would be beneficial
> since I think it fully removes the need for a system call on the AVC
> cache-hit code path.

I'll have to do a bit of digging to see how this will affect dbus, et
al. On first blush, it looks like they're just doing
avc_netlink_check_nb() in their watch thread. Presumably other object
managers are doing something similar so we would just need to make
sure there is a netlink fd available.

>
> > > Finally, I don't think you need to sanitize
> > > the enforcing value from the kernel; it takes care of that itself
> > > these days and no point in fixing it up for old kernels now.
> >
> > Very good to know, thanks! I'll update in the next version.



-- 
Mike Palmiotto
https://crunchydata.com

Re: [PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

On Tue, Jul 14, 2020 at 6:42 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Tue, Jul 14, 2020 at 5:35 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
> > <mike.palmiotto@crunchydata.com> wrote:
<snip>
> > > Do you think we should go ahead and completely swap in sestatus? I was
> > > just worried about breaking userspace object managers that are
> > > currently using netlink threads by default, for instance
> > > systemd-dbusd.  I can spend some more time getting those to work with
> > > the status page if you think that's worthwhile.
> >
> > I'd be interested in understanding the impact of such a change on
> > existing userspace object managers.  If we can switch the default
> > behavior for applications that are not explicitly using
> > avc_netlink_*() interfaces themselves (e.g. they are only using
> > selinux_check_access or avc_has_perm), then that would be beneficial
> > since I think it fully removes the need for a system call on the AVC
> > cache-hit code path.
>
> I'll have to do a bit of digging to see how this will affect dbus, et
> al. On first blush, it looks like they're just doing
> avc_netlink_check_nb() in their watch thread. Presumably other object
> managers are doing something similar so we would just need to make
> sure there is a netlink fd available.

So it looks like dbus (at least) is directly checking for a netlink
socket[1], so just doing away with the avc_netlink_open call wouldn't
work out. My thinking is we have two options:

1) Add a new seopt to use sestatus and let userspace object managers opt-in

2) Call both selinux_status_open and avc_netlink_open in
avc_init_internal. This would satisfy the hard requirement for a
netlink socket. Then we can default to using sestatus in all of the
netlink processing paths, as you suggested in your last reply. We
could

Option 2 seems better from the standpoint of using sestatus by
default, but it looks like recvfrom will never be called and the
messages will just sit in kernel memory.

I'm inclined to go with option 1 at this point.

[1] https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/bus/selinux.c#L269

--
Mike Palmiotto
https://crunchydata.com

Re: [PATCH] libselinux: Use sestatus if open

[Stephen Smalley]

On Wed, Jul 15, 2020 at 12:04 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Tue, Jul 14, 2020 at 6:42 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 5:35 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
> > > <mike.palmiotto@crunchydata.com> wrote:
> <snip>
> > > > Do you think we should go ahead and completely swap in sestatus? I was
> > > > just worried about breaking userspace object managers that are
> > > > currently using netlink threads by default, for instance
> > > > systemd-dbusd.  I can spend some more time getting those to work with
> > > > the status page if you think that's worthwhile.
> > >
> > > I'd be interested in understanding the impact of such a change on
> > > existing userspace object managers.  If we can switch the default
> > > behavior for applications that are not explicitly using
> > > avc_netlink_*() interfaces themselves (e.g. they are only using
> > > selinux_check_access or avc_has_perm), then that would be beneficial
> > > since I think it fully removes the need for a system call on the AVC
> > > cache-hit code path.
> >
> > I'll have to do a bit of digging to see how this will affect dbus, et
> > al. On first blush, it looks like they're just doing
> > avc_netlink_check_nb() in their watch thread. Presumably other object
> > managers are doing something similar so we would just need to make
> > sure there is a netlink fd available.
>
> So it looks like dbus (at least) is directly checking for a netlink
> socket[1], so just doing away with the avc_netlink_open call wouldn't
> work out. My thinking is we have two options:
>
> 1) Add a new seopt to use sestatus and let userspace object managers opt-in
>
> 2) Call both selinux_status_open and avc_netlink_open in
> avc_init_internal. This would satisfy the hard requirement for a
> netlink socket. Then we can default to using sestatus in all of the
> netlink processing paths, as you suggested in your last reply. We
> could
>
> Option 2 seems better from the standpoint of using sestatus by
> default, but it looks like recvfrom will never be called and the
> messages will just sit in kernel memory.
>
> I'm inclined to go with option 1 at this point.
>
> [1] https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/bus/selinux.c#L269

Couldn't we change avc_netlink_acquire_fd() to test whether fd hasn't
yet been set (i.e. == -1) and call avc_netlink_open() in that case?
Then dbus would still gets its netlink fd as expected but we wouldn't
need to open it inside of avc_init?

Re: [PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

On Wed, Jul 15, 2020 at 12:28 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, Jul 15, 2020 at 12:04 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 6:42 PM Mike Palmiotto
> > <mike.palmiotto@crunchydata.com> wrote:
> > >
> > > On Tue, Jul 14, 2020 at 5:35 PM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > >
> > > > On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
> > > > <mike.palmiotto@crunchydata.com> wrote:
> > <snip>
> > > > > Do you think we should go ahead and completely swap in sestatus? I was
> > > > > just worried about breaking userspace object managers that are
> > > > > currently using netlink threads by default, for instance
> > > > > systemd-dbusd.  I can spend some more time getting those to work with
> > > > > the status page if you think that's worthwhile.
> > > >
> > > > I'd be interested in understanding the impact of such a change on
> > > > existing userspace object managers.  If we can switch the default
> > > > behavior for applications that are not explicitly using
> > > > avc_netlink_*() interfaces themselves (e.g. they are only using
> > > > selinux_check_access or avc_has_perm), then that would be beneficial
> > > > since I think it fully removes the need for a system call on the AVC
> > > > cache-hit code path.
> > >
> > > I'll have to do a bit of digging to see how this will affect dbus, et
> > > al. On first blush, it looks like they're just doing
> > > avc_netlink_check_nb() in their watch thread. Presumably other object
> > > managers are doing something similar so we would just need to make
> > > sure there is a netlink fd available.
> >
> > So it looks like dbus (at least) is directly checking for a netlink
> > socket[1], so just doing away with the avc_netlink_open call wouldn't
> > work out. My thinking is we have two options:
> >
> > 1) Add a new seopt to use sestatus and let userspace object managers opt-in
> >
> > 2) Call both selinux_status_open and avc_netlink_open in
> > avc_init_internal. This would satisfy the hard requirement for a
> > netlink socket. Then we can default to using sestatus in all of the
> > netlink processing paths, as you suggested in your last reply. We
> > could
> >
> > Option 2 seems better from the standpoint of using sestatus by
> > default, but it looks like recvfrom will never be called and the
> > messages will just sit in kernel memory.
> >
> > I'm inclined to go with option 1 at this point.
> >
> > [1] https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/bus/selinux.c#L269
>
> Couldn't we change avc_netlink_acquire_fd() to test whether fd hasn't
> yet been set (i.e. == -1) and call avc_netlink_open() in that case?
> Then dbus would still gets its netlink fd as expected but we wouldn't
> need to open it inside of avc_init?

Much better. I'll send a new patch up shortly.


-- 
Mike Palmiotto
https://crunchydata.com

Re: [PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

On Wed, Jul 15, 2020 at 1:10 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Wed, Jul 15, 2020 at 12:28 PM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Wed, Jul 15, 2020 at 12:04 PM Mike Palmiotto
> > <mike.palmiotto@crunchydata.com> wrote:
> > >
> > > On Tue, Jul 14, 2020 at 6:42 PM Mike Palmiotto
> > > <mike.palmiotto@crunchydata.com> wrote:
> > > >
> > > > On Tue, Jul 14, 2020 at 5:35 PM Stephen Smalley
> > > > <stephen.smalley.work@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
> > > > > <mike.palmiotto@crunchydata.com> wrote:
> > > <snip>
> > > > > > Do you think we should go ahead and completely swap in sestatus? I was
> > > > > > just worried about breaking userspace object managers that are
> > > > > > currently using netlink threads by default, for instance
> > > > > > systemd-dbusd.  I can spend some more time getting those to work with
> > > > > > the status page if you think that's worthwhile.
> > > > >
> > > > > I'd be interested in understanding the impact of such a change on
> > > > > existing userspace object managers.  If we can switch the default
> > > > > behavior for applications that are not explicitly using
> > > > > avc_netlink_*() interfaces themselves (e.g. they are only using
> > > > > selinux_check_access or avc_has_perm), then that would be beneficial
> > > > > since I think it fully removes the need for a system call on the AVC
> > > > > cache-hit code path.
> > > >
> > > > I'll have to do a bit of digging to see how this will affect dbus, et
> > > > al. On first blush, it looks like they're just doing
> > > > avc_netlink_check_nb() in their watch thread. Presumably other object
> > > > managers are doing something similar so we would just need to make
> > > > sure there is a netlink fd available.
> > >
> > > So it looks like dbus (at least) is directly checking for a netlink
> > > socket[1], so just doing away with the avc_netlink_open call wouldn't
> > > work out. My thinking is we have two options:
> > >
> > > 1) Add a new seopt to use sestatus and let userspace object managers opt-in
> > >
> > > 2) Call both selinux_status_open and avc_netlink_open in
> > > avc_init_internal. This would satisfy the hard requirement for a
> > > netlink socket. Then we can default to using sestatus in all of the
> > > netlink processing paths, as you suggested in your last reply. We
> > > could
> > >
> > > Option 2 seems better from the standpoint of using sestatus by
> > > default, but it looks like recvfrom will never be called and the
> > > messages will just sit in kernel memory.
> > >
> > > I'm inclined to go with option 1 at this point.
> > >
> > > [1] https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/bus/selinux.c#L269
> >
> > Couldn't we change avc_netlink_acquire_fd() to test whether fd hasn't
> > yet been set (i.e. == -1) and call avc_netlink_open() in that case?
> > Then dbus would still gets its netlink fd as expected but we wouldn't
> > need to open it inside of avc_init?
>
> Much better. I'll send a new patch up shortly.

Okay, maybe not that shortly. I tried your suggestion and dbus doesn't
appear to receive the netlink messages.

Initially I figured it had something to do with
avc_create_thread(avc_netlink_loop) call still being in
avc_init_internal, so I exposed the thread pointer in avc_internal.h
and moved the call into avc_netlink_open, which seemed more
appropriate. Still no dice.

I'm probably doing something wrong -- I'll figure it out, but it's
going to take me longer than I thought to track this down.

-- 
Mike Palmiotto
https://crunchydata.com

Re: [PATCH] libselinux: Use sestatus if open

[Stephen Smalley]

On Wed, Jul 15, 2020 at 2:52 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Wed, Jul 15, 2020 at 1:10 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Wed, Jul 15, 2020 at 12:28 PM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Wed, Jul 15, 2020 at 12:04 PM Mike Palmiotto
> > > <mike.palmiotto@crunchydata.com> wrote:
> > > >
> > > > On Tue, Jul 14, 2020 at 6:42 PM Mike Palmiotto
> > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > >
> > > > > On Tue, Jul 14, 2020 at 5:35 PM Stephen Smalley
> > > > > <stephen.smalley.work@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
> > > > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > <snip>
> > > > > > > Do you think we should go ahead and completely swap in sestatus? I was
> > > > > > > just worried about breaking userspace object managers that are
> > > > > > > currently using netlink threads by default, for instance
> > > > > > > systemd-dbusd.  I can spend some more time getting those to work with
> > > > > > > the status page if you think that's worthwhile.
> > > > > >
> > > > > > I'd be interested in understanding the impact of such a change on
> > > > > > existing userspace object managers.  If we can switch the default
> > > > > > behavior for applications that are not explicitly using
> > > > > > avc_netlink_*() interfaces themselves (e.g. they are only using
> > > > > > selinux_check_access or avc_has_perm), then that would be beneficial
> > > > > > since I think it fully removes the need for a system call on the AVC
> > > > > > cache-hit code path.
> > > > >
> > > > > I'll have to do a bit of digging to see how this will affect dbus, et
> > > > > al. On first blush, it looks like they're just doing
> > > > > avc_netlink_check_nb() in their watch thread. Presumably other object
> > > > > managers are doing something similar so we would just need to make
> > > > > sure there is a netlink fd available.
> > > >
> > > > So it looks like dbus (at least) is directly checking for a netlink
> > > > socket[1], so just doing away with the avc_netlink_open call wouldn't
> > > > work out. My thinking is we have two options:
> > > >
> > > > 1) Add a new seopt to use sestatus and let userspace object managers opt-in
> > > >
> > > > 2) Call both selinux_status_open and avc_netlink_open in
> > > > avc_init_internal. This would satisfy the hard requirement for a
> > > > netlink socket. Then we can default to using sestatus in all of the
> > > > netlink processing paths, as you suggested in your last reply. We
> > > > could
> > > >
> > > > Option 2 seems better from the standpoint of using sestatus by
> > > > default, but it looks like recvfrom will never be called and the
> > > > messages will just sit in kernel memory.
> > > >
> > > > I'm inclined to go with option 1 at this point.
> > > >
> > > > [1] https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/bus/selinux.c#L269
> > >
> > > Couldn't we change avc_netlink_acquire_fd() to test whether fd hasn't
> > > yet been set (i.e. == -1) and call avc_netlink_open() in that case?
> > > Then dbus would still gets its netlink fd as expected but we wouldn't
> > > need to open it inside of avc_init?
> >
> > Much better. I'll send a new patch up shortly.
>
> Okay, maybe not that shortly. I tried your suggestion and dbus doesn't
> appear to receive the netlink messages.
>
> Initially I figured it had something to do with
> avc_create_thread(avc_netlink_loop) call still being in
> avc_init_internal, so I exposed the thread pointer in avc_internal.h
> and moved the call into avc_netlink_open, which seemed more
> appropriate. Still no dice.
>
> I'm probably doing something wrong -- I'll figure it out, but it's
> going to take me longer than I thought to track this down.

Recommend writing a little test program to exercise it first without
bringing in all of the dbus baggage.  Don't think Fedora is even using
dbusd anymore, just dbus-broker.
If you get stuck, feel free to send your patch and I can take a look too.

Re: [PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

On Wed, Jul 15, 2020 at 3:46 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, Jul 15, 2020 at 2:52 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Wed, Jul 15, 2020 at 1:10 PM Mike Palmiotto
> > <mike.palmiotto@crunchydata.com> wrote:
> > >
> > > On Wed, Jul 15, 2020 at 12:28 PM Stephen Smalley
> > > <stephen.smalley.work@gmail.com> wrote:
> > > >
> > > > On Wed, Jul 15, 2020 at 12:04 PM Mike Palmiotto
> > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > >
> > > > > On Tue, Jul 14, 2020 at 6:42 PM Mike Palmiotto
> > > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > > >
> > > > > > On Tue, Jul 14, 2020 at 5:35 PM Stephen Smalley
> > > > > > <stephen.smalley.work@gmail.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jul 14, 2020 at 5:20 PM Mike Palmiotto
> > > > > > > <mike.palmiotto@crunchydata.com> wrote:
> > > > > <snip>
> > > > > > > > Do you think we should go ahead and completely swap in sestatus? I was
> > > > > > > > just worried about breaking userspace object managers that are
> > > > > > > > currently using netlink threads by default, for instance
> > > > > > > > systemd-dbusd.  I can spend some more time getting those to work with
> > > > > > > > the status page if you think that's worthwhile.
> > > > > > >
> > > > > > > I'd be interested in understanding the impact of such a change on
> > > > > > > existing userspace object managers.  If we can switch the default
> > > > > > > behavior for applications that are not explicitly using
> > > > > > > avc_netlink_*() interfaces themselves (e.g. they are only using
> > > > > > > selinux_check_access or avc_has_perm), then that would be beneficial
> > > > > > > since I think it fully removes the need for a system call on the AVC
> > > > > > > cache-hit code path.
> > > > > >
> > > > > > I'll have to do a bit of digging to see how this will affect dbus, et
> > > > > > al. On first blush, it looks like they're just doing
> > > > > > avc_netlink_check_nb() in their watch thread. Presumably other object
> > > > > > managers are doing something similar so we would just need to make
> > > > > > sure there is a netlink fd available.
> > > > >
> > > > > So it looks like dbus (at least) is directly checking for a netlink
> > > > > socket[1], so just doing away with the avc_netlink_open call wouldn't
> > > > > work out. My thinking is we have two options:
> > > > >
> > > > > 1) Add a new seopt to use sestatus and let userspace object managers opt-in
> > > > >
> > > > > 2) Call both selinux_status_open and avc_netlink_open in
> > > > > avc_init_internal. This would satisfy the hard requirement for a
> > > > > netlink socket. Then we can default to using sestatus in all of the
> > > > > netlink processing paths, as you suggested in your last reply. We
> > > > > could
> > > > >
> > > > > Option 2 seems better from the standpoint of using sestatus by
> > > > > default, but it looks like recvfrom will never be called and the
> > > > > messages will just sit in kernel memory.
> > > > >
> > > > > I'm inclined to go with option 1 at this point.
> > > > >
> > > > > [1] https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/bus/selinux.c#L269
> > > >
> > > > Couldn't we change avc_netlink_acquire_fd() to test whether fd hasn't
> > > > yet been set (i.e. == -1) and call avc_netlink_open() in that case?
> > > > Then dbus would still gets its netlink fd as expected but we wouldn't
> > > > need to open it inside of avc_init?
> > >
> > > Much better. I'll send a new patch up shortly.
> >
> > Okay, maybe not that shortly. I tried your suggestion and dbus doesn't
> > appear to receive the netlink messages.
> >
> > Initially I figured it had something to do with
> > avc_create_thread(avc_netlink_loop) call still being in
> > avc_init_internal, so I exposed the thread pointer in avc_internal.h
> > and moved the call into avc_netlink_open, which seemed more
> > appropriate. Still no dice.
> >
> > I'm probably doing something wrong -- I'll figure it out, but it's
> > going to take me longer than I thought to track this down.
>
> Recommend writing a little test program to exercise it first without
> bringing in all of the dbus baggage.  Don't think Fedora is even using
> dbusd anymore, just dbus-broker.
> If you get stuck, feel free to send your patch and I can take a look too.

Interestingly, the test program is working fine:
https://github.com/mpalmi/selinux/tree/sestatus
https://github.com/mpalmi/sestatus-test

On a test run, I'm seeing both the status page and netlink socket
notifications for load_polcy (twice for each case):

```
 ./test
opened avc successfully
got netlink socket: 4

watching netlink socket for events
avc:  received policyload notice (seqno=3)
policy reload notice received
avc:  received policyload notice (seqno=4)
policy reload notice received
^C
watching sestatus page for events
avc:  received policyload notice (seqno=5)
policy reload notice received
avc:  received policyload notice (seqno=6)
policy reload notice received
^Cclosing netlink socket: 4
destroying avc
goodbye
```

Still seeing the MAC_POLICY_LOAD audit message, but none of the usual
USER_AVC policyload notices.

I'm not certain if I've covered enough, but this is where things
currently stand. I can submit another patch to the list if that's
preferable. I just figured I'd keep the noise down as much as possible
until we figure out what's going on with dbus.

Thanks for all the help

--
Mike Palmiotto
https://crunchydata.com

Re: [PATCH] libselinux: Use sestatus if open

[Stephen Smalley]

On Wed, Jul 15, 2020 at 6:45 PM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
> Interestingly, the test program is working fine:
> https://github.com/mpalmi/selinux/tree/sestatus
> https://github.com/mpalmi/sestatus-test
>
> On a test run, I'm seeing both the status page and netlink socket
> notifications for load_polcy (twice for each case):
>
> ```
>  ./test
> opened avc successfully
> got netlink socket: 4
>
> watching netlink socket for events
> avc:  received policyload notice (seqno=3)
> policy reload notice received
> avc:  received policyload notice (seqno=4)
> policy reload notice received
> ^C
> watching sestatus page for events
> avc:  received policyload notice (seqno=5)
> policy reload notice received
> avc:  received policyload notice (seqno=6)
> policy reload notice received
> ^Cclosing netlink socket: 4
> destroying avc
> goodbye
> ```
>
> Still seeing the MAC_POLICY_LOAD audit message, but none of the usual
> USER_AVC policyload notices.

I only see one notification per load_policy invocation.  What versions
of kernel and dbus are you using?  Are you using dbus-daemon or
dbus-broker?  How are you testing dbus with this change - just doing a
make install relabel of libselinux and restarting dbus-daemon or
dbus-broker, then running load_policy and checking for USER_AVC
messages?  Is this on CentOS 7/8?

Re: [PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

On Thu, Jul 16, 2020 at 8:40 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Wed, Jul 15, 2020 at 6:45 PM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> > Interestingly, the test program is working fine:
> > https://github.com/mpalmi/selinux/tree/sestatus
> > https://github.com/mpalmi/sestatus-test
> >
> > On a test run, I'm seeing both the status page and netlink socket
> > notifications for load_polcy (twice for each case):
> >
> > ```
> >  ./test
> > opened avc successfully
> > got netlink socket: 4
> >
> > watching netlink socket for events
> > avc:  received policyload notice (seqno=3)
> > policy reload notice received
> > avc:  received policyload notice (seqno=4)
> > policy reload notice received
> > ^C
> > watching sestatus page for events
> > avc:  received policyload notice (seqno=5)
> > policy reload notice received
> > avc:  received policyload notice (seqno=6)
> > policy reload notice received
> > ^Cclosing netlink socket: 4
> > destroying avc
> > goodbye
> > ```
> >
> > Still seeing the MAC_POLICY_LOAD audit message, but none of the usual
> > USER_AVC policyload notices.
>
> I only see one notification per load_policy invocation.

I meant I ran load_policy twice for netlink and then twice for
sestatus. You're correct, there is only one notification per
invocation.

> What versions of kernel and dbus are you using? Are you using dbus-daemon or
> dbus-broker?  How are you testing dbus with this change - just doing a
> make install relabel of libselinux and restarting dbus-daemon or
> dbus-broker, then running load_policy and checking for USER_AVC
> messages?  Is this on CentOS 7/8?

Here's my setup:

[vagrant/staff_r/SystemLow@mls:~]$ uname -r
4.18.0-193.el8.x86_64
[vagrant/staff_r/SystemLow@mls:~]$ cat /etc/redhat-release
Red Hat Enterprise Linux release 8.2 (Ootpa)
[vagrant/staff_r/SystemLow@mls:~]$ dbus-daemon --version | head -1
D-Bus Message Bus Daemon 1.12.8
[vagrant/staff_r/SystemLow@mls:~]$ systemctl status --no-pager dbus --full
● dbus.service - D-Bus System Message Bus
   Loaded: loaded (/usr/lib/systemd/system/dbus.service; static;
vendor preset: disabled)
   Active: active (running) since Thu 2020-07-16 01:19:31 UTC; 47min ago
     Docs: man:dbus-daemon(1)
 Main PID: 865 (dbus-daemon)
    Tasks: 1 (limit: 11480)
   Memory: 3.6M
   CGroup: /system.slice/dbus.service
           └─865 /usr/bin/dbus-daemon --system --address=systemd:
--nofork --nopidfile --systemd-activation --syslog-only

This is my test thread:

1) `make clean distclean`
2) `sudo make DEBUG=1 LIBDIR=/usr/lib64 SHLIBDIR=/lib64 install
install-pywrap relabel -s`
3) `sudo restorecon -RF /` (mcstrans apparently doesn't have the
relabel target -- I should submit a patch for that)
5) reboot
6) load_policy/setenforce/setsebool and look for USER_AVC notices in audit.log

Please let me know if there's any other information you need. I'm
still trying to track this down on my end. I will probably build dbus
with debugging and take a look at what's going on.

-- 
Mike Palmiotto
https://crunchydata.com

Re: [PATCH] libselinux: Use sestatus if open

[Stephen Smalley]

On Thu, Jul 16, 2020 at 9:36 AM Mike Palmiotto
<mike.palmiotto@crunchydata.com> wrote:
>
> On Thu, Jul 16, 2020 at 8:40 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
> >
> > On Wed, Jul 15, 2020 at 6:45 PM Mike Palmiotto
> > <mike.palmiotto@crunchydata.com> wrote:
> > > Interestingly, the test program is working fine:
> > > https://github.com/mpalmi/selinux/tree/sestatus
> > > https://github.com/mpalmi/sestatus-test
> > >
> > > On a test run, I'm seeing both the status page and netlink socket
> > > notifications for load_polcy (twice for each case):
> > >
> > > ```
> > >  ./test
> > > opened avc successfully
> > > got netlink socket: 4
> > >
> > > watching netlink socket for events
> > > avc:  received policyload notice (seqno=3)
> > > policy reload notice received
> > > avc:  received policyload notice (seqno=4)
> > > policy reload notice received
> > > ^C
> > > watching sestatus page for events
> > > avc:  received policyload notice (seqno=5)
> > > policy reload notice received
> > > avc:  received policyload notice (seqno=6)
> > > policy reload notice received
> > > ^Cclosing netlink socket: 4
> > > destroying avc
> > > goodbye
> > > ```
> > >
> > > Still seeing the MAC_POLICY_LOAD audit message, but none of the usual
> > > USER_AVC policyload notices.
> >
> > I only see one notification per load_policy invocation.
>
> I meant I ran load_policy twice for netlink and then twice for
> sestatus. You're correct, there is only one notification per
> invocation.
>
> > What versions of kernel and dbus are you using? Are you using dbus-daemon or
> > dbus-broker?  How are you testing dbus with this change - just doing a
> > make install relabel of libselinux and restarting dbus-daemon or
> > dbus-broker, then running load_policy and checking for USER_AVC
> > messages?  Is this on CentOS 7/8?
>
> Here's my setup:
>
> [vagrant/staff_r/SystemLow@mls:~]$ uname -r
> 4.18.0-193.el8.x86_64
> [vagrant/staff_r/SystemLow@mls:~]$ cat /etc/redhat-release
> Red Hat Enterprise Linux release 8.2 (Ootpa)
> [vagrant/staff_r/SystemLow@mls:~]$ dbus-daemon --version | head -1
> D-Bus Message Bus Daemon 1.12.8
> [vagrant/staff_r/SystemLow@mls:~]$ systemctl status --no-pager dbus --full
> ● dbus.service - D-Bus System Message Bus
>    Loaded: loaded (/usr/lib/systemd/system/dbus.service; static;
> vendor preset: disabled)
>    Active: active (running) since Thu 2020-07-16 01:19:31 UTC; 47min ago
>      Docs: man:dbus-daemon(1)
>  Main PID: 865 (dbus-daemon)
>     Tasks: 1 (limit: 11480)
>    Memory: 3.6M
>    CGroup: /system.slice/dbus.service
>            └─865 /usr/bin/dbus-daemon --system --address=systemd:
> --nofork --nopidfile --systemd-activation --syslog-only
>
> This is my test thread:
>
> 1) `make clean distclean`
> 2) `sudo make DEBUG=1 LIBDIR=/usr/lib64 SHLIBDIR=/lib64 install
> install-pywrap relabel -s`
> 3) `sudo restorecon -RF /` (mcstrans apparently doesn't have the
> relabel target -- I should submit a patch for that)
> 5) reboot
> 6) load_policy/setenforce/setsebool and look for USER_AVC notices in audit.log
>
> Please let me know if there's any other information you need. I'm
> still trying to track this down on my end. I will probably build dbus
> with debugging and take a look at what's going on.

That version of dbus did not call avc_netlink_acquire_fd().  It only
calls avc_init() with a thread callback,
with the expectation that avc_init() will create the thread (as it did
prior to your patch).  So you can't move that part.
Not sure what happens if you leave it there.

If you can't make it work cleanly with legacy object managers, then I
guess we can go with the opt-in route.

Re: [PATCH] libselinux: Use sestatus if open

[Stephen Smalley]

On Thu, Jul 16, 2020 at 11:12 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Jul 16, 2020 at 9:36 AM Mike Palmiotto
> <mike.palmiotto@crunchydata.com> wrote:
> >
> > On Thu, Jul 16, 2020 at 8:40 AM Stephen Smalley
> > <stephen.smalley.work@gmail.com> wrote:
> > >
> > > On Wed, Jul 15, 2020 at 6:45 PM Mike Palmiotto
> > > <mike.palmiotto@crunchydata.com> wrote:
> > > > Interestingly, the test program is working fine:
> > > > https://github.com/mpalmi/selinux/tree/sestatus
> > > > https://github.com/mpalmi/sestatus-test
> > > >
> > > > On a test run, I'm seeing both the status page and netlink socket
> > > > notifications for load_polcy (twice for each case):
> > > >
> > > > ```
> > > >  ./test
> > > > opened avc successfully
> > > > got netlink socket: 4
> > > >
> > > > watching netlink socket for events
> > > > avc:  received policyload notice (seqno=3)
> > > > policy reload notice received
> > > > avc:  received policyload notice (seqno=4)
> > > > policy reload notice received
> > > > ^C
> > > > watching sestatus page for events
> > > > avc:  received policyload notice (seqno=5)
> > > > policy reload notice received
> > > > avc:  received policyload notice (seqno=6)
> > > > policy reload notice received
> > > > ^Cclosing netlink socket: 4
> > > > destroying avc
> > > > goodbye
> > > > ```
> > > >
> > > > Still seeing the MAC_POLICY_LOAD audit message, but none of the usual
> > > > USER_AVC policyload notices.
> > >
> > > I only see one notification per load_policy invocation.
> >
> > I meant I ran load_policy twice for netlink and then twice for
> > sestatus. You're correct, there is only one notification per
> > invocation.
> >
> > > What versions of kernel and dbus are you using? Are you using dbus-daemon or
> > > dbus-broker?  How are you testing dbus with this change - just doing a
> > > make install relabel of libselinux and restarting dbus-daemon or
> > > dbus-broker, then running load_policy and checking for USER_AVC
> > > messages?  Is this on CentOS 7/8?
> >
> > Here's my setup:
> >
> > [vagrant/staff_r/SystemLow@mls:~]$ uname -r
> > 4.18.0-193.el8.x86_64
> > [vagrant/staff_r/SystemLow@mls:~]$ cat /etc/redhat-release
> > Red Hat Enterprise Linux release 8.2 (Ootpa)
> > [vagrant/staff_r/SystemLow@mls:~]$ dbus-daemon --version | head -1
> > D-Bus Message Bus Daemon 1.12.8
> > [vagrant/staff_r/SystemLow@mls:~]$ systemctl status --no-pager dbus --full
> > ● dbus.service - D-Bus System Message Bus
> >    Loaded: loaded (/usr/lib/systemd/system/dbus.service; static;
> > vendor preset: disabled)
> >    Active: active (running) since Thu 2020-07-16 01:19:31 UTC; 47min ago
> >      Docs: man:dbus-daemon(1)
> >  Main PID: 865 (dbus-daemon)
> >     Tasks: 1 (limit: 11480)
> >    Memory: 3.6M
> >    CGroup: /system.slice/dbus.service
> >            └─865 /usr/bin/dbus-daemon --system --address=systemd:
> > --nofork --nopidfile --systemd-activation --syslog-only
> >
> > This is my test thread:
> >
> > 1) `make clean distclean`
> > 2) `sudo make DEBUG=1 LIBDIR=/usr/lib64 SHLIBDIR=/lib64 install
> > install-pywrap relabel -s`
> > 3) `sudo restorecon -RF /` (mcstrans apparently doesn't have the
> > relabel target -- I should submit a patch for that)
> > 5) reboot
> > 6) load_policy/setenforce/setsebool and look for USER_AVC notices in audit.log
> >
> > Please let me know if there's any other information you need. I'm
> > still trying to track this down on my end. I will probably build dbus
> > with debugging and take a look at what's going on.
>
> That version of dbus did not call avc_netlink_acquire_fd().  It only
> calls avc_init() with a thread callback,
> with the expectation that avc_init() will create the thread (as it did
> prior to your patch).  So you can't move that part.
> Not sure what happens if you leave it there.

Oh, I see - you'd need to ensure that the netlink socket is created
first, or change the thread function to call
selinux_status_updated() instead of checking netlink.  I guess the
question is what is the actual behavior required.
dbus doesn't care so much whether we are using netlink here but only
that the thread gets created, checks whether
there is a notification, and calls a callback if so.  So it seems that
you could just change avc_init to call selinux_status_open(1),
then if avc_using_threads, create a thread with a function that just
loops on selinux_status_updated() calls.  No need to
call an avc_netlink_* function at all (except in the fallback case
inside of sestatus.c).  Does that make sense?


> If you can't make it work cleanly with legacy object managers, then I
> guess we can go with the opt-in route.

Re: [PATCH] libselinux: Use sestatus if open

[Mike Palmiotto]

On Thu, Jul 16, 2020 at 11:28 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Jul 16, 2020 at 11:12 AM Stephen Smalley
> <stephen.smalley.work@gmail.com> wrote:
<snip>
> >
> > That version of dbus did not call avc_netlink_acquire_fd().  It only
> > calls avc_init() with a thread callback,
> > with the expectation that avc_init() will create the thread (as it did
> > prior to your patch).  So you can't move that part.
> > Not sure what happens if you leave it there.
>
> Oh, I see - you'd need to ensure that the netlink socket is created
> first, or change the thread function to call
> selinux_status_updated() instead of checking netlink.  I guess the
> question is what is the actual behavior required.
> dbus doesn't care so much whether we are using netlink here but only
> that the thread gets created, checks whether
> there is a notification, and calls a callback if so.  So it seems that
> you could just change avc_init to call selinux_status_open(1),
> then if avc_using_threads, create a thread with a function that just
> loops on selinux_status_updated() calls.  No need to
> call an avc_netlink_* function at all (except in the fallback case
> inside of sestatus.c).  Does that make sense?

Yeah, that all makes sense. I'll test it out and hopefully post a new
patch later today.

Thanks again.

-- 
Mike Palmiotto
https://crunchydata.com