search

openwall / tcb

Alternative password shadowing scheme
https://www.openwall.com/tcb/
Other
8 stars 3 forks source link

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

Open besser82 opened 3 years ago

besser82 commented 3 years ago

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.

The needed changes have already been applied to the SELinux reference-policy in https://github.com/SELinuxProject/refpolicy/commit/bc88a1ca4b37df37c3654fc9e5368d7d96b11548.

besser82 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

  1. Add the needed code to tcb_chkpwd and call it with a cli switch to perform account verification, or
  2. 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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

besser82 commented 3 years ago

@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 commented 3 years ago

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.