Bug 244967

Summary: pw: userdel will delete a home directory not owned by the user (if it is empty)
Product: Base System Reporter: Eric Hanneken <eric>
Component: binAssignee: Baptiste Daroussin <bapt>
Status: In Progress ---    
Severity: Affects Some People CC: emaste, koobs
Priority: Normal Flags: koobs: mfc-stable12+
koobs: mfc-stable11+
Version: 12.1-RELEASE   
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Patch to pw(8)
none
Patch to pw(8) none

Description Eric Hanneken 2020-03-22 04:03:55 UTC
Created attachment 212604 [details]
Patch to pw(8)

According to the man page for pw(8), pw userdel -n name -r

    will only remove files and directories that are actually owned by the
    user, or symbolic links owned by anyone under the user's
    home directory.

The man page for rmuser(8), which wraps pw(8), makes a similar promise. In reality, the ownership check is performed only for files in the user's home directory. The home directory itself, and any subdirectories, are deleted without regard to who owns them. In most cases this is harmless because a directory not owned by the user will usually also contain files not owned by the user. pw won't delete the files, and its attempt to remove a non-empty directory will fail.

But if the other-owned home directory is empty (or if its files are weirdly all owned by the user being deleted), pw will delete it:

    # mkdir /var/shared
    # ls -ld /var/shared
    drwxr-xr-x  2 root  wheel  2 Mar 21 23:40 /var/shared
    # pw useradd -n testuser1 -d /var/shared
    # pw useradd -n testuser2 -d /var/shared
    # grep 'testuser' /etc/passwd
    testuser1:*:1002:1002:User &:/var/shared:/bin/sh
    testuser2:*:1003:1003:User &:/var/shared:/bin/sh
    # pw userdel -n testuser1 -r
    # grep 'testuser' /etc/passwd
    testuser2:*:1003:1003:User &:/var/shared:/bin/sh
    # ls -ld /var/shared
    ls: /var/shared: No such file or directory

I have attached a patch which adds the ownership check to directories. It was made against revision 359195.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2020-03-22 04:12:25 UTC
Thank you for the report and patch Eric. Could you update your patch to be against svn base root please? Thanks :)
Comment 2 Eric Hanneken 2020-03-22 04:21:10 UTC
Comment on attachment 212604 [details]
Patch to pw(8)

Index: usr.sbin/pw/rm_r.c
===================================================================
--- usr.sbin/pw/rm_r.c	(revision 359195)
+++ usr.sbin/pw/rm_r.c	(working copy)
@@ -71,5 +71,8 @@
 	closedir(d);
 	if (fstatat(rootfd, path, &st, AT_SYMLINK_NOFOLLOW) != 0)
 		return;
-	unlinkat(rootfd, path, S_ISDIR(st.st_mode) ? AT_REMOVEDIR : 0);
+	if (S_ISLNK(st.st_mode))
+		unlinkat(rootfd, path, 0);
+	else if (st.st_uid == uid)
+		unlinkat(rootfd, path, AT_REMOVEDIR);
 }
Comment 3 Eric Hanneken 2020-03-22 04:21:48 UTC
(In reply to Kubilay Kocak from comment #1)
Done.
Comment 4 Eric Hanneken 2020-03-22 04:23:48 UTC
Comment on attachment 212604 [details]
Patch to pw(8)

>Index: rm_r.c
>===================================================================
>--- rm_r.c	(revision 359195)
>+++ rm_r.c	(working copy)
>@@ -71,5 +71,8 @@
> 	closedir(d);
> 	if (fstatat(rootfd, path, &st, AT_SYMLINK_NOFOLLOW) != 0)
> 		return;
>-	unlinkat(rootfd, path, S_ISDIR(st.st_mode) ? AT_REMOVEDIR : 0);
>+	if (S_ISLNK(st.st_mode))
>+		unlinkat(rootfd, path, 0);
>+	else if (st.st_uid == uid)
>+		unlinkat(rootfd, path, AT_REMOVEDIR);
> }
Comment 5 Eric Hanneken 2020-03-22 04:24:24 UTC
Created attachment 212605 [details]
Patch to pw(8)
Comment 6 Kubilay Kocak freebsd_committer freebsd_triage 2020-03-22 04:45:38 UTC
Thank you Eric
Comment 7 commit-hook freebsd_committer 2020-03-23 08:23:34 UTC
A commit references this bug:

Author: bapt
Date: Mon Mar 23 08:23:23 UTC 2020
New revision: 359232
URL: https://svnweb.freebsd.org/changeset/base/359232

Log:
  pw: do not removed home directories if not owned

  When deleting a user, if its home directory does not belong to it, it should
  not be removed. This is the promise that the manpage makes, the tool should
  ensure that it respects that promise.

  Add a regression test about it

  PR:		244967
  Submitted by:	Eric Hanneken <eric@erichanneken.com>
  MFC after:	3 days

Changes:
  head/usr.sbin/pw/rm_r.c
  head/usr.sbin/pw/tests/pw_userdel_test.sh
Comment 8 commit-hook freebsd_committer 2020-04-03 05:06:38 UTC
A commit references this bug:

Author: bapt
Date: Fri Apr  3 05:06:09 UTC 2020
New revision: 359588
URL: https://svnweb.freebsd.org/changeset/base/359588

Log:
  MFC r359232:

  pw: do not removed home directories if not owned

  When deleting a user, if its home directory does not belong to it, it should
  not be removed. This is the promise that the manpage makes, the tool should
  ensure that it respects that promise.

  Add a regression test about it

  PR:  244967
  Submitted by: Eric Hanneken <eric@erichanneken.com>

Changes:
_U  stable/11/
  stable/11/usr.sbin/pw/rm_r.c
  stable/11/usr.sbin/pw/tests/pw_userdel_test.sh
Comment 9 commit-hook freebsd_committer 2020-04-03 05:10:40 UTC
A commit references this bug:

Author: bapt
Date: Fri Apr  3 05:08:07 UTC 2020
New revision: 359589
URL: https://svnweb.freebsd.org/changeset/base/359589

Log:
  MFC r359232:

  pw: do not removed home directories if not owned

  When deleting a user, if its home directory does not belong to it, it should
  not be removed. This is the promise that the manpage makes, the tool should
  ensure that it respects that promise.

  Add a regression test about it

  PR:  244967
  Submitted by: Eric Hanneken <eric@erichanneken.com>

Changes:
_U  stable/12/
  stable/12/usr.sbin/pw/rm_r.c
  stable/12/usr.sbin/pw/tests/pw_userdel_test.sh