Bug 280099 - adduser not respecting home directory mode
Summary: adduser not respecting home directory mode
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 14.1-RELEASE
Hardware: amd64 Any
: --- Affects Many People
Assignee: Kyle Evans
URL: https://reviews.freebsd.org/D46443
Keywords:
Depends on:
Blocks:
 
Reported: 2024-07-02 20:36 UTC by Daniel Li
Modified: 2024-12-18 10:47 UTC (History)
6 users (show)

See Also:
kevans: mfc-stable14?
kevans: mfc-stable13?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Li 2024-07-02 20:36:52 UTC
I noticed today that the home directory for a new user created via any of these methods will always have permissions `755`:

1. `adduser -M 700`
2. `adduser`, and then manually specify `700` in interactive mode
3. Configure default mode via `adduser -C`, then execute the above #1 or #2

`adduser` seems to always ignore the specified mode. Is this a bug, or is this user error?
Comment 1 Li-Wen Hsu freebsd_committer freebsd_triage 2024-07-03 04:34:50 UTC
(In reply to Daniel Li from comment #0)
Just did a very quick test on my root on zfs setup and it seems creating a zfs dataset as new user's home dir without setting the permission as described.

Daniel, I want to check, are you also using zfs on root? Also, if possible, can use check the case of root on ufs?
Comment 2 Daniel Li 2024-07-03 04:42:36 UTC
Yes, it was on zfs.

I just retried on ufs and cannot reproduce --- `adduser` on ufs respects the umask. So this seems to be zfs home dir specific.
Comment 3 Daniel Li 2024-07-03 04:43:04 UTC
(In reply to Li-Wen Hsu from comment #1)

Yes, it was on zfs.

I just retried on ufs and cannot reproduce — adduser on ufs respects the umask. So this seems to be zfs home dir specific.
Comment 4 Li-Wen Hsu freebsd_committer freebsd_triage 2024-07-03 04:52:39 UTC
John, as you're the author of base 215c0a5158f17f515f365fc28a9ff0b367be8fc9 for adding zfs support, can you also help to check if this is related?
Comment 5 Daniel Li 2024-07-03 05:17:34 UTC
After reading over 215c0a5, it doesn't appear to be anything wrong with the commit itself.

The issue seems to be upstream in `pw` and unrelated to zfs; specifically, `pw` doesn't seem to change the directory permissions if the home directory is a mounted file system.

To reproduce via, say, nullfs:

      mkdir /any/arbitrary/new/dir
      mount -t nullfs /any/arbitrary/new/dir /home/newuser
      pw useradd newuser -m -M 700

The /home/newuser directory will be unchanged, as if 700 were not specified.
Comment 6 Graham Perrin 2024-08-26 15:17:01 UTC
Duplicate of bug 150988.
Comment 7 Graham Perrin 2024-08-26 15:25:18 UTC
… maybe not a duplicate. Apologies for the noise.
Comment 8 Kyle Evans freebsd_committer freebsd_triage 2024-08-26 15:32:18 UTC
(In reply to Daniel Li from comment #5)

Not mounted filesystem specific, it just won't change the mode of an existing directory period.  The mode is used for mkdirat(2) and files created within.
Comment 9 Kyle Evans freebsd_committer freebsd_triage 2024-08-26 15:34:42 UTC
pw(8) probably should have been doing this all along:

diff --git a/usr.sbin/pw/cpdir.c b/usr.sbin/pw/cpdir.c
index 504933ab88af..856ab95ee781 100644
--- a/usr.sbin/pw/cpdir.c
+++ b/usr.sbin/pw/cpdir.c
@@ -49,9 +49,13 @@ copymkdir(int rootfd, char const * dir, int skelfd, mode_t mode, uid_t uid,
        if (*dir == '/')
                dir++;
 
-       if (mkdirat(rootfd, dir, mode) != 0 && errno != EEXIST) {
-               warn("mkdir(%s)", dir);
-               return;
+       if (mkdirat(rootfd, dir, mode) != 0) {
+               if (errno != EEXIST) {
+                       warn("mkdir(%s)", dir);
+                       return;
+               }
+
+               (void)fchmodat(rootfd, dir, mode, AT_SYMLINK_NOFOLLOW);
        }
        fchownat(rootfd, dir, uid, gid, AT_SYMLINK_NOFOLLOW);
        if (flags > 0)
Comment 10 commit-hook freebsd_committer freebsd_triage 2024-12-01 19:11:55 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=6a7238fd7c60f35191eadaa026d3d395c6140c47

commit 6a7238fd7c60f35191eadaa026d3d395c6140c47
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-12-01 19:05:57 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-12-01 19:08:31 +0000

    pw: set the user's home directory mode if it existed

    The adduser(8) prompt allows one to set the mode of a new home
    directory, but pw(8) doesn't honor the -M mode if the home directory
    already exists at creation time.  It doesn't seem to make sense to
    ignore the mode (which may lead to a security issue on the system being
    configured) when we'll happily chown an existing directory, so fix the
    inconsistency.

    PR:             280099
    Reviewed by:    des, jlduran (previous version)
    Differential Revision:  https://reviews.freebsd.org/D46443

 usr.sbin/adduser/adduser.8 |  6 ++++--
 usr.sbin/pw/cpdir.c        | 27 +++++++++++++++++++++------
 2 files changed, 25 insertions(+), 8 deletions(-)
Comment 11 commit-hook freebsd_committer freebsd_triage 2024-12-01 19:11:56 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=f7cf62cf728a942e494a5b58438600998606617a

commit f7cf62cf728a942e494a5b58438600998606617a
Author:     Jose Luis Duran <jlduran@FreeBSD.org>
AuthorDate: 2024-12-01 19:05:58 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-12-01 19:10:35 +0000

    pw: tests: add a test for -M with a pre-existing home directory

    Previous versions of pw(8) wouldn't chmod the home directory if it
    already existed prior to user creation, rendering adduser(8) -M
    ineffective in some cases.  Add a test to cover that situation.

    PR:             280099
    Reviewed by:    kevans

 usr.sbin/pw/tests/pw_useradd_test.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Comment 12 commit-hook freebsd_committer freebsd_triage 2024-12-10 23:38:52 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9aac1584c846cbb1dce638222ffc239ed712eb9b

commit 9aac1584c846cbb1dce638222ffc239ed712eb9b
Author:     Jose Luis Duran <jlduran@FreeBSD.org>
AuthorDate: 2024-12-01 19:05:58 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-12-10 23:05:51 +0000

    pw: tests: add a test for -M with a pre-existing home directory

    Previous versions of pw(8) wouldn't chmod the home directory if it
    already existed prior to user creation, rendering adduser(8) -M
    ineffective in some cases.  Add a test to cover that situation.

    PR:             280099
    Reviewed by:    kevans

    (cherry picked from commit f7cf62cf728a942e494a5b58438600998606617a)

 usr.sbin/pw/tests/pw_useradd_test.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Comment 13 commit-hook freebsd_committer freebsd_triage 2024-12-10 23:38:53 UTC
A commit in branch stable/14 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=b50d2127d9718e54b68ffead90e9de8b5cee72f5

commit b50d2127d9718e54b68ffead90e9de8b5cee72f5
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-12-01 19:05:57 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-12-10 23:05:51 +0000

    pw: set the user's home directory mode if it existed

    The adduser(8) prompt allows one to set the mode of a new home
    directory, but pw(8) doesn't honor the -M mode if the home directory
    already exists at creation time.  It doesn't seem to make sense to
    ignore the mode (which may lead to a security issue on the system being
    configured) when we'll happily chown an existing directory, so fix the
    inconsistency.

    PR:             280099
    Reviewed by:    des, jlduran (previous version)

    (cherry picked from commit 6a7238fd7c60f35191eadaa026d3d395c6140c47)

 usr.sbin/adduser/adduser.8 |  6 ++++--
 usr.sbin/pw/cpdir.c        | 27 +++++++++++++++++++++------
 2 files changed, 25 insertions(+), 8 deletions(-)
Comment 14 commit-hook freebsd_committer freebsd_triage 2024-12-10 23:38:56 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9d8a178b38ed0953f2beaa49c0dd3981cce56179

commit 9d8a178b38ed0953f2beaa49c0dd3981cce56179
Author:     Jose Luis Duran <jlduran@FreeBSD.org>
AuthorDate: 2024-12-01 19:05:58 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-12-10 23:06:16 +0000

    pw: tests: add a test for -M with a pre-existing home directory

    Previous versions of pw(8) wouldn't chmod the home directory if it
    already existed prior to user creation, rendering adduser(8) -M
    ineffective in some cases.  Add a test to cover that situation.

    PR:             280099
    Reviewed by:    kevans

    (cherry picked from commit f7cf62cf728a942e494a5b58438600998606617a)

 usr.sbin/pw/tests/pw_useradd_test.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
Comment 15 commit-hook freebsd_committer freebsd_triage 2024-12-10 23:38:58 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=1eaaa55f2ba9b45dfd955743a9bc5cbe7a6437f3

commit 1eaaa55f2ba9b45dfd955743a9bc5cbe7a6437f3
Author:     Kyle Evans <kevans@FreeBSD.org>
AuthorDate: 2024-12-01 19:05:57 +0000
Commit:     Kyle Evans <kevans@FreeBSD.org>
CommitDate: 2024-12-10 23:06:15 +0000

    pw: set the user's home directory mode if it existed

    The adduser(8) prompt allows one to set the mode of a new home
    directory, but pw(8) doesn't honor the -M mode if the home directory
    already exists at creation time.  It doesn't seem to make sense to
    ignore the mode (which may lead to a security issue on the system being
    configured) when we'll happily chown an existing directory, so fix the
    inconsistency.

    PR:             280099
    Reviewed by:    des, jlduran (previous version)

    (cherry picked from commit 6a7238fd7c60f35191eadaa026d3d395c6140c47)

 usr.sbin/adduser/adduser.8 |  6 ++++--
 usr.sbin/pw/cpdir.c        | 27 +++++++++++++++++++++------
 2 files changed, 25 insertions(+), 8 deletions(-)