[libvirt] [PATCH 7/8] Add access control filtering of interface objects

[Eric Blake]

On 06/27/2013 10:57 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> Ensure that all APIs which list interface objects filter
> them against the access control system.
> 
> This makes the APIs for listing names and counting devices
> slightly less efficient, since we can't use the direct
> netcf APIs for these tasks. Instead we have to ask netcf
> for the full list of objects & iterate over the list
> filtering them out.

Such is life with security filtering.

> 
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>  src/conf/interface_conf.h               |   3 +
>  src/interface/interface_backend_netcf.c | 262 +++++++++++++++++++++++++++-----
>  src/interface/interface_backend_udev.c  |  56 +++++--
>  3 files changed, 270 insertions(+), 51 deletions(-)
> 

> +++ b/src/interface/interface_backend_netcf.c
> @@ -209,48 +209,233 @@ static int netcfInterfaceClose(virConnectPtr conn)
>      return 0;
>  }
>  
> -static int netcfConnectNumOfInterfaces(virConnectPtr conn)
> +static int netcfConnectNumOfInterfacesImpl(virConnectPtr conn,
> +                                           int status,
> +                                           virInterfaceObjListFilter filter)
>  {
> -    int count;
>      struct interface_driver *driver = conn->interfacePrivateData;
> +    int count;
> +    int want = 0;
> +    int ret = -1;
> +    int i;
> +    char **names = NULL;
>  
> -    if (virConnectNumOfInterfacesEnsureACL(conn) < 0)
> -        return -1;
> -
> -    interfaceDriverLock(driver);
> -    count = ncf_num_of_interfaces(driver->netcf, NETCF_IFACE_ACTIVE);
> +    /* List all interfaces, in case of we might support new filter flags
> +     * except active|inactive in future.

s/case of/case/
s/except/beyond/

> +
> +    if (VIR_ALLOC_N(names, count) < 0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }

[Uggh - we still need to revisit the goal to hoist OOM reporting into
VIR_ALLOC and friends - nothing magic happened during my 3-week vacation
on that front.  But that's a completely separate project.]

> +static int netcfConnectListInterfacesImpl(virConnectPtr conn,
> +                                          int status,
> +                                          char **const names, int nnames,
> +                                          virInterfaceObjListFilter filter)
>  {
>      struct interface_driver *driver = conn->interfacePrivateData;
> -    int count;
> +    int count = 0;
> +    int want = 0;
> +    int ret = -1;
> +    int i;
> +    char **allnames = NULL;
>  
> -    if (virConnectListInterfacesEnsureACL(conn) < 0)
> -        return -1;
> +    count = ncf_num_of_interfaces(driver->netcf, status);

Can any of this be shared?  You have:

long setup
foreach element
  if not filtered
    count++

and

long setup
foreach element
  if not filtered
    append object

where the long setup is the same.  I think some of the other filtering
code has used a single implementation, where passing in NULL for the
**names parameter implies just counting, and passing in non-NULL implies
object appending.  Then you wouldn't be duplicating as much code, making
it easier to add new filters in one place in the future.

> +static int netcfConnectListInterfaces(virConnectPtr conn, char **const names, int nnames)
> +{
> +    struct interface_driver *driver = conn->interfacePrivateData;
> +    int count;
> +
> +    if (virConnectListInterfacesEnsureACL(conn) < 0)
> +        return -1;
>  
> +    interfaceDriverLock(driver);
> +    count = netcfConnectListInterfacesImpl(conn,
> +                                            NETCF_IFACE_ACTIVE,
> +                                            names, nnames,
> +                                            virConnectListInterfacesCheckACL);

Indentation is off.

> @@ -403,6 +575,17 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
>              goto cleanup;
>          }
>  
> +        if (!(def = netcfGetMinimalDefForDevice(iface)))
> +            goto cleanup;
> +
> +        if (!virConnectListAllInterfacesCheckACL(conn, def)) {
> +            ncf_if_free(iface);
> +            iface = NULL;
> +            virInterfaceDefFree(def);
> +            continue;
> +        }
> +        virInterfaceDefFree(def);
> +
>          /* XXX: Filter the result, need to be splitted once new filter flags
>           * except active|inactive are supported.

Pre-existing, but as long as you are here, s/splitted/split/.  Or maybe
even nuke the comment, now that you have set up the framework for
additional filtering so it no longer applies.

> @@ -412,6 +595,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
>                (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
>                 (status & NETCF_IFACE_INACTIVE)))) {
>              ncf_if_free(iface);
> +            iface = NULL;
>              continue;

Oooh tricky - you posted the CVE-2013-2218 patch embedded inside this
patch, prior to the embargo date, but no one picked up on it until now :)

At any rate, a consolidation patch between the count vs. list function
is probably worth a separate patch, at which point your code as-is looks
mostly okay.  ACK with grammar/spacing nits fixed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130702/00da2e78/attachment-0001.sig>