linux-kernel - Re: [PATCH] Version 7 (2.6.23) Smack: Simplified Mandatory Access Control Kernel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 14 Oct 2007 10:56:39 -0700 (PDT)
From:	Casey Schaufler <casey@...aufler-ca.com>
To:	Al Viro <viro@....linux.org.uk>,
	Casey Schaufler <casey@...aufler-ca.com>
Cc:	torvalds@...l.org, akpm@...l.org,
	linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Version 7 (2.6.23) Smack: Simplified Mandatory Access Control Kernel


--- Al Viro <viro@....linux.org.uk> wrote:

> On Sun, Oct 14, 2007 at 10:15:42AM -0700, Casey Schaufler wrote:
> > This version fixes a major blunder in label handling. The system
> > works, but has a serious memory leak that also induces a gradual
> > performance degradation. Al Viro gets the credit for pointing out
> > that one. Al suggested several other improvements that are not
> > included. They should come soon, but I wanted to get this flaw
> > out of the code before too many people hit it.
> 
> Ahem... This
> 
> > +static ssize_t smk_write_doi(struct file *file, const char __user *buf,
> > +			     size_t count, loff_t *ppos)
> > +{
> > +	char temp[80];
> > +	int i;
> > +
> > +	if (!capable(CAP_MAC_OVERRIDE))
> > +		return -EPERM;
> > +
> > +	if (count > sizeof(temp))
> > +		return -EINVAL;
> > +
> > +	if (copy_from_user(temp, buf, count) != 0)
> > +		return -EFAULT;
> > +
> > +	if (sscanf(temp, "%d", &i) != 1)
> > +		return -EINVAL;
> 
> is not really a missing improvement; it's a geniune undefined behaviour.
> temp[] is uninitialized, then you copy there some data that doesn't have
> to contain NUL, then you call sscanf().  Boom.  The same goes for the rest
> of similar places.
> 
> And this
> 
> > +static ssize_t smk_read_ambient(struct file *filp, char __user *buf,
> > +				size_t cn, loff_t *ppos)
> > +{
> > +	ssize_t rc;
> > +	int asize = strlen(smack_net_ambient) + 1;
> > +
> > +	if (cn < asize)
> > +		return -EINVAL;
> > +
> > +	if (*ppos != 0)
> > +		return 0;
> > +
> > +	rc = simple_read_from_buffer(buf, cn, ppos, smack_net_ambient, asize);
> 
> is honest-to-$DEITY security hole - that file is world-readable and there's
> nothing to prevent simple_read_from_buffer() blocking on page-in of buf,
> then root writing to that file changing smack_net_ambient and doing kfree()
> on the old value - one we'd already passed to simple_read_from_buffer().
> At which point reader is about to get whatever data that might land in
> whatever that memory gets reused for.
> 
> Besides, as I said the last time, smack_net_ambient has every right to
> get changed between strlen() and passing argument to
> simple_read_from_buffer(),
> in which case you'll be copying the amount of data that used to be in the
> old one, taking it from the new one.  New one might very well be shorter.

Yep. I got work to do. I know it ain't getting done today, and I know
that the change I put out is affecting people, so I decided not to
wait until I'd done the whole lot.

Sorry if it sounded as if I wasn't taking the comments seriously.
I am. Thank you again.


Casey Schaufler
casey@...aufler-ca.com
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/