2005-04-15 06:27:14

by Ted Kremenek

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

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


2005-04-15 07:11:15

by Mitchell Blank Jr

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

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

2005-04-15 16:01:11

by Chris Wright

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

* Ted Kremenek ([email protected]) 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