dynamic /dev security hole?

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.

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

Patches to the udev HOWTO and FAQ are always welcome.

thanks,

greg k-h

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

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.

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

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

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

@@ -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 [email protected]

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

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

@@ -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 [email protected]

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

Quoting Eric Lammerts <[email protected]>:

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-----

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.

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.

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

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

I guess I'm missing something here...

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

So this wouldn't work.

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

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

Quoting Albert Cahalan <[email protected]>:

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-----

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

Quoting Eric Lammerts <[email protected]>:

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-----

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..

Thanks, I've applied this patch.

greg k-h

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

greg k-h

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

Quoting Albert Cahalan <[email protected]>:

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-----

===== arch/i386/kernel/entry.S 1.77 vs edited =====

@@ -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 =====

===== include/asm-i386/unistd.h 1.39 vs edited =====

@@ -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> */
 

#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;
}

-----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 <[email protected]>:

- --
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-----

Two other choices:

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

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.

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

Quoting Alan Cox <[email protected]>:

[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

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?

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.

Yes, yes. I know. ;)

- --
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-----