userdel -r -f ... can remove random files under '/'
wolfsage opened this issue · 17 comments
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
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?
Debian 6.1.94-1
What is that? Debian 6 was dead many years ago.
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
Why should any user have its homedir set to /?
It certainly seems weird to me, but adding this protection still seems like a good thing to do.
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.
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.
Please report a bug to systemd (or maybe it's a distro issue?), and please add a link here to see what they say.
Cheers.
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)
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.
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.
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
-fskips 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?
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?
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 /".
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,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"),yeah, '-f -f' would suffice imo.