Bug 244967 - pw: userdel will delete a home directory not owned by the user (if it is empty)
Summary: pw: userdel will delete a home directory not owned by the user (if it is empty)
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.1-RELEASE
Hardware: Any Any
: Normal Affects Some People
Assignee: Baptiste Daroussin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-03-22 04:03 UTC by Eric Hanneken
Modified: 2020-04-04 00:10 UTC (History)
2 users (show)

See Also:
koobs: mfc-stable12+
koobs: mfc-stable11+


Attachments
Patch to pw(8) (437 bytes, patch)
2020-03-22 04:03 UTC, Eric Hanneken
no flags Details | Diff
Patch to pw(8) (473 bytes, patch)
2020-03-22 04:24 UTC, Eric Hanneken
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
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