[CHECKER] possible missing capability check in ioctl function, drivers/net/cris/eth_v10.c, kernel 2.6.11

[CHECKER] possible missing capability check in ioctl function, drivers/net/cris/eth_v10.c, kernel 2.6.11

[Ted Kremenek]

Hello,

I'm a researcher in the Stanford Metacompilation group.  I am 
collaborating with Bryan Fulton at Coverity on using static analysis to 
find capability related security errors.  We're currently looking into 
creating a checker using statistical analysis to detect improper or 
missing capability checks in the Linux kernel.

Here's an example of what we think might be a bug (kernel version 
2.6.11):

In several network drivers that handle the ioctl command SIOCSMIIREG 
(writes a register on the network card) most implementations check for 
the CAP_NET_ADMIN capability.  Several drivers use the function 
"generic_mii_ioctl" to process this command (defined in 
drivers/net/mii.c).  In mii.c, we see:

line 291
	case SIOCSMIIREG: {
		u16 val = mii_data->val_in;

		if (!capable(CAP_NET_ADMIN))
			return -EPERM;

		if (mii_data->phy_id == mii_if->phy_id) {
			switch(mii_data->reg_num) {
			case MII_BMCR: {
...

Here the capability check is clearly executed before any state is 
modified.

In drivers/net/cris/eth_v10.c, the capability check is elided in 
e100_ioctl:

static int
e100_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
	struct mii_ioctl_data *data = if_mii(ifr);
	struct net_local *np = netdev_priv(dev);

	spin_lock(&np->lock); /* Preempt protection */
	switch (cmd) {
		case SIOCETHTOOL:
			return e100_ethtool_ioctl(dev,ifr);
		case SIOCGMIIPHY: /* Get PHY address */
			data->phy_id = mdio_phy_addr;
			break;
		case SIOCGMIIREG: /* Read MII register */
			data->val_out = e100_get_mdio_reg(dev, mdio_phy_addr, data->reg_num);
			break;
		case SIOCSMIIREG: /* Write MII register */    <===== MISSING 
CAPABILITY CHECK
			e100_set_mdio_reg(dev, mdio_phy_addr, data->reg_num, data->val_in);
			break;
...

Does this seem valid? Currently we are looking primarily into the 
ioctls in drivers/net, but we would like to extend this to other parts 
of the kernel.  Our understanding of what code should be protected by 
what capabilities is limited, so any feedback on this would be 
wonderful.

Thanks,
Ted Kremenek and Bryan Fulton

Re: [CHECKER] possible missing capability check in ioctl function, drivers/net/cris/eth_v10.c, kernel 2.6.11

[Mitchell Blank Jr]

Ted Kremenek wrote:
> Currently we are looking primarily into the 
> ioctls in drivers/net,

Just as a small aside, a little over five years ago (wow does time fly!) I
did a manual audit for mistakes like this:
  http://lkml.org/lkml/2000/3/7/156
Not sure if that's relevant to your work your not...I'm not sure how you
would can catch most of these errors statistically since a lot of the
dangerous ioctl's were only for a single device.

-Mitch

Re: [CHECKER] possible missing capability check in ioctl function, drivers/net/cris/eth_v10.c, kernel 2.6.11

[Chris Wright]

* Ted Kremenek (kremenek@cs.stanford.edu) wrote:
> In several network drivers that handle the ioctl command SIOCSMIIREG 
> (writes a register on the network card) most implementations check for 
> the CAP_NET_ADMIN capability.  Several drivers use the function 
> "generic_mii_ioctl" to process this command (defined in 
> drivers/net/mii.c).  In mii.c, we see:

This, to me, looks like the device specific (or generic_mii_ioctl)
capability test winds up being redundant.  The top-level checks
capabilities already in net/dev/core.c::dev_ioctl().  So, while there's
some room for cleanup, I don't think this is an acutal bug.

thanks,
-chris
-- 
Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net