[libvirt] [PATCH] Fix libvirtd crash possibility
When generating RPC protocol messages, it's strictly needed to have continuousline of numbers or RPC messages. However in case anyone tries backporting some functionality and will skip a number, there is a possibility to make the daemon segfault with newer virsh (version of the library, rpc call, etc.) even unintentionally. The problem is that the skipped numbers will get func filled with NULLs, but there is no check whether these are set before the daemon tries to run them. This patch very simply enhances one check and fixes that. I haven't investigated into such a deepness to say if the possibility comes as well with hacked RPC call (calling event instead of function), but this patch gets rid of both these problems. --- src/rpc/virnetserverprogram.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index d13b621..bc85df0 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -1,7 +1,7 @@ /* * virnetserverprogram.c: generic network RPC server program * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -379,7 +379,7 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog, dispatcher = virNetServerProgramGetProc(prog, msg->header.proc); - if (!dispatcher) { + if (!dispatcher || !dispatcher->func) { virReportError(VIR_ERR_RPC, _("unknown procedure: %d"), msg->header.proc); -- 1.7.12
On 09/12/2012 03:54 PM, Martin Kletzander wrote:
When generating RPC protocol messages, it's strictly needed to have continuousline of numbers or RPC messages. However in case anyone tries backporting some functionality and will skip a number, there is a possibility to make the daemon segfault with newer virsh (version of the library, rpc call, etc.) even unintentionally.
Indeed, upstream is contiguous, but downstream, we _can't_ backport new calls (because that would break .so versioning), but _must_ backport new events using the same RPC number as upstream (since events don't break .so), so downstream can definitely get into a state of gaps in the numbering. Although upstream will never have the gap, I think a patch along these lines is appropriate for upstream, rather than forcing each downstream to deal with the gap they create by selective backports.
The problem is that the skipped numbers will get func filled with NULLs, but there is no check whether these are set before the daemon tries to run them. This patch very simply enhances one check and fixes that.
I haven't investigated into such a deepness to say if the possibility comes as well with hacked RPC call (calling event instead of function), but this patch gets rid of both these problems.
You may want to modify this paragraph before pushing, perhaps based on my details above (and feel free to ask me on IRC if you need a better understanding of the rules of what RPC numbers can and can't be backported).
--- src/rpc/virnetserverprogram.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetserverprogram.c b/src/rpc/virnetserverprogram.c index d13b621..bc85df0 100644 --- a/src/rpc/virnetserverprogram.c +++ b/src/rpc/virnetserverprogram.c @@ -1,7 +1,7 @@ /* * virnetserverprogram.c: generic network RPC server program * - * Copyright (C) 2006-2011 Red Hat, Inc. + * Copyright (C) 2006-2012 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -379,7 +379,7 @@ virNetServerProgramDispatchCall(virNetServerProgramPtr prog,
dispatcher = virNetServerProgramGetProc(prog, msg->header.proc);
- if (!dispatcher) { + if (!dispatcher || !dispatcher->func) { virReportError(VIR_ERR_RPC, _("unknown procedure: %d"), msg->header.proc);
Not quite right. I think the correct fix is to instead make virNetServerProgramGetProc() return NULL instead of an uninitialized entry, if that entry has a NULL func pointer. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 09/12/2012 04:07 PM, Eric Blake wrote:
On 09/12/2012 03:54 PM, Martin Kletzander wrote:
When generating RPC protocol messages, it's strictly needed to have continuousline of numbers or RPC messages. However in case anyone tries backporting some functionality and will skip a number, there is a possibility to make the daemon segfault with newer virsh (version of the library, rpc call, etc.) even unintentionally.
Indeed, upstream is contiguous, but downstream, we _can't_ backport new calls (because that would break .so versioning), but _must_ backport new events using the same RPC number as upstream (since events don't break .so), so downstream can definitely get into a state of gaps in the numbering. Although upstream will never have the gap, I think a patch along these lines is appropriate for upstream, rather than forcing each downstream to deal with the gap they create by selective backports.
Correcting myself, there are gaps even in upstream - we MUST assume malicious clients, that try to invoke an event, and therefore this can crash upstream libvirtd without any backporting involved if it is not patched. Looking at the generated daemon/remote_dispatch.h, we can see things like: { /* Async event DomainEventBalloonChange => 276 */ NULL, 0, (xdrproc_t)xdr_void, 0, (xdrproc_t)xdr_void, true, 0 }, { /* Method DomainGetHostname => 277 */ remoteDispatchDomainGetHostnameHelper, sizeof(remote_domain_get_hostname_args), (xdrproc_t)xdr_remote_domain_get_hostname_args, sizeof(remote_domain_get_hostname_ret), (xdrproc_t)xdr_remote_domain_get_hostname_ret, true, 0 }, where a malicious client trying to call RPC 276 will trigger the failure if we don't patch things.
Not quite right. I think the correct fix is to instead make virNetServerProgramGetProc() return NULL instead of an uninitialized entry, if that entry has a NULL func pointer.
I still think my suggestion is correct, though - fix it at the source, not at each caller. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Martin Kletzander