Skip to contentSkip to conversation

fix NFS mount.nfs options v4

PATCH In review Amit Gud 3 participants 4 messages Jul 28, 2006 – Aug 7, 2006

Subsystems (2)
lore
Share & tools
lore

Amit Gud

author
#0 lore raw

PATCH 1/1 fix NFS mount.nfs options v4

AG

Signature · 2 lines
May the source be with you.
http://www.cis.ksu.edu/~gud

If the 'user' or 'users' mount option is specified on commandline, verify with /etc/fstab if it is really specified. Setuid bit is still ON and should remain ON since mount drops the root privileges.

I've given this patch a good hammering, but not exhaustive by any means.

---

support/include/fstab.h +8 -0
diff -uprN -X ../dontdiff nfs-utils/support/include/fstab.h nfs-utils-ag/support/include/fstab.h
@@ -3,6 +3,10 @@
 
 #include "nfs_mntent.h"
 
+#ifndef _PATH_FSTAB
+#define _PATH_FSTAB "/etc/fstab"
+#endif
+
 int mtab_is_writable(void);
 int mtab_does_not_exist(void);
 
@@ -16,6 +20,10 @@ struct mntentchn *getmntoptfile (const c
 struct mntentchn *getmntdirbackward (const char *dir, struct mntentchn *mc);
 struct mntentchn *getmntdevbackward (const char *dev, struct mntentchn *mc);
 
+struct mntentchn *fstab_head (void);
+struct mntentchn *getfsfile (const char *file);
+struct mntentchn *getfsspec (const char *spec);
+
 void lock_mtab (void);
 void unlock_mtab (void);
 void update_mtab (const char *special, nfs_mntent_t *with);
support/include/nfs_mntent.h +4 -4
diff -uprN -X ../dontdiff nfs-utils/support/include/nfs_mntent.h nfs-utils-ag/support/include/nfs_mntent.h
@@ -7,10 +7,10 @@
 #define _NFS_MNTENT_H
 
 typedef struct nfs_mntent_s {
-	const char *mnt_fsname;
-	const char *mnt_dir;
-	const char *mnt_type;
-	const char *mnt_opts;
+	char *mnt_fsname;
+	char *mnt_dir;
+	char *mnt_type;
+	char *mnt_opts;
 	int mnt_freq;
 	int mnt_passno;
 } nfs_mntent_t;
support/nfs/fstab.c +54 -3

82 lines not on this page · raw message

utils/mount/Makefile.am +4 -3
diff -uprN -X ../dontdiff nfs-utils/utils/mount/Makefile.am nfs-utils-ag/utils/mount/Makefile.am
@@ -14,9 +14,10 @@ MAINTAINERCLEANFILES = Makefile.in
 
 install-exec-hook:
 	(cd $(DESTDIR)$(sbindir) && \
-	  ln -sf $(sbin_PROGRAMS) mount.nfs4 && \
-	  ln -sf $(sbin_PROGRAMS) umount.nfs && \
-	  ln -sf $(sbin_PROGRAMS) umount.nfs4)
+	  chmod +s $(sbin_PROGRAMS) && \
+	  cp -p $(sbin_PROGRAMS) $(DESTDIR)$(sbindir)/mount.nfs4 && \
+	  cp -p $(sbin_PROGRAMS) $(DESTDIR)$(sbindir)/umount.nfs && \
+	  cp -p $(sbin_PROGRAMS) $(DESTDIR)$(sbindir)/umount.nfs4)
 uninstall-hook:
 	(cd $(DESTDIR)$(sbindir) && \
 	    rm -f mount.nfs4 umount.nfs umount.nfs4)
utils/mount/mount.c +123 -18

231 lines not on this page · raw message

utils/mount/nfs_mount.h +2 -0
diff -uprN -X ../dontdiff nfs-utils/utils/mount/nfs_mount.h nfs-utils-ag/utils/mount/nfs_mount.h
@@ -80,5 +80,7 @@ struct nfs_mount_data {
 
 int nfsmount(const char *, const char *, int *, char **, char **, int *, int);
 void mount_errors(char *, int, int);
+int contains(const char *, const char *);
+char *get_value(const char *, const char *);
 
 #endif /* _NFS_MOUNT_H */
utils/mount/nfsumount.c +18 -1
diff -uprN -X ../dontdiff nfs-utils/utils/mount/nfsumount.c nfs-utils-ag/utils/mount/nfsumount.c
@@ -23,12 +23,14 @@
 #include <mntent.h>
 #include <sys/mount.h>
 #include <ctype.h>
+#include <pwd.h>
 
 #include "xcommon.h"
 #include "fstab.h"
 #include "nls.h"
 #include "conn.h"
 
+#include "nfs_mount.h"
 #include "mount_constants.h"
 #include "mount.h"
 #include "nfsumount.h"
@@ -307,7 +309,7 @@ int _nfsumount(const char *spec, const c
 		goto out_bad;
 	return nfs_call_umount(&mnt_server, &dirname);
  out_bad:
-	printf("%s: %s: not found or not mounted\n", progname, spec);
+	fprintf(stderr, "%s: %s: not found or not mounted\n", progname, spec);
 	return 0;
 }
 
@@ -376,6 +378,21 @@ int nfsumount(int argc, char *argv[])
 		printf(_("Could not find %s in mtab\n"), spec);
 
 	if(mc) {
+		if(contains(mc->m.mnt_opts, "user") && getuid() != 0) {
+			struct passwd *pw = getpwuid(getuid());
+			if(!pw || strcmp(pw->pw_name, get_value(mc->m.mnt_opts, "user="))) {
+				fprintf(stderr, "%s: permission denied to unmount %s\n",
+						progname, spec);
+				exit(1);
+			}
+		} else {
+			if(!contains(mc->m.mnt_opts, "users") && getuid() != 0) {
+				fprintf(stderr, "%s: only root can unmount %s from %s\n",
+						progname, mc->m.mnt_fsname, mc->m.mnt_dir);
+				exit(1);
+			}
+		}
+
 		ret = _nfsumount(mc->m.mnt_fsname, mc->m.mnt_opts);
 		if(ret)
 			ret = add_mtab2(mc->m.mnt_fsname, mc->m.mnt_dir,

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

#1 lore raw
Quoted 47 lines · Amit Gud

AG

------------------------------------------------------------------------

If the 'user' or 'users' mount option is specified on commandline, verify with /etc/fstab if it is really specified. Setuid bit is still ON and should remain ON since mount drops the root privileges.

I've given this patch a good hammering, but not exhaustive by any means.

---

diff -uprN -X ../dontdiff nfs-utils/utils/mount/mount.c nfs-utils-ag/utils/mount/mount.c

utils/mount/mount.c +18 -1
@@ -355,9 +433,33 @@ int main(int argc, char *argv[])
 
 	spec = argv[1];
 	mount_point = canonicalize(argv[2]);
-	
+
 	parse_opts(mount_opts, &flags, &extra_opts);
 
+	if (uid != 0 && !(flags & MS_USERS) && !(flags & MS_USER)) {
+		fprintf(stderr, "%s: permission denied\n", progname);
+		exit(1);
+	}
+
+	if ((flags & MS_USER || flags & MS_USERS) && uid != 0) {
+		/* check if fstab has entry, and further see if the user or users option is given */
+		if ((mc = getfsspec(spec)) == NULL &&
+		    (mc = getfsfile(spec)) == NULL) {
+			fprintf(stderr, "%s: permission denied - invalid option\n", progname);
+			exit(1);
+		}
+		else {
+			if((flags & MS_USER) && !contains(mc->m.mnt_opts, "user")) {
+				fprintf(stderr, "%s: permission denied - invalid option\n", progname);
+				exit(1);
+			}

utils/mount/mount.c int main(int argc, char *argv[]) · line 465

Quoted 10 lines
utils/mount/mount.c +7 -0
+			if((flags & MS_USERS) && !contains(mc->m.mnt_opts, "users")) {
+				fprintf(stderr, "%s: permission denied - invalid option\n", progname);
+				exit(1);
+			}
+		}
+	}
+
 	if (!strcmp(progname, "mount.nfs4") || nfs_mount_vers == 4) {
 		nfs_mount_vers = 4;
 		mnt_err = nfs4mount(spec, mount_point, &flags, &extra_opts, &mount_opts, 0);

It seems to me that these two "permission denied" messages may be somewhat less than completely helpful. Would it be possible to be a little more explicit about why permission was being denied?

    Thanx...

       ps

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

author
#2 lore raw
Quoted 5 lines · Peter Staubach

It seems to me that these two "permission denied" messages may be somewhat less than completely helpful. Would it be possible to be a little more explicit about why permission was being denied?

Yes possible. I was thinking of not letting the users know which user mounted the export, but that is known by mount command anyways. So, yes these messages could be more elaborate :)

AG

Signature · 12 lines
May the source be with you.
http://www.cis.ksu.edu/~gud


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
#3 lore raw
In reply to Amit Gud

On Friday July 28, [email protected] wrote:

Sorry for the delay in reviewing this.

Quoted 6 lines

If the 'user' or 'users' mount option is specified on commandline, verify with /etc/fstab if it is really specified. Setuid bit is still ON and should remain ON since mount drops the root privileges.

I've given this patch a good hammering, but not exhaustive by any means.

I still don't see this as being adequately secure.
All you are checking is whether the user has permission to mount onto the given directory, or from the given server:/directory, but not if they are allow to mount that server on that directory, or what options are required.

e.g. /etc/fstab could say you are only allowed to mount with noexec,nosuid, but mount.nfs isn't enforcing that.

It seems to me that if getuid() returns non-zero, then you have to either ignore or check everything given on the command line.

i.e.
find the entry in /etc/fstab which has the same mointpoint and the same 'special' (server:/path).
Make sure the options in /etc/fstab have an appropriate user or users entry.
Use the options from /etc/fstab and ignore the options on the command line.

Does that sound fair?

NeilBrown

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your opinions on IT & business topics through brief surveys -- and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs