Bug 797511

Created attachment 565882 [details]
*corrected* corrected getgrouplist patch

Description of problem:
When asking to see commands granted to a different user via "sudo -l -U", the output will list commands granted to any group that the invoker is a member of, in addition to the actual privileges of the specified user.


Version-Release number of selected component (if applicable):
Any sudo-1.7.3+ with the fix for Bug 668843 (CVE-2011-0008) applied.
(Releases 1.8+ do not show this bug, as the code has been refactored.)

How reproducible:
Always

Steps to Reproduce:
1. Put "user1" in group "group", and keep "user2" out of "group"
2. Grant (via, e.g. "%group   ALL=(ALL) ALL") some command to members of "group"
3. As "user1", run "sudo -l -U user2"

Actual results:
The output will indicate "user2" can run the commands granted in step 2.
Note that "user2" does not actually have the ability to run these commands, and when "user2" runs "sudo -l" as himself the output is correct. 


Expected results:
The output of "user1" invoking "sudo -l -U user2" should correctly reflect the privileges of "user2" (and match the output of "user2" invoking "sudo -l").

Additional info:

Bug 235915 states that "sudo can't always correctly determine group membership" and in the description goes on to specify cases where this would happen.  The patch provided there, which uses getgrouplist() to find groups not visible to getgrnam(), was made only against the group-checking code in user_is_exempt() - it's unclear if the analogous code to check or display a user's access needed similar treatment to pick up groups that might otherwise be missed for the reasons listed in that bug report.  This fix went in for 1.6.8p12-14 and 1.6.9+ but evidently was never accepted upstream, so remained a rh/fedora patch.

Bug 481720 (CVE-2009-0034) was introduced in 1.6.9p10 when a different bit of group-checking code, in usergr_matches(), was optimized to cache supplementary group information of the invoker (which was then accidentally used to check group membership of users other than the invoker, making it appear that arbitrary users were members of any group the invoker was a member of).  This was fixed by using the supplementary group cache only if the user being checked was sudo's invoker; the fix went in upstream with 1.6.9p20.

For 1.7.3, group-checking code was moved to a new function user_in_group(), allowing the various bits of checking code to be replaced with a call to the new function in user_is_exempt(), usergr_matches(), and the check/display access code.
The new function included the requisite check to avoid the 481720 bug (and due to other code changes elsewhere, now used the cache only if the user being checked was either sudo's invoker, or the target of "-U").  235915, however, was still not adopted upstream and still existed only as a rh/fedora patch.  This patch's former home, user_is_exempt(), was now a stub function calling user_in_group().  So for 1.7.3, the patch code was moved essentially unchanged to user_in_group().  Unfortunately its code was written for the user_is_exempt() code path, which cares only about group memberships for sudo's invoker, so only checked the invoker's groups regardless of whose it was supposed to be checking...thus effectively reopening the same privilege leak of 481720 by a different mechanism.

Bug 668843 (CVE-2011-0008) identified the new leak, which was addressed by amending 235915's patch to mimic the handling of 481720 by only using getgrouplist() when the user being checked was sudo's invoker or the target of "-U".  The operational part of the getgrouplist() code, however, still only checked the invoker's groups, as it did before.

Thus, specifying "-U user" can hit the getgrouplist() code ostensibly to check group memberships for the specified user, but the code itself always checks the invoker's group memberships instead.  Bringing about the current bug.

The attached patch fixes the getgrouplist() code to properly look at the group membership of the intended user, rather than always checking the invoker's.  It also drops the 481720esque check, as it is no longer needed (and may in fact prevent some of user_in_group()'s callers from reaping the benefits of using getgrouplist() to avoid the "missing groups" of 235915 that started this all in the first place).