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