openssh bugs - Dec 2009 - [Bug 1690] New: AllowUsers and DenyGroups directives are not parsed in the order specified

https://bugzilla.mindrot.org/show_bug.cgi?id=1690

           Summary: AllowUsers and DenyGroups directives are not parsed in
                    the order specified
           Product: Portable OpenSSH
           Version: 5.3p1
          Platform: ix86
        OS/Version: Linux
            Status: NEW
          Keywords: patch
          Severity: trivial
          Priority: P2
         Component: sshd
        AssignedTo: unassigned-bugs at mindrot.org
        ReportedBy: pallenpost at gmail.com


Created an attachment (id=1762)
 --> (https://bugzilla.mindrot.org/attachment.cgi?id=1762)
Patch for src/auth.c to process AllowUsers/DenyGroups config directives
correctly

Details:

When logging into the sshd server, if the server's sshd_config
configuration file contains both "AllowUsers joe" and "DenyGroups
joe",
a user "joe" belonging to group "joe" will be denied access
based on
his group.  However, the sshd_config man page states that AllowUsers
should be processed before DenyGroups, thereby allowing joe to log in:

"... The allow/deny directives are processed in the following order:
DenyUsers, AllowUsers, DenyGroups, and finally AllowGroups."

To reproduce:

1) Create a user 'test' and give him a password.
2) Add these lines in sshd_config:

AllowUsers test
DenyGroups test

3) Restart sshd.
4) Attempt to SSH in as user 'test'.
5) Check /var/log/auth.log.  The attempt will be reported as denied
because the user is in a denied group.

Solution:

The solution depends on what the problem actually is:  If the way it is
currently working is the desired functionality, the man page just needs
to be updated.  However, if the order currently listed in the man page
is desired, auth.c needs a quick patch.  In that case, I have attached
a patch for review.  

Basically, it tells allowed_user() to return true if the username is
indeed present in the AllowUsers list.  This falls in the correct order
between DenyUsers and DenyGroups according to the man page.  There was
some ifdef'd auth check for AIX (?) that I also moved ahead so it would
be checked before the user was authorized.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1690

Darren Tucker <dtucker at zip.com.au> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dtucker at zip.com.au

--- Comment #1 from Darren Tucker <dtucker at zip.com.au> 2010-01-04
13:10:57 EST ---
The problem is that the way these things work is that they only ever
provide a way to deny a login, not allow it.  That is to say if that if
a given login would be denied by any one of the directives then it'll
be denied.  It's neither first-match nor last-match.

Changing this would require changing the semantics of the directives,
which would change the behaviour of existing configurations.  We could
maybe do this, but it would need to be well documented in the release
notes, and it's almost inevitable that someone somewhere wants the
current behaviour.

Instead, I think we should (a) improve the documentation, and (b) add a
new directive that can work with the Match directive which would allow
the rules to be expressed as first-match in whichever order makes sense
for your purpose.  You would be able to express your rules as something
like:

Match User joe
  AllowLogin yes

Match Group joe
  AllowLogin no

Match rules are processed first-match per directive, so this should do
what you want, and also allows easier use of Address rules and
suchlike.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=1690

junk at buler.net changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |junk at buler.net

--- Comment #2 from junk at buler.net 2011-08-25 14:40:39 EST ---
I have to agree that this is a bug. The logical order of DenyUsers,
AllowUsers, DenyGroups, AllowGroups that is noted in the documentation
makes sense. The implementation does not.

First off, DenyUsers works properly and can be ignored for this
discussion.

However, ask this: if an administrator SPECIFICALLY grants access to a
user by listing their ID in AllowUsers, why should they be subject to
further restrictions?

In any case, here are the Allow/Deny tables for the existing and
patched systems. The cases that would be changed are noted with an "*"

AllowUsers    DenyGroups    AllowGroups    Result
F            F            F            Deny
F            F            T             Allow
F            T            F             Deny
F            T            T            Deny
T            F            F            Deny
T            F            T            Allow
T            T            F            Deny
T            T            T            Deny

New            
AllowUsers    DenyGroups    AllowGroups    Result
F            F            F            Deny
F            F            T            Allow
F            T            F            Deny
F            T            T             Deny
T            F            F            *Allow* Case 1
T            F            T             Allow
T            T            F            *Allow* Case 2
T            T            T            *Allow* Case 3

Take each of the three cases:

1. User is AllowUser but not in DenyGroups or AllowGroups. This adds
valuable functionality. Say you have users A and B in group 1 and users
C, D, and E in group 2. You want to allow users A, C, D, E. If you
define AllowUsers A and AllowGroups 2, user A will be rejected because
they are not part of group 2. The only alternative is to AllowUsers A,
C, D, E and not define AllowGroups. That is illogical.

2,3. The change in result from Deny to Allow where a user is an
AllowedUser yet is a member of a DenyGroup is not as controversial a
change as it may appear. Currently, the system is totally ignoring the
AllowUser configuration when the user is a member of a DenyGroup. So
the AllowUser configuration shouldn't even be in the configuration. 
Assuming most admins are smart enough to understand that, and don't
have AllowedUsers that are also members of DenyGroups there should't be
any significant repercussions.

-- 
Configure bugmail: https://bugzilla.mindrot.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.