[PATCH 0/3] libselinux: quirks of the status page

[PATCH 0/3] libselinux: quirks of the status page

[Christian Göttsche]

Dominick Grift made me over IRC aware of the issue that systemd on
Fedora 34 no longer updates its selabel database automatically on
SELinux policy reloads.
The issue is caused by libselinux 3.2 defaulting to use the status page
instead of a netlink socket for reload/enforcing change queries[1].
I prepared a patch for systemd over at [2].

While writing the patch I noticed two possible issues:

1. selinux_status_open(3) is not reentrant
selinux_status_open() unconditionally calls mmap(2), regardless whether
the page is already opened.
selinux_status_open() might get called multiple times by a client
application unintentionally, e.g. once manually to be able to call
selinux_status_updated(3) and react to changes, and indirectly by
calling selinux_check_access(3), which calls avc_open(3), which since
3.2[1] also calls selinux_status_open().

2. In fallback mode selinux_status_open(3) sets internal callbacks
If selinux_status_open() gets called with fallback enabled and the
fallback is actually used, it sets the two callbacks for
SELINUX_CB_SETENFORCE and SELINUX_CB_POLICYLOAD.
These might be later overridden by client applications, which want to
install their own callbacks.
avc_open(3) since 3.2 calls selinux_status_open() with fallback mode
enabled.

[1]: https://github.com/SELinuxProject/selinux/commit/05bdc03130d741e53e1fb45a958d0a2c184be503
[2]: https://github.com/systemd/systemd/pull/19551

Christian Göttsche (3):
  libselinux: avc_destroy(3) closes status page
  libselinux: make selinux_status_open(3) reentrant
  libselinux: do not use status page fallback mode internally

 libselinux/man/man3/avc_open.3 | 3 +++
 libselinux/src/avc.c           | 2 +-
 libselinux/src/sestatus.c      | 4 ++++
 3 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH 1/3] libselinux: avc_destroy(3) closes status page

[Christian Göttsche]

Mention in the manpage of avc_destroy(3) that it does close the SELinux
status page, which might have been opened manually by the client
application.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/man/man3/avc_open.3 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libselinux/man/man3/avc_open.3 b/libselinux/man/man3/avc_open.3
index 3090dd50..55683bb6 100644
--- a/libselinux/man/man3/avc_open.3
+++ b/libselinux/man/man3/avc_open.3
@@ -26,6 +26,9 @@ initializes the userspace AVC and must be called before any other AVC operation
 destroys the userspace AVC, freeing all internal memory structures.  After this call has been made, 
 .BR avc_open ()
 must be called again before any AVC operations can be performed.
+.BR avc_destroy ()
+also closes the SELinux status page, which might have been opened manually by
+.BR selinux_status_open (3).
 
 .BR avc_reset ()
 flushes the userspace AVC, causing it to forget any cached access decisions.  The userspace AVC normally calls this function automatically when needed, see
-- 
2.31.1

Re: [PATCH 1/3] libselinux: avc_destroy(3) closes status page

[Petr Lautrbach]

Christian Göttsche <cgzones@googlemail.com> writes:

> Mention in the manpage of avc_destroy(3) that it does close the SELinux
> status page, which might have been opened manually by the client
> application.
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>

Acked-by: Petr Lautrbach <plautrba@redhat.com>


> ---
>  libselinux/man/man3/avc_open.3 | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/libselinux/man/man3/avc_open.3 b/libselinux/man/man3/avc_open.3
> index 3090dd50..55683bb6 100644
> --- a/libselinux/man/man3/avc_open.3
> +++ b/libselinux/man/man3/avc_open.3
> @@ -26,6 +26,9 @@ initializes the userspace AVC and must be called before any other AVC operation
>  destroys the userspace AVC, freeing all internal memory structures.  After this call has been made, 
>  .BR avc_open ()
>  must be called again before any AVC operations can be performed.
> +.BR avc_destroy ()
> +also closes the SELinux status page, which might have been opened manually by
> +.BR selinux_status_open (3).
>  
>  .BR avc_reset ()
>  flushes the userspace AVC, causing it to forget any cached access decisions.  The userspace AVC normally calls this function automatically when needed, see
> -- 
> 2.31.1

[PATCH 2/3] libselinux: make selinux_status_open(3) reentrant

[Christian Göttsche]

Do not mmap the status page again if `selinux_status_open(3)` has already
been called with success.

`selinux_status_open(3)` might be called unintentionally multiple times,
e.g. once to manually be able to call `selinux_status_getenforce(3)` and
once indirectly through `selinux_check_access(3)`
(since libselinux 3.2).

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/sestatus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
index 12b015e0..531a522c 100644
--- a/libselinux/src/sestatus.c
+++ b/libselinux/src/sestatus.c
@@ -282,6 +282,10 @@ int selinux_status_open(int fallback)
 	long		pagesize;
 	uint32_t	seqno;
 
+	if (selinux_status != NULL) {
+		return 0;
+	}
+
 	if (!selinux_mnt) {
 		errno = ENOENT;
 		return -1;
-- 
2.31.1

Re: [PATCH 2/3] libselinux: make selinux_status_open(3) reentrant

[Petr Lautrbach]

Christian Göttsche <cgzones@googlemail.com> writes:

> Do not mmap the status page again if `selinux_status_open(3)` has already
> been called with success.
>
> `selinux_status_open(3)` might be called unintentionally multiple times,
> e.g. once to manually be able to call `selinux_status_getenforce(3)` and
> once indirectly through `selinux_check_access(3)`
> (since libselinux 3.2).
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

Acked-by: Petr Lautrbach <plautrba@redhat.com>


> ---
>  libselinux/src/sestatus.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/libselinux/src/sestatus.c b/libselinux/src/sestatus.c
> index 12b015e0..531a522c 100644
> --- a/libselinux/src/sestatus.c
> +++ b/libselinux/src/sestatus.c
> @@ -282,6 +282,10 @@ int selinux_status_open(int fallback)
>  	long		pagesize;
>  	uint32_t	seqno;
>  
> +	if (selinux_status != NULL) {
> +		return 0;
> +	}
> +
>  	if (!selinux_mnt) {
>  		errno = ENOENT;
>  		return -1;
> -- 
> 2.31.1

[PATCH 3/3] libselinux: do not use status page fallback mode internally

[Christian Göttsche]

Currently `avc_init_internal()`, called by `avc_open(3)` and
`avc_init(3)`, does open the SELinux status page with fallback mode
enabled.

Quote from man:selinux_status_open(3):
    In this case, this function tries to open a netlink socket using
    .BR avc_netlink_open (3) and overwrite corresponding callbacks
    (setenforce and policyload).  Thus, we need to pay attention to the
    interaction with these interfaces, when fallback mode is enabled.

Calling `selinux_status_open` internally in fallback mode is bad, cause
it overrides callbacks from client applications or the internal
fallback-callbacks get overridden by client applications.
Note that `avc_open(3)` gets called under the hood by
`selinux_check_access(3)` without checking for failure.
Also the status page is available since Linux 2.6.37, so failures of
`selinux_status_open(3)` in non-fallback mode should only be caused by
policies not allowing the client process to open/read/map
the /sys/fs/selinux/status file.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/avc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
index 8314d7ba..daaedbc6 100644
--- a/libselinux/src/avc.c
+++ b/libselinux/src/avc.c
@@ -214,7 +214,7 @@ static int avc_init_internal(const char *prefix,
 		avc_enforcing = rc;
 	}
 
-	rc = selinux_status_open(1);
+	rc = selinux_status_open(0);
 	if (rc < 0) {
 		avc_log(SELINUX_ERROR,
 			"%s: could not open selinux status page: %d (%s)\n",
-- 
2.31.1

Re: [PATCH 3/3] libselinux: do not use status page fallback mode internally

[Petr Lautrbach]

Christian Göttsche <cgzones@googlemail.com> writes:

> Currently `avc_init_internal()`, called by `avc_open(3)` and
> `avc_init(3)`, does open the SELinux status page with fallback mode
> enabled.
>
> Quote from man:selinux_status_open(3):
>     In this case, this function tries to open a netlink socket using
>     .BR avc_netlink_open (3) and overwrite corresponding callbacks
>     (setenforce and policyload).  Thus, we need to pay attention to the
>     interaction with these interfaces, when fallback mode is enabled.
>
> Calling `selinux_status_open` internally in fallback mode is bad, cause
> it overrides callbacks from client applications or the internal
> fallback-callbacks get overridden by client applications.
> Note that `avc_open(3)` gets called under the hood by
> `selinux_check_access(3)` without checking for failure.
> Also the status page is available since Linux 2.6.37, so failures of
> `selinux_status_open(3)` in non-fallback mode should only be caused by
> policies not allowing the client process to open/read/map
> the /sys/fs/selinux/status file.

Acked-by: Petr Lautrbach <plautrba@redhat.com>

All 3 are merged now.

Thanks!

> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  libselinux/src/avc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
> index 8314d7ba..daaedbc6 100644
> --- a/libselinux/src/avc.c
> +++ b/libselinux/src/avc.c
> @@ -214,7 +214,7 @@ static int avc_init_internal(const char *prefix,
>  		avc_enforcing = rc;
>  	}
>  
> -	rc = selinux_status_open(1);
> +	rc = selinux_status_open(0);
>  	if (rc < 0) {
>  		avc_log(SELINUX_ERROR,
>  			"%s: could not open selinux status page: %d (%s)\n",
> -- 
> 2.31.1