Re: [libvirt] [PATCH 01/12] Add bounds checking on virDomainMigrate*Params RPC calls (CVE-2013-4292)

On Thu, Aug 29, 2013 at 02:34:15PM +0200, Michal Privoznik wrote: > On 29.08.2013 12:49, Daniel P. Berrange wrote: >> From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; >> >> The parameters for the virDomainMigrate*Params RPC calls were >> not bounds checks, meaning a malicious client can cause libvirtd >> to consume arbitrary memory >> >> This issue was introduced in the 1.1.0 release of libvirt >> >> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; >> --- >> daemon/remote.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> src/remote/remote_driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++ >> src/remote/remote_protocol.x | 15 +++++++++------ >> 3 files changed, 93 insertions(+), 6 deletions(-) >> >> diff --git a/daemon/remote.c b/daemon/remote.c >> index 03d5557..a11ba94 100644 >> --- a/daemon/remote.c >> +++ b/daemon/remote.c >> @@ -4620,6 +4620,13 @@ remoteDispatchDomainMigrateBegin3Params( >> goto cleanup; >> } >> >> + if (args->params.params_len > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { >> + virReportError(VIR_ERR_RPC, >> + _("Too many migration parameters '%d' for limit '%d'"), >> + args->params.params_len, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); >> + goto cleanup; >> + } >> + >> if (!(dom = get_nonnull_domain(priv->conn, args->dom))) >> goto cleanup; >> >> @@ -4671,6 +4678,13 @@ remoteDispatchDomainMigratePrepare3Params( >> goto cleanup; >> } >> >> + if (args->params.params_len > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { >> + virReportError(VIR_ERR_RPC, >> + _("Too many migration parameters '%d' for limit '%d'"), >> + args->params.params_len, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); >> + goto cleanup; >> + } >> + >> if (!(params = remoteDeserializeTypedParameters(args->params.params_val, >> args->params.params_len, >> 0, &nparams))) >> @@ -4726,6 +4740,13 @@ remoteDispatchDomainMigratePrepareTunnel3Params( >> goto cleanup; >> } >> >> + if (args->params.params_len > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { >> + virReportError(VIR_ERR_RPC, >> + _("Too many migration parameters '%d' for limit '%d'"), >> + args->params.params_len, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); >> + goto cleanup; >> + } >> + >> if (!(params = remoteDeserializeTypedParameters(args->params.params_val, >> args->params.params_len, >> 0, &nparams))) >> @@ -4790,6 +4811,13 @@ remoteDispatchDomainMigratePerform3Params( >> goto cleanup; >> } >> >> + if (args->params.params_len > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { >> + virReportError(VIR_ERR_RPC, >> + _("Too many migration parameters '%d' for limit '%d'"), >> + args->params.params_len, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); >> + goto cleanup; >> + } >> + >> if (!(dom = get_nonnull_domain(priv->conn, args->dom))) >> goto cleanup; >> >> @@ -4845,6 +4873,13 @@ remoteDispatchDomainMigrateFinish3Params( >> goto cleanup; >> } >> >> + if (args->params.params_len > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { >> + virReportError(VIR_ERR_RPC, >> + _("Too many migration parameters '%d' for limit '%d'"), >> + args->params.params_len, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); >> + goto cleanup; >> + } >> + >> if (!(params = remoteDeserializeTypedParameters(args->params.params_val, >> args->params.params_len, >> 0, &nparams))) >> @@ -4897,6 +4932,13 @@ remoteDispatchDomainMigrateConfirm3Params( >> goto cleanup; >> } >> >> + if (args->params.params_len > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { >> + virReportError(VIR_ERR_RPC, >> + _("Too many migration parameters '%d' for limit '%d'"), >> + args->params.params_len, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); >> + goto cleanup; >> + } >> + >> if (!(dom = get_nonnull_domain(priv->conn, args->dom))) >> goto cleanup; > > While the above is correct as it fixes the libvirtd (sanitizes input). > However, I don't think we need the following, esp. if we ever change the > limit. The older client won't be able to pass more parameters whereas it > currently can. As mentioned in the cover letter, we explicitly want to check data received from the server, because this is a robustness issue. If the data stream gets corrupted for some reason, it can cause the client to allocate unbounded amounts of memory. We want to prevent that. Compatibility when raising limits is already going to be a problem, because the generated xdr methods for decoding the return values will be enforcing this limit. They have really crappy (ie non-existant) error reporting, so the user wil just get a "Unable to decode message payload" error message. These explicit checks in libvirt code ensure we get a useful error message instead. >> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x >> index 7cfebdf..4262c34 100644 >> --- a/src/remote/remote_protocol.x >> +++ b/src/remote/remote_protocol.x >> @@ -234,6 +234,9 @@ const REMOTE_DOMAIN_DISK_ERRORS_MAX = 256; >> */ >> const REMOTE_NODE_MEMORY_PARAMETERS_MAX = 64; >> >> +/* Upper limit on migrate parameters */ >> +const REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX = 64; >> + >> /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ >> typedef opaque remote_uuid[VIR_UUID_BUFLEN]; >> >> @@ -2770,7 +2773,7 @@ struct remote_domain_fstrim_args { >> >> struct remote_domain_migrate_begin3_params_args { >> remote_nonnull_domain dom; >> - remote_typed_param params<>; >> + remote_typed_param params<REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX>; >> unsigned int flags; >> }; > > Moreover, after you introduce this - won't XDR complain if we try to > encode more than _MAX items? Yes, it will return an error both at decode or encode time if the length exceeds the limit. We want to get better error reporting though, hence duplicating the checks ourselves. One day we really ought to just throw away the XDR library code and write our own, can we can dynamic buffer allocation and sensible error reporting. Daniel