Bug in the deluser applet

tito farmatito at tiscali.it
Wed Feb 18 15:54:08 UTC 2015


On Wednesday 18 February 2015 16:37:59 you wrote:
> > Actually, the locking and swapping logic would be common anyway which
> > already resides in update_passwd, so if that reusable component is not
> > moved to a separate function to be reused, we could use put our
> > conditional magic into the while loop. I am preparing with that change
> > now.
> 
> Please check the patch below. I tested it and it works for me. The
> coding style may not suit the existing busybox coding style, but I do
> not mind that for now as I will use the patch as is for now in my
> local fork anyhow. Any (non-stylistic) feedback is welcome.
> 
> commit 22a7da560af82c33ca9334039522e9a03aab29b3
> Author: Laszlo Papp <laszlo.papp at polatis.com>
> Date:   Wed Feb 18 15:20:58 2015 +0000
> 
>     Delete the user from all the groups for user deletion
> 
> diff --git a/libbb/update_passwd.c b/libbb/update_passwd.c
> index a30af6f..42998a9 100644
> --- a/libbb/update_passwd.c
> +++ b/libbb/update_passwd.c
> @@ -62,6 +62,8 @@ static void check_selinux_update_passwd(const char *username)
>      only if CONFIG_PASSWD=y and applet_name[0] == 'p' like in passwd
>      or if CONFIG_CHPASSWD=y and applet_name[0] == 'c' like in chpasswd
> 
> + 8) delete a user from all groups : update_passwd(FILE, NULL, NULL, MEMBER)
> +
>   This function does not validate the arguments fed to it
>   so the calling program should take care of that.
> 
> @@ -81,7 +83,7 @@ int FAST_FUNC update_passwd(const char *filename,
>         FILE *new_fp;
>         char *fnamesfx;
>         char *sfx_char;
> -       char *name_colon;
> +       char *name_colon = 0;
>         unsigned user_len;
>         int old_fd;
>         int new_fd;
> @@ -99,13 +101,15 @@ int FAST_FUNC update_passwd(const char *filename,
>         if (filename == NULL)
>                 return ret;
> 
> -       check_selinux_update_passwd(name);
> +       if (name) check_selinux_update_passwd(name);
> 
>         /* New passwd file, "/etc/passwd+" for now */
>         fnamesfx = xasprintf("%s+", filename);
>         sfx_char = &fnamesfx[strlen(fnamesfx)-1];
> -       name_colon = xasprintf("%s:", name);
> -       user_len = strlen(name_colon);
> +    if (name) {
> +        name_colon = xasprintf("%s:", name);
> +        user_len = strlen(name_colon);
> +    }
> 
>         if (shadow)
>                 old_fp = fopen(filename, "r+");
> @@ -162,21 +166,37 @@ int FAST_FUNC update_passwd(const char *filename,
>         /* Read current password file, write updated /etc/passwd+ */
>         changed_lines = 0;
>         while (1) {
> -               char *cp, *line;
> -
> -               line = xmalloc_fgetline(old_fp);
> -               if (!line) /* EOF/error */
> -                       break;
> -               if (strncmp(name_colon, line, user_len) != 0) {
> -                       fprintf(new_fp, "%s\n", line);
> -                       goto next;
> -               }
> -
> -               /* We have a match with "name:"... */
> -               cp = line + user_len; /* move past name: */
> +        char *cp, *line;
> +        if (!name && member) {
> +            struct group* g;
> +            if ((g = getgrent())) {
> +                char gline[LINE_MAX];
> +                char **s= g->gr_mem;
> +                snprintf(gline, sizeof(gline), "%s:%s:%i:",
> g->gr_name, g->gr_passwd, g->gr_gid);
> +                while (*s) {
> +                    if (strcmp(*s, member)) { if (s != g->gr_mem)
> strcat(gline, ","); strcat(gline, *s); }
> +                    ++s;
> +                }
> +                fprintf(new_fp, "%s\n", gline);
> +                goto next;
> +            } else {
> +                break;
> +            }
> +        } else {
> +            line = xmalloc_fgetline(old_fp);
> +            if (!line) /* EOF/error */
> +                break;
> +            if (!name_colon || strncmp(name_colon, line, user_len) != 0) {
> +                fprintf(new_fp, "%s\n", line);
> +                goto next;
> +            }
> +
> +            /* We have a match with "name:"... */
> +            cp = line + user_len; /* move past name: */
> +        }
> 
>  #if ENABLE_FEATURE_ADDUSER_TO_GROUP || ENABLE_FEATURE_DEL_USER_FROM_GROUP
> -               if (member) {
> +               if (name && member) {
>                         /* It's actually /etc/group+, not /etc/passwd+ */
>                         if (ENABLE_FEATURE_ADDUSER_TO_GROUP
>                          && applet_name[0] == 'a'
> @@ -240,7 +260,7 @@ int FAST_FUNC update_passwd(const char *filename,
> 
>         if (changed_lines == 0) {
>  #if ENABLE_FEATURE_ADDUSER_TO_GROUP || ENABLE_FEATURE_DEL_USER_FROM_GROUP
> -               if (member) {
> +               if (name && member) {
>                         if (ENABLE_ADDGROUP && applet_name[0] == 'a')
>                                 bb_error_msg("can't find %s in %s",
> name, filename);
>                         if (ENABLE_DELGROUP && applet_name[0] == 'd')
> diff --git a/loginutils/deluser.c b/loginutils/deluser.c
> index 01a9386..a3f5f3a 100644
> --- a/loginutils/deluser.c
> +++ b/loginutils/deluser.c
> @@ -82,6 +82,9 @@ int deluser_main(int argc, char **argv)
>   do_delgroup:
>                         /* "delgroup GROUP" or "delgroup USER GROUP" */
>                         if (do_deluser < 0) { /* delgroup after deluser? */
> +                pfile = bb_path_group_file;
> +                if (update_passwd(pfile, NULL, NULL, name) == -1)
> +                    return EXIT_FAILURE;
>                                 gr = getgrnam(name);
>                                 if (!gr)
>                                         return EXIT_SUCCESS;
> @@ -99,7 +102,6 @@ int deluser_main(int argc, char **argv)
>                                 }
>                                 //endpwent();
>                         }
> -                       pfile = bb_path_group_file;
>                         if (ENABLE_FEATURE_SHADOWPASSWDS)
>                                 sfile = bb_path_gshadow_file;
>                 }
> 

Hi,
just for fun a different approach, this seems to work
without touching update_passwd. Probably it could be
shrinked and refactored even more. Doesn't work
with bb's internal pwdgrp as it triggers some bug,
will post in a separate thread about that.

Ciao,
Tito

--- loginutils/deluser.c.original       2015-02-18 15:12:10.810954596 +0100
+++ loginutils/deluser.c        2015-02-18 16:47:34.186322005 +0100
@@ -37,7 +37,8 @@
        const char *sfile;
        /* Are we deluser or delgroup? */
        int do_deluser = (ENABLE_DELUSER && (!ENABLE_DELGROUP || applet_name[3] == 'u'));
-
+       struct group *gr;
+       struct passwd *pw;
 #if !ENABLE_LONG_OPTS
        const int opt_delhome = 0;
 #else
@@ -69,8 +70,6 @@
        case 2:
                if (do_deluser) {
                        /* "deluser USER" */
-                       struct passwd *pw;
-
                        pw = xgetpwnam(name); /* bail out if USER is wrong */
                        pfile = bb_path_passwd_file;
                        if (ENABLE_FEATURE_SHADOWPASSWDS)
@@ -78,7 +77,6 @@
                        if (opt_delhome)
                                remove_file(pw->pw_dir, FILEUTILS_RECUR);
                } else {
-                       struct group *gr;
  do_delgroup:
                        /* "delgroup GROUP" or "delgroup USER GROUP" */
                        if (do_deluser < 0) { /* delgroup after deluser? */
@@ -90,7 +88,6 @@
                        }
                        if (!member) {
                                /* "delgroup GROUP" */
-                               struct passwd *pw;
                                /* Check if the group is in use */
                                while ((pw = getpwent()) != NULL) {
                                        if (pw->pw_gid == gr->gr_gid)
@@ -116,6 +113,26 @@
 
                if (ENABLE_DELGROUP && do_deluser > 0) {
                        /* "deluser USER" also should try to delete
+                        * the USER from all groups in which he is member.
+                        */
+                       gid_t *groups = NULL;
+                       int n = 64;
+retry:
+                       groups = xrealloc(groups, n * sizeof(gid_t));
+                       if(getgrouplist(name, pw->pw_gid, groups, &n) == -1)
+                               goto retry;
+
+                       while (--n >= 1) { /* our gid is the last group */
+                               gr = getgrgid(groups[n]);
+                               if (!gr)
+                                       continue;
+                               update_passwd(bb_path_group_file, gr->gr_name, NULL, name);
+                               if (ENABLE_FEATURE_SHADOWPASSWDS)
+                                       update_passwd(bb_path_gshadow_file, gr->gr_name, NULL, name);
+                       }
+                       if (ENABLE_FEATURE_CLEAN_UP)
+                               free(groups);
+                       /* "deluser USER" also should try to delete
                         * same-named group. IOW: do "delgroup USER"
                         */
 // On debian deluser is a perl script that calls userdel.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20150218/c61ff5f9/attachment-0001.html>


More information about the busybox mailing list