dynamic /dev security hole?

dynamic /dev security hole?

[Albert Cahalan]

Suppose I have access to a device, for whatever legit
reason. Maybe I'm given access to a USB key with
some particular serial number.

I hard link this to somewhere else. Never mind that an
admin could in theory use 42 separate partitions and
mount most of the system with the "nodev" option. This
is rarely done.

Now the device is removed. The /dev entry goes away.
A new device is added, and it gets the same device
number as the device I had legit access to. Hmmm?

I should mention open file descriptors too, though I
think the transition away from doing dev_t lookups in
the read()/write()/etc. code has taken care of that.

Re: dynamic /dev security hole?

[Marc Ballarin]

On 08 Aug 2004 08:47:40 -0400
Albert Cahalan <albert@users.sf.net> wrote:

> Suppose I have access to a device, for whatever legit
> reason. Maybe I'm given access to a USB key with
> some particular serial number.
> 
> I hard link this to somewhere else. Never mind that an
> admin could in theory use 42 separate partitions and
> mount most of the system with the "nodev" option. This
> is rarely done.
> 
> Now the device is removed. The /dev entry goes away.
> A new device is added, and it gets the same device
> number as the device I had legit access to. Hmmm?

This would require (1) /dev to be inside the root filesystem and (2) users
having write access somewhere in that filesystem.

(1) is uncommon, often a separate filesystem is used for udev
(2) is very bad practice anyway and should never be seen on a true
multi-user machine

Besides, this is nothing new. Dynamic permission updates through PAM have
the same issue, and anyone calling themselves "admin" should be well aware
of those issues. This is basic Unix-security, after all.

Yet, this issue should probably mentioned in documentation. It certainly
should be mentioned in the udev-HOWTO.

Regards

Re: dynamic /dev security hole?

[Albert Cahalan]

On Sun, 2004-08-08 at 11:58, Marc Ballarin wrote:
> On 08 Aug 2004 08:47:40 -0400
> Albert Cahalan <albert@users.sf.net> wrote:
> 
> > Suppose I have access to a device, for whatever legit
> > reason. Maybe I'm given access to a USB key with
> > some particular serial number.
> > 
> > I hard link this to somewhere else. Never mind that an
> > admin could in theory use 42 separate partitions and
> > mount most of the system with the "nodev" option. This
> > is rarely done.
> > 
> > Now the device is removed. The /dev entry goes away.
> > A new device is added, and it gets the same device
> > number as the device I had legit access to. Hmmm?
> 
> This would require (1) /dev to be inside the root filesystem and (2) users
> having write access somewhere in that filesystem.
> 
> (1) is uncommon, often a separate filesystem is used for udev
> (2) is very bad practice anyway and should never be seen on a true
> multi-user machine

Sure, blame the admins. This is like leaving landmines
in your front yard, then blaming people who step on them.

> Besides, this is nothing new. Dynamic permission updates through PAM have
> the same issue, and anyone calling themselves "admin" should be well aware
> of those issues. This is basic Unix-security, after all.

This isn't, or at least shouldn't, be basic UNIX security.

PAM should call revoke(), except that Linux still doesn't
have this system call. It's been in BSD since 4.3BSD-Reno.
Calling revoke() would fully solve the PAM problems. Any
open file descriptors would be useless. Any hard links to
the device file would get the new permission bits via the
shared inode.

IMHO, hard links need to be severely restricted by default.
It's a waste to make this even a boot option, since almost
nobody would have a legitimate need for the current behavior.
Perhaps there are other ways to deal with the problem though.

> Yet, this issue should probably mentioned in documentation. It certainly
> should be mentioned in the udev-HOWTO.

You don't have to document problems if you fix them.

Re: dynamic /dev security hole?

[Greg KH]

On Sun, Aug 08, 2004 at 11:04:31AM -0400, Albert Cahalan wrote:
> Perhaps there are other ways to deal with the problem though.

Do what a number of distros do these days, make /dev a ramfs or tmpfs.
That way no hardlinks outside /dev will work:
	$ ln /dev/hda /tmp/foo
	ln: creating hard link `/tmp/foo' to `/dev/hda': Invalid cross-device link

thanks,

greg k-h

Re: dynamic /dev security hole?

[Greg KH]

On Sun, Aug 08, 2004 at 05:58:34PM +0200, Marc Ballarin wrote:
> Yet, this issue should probably mentioned in documentation. It certainly
> should be mentioned in the udev-HOWTO.

Patches to the udev HOWTO and FAQ are always welcome.

thanks,

greg k-h

Re: dynamic /dev security hole?

[Marc Ballarin]

On Sun, 8 Aug 2004 09:21:15 -0700
Greg KH <greg@kroah.com> wrote:

> Patches to the udev HOWTO and FAQ are always welcome.
> 

How about this? The first part is a spelling fix.

--- udev-FAQ.orig	2004-08-08 18:42:03.639348944 +0200
+++ udev-FAQ	2004-08-08 23:14:07.895684768 +0200
@@ -23,7 +23,7 @@
 	- the former had stayed around for many months with maintainer
 	  claiming that everything works fine
 	- the latter had stayed, period.
-	- the devfs maintainer/author disappeared and stoped maintaining
+	- the devfs maintainer/author disappeared and stopped maintaining
 	  the code.
 
 Q: But udev will not automatically load a driver if a /dev node is opened
@@ -98,6 +98,19 @@
    And don't have to be root but will get full permissions on /pendrive.
    This works even without udev if /udev/pendrive is replaced by
/dev/sda1
 
+Q: Are there any security issues that I should be aware of?
+A: When using dynamic device numbers, a given pair of major/minor numbers
may
+   point to different hardware over time. If a user has permission to
access a
+   specific device node directly and is able to create hard links to this
node,
+   he or she can do so to create a copy of the device node. When the
device is
+   unplugged and udev removes the device node, the user's hard link
remains.
+   If the device node is later recreated with different permissions the
hard 
+   link can still be used to access the device using the old permissions.
+   (The same problem exists when using PAM to change permissions on
login.)
+    
+   The simplest solution is to prevent the creation of hard links by
putting
+   /dev in a separate filesystem (tmpfs, ramfs, ...).
+    
 Q: I have other questions about udev, where do I ask them?
 A: The linux-hotplug-devel mailing list is the proper place for it.  The
    address for it is linux-hotplug-devel@lists.sourceforge.net

Re: dynamic /dev security hole?

[Marc Ballarin]

On Sun, 8 Aug 2004 09:21:15 -0700
Greg KH <greg@kroah.com> wrote:

> Patches to the udev HOWTO and FAQ are always welcome.
> 

How about this? The first part is a spelling fix.

(Resend, I hate "smart" features in software...)

--- udev-FAQ.orig	2004-08-08 18:42:03.639348944 +0200
+++ udev-FAQ	2004-08-08 23:14:07.895684768 +0200
@@ -23,7 +23,7 @@
 	- the former had stayed around for many months with maintainer
 	  claiming that everything works fine
 	- the latter had stayed, period.
-	- the devfs maintainer/author disappeared and stoped maintaining
+	- the devfs maintainer/author disappeared and stopped maintaining
 	  the code.
 
 Q: But udev will not automatically load a driver if a /dev node is opened
@@ -98,6 +98,19 @@
    And don't have to be root but will get full permissions on /pendrive.
    This works even without udev if /udev/pendrive is replaced by /dev/sda1
 
+Q: Are there any security issues that I should be aware of?
+A: When using dynamic device numbers, a given pair of major/minor numbers may
+   point to different hardware over time. If a user has permission to access a
+   specific device node directly and is able to create hard links to this node,
+   he or she can do so to create a copy of the device node. When the device is
+   unplugged and udev removes the device node, the user's copy remains.
+   If the device node is later recreated with different permissions the hard 
+   link can still be used to access the device using the old permissions.
+   (The same problem exists when using PAM to change permissions on login.)
+    
+   The simplest solution is to prevent the creation of hard links by putting
+   /dev in a separate filesystem (tmpfs, ramfs, ...).
+    
 Q: I have other questions about udev, where do I ask them?
 A: The linux-hotplug-devel mailing list is the proper place for it.  The
    address for it is linux-hotplug-devel@lists.sourceforge.net

Re: dynamic /dev security hole?

[Eric Lammerts]

On Mon, 9 Aug 2004, Marc Ballarin wrote:
> +Q: Are there any security issues that I should be aware of?
> +A: When using dynamic device numbers, a given pair of major/minor numbers may
> +   point to different hardware over time. If a user has permission to access a
> +   specific device node directly and is able to create hard links to this node,
> +   he or she can do so to create a copy of the device node. When the device is
> +   unplugged and udev removes the device node, the user's copy remains.
> +   If the device node is later recreated with different permissions the hard
> +   link can still be used to access the device using the old permissions.

Just an idea for a fix for this problem: If udev would change the
permissions to 000 and ownership to root.root just before it unlinks
the device node, the copy would become useless.

Eric

Re: dynamic /dev security hole?

[Michael Buesch]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting Eric Lammerts <eric@lammerts.org>:
> Just an idea for a fix for this problem: If udev would change the
> permissions to 000 and ownership to root.root just before it unlinks
> the device node, the copy would become useless.

Like this?
Only compile tested against glibc.


===== udev-remove.c 1.31 vs edited =====
- --- 1.31/udev-remove.c	2004-04-01 04:12:56 +02:00
+++ edited/udev-remove.c	2004-08-09 15:23:12 +02:00
@@ -79,6 +79,23 @@
 	strfieldcat(filename, dev->name);
 
 	info("removing device node '%s'", filename);
+	/* first remove all permissions on the device node.
+	 * This fixes a security issue. If the user created
+	 * a hard-link to the device node, he can't use this
+	 * anymore, if we change permissions.
+	 */
+	retval = chmod(filename, 0000);
+	if (retval) {
+		info("chmod(%s, 0000) failed with error '%s'",
+		     filename, strerror(errno));
+		// we continue nevertheless.
+	}
+	retval = chown(filename, 0, 0);
+	if (retval) {
+		info("chown(%s, 0, 0) failed with error '%s'",
+		     filename, strerror(errno));
+		// we continue nevertheless.
+	}
 	retval = unlink(filename);
 	if (errno == ENOENT)
 		retval = 0;

- -- 
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFBF3yMFGK1OIvVOP4RArMrAJ0SvykBehCxPUYPLPIQuGRn3m5eHgCfShXs
e/S0iWzu/qTtDVb1+zp9LRo=
=UsXa
-----END PGP SIGNATURE-----

Re: dynamic /dev security hole?

[Albert Cahalan]

On Mon, 2004-08-09 at 09:30, Michael Buesch wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Quoting Eric Lammerts <eric@lammerts.org>:
> > Just an idea for a fix for this problem: If udev would change the
> > permissions to 000 and ownership to root.root just before it unlinks
> > the device node, the copy would become useless.
> 
> Like this?
> Only compile tested against glibc.

Pretty much, but you must change ownership first to
keep the user from changing the mode back. There are
ways for an evildoer to win this race if you don't
change the ownership first.

Now all we need is revoke() and we're all set.
Ordering: chown, chmod, revoke, unlink

BTW, I'm make revoke() just force re-verification
of file access.

> ===== udev-remove.c 1.31 vs edited =====
> - --- 1.31/udev-remove.c	2004-04-01 04:12:56 +02:00
> +++ edited/udev-remove.c	2004-08-09 15:23:12 +02:00
> @@ -79,6 +79,23 @@
>  	strfieldcat(filename, dev->name);
>  
>  	info("removing device node '%s'", filename);
> +	/* first remove all permissions on the device node.
> +	 * This fixes a security issue. If the user created
> +	 * a hard-link to the device node, he can't use this
> +	 * anymore, if we change permissions.
> +	 */
> +	retval = chmod(filename, 0000);
> +	if (retval) {
> +		info("chmod(%s, 0000) failed with error '%s'",
> +		     filename, strerror(errno));
> +		// we continue nevertheless.
> +	}
> +	retval = chown(filename, 0, 0);
> +	if (retval) {
> +		info("chown(%s, 0, 0) failed with error '%s'",
> +		     filename, strerror(errno));
> +		// we continue nevertheless.
> +	}
>  	retval = unlink(filename);
>  	if (errno == ENOENT)
>  		retval = 0;

Re: dynamic /dev security hole?

[Michael Buesch]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting Albert Cahalan <albert@users.sf.net>:
> Pretty much, but you must change ownership first to
> keep the user from changing the mode back. There are
> ways for an evildoer to win this race if you don't
> change the ownership first.
>
> Now all we need is revoke() and we're all set.
> Ordering: chown, chmod, revoke, unlink
>
> BTW, I'm make revoke() just force re-verification
> of file access.

So what about this updated patch?
I'm running latest udev with klibc and my patch.
It seems to work. At least with my USB stick. :)

Greg? Any comments?


===== udev-remove.c 1.31 vs edited =====
- --- 1.31/udev-remove.c	2004-04-01 04:12:56 +02:00
+++ edited/udev-remove.c	2004-08-09 18:42:08 +02:00
@@ -65,6 +65,41 @@
 	return 0;
 }

+/** Remove all permissions on the device node, before
+  * unlinking it. This fixes a security issue.
+  * If the user created a hard-link to the device node,
+  * he can't use it any longer, because he lost permission
+  * to do so.
+  */
+static int secure_unlink(const char *filename)
+{
+	int retval;
+
+	retval = chown(filename, 0, 0);
+	if (retval) {
+		dbg("chown(%s, 0, 0) failed with error '%s'",
+		    filename, strerror(errno));
+		/* We continue nevertheless.
+		 * I think it's very unlikely for chown
+		 * to fail here, if the file exists.
+		 */
+	}
+	retval = chmod(filename, 0000);
+	if (retval) {
+		dbg("chmod(%s, 0000) failed with error '%s'",
+		    filename, strerror(errno));
+		/* We continue nevertheless. */
+	}
+	retval = unlink(filename);
+	if (errno == ENOENT)
+		retval = 0;
+	if (retval) {
+		dbg("unlink(%s) failed with error '%s'",
+			filename, strerror(errno));
+	}
+	return retval;
+}
+
 static int delete_node(struct udevice *dev)
 {
 	char filename[NAME_SIZE];
@@ -79,14 +114,9 @@
 	strfieldcat(filename, dev->name);

 	info("removing device node '%s'", filename);
- -	retval = unlink(filename);
- -	if (errno == ENOENT)
- -		retval = 0;
- -	if (retval) {
- -		dbg("unlink(%s) failed with error '%s'",
- -			filename, strerror(errno));
+	retval = secure_unlink(filename);
+	if (retval)
 		return retval;
- -	}

 	/* remove partition nodes */
 	if (dev->partitions > 0) {
@@ -94,7 +124,7 @@
 		for (i = 1; i <= dev->partitions; i++) {
 			strfieldcpy(partitionname, filename);
 			strintcat(partitionname, i);
- -			unlink(partitionname);
+			secure_unlink(partitionname);
 		}
 	}

@@ -108,14 +138,9 @@
 		strfieldcat(filename, linkname);

 		dbg("unlinking symlink '%s'", filename);
- -		retval = unlink(filename);
- -		if (errno == ENOENT)
- -			retval = 0;
- -		if (retval) {
- -			dbg("unlink(%s) failed with error '%s'",
- -				filename, strerror(errno));
+		retval = secure_unlink(filename);
+		if (retval)
 			return retval;
- -		}
 		if (strchr(dev->symlink, '/')) {
 			delete_path(filename);
 		}

- --
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFBF6xAFGK1OIvVOP4RArfvAJwNf/CnIZTnEvUSNu3N4WI3tkqBOwCfe4B8
sM+KFpEX2PuDKsqcjTO9pV8=
=rd9Y
-----END PGP SIGNATURE-----

Re: dynamic /dev security hole?

[Eric Lammerts]

On Mon, 9 Aug 2004, Michael Buesch wrote:
> So what about this updated patch?

>  		dbg("unlinking symlink '%s'", filename);
> - -		retval = unlink(filename);
> - -		if (errno == ENOENT)
> - -			retval = 0;
> - -		if (retval) {
> - -			dbg("unlink(%s) failed with error '%s'",
> - -				filename, strerror(errno));
> +		retval = secure_unlink(filename);

Better not do it for symlinks.

Eric

Re: dynamic /dev security hole?

[Michael Buesch]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting Eric Lammerts <eric@lammerts.org>:
> Better not do it for symlinks.

Yes, you're right.

===== udev-remove.c 1.31 vs edited =====
- --- 1.31/udev-remove.c	2004-04-01 04:12:56 +02:00
+++ edited/udev-remove.c	2004-08-09 19:12:55 +02:00
@@ -65,6 +65,41 @@
 	return 0;
 }
 
+/** Remove all permissions on the device node, before
+  * unlinking it. This fixes a security issue.
+  * If the user created a hard-link to the device node,
+  * he can't use it any longer, because he lost permission
+  * to do so.
+  */
+static int secure_unlink(const char *filename)
+{
+	int retval;
+
+	retval = chown(filename, 0, 0);
+	if (retval) {
+		dbg("chown(%s, 0, 0) failed with error '%s'",
+		    filename, strerror(errno));
+		/* We continue nevertheless.
+		 * I think it's very unlikely for chown
+		 * to fail here, if the file exists.
+		 */
+	}
+	retval = chmod(filename, 0000);
+	if (retval) {
+		dbg("chmod(%s, 0000) failed with error '%s'",
+		    filename, strerror(errno));
+		/* We continue nevertheless. */
+	}
+	retval = unlink(filename);
+	if (errno == ENOENT)
+		retval = 0;
+	if (retval) {
+		dbg("unlink(%s) failed with error '%s'",
+			filename, strerror(errno));
+	}
+	return retval;
+}
+
 static int delete_node(struct udevice *dev)
 {
 	char filename[NAME_SIZE];
@@ -79,14 +114,9 @@
 	strfieldcat(filename, dev->name);
 
 	info("removing device node '%s'", filename);
- -	retval = unlink(filename);
- -	if (errno == ENOENT)
- -		retval = 0;
- -	if (retval) {
- -		dbg("unlink(%s) failed with error '%s'",
- -			filename, strerror(errno));
+	retval = secure_unlink(filename);
+	if (retval)
 		return retval;
- -	}
 
 	/* remove partition nodes */
 	if (dev->partitions > 0) {
@@ -94,7 +124,7 @@
 		for (i = 1; i <= dev->partitions; i++) {
 			strfieldcpy(partitionname, filename);
 			strintcat(partitionname, i);
- -			unlink(partitionname);
+			secure_unlink(partitionname);
 		}
 	}
 

- -- 
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFBF7DoFGK1OIvVOP4RAklbAJ0QXZkfiDOExHqTXpue+mKJCeIBHwCgwzAa
3V4LF0jNgiDyXbm6rw4wxqI=
=wXmk
-----END PGP SIGNATURE-----

Re: dynamic /dev security hole?

[Greg KH]

On Mon, Aug 09, 2004 at 07:14:16PM +0200, Michael Buesch wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Quoting Eric Lammerts <eric@lammerts.org>:
> > Better not do it for symlinks.
> 
> Yes, you're right.

Thanks, I've applied this patch.

greg k-h

[RFC, PATCH] sys_revoke(), just a try. (was: Re: dynamic /dev security hole?)

[Michael Buesch]

[-- Attachment #1: Type: Text/Plain, Size: 1057 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting Albert Cahalan <albert@users.sf.net>:
> Now all we need is revoke() and we're all set.
> Ordering: chown, chmod, revoke, unlink

So, I searched the archives and found two previous attempts to create
a revoke syscall. They were long long ago.
I picked up some of its code and did a patch for latest 2.6 bk.
I'm currently running a kernel with the patch applied and did some
basic tests on it. Testing source is attached, too.

I am a beginner in Kernel programming. I read some books and online
tutorials, but did not write much code.
So I'm sure, the code is full of bugs. I think there are nasty
race conditions.
Would be cool if someone could review it for races and report
them, please. I've not yet the experience to find them.

Thank you. Have fun.

- --
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFBGlNzFGK1OIvVOP4RAm0DAJ9CN1Q8tUpfNhLT9JlUSLL42pMkPwCeM5WC
UpTTZceoGL97cJkaPR9v9tU=
=fbpi
-----END PGP SIGNATURE-----

[-- Attachment #2: revoke_1.patch --]
[-- Type: text/x-diff, Size: 3945 bytes --]

===== arch/i386/kernel/entry.S 1.77 vs edited =====
--- 1.77/arch/i386/kernel/entry.S	2004-05-22 23:56:24 +02:00
+++ edited/arch/i386/kernel/entry.S	2004-08-10 20:11:29 +02:00
@@ -886,5 +886,6 @@
 	.long sys_mq_notify
 	.long sys_mq_getsetattr
 	.long sys_ni_syscall		/* reserved for kexec */
+	.long sys_revoke
 
 syscall_table_size=(.-sys_call_table)
===== fs/open.c 1.68 vs edited =====
--- 1.68/fs/open.c	2004-08-08 03:54:13 +02:00
+++ edited/fs/open.c	2004-08-11 18:58:26 +02:00
@@ -1043,6 +1043,134 @@
 
 EXPORT_SYMBOL(sys_close);
 
+static int revoke_ret_ebadf(void)
+{
+	return -EBADF;
+}
+
+static ssize_t revoke_read(struct file *filp,
+			   char *buf,
+			   size_t count,
+			   loff_t *ppos)
+{
+	return 0;
+}
+
+static int revoke_release(struct inode *inode, struct file *filp)
+{
+	fops_put(filp->f_op);
+	filp->f_op = NULL;
+	return 0;
+}
+
+static ssize_t revoke_readv(struct file *filp,
+			    const struct iovec *iov,
+			    unsigned long count,
+			    loff_t *ppos)
+{
+	return 0;
+}
+
+static struct file_operations revoke_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= (void *)revoke_ret_ebadf,
+	.read		= revoke_read,
+	.write		= (void *)revoke_ret_ebadf,
+	.readdir	= (void *)revoke_ret_ebadf,
+	.poll		= (void *)revoke_ret_ebadf,
+	.ioctl		= (void *)revoke_ret_ebadf,
+	.mmap		= (void *)revoke_ret_ebadf,
+	.open		= (void *)revoke_ret_ebadf,
+	.flush		= (void *)revoke_ret_ebadf,
+	.release	= revoke_release,
+	.fsync		= (void *)revoke_ret_ebadf,
+	.fasync		= (void *)revoke_ret_ebadf,
+	.lock		= (void *)revoke_ret_ebadf,
+	.readv		= revoke_readv,
+	.writev		= (void *)revoke_ret_ebadf,
+};
+
+static int filp_revoke(struct file *filp, struct inode *inode)
+{
+	struct file_operations *fops = filp->f_op;
+	int ret = 0;
+
+	down(&inode->i_sem);
+	if (!fops || !file_count(filp))
+		goto out_truncate;
+
+	filp->f_op = &revoke_fops;
+
+	if (fops->flush)
+		fops->flush(filp);
+	if (fops->release)
+		ret = fops->release(inode, filp);
+	fops_put(fops);
+
+out_truncate:
+	vmtruncate(inode, (loff_t)0);
+	up(&inode->i_sem);
+	return ret;
+}
+
+asmlinkage int sys_revoke(const char *path)
+{
+	struct nameidata nd;
+	struct super_block *sb;
+	struct list_head *p;
+	struct file *filp;
+	int ret = 0;
+printk("called sys_revoke()\n");
+
+	if (user_path_walk(path, &nd)) {
+printk("user_path_walk() failed\n");
+		ret = -ENOENT;
+		goto out;
+	}
+	if (!nd.dentry->d_inode) {
+printk("no inode\n");
+		ret = -ENOENT;
+		goto out_release;
+	}
+	/* Allow only on Character or Block Devices. */
+	if (!S_ISCHR(nd.dentry->d_inode->i_mode) &&
+	    !S_ISBLK(nd.dentry->d_inode->i_mode)) {
+printk("no CHK/BLK\n");
+		ret = -EINVAL;
+		goto out_release;
+	}
+	if ((current->fsuid != nd.dentry->d_inode->i_uid) ||
+	    !capable(CAP_FOWNER)) {
+printk("perm\n");
+		ret = -EPERM;
+		goto out_release;
+	}
+
+	sb = nd.dentry->d_inode->i_sb;
+	file_list_lock();
+	for (p = sb->s_files.next; p != &sb->s_files; p = p->next) {
+		filp = list_entry(p, struct file, f_list);
+
+		if (!filp || !filp->f_dentry)
+			continue;
+		if (nd.dentry != filp->f_dentry)
+			continue;
+		if (filp->f_op == &revoke_fops)
+			continue;
+		if (!filp->f_dentry->d_inode)
+			continue;
+
+		ret = filp_revoke(filp, filp->f_dentry->d_inode);
+	}
+	file_list_unlock();
+printk("done");
+
+out_release:
+	path_release(&nd);
+out:
+	return ret;
+}
+
 /*
  * This routine simulates a hangup on the tty, to arrange that users
  * are given clean terminals at login time.
===== include/asm-i386/unistd.h 1.39 vs edited =====
--- 1.39/include/asm-i386/unistd.h	2004-08-02 10:00:44 +02:00
+++ edited/include/asm-i386/unistd.h	2004-08-10 20:14:44 +02:00
@@ -289,8 +289,9 @@
 #define __NR_mq_notify		(__NR_mq_open+4)
 #define __NR_mq_getsetattr	(__NR_mq_open+5)
 #define __NR_sys_kexec_load	283
+#define __NR_revoke		284
 
-#define NR_syscalls 284
+#define NR_syscalls 285
 
 /* user-visible error numbers are in the range -1 - -124: see <asm-i386/errno.h> */
 

[-- Attachment #3: revoke_test.c --]
[-- Type: text/x-csrc, Size: 903 bytes --]

#include <stdio.h>
#include <syscall.h>
#include <errno.h>
#include <unistd.h>
#include <fcntl.h>

#define __NR_revoke	284

_syscall1(int, revoke, const char *, path)


int parent()
{
	int fd, ret;

	fd = open("/dev/null", O_RDWR);
	if (fd < 0) {
		printf("open() in parent failed!\n");
		return 1;
	}
	const char foo[] = "hsdfuknf23uhzfneiou1208jiomiiu";
	while (1) {
		printf("write...\n");
		ret = write(fd, foo, sizeof(foo));
		if (ret <= 0) {
			printf("write() in parent failed with %d (%s)!\n",
			       errno, strerror(errno));
			return 1;
		}
	}
}

int child()
{
	sleep(3);
	revoke("/dev/null");
	printf("revoke() returned %d (%s)\n",
	       errno, strerror(errno));

	return 0;
}

int main(int argc, char *argv[])
{
	int ret;

	ret = fork();
	if (ret == 0) {
		return child();
	} else if (ret == -1) {
		printf("fork() failed!\n");
		return 1;
	} else {
		return parent();
	}

	return 2;
}

Re: [RFC, PATCH] sys_revoke(), just a try. (was: Re: dynamic /dev security hole?)

[Michael Buesch]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Nobody interrested in this syscall?

I can't code this on my own, because I don't have the experience.
Need help for this IMHO needed syscall.

And yes, I forgot to attach the patch inline, so you
can comment on it. So here it is. :)


===== arch/i386/kernel/entry.S 1.77 vs edited =====
- --- 1.77/arch/i386/kernel/entry.S	2004-05-22 23:56:24 +02:00
+++ edited/arch/i386/kernel/entry.S	2004-08-10 20:11:29 +02:00
@@ -886,5 +886,6 @@
 	.long sys_mq_notify
 	.long sys_mq_getsetattr
 	.long sys_ni_syscall		/* reserved for kexec */
+	.long sys_revoke
 
 syscall_table_size=(.-sys_call_table)
===== fs/open.c 1.68 vs edited =====
- --- 1.68/fs/open.c	2004-08-08 03:54:13 +02:00
+++ edited/fs/open.c	2004-08-11 18:58:26 +02:00
@@ -1043,6 +1043,134 @@
 
 EXPORT_SYMBOL(sys_close);
 
+static int revoke_ret_ebadf(void)
+{
+	return -EBADF;
+}
+
+static ssize_t revoke_read(struct file *filp,
+			   char *buf,
+			   size_t count,
+			   loff_t *ppos)
+{
+	return 0;
+}
+
+static int revoke_release(struct inode *inode, struct file *filp)
+{
+	fops_put(filp->f_op);
+	filp->f_op = NULL;
+	return 0;
+}
+
+static ssize_t revoke_readv(struct file *filp,
+			    const struct iovec *iov,
+			    unsigned long count,
+			    loff_t *ppos)
+{
+	return 0;
+}
+
+static struct file_operations revoke_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= (void *)revoke_ret_ebadf,
+	.read		= revoke_read,
+	.write		= (void *)revoke_ret_ebadf,
+	.readdir	= (void *)revoke_ret_ebadf,
+	.poll		= (void *)revoke_ret_ebadf,
+	.ioctl		= (void *)revoke_ret_ebadf,
+	.mmap		= (void *)revoke_ret_ebadf,
+	.open		= (void *)revoke_ret_ebadf,
+	.flush		= (void *)revoke_ret_ebadf,
+	.release	= revoke_release,
+	.fsync		= (void *)revoke_ret_ebadf,
+	.fasync		= (void *)revoke_ret_ebadf,
+	.lock		= (void *)revoke_ret_ebadf,
+	.readv		= revoke_readv,
+	.writev		= (void *)revoke_ret_ebadf,
+};
+
+static int filp_revoke(struct file *filp, struct inode *inode)
+{
+	struct file_operations *fops = filp->f_op;
+	int ret = 0;
+
+	down(&inode->i_sem);
+	if (!fops || !file_count(filp))
+		goto out_truncate;
+
+	filp->f_op = &revoke_fops;
+
+	if (fops->flush)
+		fops->flush(filp);
+	if (fops->release)
+		ret = fops->release(inode, filp);
+	fops_put(fops);
+
+out_truncate:
+	vmtruncate(inode, (loff_t)0);
+	up(&inode->i_sem);
+	return ret;
+}
+
+asmlinkage int sys_revoke(const char *path)
+{
+	struct nameidata nd;
+	struct super_block *sb;
+	struct list_head *p;
+	struct file *filp;
+	int ret = 0;
+printk("called sys_revoke()\n");
+
+	if (user_path_walk(path, &nd)) {
+printk("user_path_walk() failed\n");
+		ret = -ENOENT;
+		goto out;
+	}
+	if (!nd.dentry->d_inode) {
+printk("no inode\n");
+		ret = -ENOENT;
+		goto out_release;
+	}
+	/* Allow only on Character or Block Devices. */
+	if (!S_ISCHR(nd.dentry->d_inode->i_mode) &&
+	    !S_ISBLK(nd.dentry->d_inode->i_mode)) {
+printk("no CHK/BLK\n");
+		ret = -EINVAL;
+		goto out_release;
+	}
+	if ((current->fsuid != nd.dentry->d_inode->i_uid) ||
+	    !capable(CAP_FOWNER)) {
+printk("perm\n");
+		ret = -EPERM;
+		goto out_release;
+	}
+
+	sb = nd.dentry->d_inode->i_sb;
+	file_list_lock();
+	for (p = sb->s_files.next; p != &sb->s_files; p = p->next) {
+		filp = list_entry(p, struct file, f_list);
+
+		if (!filp || !filp->f_dentry)
+			continue;
+		if (nd.dentry != filp->f_dentry)
+			continue;
+		if (filp->f_op == &revoke_fops)
+			continue;
+		if (!filp->f_dentry->d_inode)
+			continue;
+
+		ret = filp_revoke(filp, filp->f_dentry->d_inode);
+	}
+	file_list_unlock();
+printk("done");
+
+out_release:
+	path_release(&nd);
+out:
+	return ret;
+}
+
 /*
  * This routine simulates a hangup on the tty, to arrange that users
  * are given clean terminals at login time.
===== include/asm-i386/unistd.h 1.39 vs edited =====
- --- 1.39/include/asm-i386/unistd.h	2004-08-02 10:00:44 +02:00
+++ edited/include/asm-i386/unistd.h	2004-08-10 20:14:44 +02:00
@@ -289,8 +289,9 @@
 #define __NR_mq_notify		(__NR_mq_open+4)
 #define __NR_mq_getsetattr	(__NR_mq_open+5)
 #define __NR_sys_kexec_load	283
+#define __NR_revoke		284
 
- -#define NR_syscalls 284
+#define NR_syscalls 285
 
 /* user-visible error numbers are in the range -1 - -124: see <asm-i386/errno.h> */
 



Quoting Michael Buesch <mbuesch@freenet.de>:
> Quoting Albert Cahalan <albert@users.sf.net>:
> > Now all we need is revoke() and we're all set.
> > Ordering: chown, chmod, revoke, unlink
> 
> So, I searched the archives and found two previous attempts to create
> a revoke syscall. They were long long ago.
> I picked up some of its code and did a patch for latest 2.6 bk.
> I'm currently running a kernel with the patch applied and did some
> basic tests on it. Testing source is attached, too.
> 
> I am a beginner in Kernel programming. I read some books and online
> tutorials, but did not write much code.
> So I'm sure, the code is full of bugs. I think there are nasty
> race conditions.
> Would be cool if someone could review it for races and report
> them, please. I've not yet the experience to find them.
> 
> Thank you. Have fun.
> 

- -- 
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFBG5+NFGK1OIvVOP4RAoTgAKClBCUcLqJYk8o7aukgpkCzGEjVswCfZJVm
6ZsZw85Cps9LihNCX/3RnTI=
=LjLk
-----END PGP SIGNATURE-----

Re: [RFC, PATCH] sys_revoke(), just a try. (was: Re: dynamic /dev security hole?)

[Alan Cox]

On Iau, 2004-08-12 at 17:49, Michael Buesch wrote:
> +static ssize_t revoke_read(struct file *filp,
> +			   char *buf,
> +			   size_t count,
> +			   loff_t *ppos)
> +{
> +	return 0;
> +}

-EIO I think but I'm not sure I remember the BSD behaviour in full

> +static int filp_revoke(struct file *filp, struct inode *inode)
> +{

First problem here is that the handle might still be in use
for mmap, so you'd need to undo mmaps on it. A second is that 
while you can ->flush() here you can't really close it until the
file usage count hits zero. 

You are btw tackling a really really hard problem and its more likely
the way to do this is to add revoke() methods to drivers and do it at
the driver level - as the tty layer does with vhangup.

Re: [RFC, PATCH] sys_revoke(), just a try. (was: Re: dynamic /dev security hole?)

[Albert Cahalan]

On Thu, 2004-08-12 at 15:51, Alan Cox wrote:
> On Iau, 2004-08-12 at 17:49, Michael Buesch wrote:
> > +static ssize_t revoke_read(struct file *filp,
> > +			   char *buf,
> > +			   size_t count,
> > +			   loff_t *ppos)
> > +{
> > +	return 0;
> > +}
> 
> -EIO I think but I'm not sure I remember the BSD behaviour in full
> 
> > +static int filp_revoke(struct file *filp, struct inode *inode)
> > +{
> 
> First problem here is that the handle might still be in use
> for mmap, so you'd need to undo mmaps on it.

Two other choices:

a. map anon memory over the old mapping
b. fail the revoke() call, perhaps with EBUSY

> A second is that 
> while you can ->flush() here you can't really close it until the
> file usage count hits zero. 
> 
> You are btw tackling a really really hard problem and its more likely
> the way to do this is to add revoke() methods to drivers and do it at
> the driver level - as the tty layer does with vhangup.

What about using a signal with enforced action?
(like SIGSTOP and SIGKILL) The user could still
install a handler, but only to know when revoke()
has taken action. I have a feeling that this would
be less trouble, since the losing process performs
action on its own data structures.

BTW, one must watch out for dup2() in another thread.

Re: [RFC, PATCH] sys_revoke(), just a try. (was: Re: dynamic /dev security hole?)

[Michael Buesch]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoting Alan Cox <alan@lxorguk.ukuu.org.uk>:
> On Iau, 2004-08-12 at 17:49, Michael Buesch wrote:
> > +static ssize_t revoke_read(struct file *filp,
> > +			   char *buf,
> > +			   size_t count,
> > +			   loff_t *ppos)
> > +{
> > +	return 0;
> > +}
> 
> -EIO I think but I'm not sure I remember the BSD behaviour in full

[SNIP]
REVOKE(2)                 OpenBSD Programmer's Manual                REVOKE(2)

NAME
     revoke - revoke file access

SYNOPSIS
     #include <unistd.h>

     int
     revoke(const char *path);

DESCRIPTION
     The revoke function invalidates all current open file descriptors in the
     system for the file named by path. Subsequent operations on any such de-
     scriptors fail, with the exceptions that a read() from a character device
     file which has been revoked returns a count of zero (end of file), and a
     close() call will succeed.  If the file is a special file for a device
     which is open, the device close function is called as if all open refer-
     ences to the file had been close
[SNAP]

Means, that it returns different values for normal files and device
files? Do I understand that correctly?
- -EIO for files
0 for device nodes

> > +static int filp_revoke(struct file *filp, struct inode *inode)
> > +{
> 
> First problem here is that the handle might still be in use
> for mmap, so you'd need to undo mmaps on it. A second is that 

So I guess if we simply munmap them, programs will fail (will
segfault?). Or is it safe to munmap a mapping, while some
program is using it?

> while you can ->flush() here you can't really close it until the
> file usage count hits zero. 

Is it really required to close it in revoke? I mean, if we flush it,
no data remains unwritten to disk. The user program will call
close, later by itself.

> You are btw tackling a really really hard problem and its more likely

Yes, yes. I know. ;)

> the way to do this is to add revoke() methods to drivers and do it at
> the driver level - as the tty layer does with vhangup.

- -- 
Regards Michael Buesch  [ http://www.tuxsoft.de.vu ]


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)

iD8DBQFBHLZ1FGK1OIvVOP4RAsXCAKCacyQvhemTY3CU47IwRBRxkBJYOwCguHq6
uKebhV87/GyDIJT5hk+0tbk=
=sIL3
-----END PGP SIGNATURE-----

Re: dynamic /dev security hole?

[Alan Cox]

On Llu, 2004-08-09 at 05:40, Eric Lammerts wrote:
> Just an idea for a fix for this problem: If udev would change the
> permissions to 000 and ownership to root.root just before it unlinks
> the device node, the copy would become useless.

Unfortunately not the whole story. The permissions are checked at open
time not on read/write/ioctl so once I have the device opened you
lose. That means you may have to fix the permissions and ownership
before trying to unload the device. The unload will fail if it is still
busy but if not then nobody will be able to open the dev node and
whatever file it now points to.

Re: dynamic /dev security hole?

[Eric Lammerts]

On Mon, 9 Aug 2004, Alan Cox wrote:
> On Llu, 2004-08-09 at 05:40, Eric Lammerts wrote:
> > Just an idea for a fix for this problem: If udev would change the
> > permissions to 000 and ownership to root.root just before it unlinks
> > the device node, the copy would become useless.
>
> Unfortunately not the whole story. The permissions are checked at open
> time not on read/write/ioctl so once I have the device opened you
> lose.

It's only meant as a fix for the hardlink trick, not against the open
file descriptor trick. About the latter, if someone still has the
device opened, how can it go away? And if it doesn't go away, how can
udev create a new node with the same major/minor?

Eric

Re: dynamic /dev security hole?

[Alan Cox]

On Llu, 2004-08-09 at 17:17, Eric Lammerts wrote:
> It's only meant as a fix for the hardlink trick, not against the open
> file descriptor trick. About the latter, if someone still has the
> device opened, how can it go away? And if it doesn't go away, how can
> udev create a new node with the same major/minor?

User closes device
I have linked copy (not open)
Device unloaded
I open the linked copy
This makes new device load for me.


I'm just trying to point out that the order of operations matters here
because the old nodes must all be dead before the new device. Its even
worse for less dynamic numbering.

Alan

Re: dynamic /dev security hole?

[Eric Lammerts]

I guess I'm missing something here...

On Mon, 9 Aug 2004, Alan Cox wrote:
> User closes device
> I have linked copy (not open)
> Device unloaded

At this point, udev (with the chown/chmod patch) would chown your
linked copy (before unlinking the original device node), right?

> I open the linked copy

So this wouldn't work.

> This makes new device load for me.
>
>
> I'm just trying to point out that the order of operations matters here
> because the old nodes must all be dead before the new device. Its even
> worse for less dynamic numbering.

The only problem I can think of is a race when a new device appears
with the same major/minor before udev gets a chance to do its stuff,
eg.

1) User closes device
2) I have linked copy (not open)
3) Device unloaded
4) New device appears
5) I open the linked copy
6) Udev chowns/chmods old device node (triggered by 3)) --> too late

Eric

Re: dynamic /dev security hole?

[Alan Cox]

On Llu, 2004-08-09 at 17:47, Eric Lammerts wrote:
> > I have linked copy (not open)
> > Device unloaded
> 
> At this point, udev (with the chown/chmod patch) would chown your
> linked copy (before unlinking the original device node), right?

Providing it does so its safe I think yes

Well apart from stupid things like udev on shared NFS filestore, but 
anyone doing that probably deserves it..

Re: dynamic /dev security hole?

[Greg KH]

On Mon, Aug 09, 2004 at 12:07:27AM +0200, Marc Ballarin wrote:
> On Sun, 8 Aug 2004 09:21:15 -0700
> Greg KH <greg@kroah.com> wrote:
> 
> > Patches to the udev HOWTO and FAQ are always welcome.
> > 
> 
> How about this? The first part is a spelling fix.
> 
> (Resend, I hate "smart" features in software...)

Heh.  Thanks, I've applied this, and it will show up in the next udev
release.

greg k-h