Bug 168180 - CAN-2005-2977 unix_chkpwd helper doesn't verify requesting user if SELinux is enabled
CAN-2005-2977 unix_chkpwd helper doesn't verify requesting user if SELinux is...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: pam (Show other bugs)
4
All Linux
medium Severity low
: ---
: ---
Assigned To: Tomas Mraz
impact=low,source=redhat,embargo=2005...
: Security
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2005-09-13 05:54 EDT by Tomas Mraz
Modified: 2007-11-30 17:11 EST (History)
6 users (show)

See Also:
Fixed In Version: pam-0.79-9.6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-10-28 03:55:11 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Proposed patch with the setuid call (856 bytes, patch)
2005-09-15 11:10 EDT, Tomas Mraz
no flags Details | Diff
Proposed patch with the setuid call with added logging (1.07 KB, patch)
2005-10-07 10:51 EDT, Tomas Mraz
no flags Details | Diff
Better patch (3.07 KB, patch)
2005-10-16 18:31 EDT, Tomas Mraz
no flags Details | Diff

  None (edit)
Description Tomas Mraz 2005-09-13 05:54:17 EDT
Description of problem:


Version-Release number of selected component (if applicable):
all pam with the SELinux patch included (FC3,4 RHEL4)

How reproducible:

Try:
$ echo -n <root password> | /sbin/unix_chkpwd root nonull && echo OK

OK should not be printed.


So the problem is in this part of SELinux patch:
        /*
-        * determine the current user's name is
+        * determine the current user's name is.
+        * On a SELinux enabled system, policy will prevent third
parties from using
+        * unix_chkpwd as a password guesser.  Leaving the existing
check prevents
+        * su from working,  Since the current uid is the users and the
password is
+        * for root.
         */
-       user = getuidname(getuid());
-       if (argc == 2) {
-           /* if the caller specifies the username, verify that user
-              matches it */
-           if (strcmp(user, argv[1])) {
-               force_failure = 1;
-           }
+       if (SELINUX_ENABLED) {
+         user=argv[1];
+       }
+       else {
+         user = getuidname(getuid());
+         /* if the caller specifies the username, verify that user
+            matches it */
+         if (strcmp(user, argv[1])) {
+           return PAM_AUTH_ERR;
+         }
+       }

Obviously when you set SELinux to permissive mode it's possible to run
unix_chkpwd without problem. But even when I set it to enforcing I was
able to run it. (Probably it's disabled only by strict policy?)

So unix_chkpwd allows brute-forcing of any passwords in /etc/shadow.
Also note that there is no delay if the password is incorrect - it exits
immediately, and there is no logging of failed attempts.
Comment 1 Tomas Mraz 2005-09-14 04:18:27 EDT
Dan Walsh:
From the testing I have been doing,  It looks like this part of SELinux patch 
can be removed. And we can go back to the original code.

My comment to this:
Without selinux unix_chkpwd is invoked only when the pam_unix module cannot read
/etc/shadow which can happen only when PAM is invoked by regular user from
non-setuid program, currently this is the case probably only for xscreensaver.

With selinux however the access to /etc/shadow can be restricted even for root
and in case of the setuid binaries invoking PAM (such as su, sudo, passwd) the
unix_chkpwd will fail in the strcmp(user, argv[1]) check even if policy would
allow access to /etc/shadow by unix_chkpwd. 

Probably current targeted and strict policies don't have such restriction on
/etc/shadow so it isn't a problem now but theoretically it would be a good idea
to restrict access to /etc/shadow only to the unix_chkpwd helper.

But even then the check for uid should stay in unix_chkpwd unaffected and to
make it not fail the PAM library should probably check if euid==0 and euid!=uid
and call setuid before executing the unix_chkpwd helper in such case.

Conclusion:
Remove the 
+       if (SELINUX_ENABLED) {
+         user=argv[1];
+       }
part from the unix_chkpwd patch.

For devel branch also add the proposed setuid in pam_unix module.

Any comments?
Comment 3 Daniel Walsh 2005-09-14 09:27:20 EDT
From reading the strict policy, all login apps are not allowed to read the
shadow file.  So removing this check should cause them to fail if they have not
done a seteuid before running auth_chkpwd.

Dan
Comment 4 Tomas Mraz 2005-09-14 09:37:27 EDT
This only affects su, sudo, passwd, userhelper which have setuid bit set and
most probably don't call setuid before PAM calls.

Is strict policy supported for RHEL4? I am not sure of consequences of adding
the setuid call in pam_unix, there most probably shouldn't be any but I'd want
to be really sure before adding it.
Comment 5 Daniel Walsh 2005-09-14 10:00:50 EDT
What is we just add a 2 second delay when running with SELInux and a failure
happens.  This is what pam does.  So in strict policy you could have up to 4
second delay on login failure.

Dan
Comment 6 Tomas Mraz 2005-09-14 10:16:19 EDT
This would be another possibility but at least the failure should be also logged
to the system log.
Comment 15 Tomas Mraz 2005-09-15 11:10:52 EDT
Created attachment 118857 [details]
Proposed patch with the setuid call

Here is the proposed patch with the setuid call.

Another possibility - probably much more clear and safe from the security point
of view would be to split the functionality to two executables - the old
unix_chkpwd helper before it was patched for SELinux purposes (with setuid bit
set) and a new helper binary which wouldn't have setuid bit but would be
allowed to access /etc/shadow by SELinux policy.
Comment 21 Tomas Mraz 2005-10-07 10:51:37 EDT
Created attachment 119709 [details]
Proposed patch with the setuid call with added logging

If we agree that this patch is the right way to go I can start building the
packages immediately.
Comment 22 Daniel Walsh 2005-10-07 11:00:35 EDT
Looks good.  As soon as you have a build I will test it.
Comment 23 Mark J. Cox (Product Security) 2005-10-10 06:25:26 EDT
Adding Ubuntu security team menber Martin Pitt to this errata so he can
determine if this issue affects Ubuntu and if we therefore need to do a
co-ordinated release.
Comment 24 Martin Pitt 2005-10-10 08:18:09 EDT
Thank you for giving me the chance to check this. Our versions are not affected
by this issue (we have some SELinux patches, but not yet the PAM one).

I also checked Debian: Sarge is not affected either, just unstable and testing.
But for these it is fine to fix this bug after the embargo ends.

So there is no need for a coordinated release from my side. Thanks!
Comment 25 Chris PeBenito 2005-10-16 12:51:16 EDT
Gentoo does have the PAM SELinux patch, so I guess there will have to be some
coordination on the release.  I'll start testing on our side.
Comment 26 Josh Bressers 2005-10-16 13:31:25 EDT
The current plan is to make this public on 2005-10-26 1400UTC.
Comment 27 Chris PeBenito 2005-10-16 17:16:43 EDT
Adding the above patch breaks su'ing to root.  I get:

Oct 16 17:14:07 [su(pam_unix)] ERROR 0:Permission denied _
Oct 16 17:14:07 [su] pam_acct_mgmt: Authentication service cannot retrieve
authentication info.
Oct 16 17:14:07 [su] - pts/1 pebenito:root
Comment 28 Tomas Mraz 2005-10-16 18:29:19 EDT
Yes, the patch was not quite complete as the unix_chkpwd binary is called from
two more places.
Comment 29 Tomas Mraz 2005-10-16 18:31:09 EDT
Created attachment 120035 [details]
Better patch
Comment 31 Josh Bressers 2005-10-16 21:13:36 EDT
If there is any desire to extend the embargo date, it's not an issue.  I was
under the impression the patch was complete.
Comment 32 Josh Bressers 2005-10-26 11:46:20 EDT
Lifting embargo, this issue is now public.
Comment 33 Fedora Update System 2005-10-27 13:07:44 EDT
From User-Agent: XML-RPC

pam-0.79-9.6 has been pushed for FC4, which should resolve this issue.  If these problems are still present in this version, then please make note of it in this bug report.
Comment 34 Tomas Mraz 2005-10-28 03:55:11 EDT
Fixed also in FC-3 and devel.

Note You need to log in before you can comment on or make changes to this bug.