Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

userdel -r -f ... can remove random files under '/' #1050

Open
wolfsage opened this issue Jul 11, 2024 · 17 comments · May be fixed by #1071
Open

userdel -r -f ... can remove random files under '/' #1050

wolfsage opened this issue Jul 11, 2024 · 17 comments · May be fixed by #1071

Comments

@wolfsage
Copy link

Howdy.

It appears that userdel has no protection against erasing a system if a user's home dir is set to '/'. (A lot of users like systemd-network, etc have their homedir set to '/').

If you remove one of them with userdel -r -f, one of two things might happen:

  • userdel may open /proc first, attempt to delete something in there, fail, and abort, and you are safe
  • userdel may open some other directory first (like /usr!), delete everything in there, and move on, trashing your system.

I can't imagine a good reason userdel should ever attempt to delete / and all files under it. Can this be prevented somehow?

I've seen this issue on Debian 6.1.94-1, and it seems highly repeatable (using a DigitalOcean Debian 12 Droplet).

For some reason, on Debian 6.1.38-2 and earlier, userdel always seems to find /proc first, and never deletes '/'.

This is Debian shadow-utils 4.13 in both cases

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 12, 2024

Hi!

Hmmm, some question that may be dumb:

Does systemd-network (or any user) need to have its homedir set to /?

Could/should its homedir be changed to solve this problem?

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 12, 2024

Debian 6.1.94-1

What is that? Debian 6 was dead many years ago.

https://wiki.debian.org/LTS/Extended

@wolfsage
Copy link
Author

Woof, sorry, that's likely the kernel version

$ uname -a
Linux 6.1.0-22-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.1.94-1 (2024-06-21) x86_64 GNU/Linux

This is debian bookworm

$ cat /etc/debian_version 
12.6

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 16, 2024

Why should any user have its homedir set to /?

@wolfsage
Copy link
Author

It certainly seems weird to me, but adding this protection still seems like a good thing to do.

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Jul 16, 2024

It certainly seems weird to me,

Please report a bug to systemd (or maybe it's a distro issue?), and please add a link here to see what they say.

but adding this protection still seems like a good thing to do.

I'm not sure. Those are heuristics which increase the complexity of the software, possibly adding bugs, and I'm not sure we can write correctly those heuristics. Should we also try to prevent removing /usr/bin if it's homedir? or /usr/sbin? Where do we stop?

I'd rather keep it raw, and let one run rm -rf /. If they broke their systems, so that userdel runs rm -rf /, it's not our business. userdel is a symptom, not the cause.

@wolfsage
Copy link
Author

I'm not sure. Those are heuristics which increase the complexity of the software, possibly adding bugs, and I'm not sure we can write correctly those heuristics.

I agree that more code means more possibility for bugs, but are you really suggesting that "Does path X resolve to '/'?" even in the simplest form is too hard to do? Or am I missing something?

Even a check of "does the string user_home contain the bytes "/\0" and nothing more" would be a great start.

Should we also try to prevent removing /usr/bin if it's homedir? or /usr/sbin? Where do we stop?

I'm not sure why you're jumping to a slippery slope argument. Surely no one wants to ever rm -rf / based on some other tool trying to clean up something. Beyond that, no, I wouldn't say more protections are needed.

I'd rather keep it raw, and let one run rm -rf /. If they broke their systems, so that userdel runs rm -rf /, it's not our business. userdel is a symptom, not the cause.

I think most users would be much happier if their systems didn't mysteriously break because the tools they use come with no guardrails, and the more tools that are improved to be less dangerous the happier we will all be. I do not understand the resistance to not destroying things for users based on a mistake.

Anyway, please close this if you do not agree, thanks.

@wolfsage
Copy link
Author

Please report a bug to systemd (or maybe it's a distro issue?), and please add a link here to see what they say.

systemd/systemd#33743

Cheers.

@poettering
Copy link

I am pretty sure userdel should check if homedir is actually owned by the UID/GID of the account. If not, it should stay away from the homedir (and maybe print a warning message saying so)

@hallyn
Copy link
Member

hallyn commented Aug 2, 2024

Anyway, please close this if you do not agree, thanks.

No, don't close this :)

I'm in agreement with Lennart. My only concern there is, there are packages that create 'homedirs' for system users which are then owned by root or another user. If we suddenly stop removing such dirs, it may break some things (like package re-installs). Printing a warning doesn't help as a lot of this will be hidden behind scripts and automation.

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Aug 11, 2024

While checking #1062, I've learnt that we actually have some tests to check that would prevent removing "/", or someone else's home. Those tests acknowledge that this is rather imperfect, as expected but the tests are in place.

However, using -f skips those tests. Also, some tests are conditionally compiled out (a comment in the source says it's because they're slow. The distro is responsible for compiling them, I guess.

@hallyn
Copy link
Member

hallyn commented Aug 31, 2024

While checking #1062, I've learnt that we actually have some tests to check that would prevent removing "/", or someone else's home. Those tests acknowledge that this is rather imperfect, as expected but the tests are in place.

However, using -f skips those tests. Also, some tests are conditionally compiled out (a comment in the source says it's because they're slow. The distro is responsible for compiling them, I guess.

Ok, but '-f' should not skip those tests. -f in userdel is meant to be about whether or not to do the deletion of the user. But if HOME is /, userdel -f should not try to remove it. The !fflag check there should be removed. Do you mind posting a minimal patch for that?

@alejandro-colomar
Copy link
Collaborator

Ok, but '-f' should not skip those tests. -f in userdel is meant to be about whether or not to do the deletion of the user. But if HOME is /, userdel -f should not try to remove it. The !fflag check there should be removed.

According to the documentation (manual page), it should skip those tests:

     -f, --force
         This option forces the removal of the user account,
         [...]. It also forces
         userdel to remove the user's home directory and mail
         spool, even if another user uses the same home
         directory [...]

         Note: This option is dangerous and may leave your
         system in an inconsistent state.

Or do you mean we should also change that?

Do you mind posting a minimal patch for that?

@hallyn
Copy link
Member

hallyn commented Sep 1, 2024

Maybe we should add new options for those? "remove the user if still logged in" is just a different level of danger from "remove homedir if used by another user" and even further from "home is /".

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Sep 1, 2024

Maybe specifying the option once for "remove even if user still logged in" and similar danger levels and twice for "use nuclear bombs if necessary"?

Or maybe we want more levels?

How about this?:

$ git diff -U15
diff --git i/src/userdel.c w/src/userdel.c
index ead69604..53a7c600 100644
--- i/src/userdel.c
+++ w/src/userdel.c
@@ -67,31 +67,31 @@
 #ifdef ENABLE_SUBIDS
 #define E_SUB_UID_UPDATE 16    /* can't update the subordinate uid file */
 #define E_SUB_GID_UPDATE 18    /* can't update the subordinate gid file */
 #endif                         /* ENABLE_SUBIDS */
 
 /*
  * Global variables
  */
 static const char Prog[] = "userdel";
 
 static char *user_name;
 static uid_t user_id;
 static gid_t user_gid;
 static char *user_home;
 
-static bool fflg = false;
+static int fflg = 0;
 static bool rflg = false;
 #ifdef WITH_SELINUX
 static bool Zflg = false;
 #endif
 static bool Rflg = false;
 
 static bool is_shadow_pwd;
 
 #ifdef SHADOWGRP
 static bool is_shadow_grp;
 static bool sgr_locked = false;
 #endif                         /* SHADOWGRP */
 static bool pw_locked  = false;
 static bool gr_locked   = false;
 static bool spw_locked  = false;
@@ -300,31 +300,31 @@ static void remove_usergroup (void)
        if (grp->gr_gid != user_gid) {
                fprintf (stderr,
                         _("%s: group %s not removed because it is not the primary group of user %s.\n"),
                         Prog, grp->gr_name, user_name);
                return;
        }
 
        if (NULL != grp->gr_mem[0]) {
                /* The usergroup has other members. */
                fprintf (stderr,
                         _("%s: group %s not removed because it has other members.\n"),
                         Prog, grp->gr_name);
                return;
        }
 
-       if (!fflg) {
+       if (fflg < 1) {
                /*
                 * Scan the passwd file to check if this group is still
                 * used as a primary group.
                 */
                prefix_setpwent ();
                while ((pwd = prefix_getpwent ()) != NULL) {
                        if (strcmp (pwd->pw_name, user_name) == 0) {
                                continue;
                        }
                        if (pwd->pw_gid == grp->gr_gid) {
                                fprintf (stderr,
                                         _("%s: group %s is the primary group of another user and is not removed.\n"),
                                         Prog, grp->gr_name);
                                break;
                        }
@@ -820,31 +820,31 @@ static int remove_mailbox (void)
                } else {
                        fprintf (stderr,
                                 _("%s: warning: can't remove %s: %s\n"),
                                 Prog, mailfile, strerror (errno));
                        SYSLOG ((LOG_ERR, "Cannot remove %s: %s", mailfile, strerror (errno)));
 #ifdef WITH_AUDIT
                        audit_logger (AUDIT_DEL_USER, Prog,
                                      "deleting mail file",
                                      user_name, user_id, SHADOW_AUDIT_FAILURE);
 #endif                         /* WITH_AUDIT */
                        free(mailfile);
                        return -1;
                }
        }
 
-       if (fflg) {
+       if (fflg >= 1) {
                if (unlink (mailfile) != 0) {
                        fprintf (stderr,
                                 _("%s: warning: can't remove %s: %s\n"),
                                 Prog, mailfile, strerror (errno));
                        SYSLOG ((LOG_ERR, "Cannot remove %s: %s", mailfile, strerror (errno)));
 #ifdef WITH_AUDIT
                        audit_logger (AUDIT_DEL_USER, Prog,
                                      "deleting mail file",
                                      user_name, user_id, SHADOW_AUDIT_FAILURE);
 #endif                         /* WITH_AUDIT */
                        errors = 1;
                        /* continue */
                }
 #ifdef WITH_AUDIT
                else
@@ -986,31 +986,31 @@ int main (int argc, char **argv)
                        {"prefix",       required_argument, NULL, 'P'},
 #ifdef WITH_SELINUX
                        {"selinux-user", no_argument,       NULL, 'Z'},
 #endif                         /* WITH_SELINUX */
                        {NULL, 0, NULL, '\0'}
                };
                while ((c = getopt_long (argc, argv,
 #ifdef WITH_SELINUX
                                         "fhrR:P:Z",
 #else                          /* !WITH_SELINUX */
                                         "fhrR:P:",
 #endif                         /* !WITH_SELINUX */
                                         long_options, NULL)) != -1) {
                        switch (c) {
                        case 'f':       /* force remove even if not owned by user */
-                               fflg = true;
+                               fflg++;
                                break;
                        case 'h':
                                usage (E_SUCCESS);
                                break;
                        case 'r':       /* remove home dir and mailbox */
                                rflg = true;
                                break;
                        case 'R': /* no-op, handled in process_root_flag () */
                                Rflg = true;
                                break;
                        case 'P': /* no-op, handled in process_prefix_flag () */
                                break;
 #ifdef WITH_SELINUX
                        case 'Z':
                                if (prefix[0]) {
@@ -1119,72 +1119,72 @@ int main (int argc, char **argv)
                        user_home = xstrdup(pwd->pw_dir);
                }
                pw_close();
        }
 #ifdef WITH_TCB
        if (shadowtcb_set_user (user_name) == SHADOWTCB_FAILURE) {
                exit (E_NOTFOUND);
        }
 #endif                         /* WITH_TCB */
        /*
         * Check to make certain the user isn't logged in.
         * Note: This is a best effort basis. The user may log in between,
         * a cron job may be started on her behalf, etc.
         */
        if ((prefix[0] == '\0') && !Rflg && user_busy (user_name, user_id) != 0) {
-               if (!fflg) {
+               if (fflg < 1) {
 #ifdef WITH_AUDIT
                        audit_logger (AUDIT_DEL_USER, Prog,
                                      "deleting user logged in",
                                      user_name, AUDIT_NO_ID,
                                      SHADOW_AUDIT_FAILURE);
 #endif                         /* WITH_AUDIT */
                        exit (E_USER_BUSY);
                }
        }
 
        /*
         * Do the hard stuff - open the files, create the user entries,
         * create the home directory, then close and update the files.
         */
        open_files ();
        update_user ();
        update_groups ();
 
        if (rflg) {
                errors += remove_mailbox ();
        }
        if (rflg) {
                int home_owned = is_owner (user_id, user_home);
                if (-1 == home_owned) {
                        fprintf (stderr,
                                 _("%s: %s home directory (%s) not found\n"),
                                 Prog, user_name, user_home);
                        rflg = 0;
-               } else if ((0 == home_owned) && !fflg) {
+               } else if ((0 == home_owned) && fflg < 2) {
                        fprintf (stderr,
                                 _("%s: %s not owned by %s, not removing\n"),
                                 Prog, user_home, user_name);
                        rflg = 0;
                        errors++;
                        /* continue */
                }
        }
 
 #ifdef EXTRA_CHECK_HOME_DIR
        /* This may be slow, the above should be good enough. */
-       if (rflg && !fflg) {
+       if (rflg && fflg < 2) {
                struct passwd *pwd;
                /*
                 * For safety, refuse to remove the home directory if it
                 * would result in removing some other user's home
                 * directory. Still not perfect so be careful, but should
                 * prevent accidents if someone has /home or / as home
                 * directory...  --marekm
                 */
                prefix_setpwent ();
                while ((pwd = prefix_getpwent ())) {
                        if (strcmp (pwd->pw_name, user_name) == 0) {
                                continue;
                        }
                        if (path_prefix (user_home, pwd->pw_dir)) {
                                fprintf (stderr,

@alejandro-colomar
Copy link
Collaborator

alejandro-colomar commented Sep 1, 2024

Oops, one of the changes above is better moved to level 2:

diff --git i/src/userdel.c w/src/userdel.c
index 53a7c600..4ee51acb 100644
--- i/src/userdel.c
+++ w/src/userdel.c
@@ -832,7 +832,7 @@ static int remove_mailbox (void)
                }
        }
 
-       if (fflg >= 1) {
+       if (fflg >= 2) {
                if (unlink (mailfile) != 0) {
                        fprintf (stderr,
                                 _("%s: warning: can't remove %s: %s\n"),

@hallyn
Copy link
Member

hallyn commented Sep 1, 2024

yeah, '-f -f' would suffice imo.

alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Sep 1, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Sep 1, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Sep 2, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Sep 2, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
eriksjolund added a commit to eriksjolund/podman that referenced this issue Sep 25, 2024
Modify the home dir path in troubleshooting tip 44
to prevent files from being accidentally removed.

userdel with force argument tries to remove home dir path "/"
according to
shadow-maint/shadow#1050

Signed-off-by: Erik Sjölund <erik.sjolund@gmail.com>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Oct 14, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Oct 15, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Nov 3, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Nov 13, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Dec 2, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Dec 5, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Dec 5, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Dec 6, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
alejandro-colomar added a commit to alejandro-colomar/shadow that referenced this issue Dec 17, 2024
…ularity

Previously, with a single --force, one could only chose between not
forcing, and forcing (which could destroy one's system entirely).

Now one can pass --force, with only a small danger, to skip some checks,
and a double --force --force with the meaning of the previous --force,
which will possibly destroy the entire system.

Closes: <shadow-maint#1050>
Reported-by: Matthew Horsfall <wolfsage@gmail.com>
Cc: Lennart Poettering <lennart@poettering.net>
Cc: Serge Hallyn <serge@hallyn.com>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants