Re: [Drbd-dev] drbd_nl.c:drbd_adm_prepare() indexes drbd_genl_ops[] by cmd number

[Lars Ellenberg <lars.ellenberg@linbit.com>]

On Tue, Jun 04, 2019 at 02:18:02AM -0600, David Butterfield wrote:
> On 6/3/19 11:43 PM, Lars Ellenberg wrote:
> > Think again: how is family->ops inexed?
> 
> If you mean the genl_family, its ops are kept on a list, which is searched using genl_get_cmd().
> Constructed as a list, it doesn't even (necessarily) have an an underlying array one might be tempted to index.
> 
> > How is drbd_genl_ops indexed?
> 
> It is an array, but it isn't indexed by command number,

Why, yes it it.
Because it is constructed that way.
Uhm. Wait. It used to at some point.
But ... not so anymore.
I can swear it used to be
[op_name] = {
}

in that "magic" header...

> except in this one place in drbd_adm_prepare().

Okay.
So we check against flags we don't meant to check,
even check with out-of-bounds array access (undefined content).
And potentially someone that already has CAP_NET_ADMIN
could do stuff we meant to require CAP_SYS_ADMIN for.

That's bad :-(

-_- :: accidentally correct
!!! :: allows NET_ADMIN to do stuff we meant to require SYS_ADMIN for
OOB :: access beyond end of array, unknown, possibly bad.

leaving off the -_- entries, I find:

OOB  [5]   =  {  .cmd  =  44,  .flags  =  0x1,  .doit  =  drbd_adm_new_peer          [drbd],  .dumpit  =                           (null)  }
OOB  [6]   =  {  .cmd  =  45,  .flags  =  0x1,  .doit  =  drbd_adm_new_path          [drbd],  .dumpit  =                           (null)  }
OOB  [7]   =  {  .cmd  =  46,  .flags  =  0x1,  .doit  =  drbd_adm_del_peer          [drbd],  .dumpit  =                           (null)  }
OOB  [8]   =  {  .cmd  =  47,  .flags  =  0x1,  .doit  =  drbd_adm_del_path          [drbd],  .dumpit  =                           (null)  }
!!!  [10]  =  {  .cmd  =  29,  .flags  =  0x1,  .doit  =  drbd_adm_net_opts          [drbd],  .dumpit  =                           (null)  }
OOB  [33]  =  {  .cmd  =  38,  .flags  =  0x0,  .doit  =  (null),                    .dumpit  =        drbd_adm_get_initial_state  [drbd]  }
OOB  [34]  =  {  .cmd  =  42,  .flags  =  0x1,  .doit  =  drbd_adm_forget_peer       [drbd],  .dumpit  =                           (null)  }
OOB  [35]  =  {  .cmd  =  43,  .flags  =  0x1,  .doit  =  drbd_adm_peer_device_opts  [drbd],  .dumpit  =                           (null)  }

Someone with NET_ADMIN (but without SYS_ADMIN) could
 - change net options
potentially (OOB access, may or may not have the bit set)
 - create and delete peers and paths
   (but not connect them, or disconnect them,
   which would be required to do anything useful with a new path, or to delete an active path)
 - forget a peer (but not disconnect it, which would be required to have success)
 - be disallowed from getting the initial state, even though that should be an unpriviledged operation

Luckily, "mostly harmless".

Okay.

Either we fix it in the magic header to construct an array
that has holes in it, but can then be indexed by [cmd],
as I think it was meant to be, and used to be
(though I may be misremembering).

Or we add an additional iteration to find the correct flags.

Thank you for point this out.

-- 
: Lars Ellenberg
: LINBIT | Keeping the Digital World Running
: DRBD -- Heartbeat -- Corosync -- Pacemaker
: R&D, Integration, Ops, Consulting, Support

DRBD® and LINBIT® are registered trademarks of LINBIT