pam_tcb: Add support for user authentication with SELinux. #10

besser82 · 2021-10-08T22:00:39Z

NSA Security-Enhanced Linux (SELinux) is an implementation of a flexible mandatory access control architecture in the Linux operating system. Background information and technical documentation about SELinux can be found at https://github.com/SELinuxProject.

With SELinux running in enforced mode even read-access to the shadow files, which the hashed user passwords are stored
in, is restricted to processes that have at least been granted shadow_t:file read capabilites by the SELinux policy. For that
reason the login authentication of a user must always be performed by the tcb_chkpwd helper binary.

pam_tcb/support.c (unix_verify_password_plain): Use the chkpwd helper binary to verify the user's password if SELinux is enabled, and prevents read-access to the file storing the hashed password.
pam_tcb/support.h (SELINUX_ENABLED): Include <selinux/selinux.h> if support for SELinux is requested. Also define SELINUX_ENABLED.
progs/tcb_chkpwd.c (SELINUX_ENABLED): Likewise.
progs/tcb_chkpwd.c (unix_verify_password): Do not fail during authentication when SELinux is enabled and the UID of the user to be authenticated differs from the UID of the actual caller.
Make.defs: Add flag to (optionally) enable support for SELinux.
pam_tcb/Makefile: Apply the needed pre-processor define and link against libselinux if support for SELinux is requested.
progs/Makefile: Likewise.

The needed changes have already been applied to the SELinux reference-policy in SELinuxProject/refpolicy@bc88a1c.

besser82 · 2021-10-08T22:06:12Z

@ldv-alt and @solardiz This should be pretty straigh forward. I'm working on a PR to add the needed bits for making password changes possible with SELinux, too.

@ikerexxe, FYI.

ldv-alt · 2021-10-08T22:11:18Z

I wonder wouldn't we better off if the code was written as if selinux is always enabled?
In terminology of your patch, is there any drawbacks if we assumed SELINUX_ENABLED == 1?

besser82 · 2021-10-08T22:23:26Z

I wonder wouldn't we better off if the code was written as if selinux is always enabled? In terminology of your patch, is there any drawbacks if we assumed SELINUX_ENABLED == 1?

Well, we would lose the sanity checks on systems without SELinux. Otherwise I don't see any.

solardiz · 2021-10-08T23:15:58Z

This sounds dangerous:

	* progs/tcb_chkpwd.c (unix_verify_password): Do not fail during
	authentication when SELinux is enabled and the UID of the user to
	be authenticated differs from the UID of the actual caller.

Also, how would tcb_chkpwd even perform authentication for another user? What privileges would it need and have? Perhaps you mean only the case of using /etc/shadow rather than /etc/tcb? If so, wouldn't these changes be undesirable when using /etc/tcb?

ldv-alt · 2021-10-09T00:17:07Z

I wonder wouldn't we better off if the code was written as if selinux is always enabled? In terminology of your patch, is there any drawbacks if we assumed SELINUX_ENABLED == 1?

Well, we would lose the sanity checks on systems without SELinux. Otherwise I don't see any.

These sanity checks make sense even if selinux is enabled, they just have to be adjusted so that root is not excluded.

besser82 · 2021-10-09T07:39:27Z

I wonder wouldn't we better off if the code was written as if selinux is always enabled? In terminology of your patch, is there any drawbacks if we assumed SELINUX_ENABLED == 1?

Well, we would lose the sanity checks on systems without SELinux. Otherwise I don't see any.

These sanity checks make sense even if selinux is enabled, they just have to be adjusted so that root is not excluded.

You mean sth like this, I guess:

diff --git a/pam_tcb/support.c b/pam_tcb/support.c
index aa07772..c3539b8 100644
--- a/pam_tcb/support.c
+++ b/pam_tcb/support.c
@@ -475,7 +475,8 @@ static int unix_verify_password_plain(pam_handle_t *pamh,
        if (!salt) {
                /* we're not faking, we have an existing user, so... */
                uid_t uid = getuid();
-               if (SELINUX_ENABLED || (uid == geteuid() && uid == pw->pw_uid && uid != 0)) {
+               if ((SELINUX_ENABLED && uid == 0) ||
+                   (uid == geteuid() && uid == pw->pw_uid && uid != 0)) {
                        /* We are not root perhaps this is the reason? */
                        D(("running helper binary"));
                        retval = unix_run_helper_binary(user, pass);
diff --git a/progs/tcb_chkpwd.c b/progs/tcb_chkpwd.c
index 815067f..d023a5f 100644
--- a/progs/tcb_chkpwd.c
+++ b/progs/tcb_chkpwd.c
@@ -50,7 +50,9 @@ static int unix_verify_password(const char *user, const char *pass, int nullok)
 
        stored_hash = NULL;
        if (pw) {
-               if (!SELINUX_ENABLED && getuid() != pw->pw_uid)
+               uid_t uid = getuid();
+               if (((SELINUX_ENABLED && uid != 0) || !SELINUX_ENABLED) &&
+                   uid != pw->pw_uid)
                        return AUTH_FAILED;
 
                if (!strcmp(pw->pw_passwd, "x")) {

ldv-alt · 2021-10-09T23:49:44Z

I mean something like this (completely untested):

--- a/pam_tcb/support.c
+++ b/pam_tcb/support.c
@@ -475,7 +475,7 @@ static int unix_verify_password_plain(pam_handle_t *pamh,
        if (!salt) {
                /* we're not faking, we have an existing user, so... */
                uid_t uid = getuid();
-               if (uid == geteuid() && uid == pw->pw_uid && uid != 0) {
-                       /* We are not root perhaps this is the reason? */
+               if (uid == geteuid() && (uid == pw->pw_uid || uid == 0)) {
+                       /* We are not privileged enough perhaps this is the reason? */
                        D(("running helper binary"));
                        retval = unix_run_helper_binary(user, pass);
--- a/progs/tcb_chkpwd.c
+++ b/progs/tcb_chkpwd.c
@@ -43,7 +43,8 @@ static int unix_verify_password(const char *user, const char *pass, int nullok)
 
        stored_hash = NULL;
        if (pw) {
-               if (getuid() != pw->pw_uid)
+               uid_t uid = getuid();
+               if (uid != pw->pw_uid && uid != 0)
                        return AUTH_FAILED;
 
                if (!strcmp(pw->pw_passwd, "x")) {

besser82 · 2021-10-11T17:29:56Z

I mean something like this (completely untested):

--- a/pam_tcb/support.c
+++ b/pam_tcb/support.c
@@ -475,7 +475,7 @@ static int unix_verify_password_plain(pam_handle_t *pamh,
        if (!salt) {
                /* we're not faking, we have an existing user, so... */
                uid_t uid = getuid();
-               if (uid == geteuid() && uid == pw->pw_uid && uid != 0) {
-                       /* We are not root perhaps this is the reason? */
+               if (uid == geteuid() && (uid == pw->pw_uid || uid == 0)) {
+                       /* We are not privileged enough perhaps this is the reason? */
                        D(("running helper binary"));
                        retval = unix_run_helper_binary(user, pass);
--- a/progs/tcb_chkpwd.c
+++ b/progs/tcb_chkpwd.c
@@ -43,7 +43,8 @@ static int unix_verify_password(const char *user, const char *pass, int nullok)
 
        stored_hash = NULL;
        if (pw) {
-               if (getuid() != pw->pw_uid)
+               uid_t uid = getuid();
+               if (uid != pw->pw_uid && uid != 0)
                        return AUTH_FAILED;
 
                if (!strcmp(pw->pw_passwd, "x")) {

That solution works, tested on Fedora 36 (Rawhide) with a little tweak to the SELinux policy (tcb_chkpwd needs to be labeled to type chkpwd_t).

But pam_unix_acct.c needs some changes as well: As the account verification (expired pw, etc.) needs to have elevated privilieges (chkpwd_t) to work SELinux, we need it be performed through a helper binary as well.

Before I start to implement that, I'd like to hear your oppinions towards it, @ldv-alt and @solardiz, as we have two options here:

Add the needed code to tcb_chkpwd and call it with a cli switch to perform account verification, or
introduce another heper binary, likely called tcb_chkacct to perform this step.

After that is done, I will proceed with implementing the change pw step adjustments.

solardiz · 2021-10-11T17:38:45Z

@besser82 Out of the two options you list, I prefer adding a command-line option to tcb_chkpwd, unless we can identify a reason why the potential two helpers would require different least privileges on some systems.

That said, I am not yet sure the rest of this proposal is entirely right. I'll need to take a closer look.

Also, how does pam_unix* approach (or avoid) the same problem?

besser82 · 2021-10-11T17:55:34Z

@besser82 Out of the two options you list, I prefer adding a command-line option to tcb_chkpwd, unless we can identify a reason why the potential two helpers would require different least privileges on some systems.

From looking at the refpolicy and the fedora targeted policy chkpwd_t is the least required privilege to read shadow files (or other files of type shadow_t. That said, we will definitely need a second helper for changing the user password, as that requires updpwd_t capabilities.

That said, I am not yet sure the rest of this proposal is entirely right. I'll need to take a closer look.

I will push the reworked changes as outlined by @ldv, when I'm done with the check acct changes, will make more sense to review afterwards.

Also, how does pam_unix* approach (or avoid) the same problem?

They use two helper binaries: unix_chkpwd for pass verify and acct verify (cli switches are used to switch pwd/acct) and unix_updpwd to perform changing of user pass.

besser82 · 2021-10-12T14:25:44Z

@ldv-alt and @solardiz ready for being reviewed. I'll change some wording in the Changelog, to the commit message, and to the PR when I'm squashing all commits into one for merging. I've kept them seperate for now, for easier review.

ldv-alt · 2021-10-14T22:36:54Z

I'm sorry, I won't be able to review anything till November.

besser82 · 2021-11-17T19:48:30Z

@ldv-alt Do you have some time to spend on this, yet? Don't feel to be pushed, I'm just asking to be able to do some planning.

ldv-alt · 2021-11-25T10:53:59Z

On Wed, Nov 17, 2021 at 11:48:42AM -0800, Björn Esser wrote: @ldv-alt Do you have some time to spend on this, yet? Don't feel to be pushed, I'm just asking to be able to do some planning.

I'll look at this as soon as I get strace 5.15 released, hopefully this weekend.

pam_tcb/pam_unix_acct.c

solardiz · 2024-12-10T20:35:14Z

Thank you, @besser82! I'm sorry we still haven't reviewed this for real.

Regarding communicating more status info from the helper program (assuming that we have to, which I'm not sure of), maybe we should continue with more magic values, not a mere enum, because of the recent extension of Rowhammer to target spilled registers, IIRC as demonstrated and patched for sudo. In fact, I worry that Linux-PAM pam_unix and its helper is probably susceptible. So especially when implementing something like this anew, we need to do so with awareness.

solardiz · 2024-12-10T20:38:47Z

maybe we should continue with more magic values, not a mere enum

This is not a request for you to amend any commits yet. I think we should first review the actual functionality in its simpler form, since you already implemented it, and then decide on possible hardening. Also, it may be reasonable to keep simpler implementation in commits history, to separate actual logic from defensive programming.

ldv-alt · 2024-12-11T20:43:53Z

By the way, pam_unix ended up invoking the helper unconditionally, see linux-pam/linux-pam#686.

solardiz · 2024-12-11T21:09:15Z

By the way, pam_unix ended up invoking the helper unconditionally, see linux-pam/linux-pam#686.

Right. I was very surprised to notice such behavior on Rocky Linux 9 some months ago (an RH patch? I don't recall right now) and I vaguely recall finding (in upstream Linux-PAM commits and issues) the sort of discussion I now see on that PR you reference above. This feels like high risk to me. I think both projects should either reduce usage of the helper or at least harden its interface.

ldv-alt · 2024-12-12T07:07:49Z

By the way, pam_unix ended up invoking the helper unconditionally, see linux-pam/linux-pam#686.

Right. I was very surprised to notice such behavior on Rocky Linux 9 some months ago (an RH patch? I don't recall right now) and I vaguely recall finding (in upstream Linux-PAM commits and issues) the sort of discussion I now see on that PR you reference above. This feels like high risk to me. I think both projects should either reduce usage of the helper or at least harden its interface.

Hmm, what's the risk in using the helper? Isn't running this code in a separate process an advantage?

solardiz · 2024-12-12T16:42:41Z

Hmm, what's the risk in using the helper?

My primary concern is that process exit codes were not meant to communicate authentication/authorization results. They may not be reliable enough for that. Neither historically (e.g., a library may just happen to exit with the same code as a program was using for something else) nor especially today (when Rowhammer can flip bits in spilled registers even within a process, and perhaps even easier for an integer that passes through so many places). So we should use hardened/redundant magic values.

Isn't running this code in a separate process an advantage?

Yes, it can be an advantage - in terms of getting fresh ASLR each time and in terms of not leaving temporary data in the parent process address space.

This is needed if e.g. SELinux prevents access to file storing the hashed user password. Signed-off-by: Björn Esser <[email protected]>

Refactor the function to be non-static and to allow for more versatile use. Signed-off-by: Björn Esser <[email protected]>

The tcb_chkpwd helper binary is now able to also perform verifications for the expiration of user accounts. Signed-off-by: Björn Esser <[email protected]>

…unt. Perform verification through an external helper binary to possibly gain higher privileges if the verification fails for insufficient credentials in the first time. Signed-off-by: Björn Esser <[email protected]>

besser82 · 2024-12-22T21:20:16Z

It may take some time until I can get back to this PR, as there have been a lot of changes in the SELinux policy, which require me to write a seperate module for tcb, defining special types for the /etc/tcb and its sudirs, permissions for transitions of executable context and all…

besser82 force-pushed the topic/besser82/selinux_enforcing branch from c8858c8 to 0f39f93 Compare October 12, 2021 14:22

besser82 force-pushed the topic/besser82/selinux_enforcing branch from 0f39f93 to 1d98de0 Compare October 12, 2021 14:30

ldv-alt reviewed Jan 21, 2023

View reviewed changes

pam_tcb/pam_unix_acct.c Outdated Show resolved Hide resolved

besser82 force-pushed the topic/besser82/selinux_enforcing branch 2 times, most recently from 7bc0bd2 to 2ea4c9c Compare December 10, 2024 16:11

besser82 force-pushed the topic/besser82/selinux_enforcing branch from 2ea4c9c to fa4b72d Compare December 16, 2024 18:43

besser82 added 4 commits December 20, 2024 22:28

Allow the helper binary to be run as root.

4d2484f

This is needed if e.g. SELinux prevents access to file storing the hashed user password. Signed-off-by: Björn Esser <[email protected]>

pam_tcb: Refactor unix_run_helper_binary().

3071588

Refactor the function to be non-static and to allow for more versatile use. Signed-off-by: Björn Esser <[email protected]>

tcb_chkpwd: Enhance the functionality of the helper program.

6f52706

The tcb_chkpwd helper binary is now able to also perform verifications for the expiration of user accounts. Signed-off-by: Björn Esser <[email protected]>

besser82 force-pushed the topic/besser82/selinux_enforcing branch from fa4b72d to cefb11f Compare December 20, 2024 21:28

pam_tcb: Add support for user authentication with SELinux. #10

Are you sure you want to change the base?

pam_tcb: Add support for user authentication with SELinux. #10

Uh oh!

Conversation

besser82 commented Oct 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

besser82 commented Oct 8, 2021

Uh oh!

ldv-alt commented Oct 8, 2021

Uh oh!

besser82 commented Oct 8, 2021

Uh oh!

solardiz commented Oct 8, 2021

Uh oh!

ldv-alt commented Oct 9, 2021

Uh oh!

besser82 commented Oct 9, 2021

Uh oh!

ldv-alt commented Oct 9, 2021

Uh oh!

besser82 commented Oct 11, 2021

Uh oh!

solardiz commented Oct 11, 2021

Uh oh!

besser82 commented Oct 11, 2021

Uh oh!

besser82 commented Oct 12, 2021

Uh oh!

ldv-alt commented Oct 14, 2021

Uh oh!

besser82 commented Nov 17, 2021

Uh oh!

ldv-alt commented Nov 25, 2021 via email

Uh oh!

Uh oh!

solardiz commented Dec 10, 2024

Uh oh!

solardiz commented Dec 10, 2024

Uh oh!

ldv-alt commented Dec 11, 2024

Uh oh!

solardiz commented Dec 11, 2024

Uh oh!

ldv-alt commented Dec 12, 2024

Uh oh!

solardiz commented Dec 12, 2024

Uh oh!

besser82 commented Dec 22, 2024

Uh oh!

Uh oh!

besser82 commented Oct 8, 2021 •

edited

Loading