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