This is the first in a series of eight patches to the BSD Secure
Levels LSM. It overhauls the printk mechanism in order to reduce the
unnecessary usage of the .text area. Thanks to Brad Spengler for the
suggestion.
Signed off by: Michael Halcrow <[email protected]>
This is the second in a series of eight patches to the BSD Secure
Levels LSM. It allows setuid and setgid on directories. It also
disallows the creation of setuid/setgid executables via open or mknod.
Thanks to Brad Spengler for the suggestion.
Signed off by: Michael Halcrow <[email protected]>
This is the third in a series of eight patches to the BSD Secure
Levels LSM. It moves the claim on the block device from the inode
struct to the file struct in order to address a potential
circumvention of the control via hard links to block devices. Thanks
to Serge Hallyn for pointing this out.
Signed off by: Michael Halcrow <[email protected]>
This is the fifth in a series of eight patches to the BSD Secure
Levels LSM. It allows setuid and setgid on a process if the user is
already root. This allows non-root users to log in. Thanks to Serge
Hallyn for the suggestion.
Signed off by: Michael Halcrow <[email protected]>
This is the seventh in a series of eight patches to the BSD Secure
Levels LSM. It makes several trivial changes to comments in order to
make the code look more pretty.
Signed off by: Michael Halcrow <[email protected]>
This is the sixth in a series of eight patches to the BSD Secure
Levels LSM. It makes several trivial changes to make the code
consistent.
Signed off by: Michael Halcrow <[email protected]>
This is the eighth in a series of eight patches to the BSD Secure
Levels LSM. It removes the ptrace check because it is redundant with
the check made in kernel/ptrace.c. Thanks for Brad Spengler for this
suggestion.
Signed off by: Michael Halcrow <[email protected]>
This is the fourth in a series of eight patches to the BSD Secure
Levels LSM. It adds a check for a memory allocation failure
condition. Thanks to Vesa-Matti J Kari for pointing out this problem.
Signed off by: Michael Halcrow <[email protected]>
* Michael Halcrow ([email protected]) wrote:
> This is the third in a series of eight patches to the BSD Secure
> Levels LSM. It moves the claim on the block device from the inode
> struct to the file struct in order to address a potential
> circumvention of the control via hard links to block devices. Thanks
> to Serge Hallyn for pointing this out.
Hard links still point to same inode, what's the issue that this
addresses?
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
On Mon, 07 Feb 2005 14:26:03 PST, Chris Wright said:
> Hard links still point to same inode, what's the issue that this
> addresses?
For those systems that have everything on one big partition, you can often
do stuff like:
ln /etc/passwd /tmp/<filename_generated_by_mktemp>
and wait for /etc/passwd to get clobbered by a cron job run by root...
On Mon, 07 Feb 2005 14:26:03 PST, Chris Wright said:
> * Michael Halcrow ([email protected]) wrote:
> > This is the third in a series of eight patches to the BSD Secure
> > Levels LSM. It moves the claim on the block device from the inode
> > struct to the file struct in order to address a potential
> > circumvention of the control via hard links to block devices. Thanks
> > to Serge Hallyn for pointing this out.
>
> Hard links still point to same inode, what's the issue that this
> addresses?
Ignore that last - I thought it was the "filesystem linking permissions"
thread rather than the BSD Secure linking permissions thread. ;)
>For those systems that have everything on one big partition, you can often
>do stuff like:
>
>ln /etc/passwd /tmp/<filename_generated_by_mktemp>
>
>and wait for /etc/passwd to get clobbered by a cron job run by root...
How would /etc/passwd get clobbered? Are you thinking that a tmp
cleaner run by cron might delete /tmp/whatever (i.e., delete the hardlink
you created above)? But deleting /tmp/whatever is safe; it doesn't affect
/etc/passwd. I'm guessing I'm probably missing something.
On Tue, 08 Feb 2005 01:48:40 GMT, David Wagner said:
> How would /etc/passwd get clobbered? Are you thinking that a tmp
> cleaner run by cron might delete /tmp/whatever (i.e., delete the hardlink
> you created above)? But deleting /tmp/whatever is safe; it doesn't affect
> /etc/passwd. I'm guessing I'm probably missing something.
The attack is to hardlink some tempfile name to some file you want over-written.
This usually involves just a little bit of work, such as recognizing that a given
root cronjob uses an unsafe predictable filename in /tmp (look at the Bugtraq or
Full-Disclosure archives, there's plenty). Then you set a little program that
sleep()s till a few seconds before the cronjob runs, does a getpid(), and then
sprays hardlinks into the next 15 or 20 things that mktemp() will generate...
Consider how bash implements "here" scripts:
#!/bin/bash
echo << EOF
some trash
EOF
Now let's look at the strace (snipped for brevity..)
statfs("/tmp", {f_type="EXT2_SUPER_MAGIC", f_bsize=1024, f_blocks=253871, f_bfree=213773, f_bavail=200666, f_files=65536, f_ffree=65445, f_fsid={0, 0}, f_namelen=255, f_frsize=1024}) = 0
time(NULL) = 1107828098
open("/tmp/sh-thd-1107848098", O_WRONLY|O_CREAT|O_TRUNC|O_EXCL|O_LARGEFILE, 0600) = 3
dup(3) = 4
fcntl64(4, F_GETFL) = 0x8001 (flags O_WRONLY|O_LARGEFILE)
fstat64(4, {st_mode=S_IFREG|0600, st_size=0, ...}) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb7d71000
_llseek(4, 0, [0], SEEK_CUR) = 0
write(4, "some trash\n", 11) = 11
close(4) = 0
munmap(0xb7d71000, 4096) = 0
open("/tmp/sh-thd-1107848098", O_RDONLY|O_LARGEFILE) = 4
close(3) = 0
unlink("/tmp/sh-thd-1107848098") = 0
fcntl64(0, F_GETFD) = 0
fcntl64(0, F_DUPFD, 10) = 10
fcntl64(0, F_GETFD) = 0
fcntl64(10, F_SETFD, FD_CLOEXEC) = 0
dup2(4, 0) = 0
close(4) = 0
Wow - if my /tmp was on the same partition, and I'd hard-linked that
file to /etc/passwd, it would be toast now if root had run it.
You usually can't control what gets written - but often it's sufficient for the
attacker to simply get a file clobbered....
* [email protected] ([email protected]) wrote:
> open("/tmp/sh-thd-1107848098", O_WRONLY|O_CREAT|O_TRUNC|O_EXCL|O_LARGEFILE, 0600) = 3
O_EXCL
> Wow - if my /tmp was on the same partition, and I'd hard-linked that
> file to /etc/passwd, it would be toast now if root had run it.
So, in fact, it wouldn't ;-)
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
On Mon, 07 Feb 2005 18:20:36 PST, Chris Wright said:
> * [email protected] ([email protected]) wrote:
> > open("/tmp/sh-thd-1107848098", O_WRONLY|O_CREAT|O_TRUNC|O_EXCL|O_LARGEFILE,
0600) = 3
>
> O_EXCL
>
> > Wow - if my /tmp was on the same partition, and I'd hard-linked that
> > file to /etc/passwd, it would be toast now if root had run it.
>
> So, in fact, it wouldn't ;-)
Well.. Yeah. bash gets it right, a lot of programs botch it. ;)
>The attack is to hardlink some tempfile name to some file you want
>over-written. This usually involves just a little bit of work, such as
>recognizing that a given root cronjob uses an unsafe predictable filename
>in /tmp (look at the Bugtraq or Full-Disclosure archives, there's plenty).
>Then you set a little program that sleep()s till a few seconds before
>the cronjob runs, does a getpid(), and then sprays hardlinks into the
>next 15 or 20 things that mktemp() will generate...
Got it. Very good -- now I see. Thanks for the explanation.
On Mon, Feb 07, 2005 at 02:26:03PM -0800, Chris Wright wrote:
> * Michael Halcrow ([email protected]) wrote:
> > This is the third in a series of eight patches to the BSD Secure
> > Levels LSM. It moves the claim on the block device from the inode
> > struct to the file struct in order to address a potential
> > circumvention of the control via hard links to block devices. Thanks
> > to Serge Hallyn for pointing this out.
>
> Hard links still point to same inode, what's the issue that this
> addresses?
Actually, it turns out that hard links have nothing to do with the
vulnerability that this patch addresses:
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdio.h>
int main()
{
int fd1, fd2;
int rc;
fd1 = open( "/dev/device", O_RDONLY );
fd2 = open( "/dev/device", O_RDWR );
close(fd1);
getchar();
rc = write( fd2, "0", 1 );
printf( "write result: [%d]\n", rc );
close( fd2 );
return 0;
}
While the program is waiting for a keystroke, mount the block device.
Enter a keystroke. The result without the patch is 1, which is a
security violation. This occurs because the bd_release function will
bd_release(bdev) and set inode->i_security to NULL on the close(fd1).
Hence, we want to place the control at the level of the file struct,
not the inode.
Mike
.___________________________________________________________________.
Michael A. Halcrow
Security Software Engineer, IBM Linux Technology Center
GnuPG Fingerprint: 05B5 08A8 713A 64C1 D35D 2371 2D3C FDDA 3EB6 601D
The hokey pokey... What if that's really what it's all about?
On Tue, 08 Feb 2005 11:24:50 CST, Michael Halcrow said:
> While the program is waiting for a keystroke, mount the block device.
> Enter a keystroke. The result without the patch is 1, which is a
> security violation. This occurs because the bd_release function will
> bd_release(bdev) and set inode->i_security to NULL on the close(fd1).
Sounds like a bug, not a feature. Should it be zeroing out inode->i_security
for an inode with a non-zero reference count?
Quoting [email protected] ([email protected]):
> On Tue, 08 Feb 2005 11:24:50 CST, Michael Halcrow said:
>
> > While the program is waiting for a keystroke, mount the block device.
> > Enter a keystroke. The result without the patch is 1, which is a
> > security violation. This occurs because the bd_release function will
> > bd_release(bdev) and set inode->i_security to NULL on the close(fd1).
>
> Sounds like a bug, not a feature. Should it be zeroing out inode->i_security
> for an inode with a non-zero reference count?
Valdis,
inode->i_security is no longer used after the patch. Does your question
still apply with the proposed patch, %s/inode->i_security/file->f_security/?
Nevertheless, note that the thing being enforced is "no simultaneous
write access to a block device and mount of that block device." The
file->f_security is just used as a flag to seclvl that when this file
is closed, we can bd_release the device to allow a mount or another
open(O_RDWR) of the file. So references to the inode don't matter,
provided the other references are read accesses. Which they have to
be, since otherwise the seclvl_bd_claim() would have failed on the
second open(O_RDWR) call.
I hope I'm at least remotely answering your question :)
-serge
* Michael Halcrow ([email protected]) wrote:
> [...]. This occurs because the bd_release function will
> bd_release(bdev) and set inode->i_security to NULL on the close(fd1).
> Hence, we want to place the control at the level of the file struct,
> not the inode.
This is basically what I was referring to pre-merge. And it is still
not fully sufficient. Multiple processes can share an fd. So the test
against current is broken. Also well-behaved apps that are already
using O_EXCL will break. Using filp as the holder is sufficient to fix
both of these issues. Here's a 3.5/8 that will fix this. 6/8 no longer
applies cleanly with this change.
Signed-off-by: Chris Wright <[email protected]>
--- a/security/seclvl.c~bd_claim 2005-02-08 15:05:09.000000000 -0800
+++ b/security/seclvl.c 2005-02-08 15:05:17.000000000 -0800
@@ -492,17 +492,16 @@
*/
static int seclvl_bd_claim(struct file * filp)
{
- int holder;
struct block_device *bdev = NULL;
dev_t dev = filp->f_dentry->d_inode->i_rdev;
bdev = open_by_devnum(dev, FMODE_WRITE);
if (bdev) {
- if (bd_claim(bdev, &holder)) {
+ if (bd_claim(bdev, filp)) {
blkdev_put(bdev);
return -EPERM;
}
/* Claimed; mark it to release on close */
- filp->f_security = current;
+ filp->f_security = filp;
}
return 0;
}
@@ -597,7 +596,7 @@
if (dentry && (filp->f_mode & FMODE_WRITE)) {
struct inode * inode = dentry->d_inode;
if (inode && S_ISBLK(inode->i_mode)
- && filp->f_security == current) {
+ && filp->f_security == filp) {
struct block_device *bdev = inode->i_bdev;
if (bdev) {
bd_release(bdev);
* Michael Halcrow ([email protected]) wrote:
> This is the sixth in a series of eight patches to the BSD Secure
> Levels LSM. It makes several trivial changes to make the code
> consistent.
These are inconsistent with CodingStyle. I'd drop this, and go the
other way (patch is smaller) ala Lindent.
> struct seclvl_obj {
> - char *name;
> + char * name;
This is opposite of typical style.
> -seclvl_attr_store(struct kobject *kobj,
> - struct attribute *attr, const char *buf, size_t len)
> +seclvl_attr_store(struct kobject * kobj,
> + struct attribute * attr, const char * buf, size_t len)
same here...etc.
Lindent nearly undoes all these changes. If we're going to reformat
code, I'd prefer to see it done via Lindent.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
On Mon, Feb 07, 2005 at 01:21:08PM -0600, Michael Halcrow wrote:
> This is the first in a series of eight patches to the BSD Secure
> Levels LSM. It overhauls the printk mechanism in order to reduce the
> unnecessary usage of the .text area. Thanks to Brad Spengler for the
> suggestion.
>
> Signed off by: Michael Halcrow <[email protected]>
> Index: linux-2.6.11-rc2-mm1-modules/security/seclvl.c
> ===================================================================
> --- linux-2.6.11-rc2-mm1-modules.orig/security/seclvl.c 2005-02-03 14:55:44.799527472 -0600
> +++ linux-2.6.11-rc2-mm1-modules/security/seclvl.c 2005-02-03 14:56:18.527400056 -0600
> @@ -101,22 +101,20 @@
>
> #define MY_NAME "seclvl"
>
> -/**
> - * This time-limits log writes to one per second.
> - */
> -#define seclvl_printk(verb, type, fmt, arg...) \
> - do { \
> - if (verbosity >= verb) { \
> - static unsigned long _prior; \
> - unsigned long _now = jiffies; \
> - if ((_now - _prior) > HZ) { \
> - printk(type "%s: %s: " fmt, \
> - MY_NAME, __FUNCTION__ , \
> - ## arg); \
> - _prior = _now; \
> - } \
> - } \
> - } while (0)
> +static void seclvl_printk( int verb, const char * fmt, ... )
> +{
> + va_list args;
> + va_start( args, fmt );
> + if (verbosity >= verb) {
> + static unsigned long _prior;
> + unsigned long _now = jiffies;
> + if ((_now - _prior) > HZ) {
> + vprintk( fmt, args );
> + }
> + _prior = _now;
> + }
> + va_end( args );
> +}
This could be done with a seclvl_printk macro wrapping a
__seclvl_printk function that provides __FUNCTION__, leaving the
callers the same.
--
Mathematics is the supreme nostalgia of our time.