From: Rafael Sadowski Subject: Re: [patch] mount_mfs allow setting permissions manually To: tech@openbsd.org Date: Sat, 19 Jul 2025 08:17:43 +0200 On Wed Jul 16, 2025 at 04:20:43AM -0300, Crystal Kolipe wrote: > Currently, an mfs filesystem always inherits the permissions of it's mount > point. > > This behaviour necessarily differs from disk-based filesystems, since mfs is > volatile. > > The problem is that a common use of mfs is for mounting on /tmp, and this > requires 01777 permissions for correct operation. > > So 'solve' this problem, users commonly chmod the /tmp directory on the root > filesystem to 01777, which is a bad thing to do because if for some reason the > mfs filesystem fails to mount or is later unmounted, it will leave a > world-writable directory on /. > > This could be used by a malicious user to create hard links to various files > in /etc or elsewhere where they are not desired. > > To avoid this, let's teach mount_mfs to accept an optional -p parameter to > specify the required permissions _after_ the mfs filesystem is mounted. > > Note, the -p option was previously used in historical versions of newfs to > indicate a number of reserved sectors for each track. This was removed from > OpenBSD in 2007, and since the new -p option is only valid for mount_mfs and > not newfs, it seems reasonable to re-use it. The more obvious -m option as > used by mount_tmpfs is already in use in mount_mfs and newfs. I agree and your suggestion mitigates the problem and opens up the right options from my point of view. I have some comments on the diff including a new proposal (slightly modified based on your diff). > > --- sbin/newfs/newfs.c.dist Mon Apr 7 04:25:02 2025 > +++ sbin/newfs/newfs.c Wed Jul 16 07:47:33 2025 > @@ -203,7 +203,7 @@ > fatal("insane maxpartitions value %d", maxpartitions); > > opstring = mfs ? > - "O:P:T:b:c:e:f:i:m:o:s:" : > + "O:P:T:b:c:e:f:i:m:o:p:s:" : > "NO:S:T:b:c:e:f:g:h:i:m:o:qs:t:"; > while ((ch = getopt(argc, argv, opstring)) != -1) { > switch (ch) { > @@ -286,6 +286,18 @@ > optarg); > } > break; > + case 'p': > + errno = 0; > + mfsmode = strtoul(optarg, NULL, 8); > + if (*optarg < '0' || *optarg > '7') > + errno = EINVAL; Why not using "char **endptr" declaration for proper strtoul() usage. We can save ourselves this incomplete validation. See diff below. > + if ((mfsmode & ALLPERMS) != mfsmode) > + fatal("unrecognised permission bits"); > + if (mfsmode == 0) > + warnx("ignoring nonsensical permissions of 0x00"); I do not find this message helpful. I think we should inform the user what will happen. > + if (errno) > + fatal("invalid mode"); > + break; > case 'q': > quiet = 1; > break; > @@ -506,7 +518,8 @@ > err(ECANCELED, "stat %s", node); > mfsuid = mountpoint.st_uid; > mfsgid = mountpoint.st_gid; > - mfsmode = mountpoint.st_mode & ALLPERMS; > + if (mfsmode == 0) > + mfsmode = mountpoint.st_mode & ALLPERMS; > } > #endif > > --- sbin/newfs/newfs.8.dist Mon Apr 7 04:25:02 2025 > +++ sbin/newfs/newfs.8 Wed Jul 16 07:59:04 2025 > @@ -67,6 +67,7 @@ > .Op Fl m Ar free-space > .Op Fl O Ar filesystem-format > .Op Fl o Ar options > +.Op Fl p Ar mode > .Op Fl P Ar file > .Op Fl s Ar size > .Ar special node > @@ -270,7 +271,8 @@ > are as described for > .Nm , > except for the > -.Fl o > +.Fl o , > +.Fl p > and > .Fl P > options. > @@ -284,6 +286,13 @@ > See the > .Xr mount 8 > man page for possible options and their meanings. > +.It Fl p Ar mode > +Specifies the file mode bits to set on the mount point after the mfs > +filesystem has been mounted, overriding the default behaviour of inheriting > +the mode bits from those of the directory in the parent filesystem. > +Use of this option is preferred when mounting an mfs filesystem on /tmp > +as it avoids the potential security risks of creating a world writable > +directory on the root filesystem. > .It Fl P Ar file My suggestion also changes the text slightly. > If > .Ar file > Feedback any OKs? diff --git a/sbin/newfs/newfs.8 b/sbin/newfs/newfs.8 index 2c509f205bd..5153d79bd18 100644 --- a/sbin/newfs/newfs.8 +++ b/sbin/newfs/newfs.8 @@ -67,6 +67,7 @@ .Op Fl m Ar free-space .Op Fl O Ar filesystem-format .Op Fl o Ar options +.Op Fl p Ar mode .Op Fl P Ar file .Op Fl s Ar size .Ar special node @@ -270,7 +271,8 @@ The options to are as described for .Nm , except for the -.Fl o +.Fl o , +.Fl p and .Fl P options. @@ -284,6 +286,17 @@ flag followed by a comma separated string of options. See the .Xr mount 8 man page for possible options and their meanings. +.It Fl p Ar mode +Set permissions on the mount point after the mfs filesystem has been mounted. +The +.Ar mode +argument must be specified in octal notation. +By default, mfs inherits the permissions of the mount point directory, +which can create security issues if the underlying directory has +world-writable permissions such as when mounting on +.Pa /tmp . +This option allows setting secure permissions directly without modifying +the underlying filesystem. .It Fl P Ar file If .Ar file diff --git a/sbin/newfs/newfs.c b/sbin/newfs/newfs.c index 4e07e6d790e..704b997becd 100644 --- a/sbin/newfs/newfs.c +++ b/sbin/newfs/newfs.c @@ -179,6 +179,7 @@ main(int argc, char *argv[]) #ifdef MFS char mountfromname[BUFSIZ]; char *pop = NULL, node[PATH_MAX]; + char *ep; pid_t pid; struct stat mountpoint; #endif @@ -203,7 +204,7 @@ main(int argc, char *argv[]) fatal("insane maxpartitions value %d", maxpartitions); opstring = mfs ? - "O:P:T:b:c:e:f:i:m:o:s:" : + "O:P:T:b:c:e:f:i:m:o:p:s:" : "NO:S:T:b:c:e:f:g:h:i:m:o:qs:t:"; while ((ch = getopt(argc, argv, opstring)) != -1) { switch (ch) { @@ -286,6 +287,19 @@ main(int argc, char *argv[]) optarg); } break; + case 'p': + errno = 0; + mfsmode = strtoul(optarg, &ep, 8); + if (optarg[0] == '\0' || *ep != '\0') + fatal("invalid mode: not a number"); + if (errno == ERANGE) + fatal("invalid mode: out of range"); + if ((mfsmode & ALLPERMS) != mfsmode) + fatal("unrecognised permission bits"); + if (mfsmode == 0) + warnx("invalid mode, will inherit " + "mount point permissions"); + break; case 'q': quiet = 1; break; @@ -506,7 +520,8 @@ havelabel: err(ECANCELED, "stat %s", node); mfsuid = mountpoint.st_uid; mfsgid = mountpoint.st_gid; - mfsmode = mountpoint.st_mode & ALLPERMS; + if (mfsmode == 0) + mfsmode = mountpoint.st_mode & ALLPERMS; } #endif