Bug in the deluser applet

tito farmatito at tiscali.it
Thu Feb 19 17:42:15 UTC 2015


On Thursday 19 February 2015 17:56:27 you wrote:
> On Wed, Feb 18, 2015 at 3:37 PM, Laszlo Papp <lpapp at kde.org> 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;
> 
> Kaboom! That is undefined behavior because line is not initialized and
> then going to next would try to free that. Line needs to be
> initialized to zero either at the beginning or after free. However, I
> would probably just use continue instead of goto next to carry on
> without the operation of free as there is really nothing to free in
> this branch. The code did work on my desktop, but not on my arm
> target. This is the nature of UB, I am afraid. Once I changed this
> goto to continue, it worked everywhere.
> 
> Good luck with integrating this fix into busybox. I am not willing to
> modify as Denys usually goes around and changes patches anyway, so
> just leaving the important notes. ;-)

Hi,
just to understand it correctly, the undefined behaviour is in the patch
and not in the actual code?
I suggest as our patches are mutually exclusive to post them both
in a new thread labeled with the word [PATCH] with a short explanation
of the new feature they introduce and of the pro and contras of each one
and let Denys the time to decide what he likes best, because I doubt
he will read this whole thread.
Let me know if you agree.

Ciao,
Tito


> > +            } 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;
> >                 }
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20150219/e98c780c/attachment-0001.html>


More information about the busybox mailing list