Bug #1565567 “segv in sudo_getgrgid” : Bugs : sudo package : Ubuntu

Revision history for this message

LaMont Jones (lamont) wrote on 2016-04-03:

#1

trace, etc from gdb. Edit (3.4 KiB, text/plain)

Revision history for this message

LaMont Jones (lamont) wrote on 2016-04-03:

#2

this is with current xenial (1.8.16-0ubuntu1)

Brian Murray (brian-murray) on 2016-04-04

tags:	added: xenial
Changed in sudo (Ubuntu):
importance:	Undecided → High

Revision history for this message

Launchpad Janitor (janitor) wrote on 2016-04-09:

#3

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

Changed in sudo (Ubuntu):
status:	New → Confirmed

Revision history for this message

Eric Desrochers (slashd) wrote on 2016-05-03:

#4

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

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2016-05-04:

#5

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...

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 debug message to get username from uid.

Now, lets check when this struct should have been filled in:
item comes from "node->data", a generic redblack tree node.

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2016-05-04:

#6

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.

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.

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2016-05-04:

#7

(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)

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2016-05-04:

#10

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.

Marc Deslauriers (mdeslaur) on 2016-05-04

Changed in sudo (Ubuntu Xenial):
status:	New → Confirmed

Revision history for this message

Marc Deslauriers (mdeslaur) wrote on 2016-05-04:

#11

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)

Revision history for this message

Marc Deslauriers (mdeslaur) wrote on 2016-05-04:

#12

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

Revision history for this message

Adam Conrad (adconrad) wrote on 2016-05-04: Please test proposed package

#13

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

Revision history for this message

Launchpad Janitor (janitor) wrote on 2016-05-05:

#14

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

Revision history for this message

Rafael David Tinoco (rafaeldtinoco) wrote on 2016-05-05:

#15

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

tags:

added: verification-done
removed: verification-needed

Ben Kulbertis (ben-kulbertis) on 2016-05-07

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

Revision history for this message

Ben Kulbertis (ben-kulbertis) wrote on 2016-05-07:

#16

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.

Revision history for this message

Marc Deslauriers (mdeslaur) wrote on 2016-05-07:

#17

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

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

Revision history for this message

Ben Kulbertis (ben-kulbertis) wrote on 2016-05-07:

#18

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.

Revision history for this message

craigstrydom (cstrydom) wrote on 2016-05-09:

#19

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

Revision history for this message

Launchpad Janitor (janitor) wrote on 2016-05-11:

#20

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

Revision history for this message

Chris J Arges (arges) wrote on 2016-05-11: Update Released

#21

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.

Ubuntu
sudo package

segv in sudo_getgrgid

Bug Description

Duplicates of this bug

Other bug subscribers

Bug attachments

Remote bug watches

	Status	Importance	Assigned to
sudo	Unknown	Unknown	auto-bugzilla.sudo.ws #743
sudo (Ubuntu)	Fix Released	High	Marc Deslauriers
Xenial	Fix Released	High	Marc Deslauriers

Ubuntusudo package

segv in sudo_getgrgid

Bug Description

Duplicates of this bug

Other bug subscribers

Bug attachments

Remote bug watches

Ubuntu
sudo package