segv in sudo_getgrgid

Bug #1565567 reported by LaMont Jones on 2016-04-03
44
This bug affects 6 people
Affects Status Importance Assigned to Milestone
sudo
Unknown
Unknown
sudo (Ubuntu)
High
Marc Deslauriers
Xenial
High
Marc Deslauriers

Bug Description

[Impact]

In certain environments, for example when using LDAP, users can end up in a group with no name. When that happens, sudo crashes when attempting to look up the group name for the debug log.

Upstream has commited a simple fix for this issue, it has been commited to Yakkety, and uploaded to Xenial.

[Test Case]

I currently don't know an easy way to reproduce this, it is environment-specific. A package containing the fix was successfully tested in the problematic environment.

[Regression Potential]

A regression in the patch would prevent users from using sudo. The risk of regression is low since the patch only changes the debug log.

Original report:

If the user is in a group with no name (because libnss-db got removed and the group was defined there, for example...) then:

the call to sudo_debug_printf in sudo_getgrgid (plugins/sudoers/pwutil.c, line 462) causes a SEGV when trying to get item->d.gr->gr_name (since item->d.gr is NULL).

LaMont Jones (lamont) wrote :
LaMont Jones (lamont) wrote :

this is with current xenial (1.8.16-0ubuntu1)

tags: added: xenial
Changed in sudo (Ubuntu):
importance: Undecided → High
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in sudo (Ubuntu):
status: New → Confirmed
Eric Desrochers (slashd) wrote :

The segmentation fault[1] is due to a NULL pointer dereference[2] at : sudo-1.8.16/plugins/sudoers/pwutil.c[3]

[1] Core was generated by `sudo bash'.
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007fa01c0a6944 in sudo_getgrgid (gid=7241)
at /build/sudo-g3ghsu/sudo-1.8.16/plugins/sudoers/pwutil.c:462
462 /build/sudo-g3ghsu/sudo-1.8.16/plugins/sudoers/pwutil.c: No such file or directory.

[2] (gdb) p item->d.gr
$1 = (struct group *) 0x0

[3] - sudo-1.8.16/plugins/sudoers/pwutil.c:462
449 * Get a group entry by gid and allocate space for it.
450 */
451 struct group *
452 sudo_getgrgid(gid_t gid)
453 {
454 struct cache_item key, *item;
455 struct rbnode *node;
456 debug_decl(sudo_getgrgid, SUDOERS_DEBUG_NSS)
457
458 key.k.gid = gid;
459 getauthregistry(NULL, key.registry);
460 if ((node = rbfind(grcache_bygid, &key)) != NULL) {
461 item = node->data;
462 sudo_debug_printf(SUDO_DEBUG_DEBUG,
463 "%s: gid %u [%s] -> group %s [%s] (cache hit)", __func__,
464 (unsigned int)gid, key.registry, item->d.gr->gr_name,
465 item->registry);
466 goto done;
467 }

Three months ago, a additional debugging for pwutil functions has been introduced[4] upstream.
The addition is exactly where the segfault happen. I think it is worth looking this commit as a starting point.

[4] https://www.sudo.ws/repos/sudo/rev/908b83c3acbb

changeset 10331:908b83c3acbb
Additional debugging for pwutil functions.
author Todd C. Miller <email address hidden>
date Fri, 22 Jan 2016 17:04:59 -0700 (3 months ago)
parents 5a2921412663
children 5113a3c04494
files plugins/sudoers/pwutil.c
...

Thanks,
Eric

Download full text (3.3 KiB)

It was brought to my attention a crash just like this one. Here is my analsysis:

## INTRODUCTION

# From plugins/sudoers/pwutil.c:

/*
 * Get a password entry by uid and allocate space for it.
 */
struct passwd *
sudo_getpwuid(uid_t uid)
{
    struct cache_item key, *item;
    struct rbnode *node;
    debug_decl(sudo_getpwuid, SUDOERS_DEBUG_NSS)

    key.k.uid = uid;
    getauthregistry(IDtouser(uid), key.registry);
    if ((node = rbfind(pwcache_byuid, &key)) != NULL) {
        item = node->data;
        sudo_debug_printf(SUDO_DEBUG_DEBUG,
            "%s: uid %u [%s] -> user %s [%s] (cache hit)", __func__,
            (unsigned int)uid, key.registry, item->d.pw->pw_name,
            item->registry);
        goto done;
    }

being the last frame in the debugger:

#0 0x00007fa01c0a6944 in sudo_getgrgid (gid=7241) at /build/sudo-g3ghsu/sudo-1.8.16/plugins/sudoers/pwutil.c:462
        key = {refcnt = 1275378544, registry = "\000V\000\000\330\f\005L\027V\000\000\000\000\000", k = {uid = 7241,
            gid = 7241, name = 0x28b4cb5700001c49 <error: Cannot access memory at address 0x28b4cb5700001c49>}, d = {
            pw = 0x7ffffd9d43d0, gr = 0x7ffffd9d43d0, grlist = 0x7ffffd9d43d0}}
        item = 0x56174c050700
        node = <optimized out>
        sudo_debug_subsys = 0
        __func__ = "sudo_getgrgid"

We can see that all local variables are accessible but an union:

/*
 * Generic cache element.
 */
struct cache_item {
    unsigned int refcnt;
    char registry[16];
    /* key */
    union {
        uid_t uid;
        gid_t gid;
        char *name;
    } k;
    /* datum */
    union {
        struct passwd *pw;
        struct group *gr;
        struct group_list *grlist;
    } d;
};

(gdb) print item
$35 = (struct cache_item *) 0x56174c050700

(gdb) print item->d
$36 = {pw = 0x0, gr = 0x0, grlist = 0x0}

The union pointer "d" could be either a struct passwd, a struct group or a struct group_list (union nature).

BUT, we can see that, from sudo_getpwuid, it is being used as a struct passwd (d.pw):

        sudo_debug_printf(SUDO_DEBUG_DEBUG,
            "%s: uid %u [%s] -> user %s [%s] (cache hit)", __func__,
            (unsigned int)uid, key.registry, item->d.pw->pw_name,
            item->registry);

Meaning that, probably, the password structure, for this user, wasn't filled in.

I can tell that because the "node" (from the tree) seems good:

    if ((node = rbfind(pwcache_byuid, &key)) != NULL) {
        item = node->data;

(gdb) print item->k
$38 = {uid = 7241, gid = 7241, name = 0x1c49 ""}

since uid 7241 and gid 7241 seems reasonable.

But not the "item->d" union.

So we are only missing:

        struct passwd *pw;

From the node.

"struct passwd" is (from /usr/include/pwd.h):

/* The passwd structure. */
struct passwd
{
  char *pw_name; /* Username. */
  char *pw_passwd; /* Password. */
  __uid_t pw_uid; /* User ID. */
  __gid_t pw_gid; /* Group ID. */
  char *pw_gecos; /* Real name. */
  char *pw_dir; /* Home directory. */
  char *pw_shell; /* Shell program. */
};

And tells us that it is/was an easy way for the deb...

Read more...

Following execution path I get:

====

(1)

mode_run -> policy_check -> sudoers_policy_main -> sudoers_policy_init:
 - init_vars (fills global variable sudo_user)
 - sudo_user->pwd (from global struct passwd) will be used in "user_in_group" as "struct passwd" of executing user
 - it will check whether the executing user is in a certain group (pre-reqs for sudo execution)

/*
 * Get a local copy of the user's struct passwd if we don't already
 * have one.
 */
if (sudo_user.pw == NULL) {
    if ((sudo_user.pw = sudo_getpwnam(user_name)) == NULL) {
        /*
         * It is not unusual for users to place "sudo -k" in a .logout
         * file which can cause sudo to be run during reboot after the
         * YP/NIS/NIS+/LDAP/etc daemon has died.
         */
        if (sudo_mode == MODE_KILL || sudo_mode == MODE_INVALIDATE) {
            sudo_warnx(U_("unknown uid: %u"), (unsigned int) user_uid);
            debug_return_bool(false);
        }

        /* Need to make a fake struct passwd for the call to log_warningx(). */
        sudo_user.pw = sudo_mkpwent(user_name, user_uid, user_gid, NULL, NULL);
        unknown_user = true;
    }
}

so sudo_user->pwd comes from sudo_mkpwent(user_name).

At this time, impossible that sudo_mkpwent has cached values so we have:

...
item = sudo_make_pwitem((uid_t)-1, name);

if (item == NULL) {
    const size_t len = strlen(name) + 1;
    if (errno != ENOENT || (item = calloc(1, sizeof(*item) + len)) == NULL) {
        sudo_warnx(U_("unable to cache user %s, out of memory"), name);
        debug_return_ptr(NULL);
    }
    item->refcnt = 1;
    item->k.name = (char *) item + sizeof(*item);
    memcpy(item->k.name, name, len);
    /* item->d.pw = NULL; */
}
strlcpy(item->registry, key.registry, sizeof(item->registry));
switch (rbinsert(pwcache_byname, item, NULL)) {
...

Since sudo_make_pwitem relies on local authentication, and user is coming from ldap... item = NULL.

With item being NULL, cache item item->d (union) will be filled of zeroes.
With that, item->d>pw will be a NULL pointer.

PROBLEM:

In the future, when this cache item is recovered from redblack tree, any reference to item->d>pw will fail.

(2)

Going back to backtrace we have:

mode_run -> policy_check -> sudoers_policy_main - >create_admin_success_flag -> user_in_group (global sudo_user)

   /* Check whether the user is in the admin group. */
   if (!user_in_group(sudo_user.pw, "admin") &&
       !user_in_group(sudo_user.pw, "sudo"))
       debug_return_int(true);

and then user_in_group calls:

   if ((grlist = sudo_get_grlist(pw)) != NULL) {

 if "admin" == pw->pw_gid: matched = 1, goto done.

(gdb) print sudo_user->pw->pw_gid
$5 = 7241
Not the case.

       /*
        * Next check the supplementary group vector.
        * It usually includes the password db group too.
        */

       for (i = 0; i < grlist->ngroups; i++) {
           if (strcasecmp(group, grlist->groups[i]) == 0) {
               matched = true;
               goto done;
           }
       }

(gdb) frame 1
#1 0x00007fa01c0a7ab1 in user_in_group (pw=0x56174c050ca8, group=group@entry=0x7fa01c0b14c4 "admin")
    at /build/sudo-g3ghsu/sudo-1.8.16/plugins/sudoers/pwutil.c:842
842 if ((grp = sudo_getgrgid(pw->pw_gid)) != NULL) {
(gdb) print grlist->ngroups
$6 = 1
(gdb) print grlist->groups[0]
$7 = 0x56174c050d7f "sudo"

This should have gone to "done" because it is "sudo".
But we are checking for "admin":

(gdb) print group
$12 = 0x7fa01c0b14c4 "admin"

In both cases it should have finished and not continued. If it continued, it would go to “PROBLEM" (comment #6)

This patch was exposed by:

"also_check_sudo_group.diff"

(since user was in "sudo" group and should have stopped user_in_group before)

Right after being caused by:

commit a1663632dc5ab7e7c01e17206854b6f0ba0347dd
Author: Todd C. Miller <email address hidden>
Date: Fri Jan 22 17:04:59 2016 -0700

    Additional debugging for pwutil functions.

When debug messages accessed non-referenced pointer (struct passwd) inside the node.

---

Note:

Users that can't have "struct passwd" solved should explore something like:

---

commit d0c0662fda02260f8ffa4f59133bfe19ccd075a2
Author: Todd C. Miller <email address hidden>
Date: Sun Sep 25 06:35:40 2011 -0400

    If the invoking user cannot be resolved by uid fake the struct
    passwd and store it in the cache so we can delref it on exit.

---

Faking it, so any other function relying on accessing the node's structure will succeed.

Changed in sudo (Ubuntu Xenial):
status: New → Confirmed
Marc Deslauriers (mdeslaur) wrote :

Thanks for the detailed analysis.

I have uploaded a package for testing with the minimal fix in the following PPA:

https://launchpad.net/~mdeslaur/+archive/ubuntu/testing

Once it's finished building, could someone please check if the package resolves their issue? If so, I'll upload it to proposed as an SRU.

Thanks!

Changed in sudo (Ubuntu):
assignee: nobody → Marc Deslauriers (mdeslaur)
Changed in sudo (Ubuntu Xenial):
assignee: nobody → Marc Deslauriers (mdeslaur)
Marc Deslauriers (mdeslaur) wrote :

Packages in the PPA have been successfully tested, and upstream has commited a slightly more exhaustive fix:

https://www.sudo.ws/repos/sudo/rev/1d13341d53ec

I have uploaded the fix to yakkety.
I have uploaded the fix to xenial for processing by the SRU team.

Changed in sudo (Ubuntu):
status: Confirmed → Fix Committed
Changed in sudo (Ubuntu Xenial):
status: Confirmed → In Progress
importance: Undecided → High
description: updated

Hello LaMont, or anyone else affected,

Accepted sudo into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/sudo/1.8.16-0ubuntu1.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested, and change the tag from verification-needed to verification-done. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Changed in sudo (Ubuntu Xenial):
status: In Progress → Fix Committed
tags: added: verification-needed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package sudo - 1.8.16-0ubuntu2

---------------
sudo (1.8.16-0ubuntu2) yakkety; urgency=medium

  * debian/patches/lp1565567.patch: fix crash when looking up a negative
    cached entry which is stored as a NULL passwd or group struct pointer
    in plugins/sudoers/pwutil.c. (LP: #1565567)

 -- Marc Deslauriers <email address hidden> Wed, 04 May 2016 11:31:55 -0400

Changed in sudo (Ubuntu):
status: Fix Committed → Fix Released

I just verified that sudo fix seems good within yakkety. Marking it as verification-done.

tags: added: verification-done
removed: verification-needed
Changed in sudo (Ubuntu Xenial):
status: Fix Committed → Fix Released
Ben Kulbertis (ben-kulbertis) wrote :

For the record, I did not intend to edit the status of this as recorded above and am unable to revert it. That's embarrassing, sorry. I was simply looking to see if there was more info about when this might be released.

Marc Deslauriers (mdeslaur) wrote :

Don't worry about it, I changed it back. :)

Changed in sudo (Ubuntu Xenial):
status: Fix Released → Fix Committed
Ben Kulbertis (ben-kulbertis) wrote :

I've tested 1.8.16-0ubuntu1.1 in xenial-proposed on my xenial install where sudo was segfaulting when a user authenticated via sssd. I can confirm this fixes the issue I was having. User's commands now execute as expected instead of a segfault.

craigstrydom (cstrydom) wrote :

I also confirm that sudo is functioning correctly with sssd via ldap after installing the proposed package.

Thank you Marc, and sorry for the duplicate bug at https://bugs.launchpad.net/ubuntu/+source/sudo/+bug/1575239

Launchpad Janitor (janitor) wrote :

This bug was fixed in the package sudo - 1.8.16-0ubuntu1.1

---------------
sudo (1.8.16-0ubuntu1.1) xenial; urgency=medium

  * debian/patches/lp1565567.patch: fix crash when looking up a negative
    cached entry which is stored as a NULL passwd or group struct pointer
    in plugins/sudoers/pwutil.c. (LP: #1565567)

 -- Marc Deslauriers <email address hidden> Wed, 04 May 2016 11:36:54 -0400

Changed in sudo (Ubuntu Xenial):
status: Fix Committed → Fix Released

The verification of the Stable Release Update for sudo has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.