[libvirt] [PATCH] Crash of libvirtd by unprivileged user in virConnectListAllInterfaces

[Guannan Ren]

On 07/02/2013 10:14 PM, Eric Blake wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
>
> On Thu, Jun 27, 2013 at 03:56:42PM +0100, Daniel P. Berrange wrote:
>> Hi Security Team,
>>
>> I've discovered a way for an unprivileged user with a readonly connection
>> to libvirtd, to crash the daemon.
> Ok, the final patch for this is issue will be the simpler variant that
> Eric suggested
>
> The embargo can be considered to be lifted on Monday July 1st, at
> 0900 UTC
>
> The following is the GIT change that DV or myself will apply to libvirt
> GIT master immediately before the 1.1.0 release:
>
> >From 177b4165c531a4b3ba7f6ab6aa41dca9ceb0b8cf Mon Sep 17 00:00:00 2001
> From: "Daniel P. Berrange" <berrange at redhat.com>
> Date: Fri, 28 Jun 2013 10:48:37 +0100
> Subject: [PATCH] CVE-2013-2218: Fix crash listing network interfaces with
>   filters
>
> The virConnectListAllInterfaces method has a double-free of the
> 'struct netcf_if' object when any of the filtering flags cause
> an interface to be skipped over. For example when running the
> command 'virsh iface-list --inactive'
>
> This is a regression introduced in release 1.0.6 by
>
>    commit 7ac2c4fe624f30f2c8270116513fa2ddab07631f
>    Author: Guannan Ren <gren at redhat.com>
>    Date:   Tue May 21 21:29:38 2013 +0800
>
>      interface: list all interfaces with flags == 0
>
> Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> ---
>
> Posting as a courtesy FYI for anyone reading this list but who does
> not have access to the security list and doesn't want to crawl
> through git.  This commit has been included in 1.1.0 and has been
> applied to all affected stable branches (just v1.0.6-maint).
>
> The rule in determining that a CVE was necessary is the
> "escalation of privilege" test - any time a read-only client can
> cause a denial-of-service against a more-privileged read-write
> client (by crashing libvirtd), there is an escalation.
>
>   src/interface/interface_backend_netcf.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/src/interface/interface_backend_netcf.c b/src/interface/interface_backend_netcf.c
> index a995816..9aa673d 100644
> --- a/src/interface/interface_backend_netcf.c
> +++ b/src/interface/interface_backend_netcf.c
> @@ -412,6 +412,7 @@ netcfConnectListAllInterfaces(virConnectPtr conn,
>                 (MATCH(VIR_CONNECT_LIST_INTERFACES_INACTIVE) &&
>                  (status & NETCF_IFACE_INACTIVE)))) {
>               ncf_if_free(iface);
> +            iface = NULL;
>               continue;
>           }
>

Thanks for posting here. I have to say sorry about this bug I caused.
This problem is caused by double-free 'iface' variable as Daniel pointed out
when code path goes into cleanup label, there is a second ncf_if_free()
for the variable if we didn't initialize it to NULL after the first 
ncf_if_free().

Guannan