[libvirt] [PATCH 00/12] Ensure array bounds checking is present on all RPC calls

[libvirt] FreeBSD, no gcc present...

[libvirt] [PATCH] security:...

Daniel P. Berrange

Thursday, 29 August 2013 Thu, 29 Aug '13

5:49 a.m.

From: "Daniel P. Berrange" <berrange(a)redhat.com> Missing bounds checking on array parameters is a security issue for libvirtd, since it allows a client to make libvirtd allocate unbounded memory. Missing bounds checking on array return values is not a security issue, but it is a robustness issue. If the RPC stream somehow got corrupted, the client could end up allocating unbounded memory. The first patch in this series fixes the security flaw introduced in version 1.1.0, and indentified during discussion of this patch: https://www.redhat.com/archives/libvir-list/2013-August/msg00787.html The remaining patches address the robustness issues, and add a test suite to prevent this flaw recurring. Daniel P. Berrange (12): Add bounds checking on virDomainMigrate*Params RPC calls (CVE-2013-4292) Add bounds checking on virDomainGetJobStats RPC call Add bounds checking on virDomain{SnapshotListAllChildren,ListAllSnapshots} RPC calls Add bounds checking on virConnectListAllDomains RPC call Add bounds checking on virConnectListAllStoragePools RPC call Add bounds checking on virStoragePoolListAllVolumes RPC call Add bounds checking on virConnectListAllNetworks RPC call Add bounds checking on virConnectListAllInterfaces RPC call Add bounds checking on virConnectListAllNodeDevices RPC call Add bounds checking on virConnectListAllNWFilters RPC call Add bounds checking on virConnectListAllSecrets RPC call Prohibit unbounded arrays in XDR protocols cfg.mk | 6 ++ daemon/remote.c | 119 +++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 130 +++++++++++++++++++++++++++++++++++++++++-- src/remote/remote_protocol.x | 108 ++++++++++++++++++----------------- 4 files changed, 304 insertions(+), 59 deletions(-) -- 1.8.3.1

Show replies by date

Daniel P. Berrange

Thursday, 29 August Thu, 29 Aug

5:49 a.m.

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

From: "Daniel P. Berrange" <berrange(a)redhat.com> 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> --- 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; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 71d0034..30f8f90 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6037,6 +6037,13 @@ remoteDomainMigrateBegin3Params(virDomainPtr domain, make_nonnull_domain(&args.dom, domain); args.flags = flags; + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + if (remoteSerializeTypedParameters(params, nparams, &args.params.params_val, &args.params.params_len) < 0) { @@ -6096,6 +6103,13 @@ remoteDomainMigratePrepare3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + if (remoteSerializeTypedParameters(params, nparams, &args.params.params_val, &args.params.params_len) < 0) { @@ -6171,6 +6185,13 @@ remoteDomainMigratePrepareTunnel3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags; @@ -6250,6 +6271,13 @@ remoteDomainMigratePerform3Params(virDomainPtr dom, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + make_nonnull_domain(&args.dom, dom); args.dconnuri = dconnuri == NULL ? NULL : (char **) &dconnuri; args.cookie_in.cookie_in_val = (char *)cookiein; @@ -6315,6 +6343,13 @@ remoteDomainMigrateFinish3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags; @@ -6380,6 +6415,13 @@ remoteDomainMigrateConfirm3Params(virDomainPtr domain, memset(&args, 0, sizeof(args)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + make_nonnull_domain(&args.dom, domain); args.cookie_in.cookie_in_len = cookieinlen; args.cookie_in.cookie_in_val = (char *) cookiein; 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; }; @@ -2780,7 +2783,7 @@ struct remote_domain_migrate_begin3_params_ret { }; struct remote_domain_migrate_prepare3_params_args { - remote_typed_param params<>; + remote_typed_param params<REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX>; opaque cookie_in<REMOTE_MIGRATE_COOKIE_MAX>; unsigned int flags; }; @@ -2791,7 +2794,7 @@ struct remote_domain_migrate_prepare3_params_ret { }; struct remote_domain_migrate_prepare_tunnel3_params_args { - remote_typed_param params<>; + remote_typed_param params<REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX>; opaque cookie_in<REMOTE_MIGRATE_COOKIE_MAX>; unsigned int flags; }; @@ -2803,7 +2806,7 @@ struct remote_domain_migrate_prepare_tunnel3_params_ret { struct remote_domain_migrate_perform3_params_args { remote_nonnull_domain dom; remote_string dconnuri; - remote_typed_param params<>; + remote_typed_param params<REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX>; opaque cookie_in<REMOTE_MIGRATE_COOKIE_MAX>; unsigned int flags; }; @@ -2813,7 +2816,7 @@ struct remote_domain_migrate_perform3_params_ret { }; struct remote_domain_migrate_finish3_params_args { - remote_typed_param params<>; + remote_typed_param params<REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX>; opaque cookie_in<REMOTE_MIGRATE_COOKIE_MAX>; unsigned int flags; int cancelled; @@ -2826,7 +2829,7 @@ struct remote_domain_migrate_finish3_params_ret { struct remote_domain_migrate_confirm3_params_args { remote_nonnull_domain dom; - remote_typed_param params<>; + remote_typed_param params<REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX>; opaque cookie_in<REMOTE_MIGRATE_COOKIE_MAX>; unsigned int flags; int cancelled; -- 1.8.3.1

Michal Privoznik

7:34 a.m.

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

On 29.08.2013 12:49, Daniel P. Berrange wrote:

...

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.

...

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 71d0034..30f8f90 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -6037,6 +6037,13 @@ remoteDomainMigrateBegin3Params(virDomainPtr domain, make_nonnull_domain(&args.dom, domain); args.flags = flags; + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + if (remoteSerializeTypedParameters(params, nparams, &args.params.params_val, &args.params.params_len) < 0) { @@ -6096,6 +6103,13 @@ remoteDomainMigratePrepare3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + if (remoteSerializeTypedParameters(params, nparams, &args.params.params_val, &args.params.params_len) < 0) { @@ -6171,6 +6185,13 @@ remoteDomainMigratePrepareTunnel3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags; @@ -6250,6 +6271,13 @@ remoteDomainMigratePerform3Params(virDomainPtr dom, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + make_nonnull_domain(&args.dom, dom); args.dconnuri = dconnuri == NULL ? NULL : (char **) &dconnuri; args.cookie_in.cookie_in_val = (char *)cookiein; @@ -6315,6 +6343,13 @@ remoteDomainMigrateFinish3Params(virConnectPtr dconn, memset(&args, 0, sizeof(args)); memset(&ret, 0, sizeof(ret)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + args.cookie_in.cookie_in_val = (char *)cookiein; args.cookie_in.cookie_in_len = cookieinlen; args.flags = flags; @@ -6380,6 +6415,13 @@ remoteDomainMigrateConfirm3Params(virDomainPtr domain, memset(&args, 0, sizeof(args)); + if (nparams > REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many migration parameters '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX); + goto cleanup; + } + make_nonnull_domain(&args.dom, domain); args.cookie_in.cookie_in_len = cookieinlen; args.cookie_in.cookie_in_val = (char *) cookiein; 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? Michal

Daniel P. Berrange

7:55 a.m.

New subject: [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> > > 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> > --- > 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 -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Michal Privoznik

7:58 a.m.

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

On 29.08.2013 14:55, Daniel P. Berrange wrote:

...

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> >> >> 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> >> --- >> 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

Aah, okay. Thanks for your explanation. Makes sense now. In that case: ACK series and I vote for push now, prior the release. Michal

Daniel P. Berrange

9:53 a.m.

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

On Thu, Aug 29, 2013 at 02:58:44PM +0200, Michal Privoznik wrote:

...

On 29.08.2013 14:55, Daniel P. Berrange wrote: > 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> >>> >>> 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> >>> --- >>> 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 > Aah, okay. Thanks for your explanation. Makes sense now. In that case: ACK series and I vote for push now, prior the release.

Thanks, I pushed this series to master, and the CVE patch I have pushed to v1.1.0-maint and v1.1.1-maint too Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 02/12] Add bounds checking on virDomainGetJobStats RPC call

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virDomainGetJobStats call were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 7 +++++++ src/remote/remote_driver.c | 8 ++++++++ src/remote/remote_protocol.x | 5 ++++- 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/daemon/remote.c b/daemon/remote.c index a11ba94..ad78011 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4579,6 +4579,13 @@ remoteDispatchDomainGetJobStats(virNetServerPtr server ATTRIBUTE_UNUSED, &nparams, args->flags) < 0) goto cleanup; + if (nparams > REMOTE_DOMAIN_JOB_STATS_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many job stats '%d' for limit '%d'"), + nparams, REMOTE_DOMAIN_JOB_STATS_MAX); + goto cleanup; + } + if (remoteSerializeTypedParameters(params, nparams, &ret->params.params_val, &ret->params.params_len, diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 30f8f90..33b2b0f 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5998,6 +5998,14 @@ remoteDomainGetJobStats(virDomainPtr domain, (xdrproc_t) xdr_remote_domain_get_job_stats_ret, (char *) &ret) == -1) goto done; + if (ret.params.params_len > REMOTE_DOMAIN_JOB_STATS_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many job stats '%d' for limit '%d'"), + ret.params.params_len, + REMOTE_DOMAIN_JOB_STATS_MAX); + goto cleanup; + } + *type = ret.type; if (remoteDeserializeTypedParameters(ret.params.params_val, diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 4262c34..eff7e1c 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -237,6 +237,9 @@ const REMOTE_NODE_MEMORY_PARAMETERS_MAX = 64; /* Upper limit on migrate parameters */ const REMOTE_DOMAIN_MIGRATE_PARAM_LIST_MAX = 64; +/* Upper limit on number of job stats */ +const REMOTE_DOMAIN_JOB_STATS_MAX = 16; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -2196,7 +2199,7 @@ struct remote_domain_get_job_stats_args { struct remote_domain_get_job_stats_ret { int type; - remote_typed_param params<>; + remote_typed_param params<REMOTE_DOMAIN_JOB_STATS_MAX>; }; -- 1.8.3.1

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 03/12] Add bounds checking on virDomain{SnapshotListAllChildren, ListAllSnapshots} RPC calls

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virDomain{SnapshotListAllChildren,ListAllSnapshots} calls were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 14 ++++++++++++++ src/remote/remote_driver.c | 16 ++++++++++++++++ src/remote/remote_protocol.x | 10 +++++----- 3 files changed, 35 insertions(+), 5 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index ad78011..e228cba 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -3934,6 +3934,13 @@ remoteDispatchDomainListAllSnapshots(virNetServerPtr server ATTRIBUTE_UNUSED, args->flags)) < 0) goto cleanup; + if (nsnaps > REMOTE_DOMAIN_SNAPSHOT_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many domain snapshots '%d' for limit '%d'"), + nsnaps, REMOTE_DOMAIN_SNAPSHOT_LIST_MAX); + goto cleanup; + } + if (snaps && nsnaps) { if (VIR_ALLOC_N(ret->snapshots.snapshots_val, nsnaps) < 0) goto cleanup; @@ -3996,6 +4003,13 @@ remoteDispatchDomainSnapshotListAllChildren(virNetServerPtr server ATTRIBUTE_UNU args->flags)) < 0) goto cleanup; + if (nsnaps > REMOTE_DOMAIN_SNAPSHOT_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many domain snapshots '%d' for limit '%d'"), + nsnaps, REMOTE_DOMAIN_SNAPSHOT_LIST_MAX); + goto cleanup; + } + if (snaps && nsnaps) { if (VIR_ALLOC_N(ret->snapshots.snapshots_val, nsnaps) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 33b2b0f..14c16f6 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -5760,6 +5760,14 @@ remoteDomainListAllSnapshots(virDomainPtr dom, (char *) &ret) == -1) goto done; + if (ret.snapshots.snapshots_len > REMOTE_DOMAIN_SNAPSHOT_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many domain snapshots '%d' for limit '%d'"), + ret.snapshots.snapshots_len, + REMOTE_DOMAIN_SNAPSHOT_LIST_MAX); + goto cleanup; + } + if (snapshots) { if (VIR_ALLOC_N(snaps, ret.snapshots.snapshots_len + 1) < 0) goto cleanup; @@ -5819,6 +5827,14 @@ remoteDomainSnapshotListAllChildren(virDomainSnapshotPtr parent, (char *) &ret) == -1) goto done; + if (ret.snapshots.snapshots_len > REMOTE_DOMAIN_SNAPSHOT_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many domain snapshots '%d' for limit '%d'"), + ret.snapshots.snapshots_len, + REMOTE_DOMAIN_SNAPSHOT_LIST_MAX); + goto cleanup; + } + if (snapshots) { if (VIR_ALLOC_N(snaps, ret.snapshots.snapshots_len + 1) < 0) goto cleanup; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index eff7e1c..9e3918c 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -154,7 +154,7 @@ const REMOTE_AUTH_TYPE_LIST_MAX = 20; const REMOTE_DOMAIN_MEMORY_STATS_MAX = 1024; /* Upper limit on lists of domain snapshots. */ -const REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX = 1024; +const REMOTE_DOMAIN_SNAPSHOT_LIST_MAX = 1024; /* Maximum length of a block peek buffer message. * Note applications need to be aware of this limit and issue multiple @@ -2396,7 +2396,7 @@ struct remote_domain_snapshot_list_names_args { }; struct remote_domain_snapshot_list_names_ret { - remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_MAX>; /* insert@1 */ }; struct remote_domain_list_all_snapshots_args { @@ -2406,7 +2406,7 @@ struct remote_domain_list_all_snapshots_args { }; struct remote_domain_list_all_snapshots_ret { - remote_nonnull_domain_snapshot snapshots<>; + remote_nonnull_domain_snapshot snapshots<REMOTE_DOMAIN_SNAPSHOT_LIST_MAX>; int ret; }; @@ -2426,7 +2426,7 @@ struct remote_domain_snapshot_list_children_names_args { }; struct remote_domain_snapshot_list_children_names_ret { - remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_NAMES_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_DOMAIN_SNAPSHOT_LIST_MAX>; /* insert@1 */ }; struct remote_domain_snapshot_list_all_children_args { @@ -2436,7 +2436,7 @@ struct remote_domain_snapshot_list_all_children_args { }; struct remote_domain_snapshot_list_all_children_ret { - remote_nonnull_domain_snapshot snapshots<>; + remote_nonnull_domain_snapshot snapshots<REMOTE_DOMAIN_SNAPSHOT_LIST_MAX>; int ret; }; -- 1.8.3.1

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 04/12] Add bounds checking on virConnectListAllDomains RPC call

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virConnectListAllDomains call were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 7 +++++++ src/remote/remote_driver.c | 15 +++++++++++---- src/remote/remote_protocol.x | 15 +++++---------- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index e228cba..8289cb5 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1050,6 +1050,13 @@ remoteDispatchConnectListAllDomains(virNetServerPtr server ATTRIBUTE_UNUSED, args->flags)) < 0) goto cleanup; + if (ndomains > REMOTE_DOMAIN_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many domains '%d' for limit '%d'"), + ndomains, REMOTE_DOMAIN_LIST_MAX); + goto cleanup; + } + if (doms && ndomains) { if (VIR_ALLOC_N(ret->domains.domains_val, ndomains) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 14c16f6..cd2b9a9 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1370,10 +1370,10 @@ remoteConnectListDomains(virConnectPtr conn, int *ids, int maxids) remoteDriverLock(priv); - if (maxids > REMOTE_DOMAIN_ID_LIST_MAX) { + if (maxids > REMOTE_DOMAIN_LIST_MAX) { virReportError(VIR_ERR_RPC, - _("too many remote domain IDs: %d > %d"), - maxids, REMOTE_DOMAIN_ID_LIST_MAX); + _("Too many domains '%d' for limit '%d'"), + maxids, REMOTE_DOMAIN_LIST_MAX); goto done; } args.maxids = maxids; @@ -1386,7 +1386,7 @@ remoteConnectListDomains(virConnectPtr conn, int *ids, int maxids) if (ret.ids.ids_len > maxids) { virReportError(VIR_ERR_RPC, - _("too many remote domain IDs: %d > %d"), + _("Too many domains '%d' for limit '%d'"), ret.ids.ids_len, maxids); goto cleanup; } @@ -1433,6 +1433,13 @@ remoteConnectListAllDomains(virConnectPtr conn, (char *) &ret) == -1) goto done; + if (ret.domains.domains_len > REMOTE_DOMAIN_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many domains '%d' for limit '%d'"), + ret.domains.domains_len, REMOTE_DOMAIN_LIST_MAX); + goto cleanup; + } + if (domains) { if (VIR_ALLOC_N(doms, ret.domains.domains_len + 1) < 0) goto cleanup; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 9e3918c..718e398 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -73,13 +73,8 @@ typedef string remote_nonnull_string<REMOTE_STRING_MAX>; /* A long string, which may be NULL. */ typedef remote_nonnull_string *remote_string; -/* This just places an upper limit on the length of lists of - * domain IDs which may be sent via the protocol. - */ -const REMOTE_DOMAIN_ID_LIST_MAX = 16384; - -/* Upper limit on lists of domain names. */ -const REMOTE_DOMAIN_NAME_LIST_MAX = 16384; +/* Upper limit on lists of domains. */ +const REMOTE_DOMAIN_LIST_MAX = 16384; /* Upper limit on cpumap (bytes) passed to virDomainPinVcpu. */ const REMOTE_CPUMAP_MAX = 2048; @@ -724,7 +719,7 @@ struct remote_connect_list_domains_args { }; struct remote_connect_list_domains_ret { - int ids<REMOTE_DOMAIN_ID_LIST_MAX>; /* insert@1 */ + int ids<REMOTE_DOMAIN_LIST_MAX>; /* insert@1 */ }; struct remote_connect_num_of_domains_ret { @@ -990,7 +985,7 @@ struct remote_connect_list_defined_domains_args { }; struct remote_connect_list_defined_domains_ret { - remote_nonnull_string names<REMOTE_DOMAIN_NAME_LIST_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_DOMAIN_LIST_MAX>; /* insert@1 */ }; struct remote_connect_num_of_defined_domains_ret { @@ -2665,7 +2660,7 @@ struct remote_connect_list_all_domains_args { }; struct remote_connect_list_all_domains_ret { - remote_nonnull_domain domains<>; + remote_nonnull_domain domains<REMOTE_DOMAIN_LIST_MAX>; unsigned int ret; }; -- 1.8.3.1

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 05/12] Add bounds checking on virConnectListAllStoragePools RPC call

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virConnectListAllStoragePools call were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 7 +++++++ src/remote/remote_driver.c | 7 +++++++ src/remote/remote_protocol.x | 10 +++++----- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 8289cb5..42c1c47 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4073,6 +4073,13 @@ remoteDispatchConnectListAllStoragePools(virNetServerPtr server ATTRIBUTE_UNUSED args->flags)) < 0) goto cleanup; + if (npools > REMOTE_STORAGE_POOL_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many storage pools '%d' for limit '%d'"), + npools, REMOTE_STORAGE_POOL_LIST_MAX); + goto cleanup; + } + if (pools && npools) { if (VIR_ALLOC_N(ret->pools.pools_val, npools) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index cd2b9a9..622647b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3276,6 +3276,13 @@ remoteConnectListAllStoragePools(virConnectPtr conn, (char *) &ret) == -1) goto done; + if (ret.pools.pools_len > REMOTE_STORAGE_POOL_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many storage pools '%d' for limit '%d'"), + ret.pools.pools_len, REMOTE_STORAGE_POOL_LIST_MAX); + goto cleanup; + } + if (pools) { if (VIR_ALLOC_N(tmp_pools, ret.pools.pools_len + 1) < 0) goto cleanup; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 718e398..6e996b4 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -97,8 +97,8 @@ const REMOTE_INTERFACE_NAME_LIST_MAX = 16384; /* Upper limit on lists of defined interface names. */ const REMOTE_DEFINED_INTERFACE_NAME_LIST_MAX = 16384; -/* Upper limit on lists of storage pool names. */ -const REMOTE_STORAGE_POOL_NAME_LIST_MAX = 4096; +/* Upper limit on lists of storage pools. */ +const REMOTE_STORAGE_POOL_LIST_MAX = 4096; /* Upper limit on lists of storage vol names. */ const REMOTE_STORAGE_VOL_NAME_LIST_MAX = 16384; @@ -1604,7 +1604,7 @@ struct remote_connect_list_storage_pools_args { }; struct remote_connect_list_storage_pools_ret { - remote_nonnull_string names<REMOTE_STORAGE_POOL_NAME_LIST_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_STORAGE_POOL_LIST_MAX>; /* insert@1 */ }; struct remote_connect_num_of_defined_storage_pools_ret { @@ -1616,7 +1616,7 @@ struct remote_connect_list_defined_storage_pools_args { }; struct remote_connect_list_defined_storage_pools_ret { - remote_nonnull_string names<REMOTE_STORAGE_POOL_NAME_LIST_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_STORAGE_POOL_LIST_MAX>; /* insert@1 */ }; struct remote_connect_find_storage_pool_sources_args { @@ -2670,7 +2670,7 @@ struct remote_connect_list_all_storage_pools_args { }; struct remote_connect_list_all_storage_pools_ret { - remote_nonnull_storage_pool pools<>; + remote_nonnull_storage_pool pools<REMOTE_STORAGE_POOL_LIST_MAX>; unsigned int ret; }; -- 1.8.3.1

Eric Blake

10:25 a.m.

New subject: [libvirt] [PATCH 05/12] Add bounds checking on virConnectListAllStoragePools RPC call

On 08/29/2013 04:49 AM, Daniel P. Berrange wrote:

...

Just noticed after you had already pushed (oh well), but s/where/were/ in your copied-and-pasted commit message. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 06/12] Add bounds checking on virStoragePoolListAllVolumes RPC call

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virStoragePoolListAllVolumes call were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 7 +++++++ src/remote/remote_driver.c | 7 +++++++ src/remote/remote_protocol.x | 8 ++++---- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 42c1c47..512170b 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4136,6 +4136,13 @@ remoteDispatchStoragePoolListAllVolumes(virNetServerPtr server ATTRIBUTE_UNUSED, args->flags)) < 0) goto cleanup; + if (nvols > REMOTE_STORAGE_VOL_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many storage volumes '%d' for limit '%d'"), + nvols, REMOTE_STORAGE_VOL_LIST_MAX); + goto cleanup; + } + if (vols && nvols) { if (VIR_ALLOC_N(ret->vols.vols_val, nvols) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 622647b..542cb39 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3343,6 +3343,13 @@ remoteStoragePoolListAllVolumes(virStoragePoolPtr pool, (char *) &ret) == -1) goto done; + if (ret.vols.vols_len > REMOTE_STORAGE_VOL_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many storage volumes '%d' for limit '%d'"), + ret.vols.vols_len, REMOTE_STORAGE_VOL_LIST_MAX); + goto cleanup; + } + if (vols) { if (VIR_ALLOC_N(tmp_vols, ret.vols.vols_len + 1) < 0) goto cleanup; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 6e996b4..2034aa1 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -100,8 +100,8 @@ const REMOTE_DEFINED_INTERFACE_NAME_LIST_MAX = 16384; /* Upper limit on lists of storage pools. */ const REMOTE_STORAGE_POOL_LIST_MAX = 4096; -/* Upper limit on lists of storage vol names. */ -const REMOTE_STORAGE_VOL_NAME_LIST_MAX = 16384; +/* Upper limit on lists of storage vols. */ +const REMOTE_STORAGE_VOL_LIST_MAX = 16384; /* Upper limit on lists of node device names. */ const REMOTE_NODE_DEVICE_NAME_LIST_MAX = 16384; @@ -1746,7 +1746,7 @@ struct remote_storage_pool_list_volumes_args { }; struct remote_storage_pool_list_volumes_ret { - remote_nonnull_string names<REMOTE_STORAGE_VOL_NAME_LIST_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_STORAGE_VOL_LIST_MAX>; /* insert@1 */ }; @@ -2681,7 +2681,7 @@ struct remote_storage_pool_list_all_volumes_args { }; struct remote_storage_pool_list_all_volumes_ret { - remote_nonnull_storage_vol vols<>; + remote_nonnull_storage_vol vols<REMOTE_STORAGE_VOL_LIST_MAX>; unsigned int ret; }; -- 1.8.3.1

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 07/12] Add bounds checking on virConnectListAllNetworks RPC call

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virConnectListAllNetworks call were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 7 +++++++ src/remote/remote_driver.c | 7 +++++++ src/remote/remote_protocol.x | 10 +++++----- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 512170b..b6501e0 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4197,6 +4197,13 @@ remoteDispatchConnectListAllNetworks(virNetServerPtr server ATTRIBUTE_UNUSED, args->flags)) < 0) goto cleanup; + if (nnets > REMOTE_NETWORK_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many networks '%d' for limit '%d'"), + nnets, REMOTE_NETWORK_LIST_MAX); + goto cleanup; + } + if (nets && nnets) { if (VIR_ALLOC_N(ret->nets.nets_val, nnets) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 542cb39..e555704 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2843,6 +2843,13 @@ remoteConnectListAllNetworks(virConnectPtr conn, (char *) &ret) == -1) goto done; + if (ret.nets.nets_len > REMOTE_NETWORK_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many networks '%d' for limit '%d'"), + ret.nets.nets_len, REMOTE_NETWORK_LIST_MAX); + goto cleanup; + } + if (nets) { if (VIR_ALLOC_N(tmp_nets, ret.nets.nets_len + 1) < 0) goto cleanup; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 2034aa1..f1e27fa 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -88,8 +88,8 @@ const REMOTE_CPUMAPS_MAX = 8388608; /* Upper limit on migrate cookie. */ const REMOTE_MIGRATE_COOKIE_MAX = 16384; -/* Upper limit on lists of network names. */ -const REMOTE_NETWORK_NAME_LIST_MAX = 16384; +/* Upper limit on lists of networks. */ +const REMOTE_NETWORK_LIST_MAX = 16384; /* Upper limit on lists of interface names. */ const REMOTE_INTERFACE_NAME_LIST_MAX = 16384; @@ -1317,7 +1317,7 @@ struct remote_connect_list_networks_args { }; struct remote_connect_list_networks_ret { - remote_nonnull_string names<REMOTE_NETWORK_NAME_LIST_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_NETWORK_LIST_MAX>; /* insert@1 */ }; struct remote_connect_num_of_defined_networks_ret { @@ -1329,7 +1329,7 @@ struct remote_connect_list_defined_networks_args { }; struct remote_connect_list_defined_networks_ret { - remote_nonnull_string names<REMOTE_NETWORK_NAME_LIST_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_NETWORK_LIST_MAX>; /* insert@1 */ }; struct remote_network_lookup_by_uuid_args { @@ -2691,7 +2691,7 @@ struct remote_connect_list_all_networks_args { }; struct remote_connect_list_all_networks_ret { - remote_nonnull_network nets<>; + remote_nonnull_network nets<REMOTE_NETWORK_LIST_MAX>; unsigned int ret; }; -- 1.8.3.1

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 08/12] Add bounds checking on virConnectListAllInterfaces RPC call

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virConnectListAllInterfaces call were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 7 +++++++ src/remote/remote_driver.c | 7 +++++++ src/remote/remote_protocol.x | 13 +++++-------- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index b6501e0..871b0df 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4256,6 +4256,13 @@ remoteDispatchConnectListAllInterfaces(virNetServerPtr server ATTRIBUTE_UNUSED, args->flags)) < 0) goto cleanup; + if (nifaces > REMOTE_INTERFACE_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many interfaces '%d' for limit '%d'"), + nifaces, REMOTE_INTERFACE_LIST_MAX); + goto cleanup; + } + if (ifaces && nifaces) { if (VIR_ALLOC_N(ret->ifaces.ifaces_val, nifaces) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e555704..4d82452 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2909,6 +2909,13 @@ remoteConnectListAllInterfaces(virConnectPtr conn, (char *) &ret) == -1) goto done; + if (ret.ifaces.ifaces_len > REMOTE_INTERFACE_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many interfaces '%d' for limit '%d'"), + ret.ifaces.ifaces_len, REMOTE_INTERFACE_LIST_MAX); + goto cleanup; + } + if (ifaces) { if (VIR_ALLOC_N(tmp_ifaces, ret.ifaces.ifaces_len + 1) < 0) goto cleanup; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index f1e27fa..573d63f 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -91,11 +91,8 @@ const REMOTE_MIGRATE_COOKIE_MAX = 16384; /* Upper limit on lists of networks. */ const REMOTE_NETWORK_LIST_MAX = 16384; -/* Upper limit on lists of interface names. */ -const REMOTE_INTERFACE_NAME_LIST_MAX = 16384; - -/* Upper limit on lists of defined interface names. */ -const REMOTE_DEFINED_INTERFACE_NAME_LIST_MAX = 16384; +/* Upper limit on lists of interfaces. */ +const REMOTE_INTERFACE_LIST_MAX = 16384; /* Upper limit on lists of storage pools. */ const REMOTE_STORAGE_POOL_LIST_MAX = 4096; @@ -1478,7 +1475,7 @@ struct remote_connect_list_interfaces_args { }; struct remote_connect_list_interfaces_ret { - remote_nonnull_string names<REMOTE_INTERFACE_NAME_LIST_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_INTERFACE_LIST_MAX>; /* insert@1 */ }; struct remote_connect_num_of_defined_interfaces_ret { @@ -1490,7 +1487,7 @@ struct remote_connect_list_defined_interfaces_args { }; struct remote_connect_list_defined_interfaces_ret { - remote_nonnull_string names<REMOTE_DEFINED_INTERFACE_NAME_LIST_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_INTERFACE_LIST_MAX>; /* insert@1 */ }; struct remote_interface_lookup_by_name_args { @@ -2701,7 +2698,7 @@ struct remote_connect_list_all_interfaces_args { }; struct remote_connect_list_all_interfaces_ret { - remote_nonnull_interface ifaces<>; + remote_nonnull_interface ifaces<REMOTE_INTERFACE_LIST_MAX>; unsigned int ret; }; -- 1.8.3.1

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 09/12] Add bounds checking on virConnectListAllNodeDevices RPC call

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virConnectListAllNodeDevices call were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 7 +++++++ src/remote/remote_driver.c | 7 +++++++ src/remote/remote_protocol.x | 8 ++++---- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 871b0df..697aa58 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4315,6 +4315,13 @@ remoteDispatchConnectListAllNodeDevices(virNetServerPtr server ATTRIBUTE_UNUSED, args->flags)) < 0) goto cleanup; + if (ndevices > REMOTE_NODE_DEVICE_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many node devices '%d' for limit '%d'"), + ndevices, REMOTE_NODE_DEVICE_LIST_MAX); + goto cleanup; + } + if (devices && ndevices) { if (VIR_ALLOC_N(ret->devices.devices_val, ndevices) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 4d82452..b170a06 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2975,6 +2975,13 @@ remoteConnectListAllNodeDevices(virConnectPtr conn, (char *) &ret) == -1) goto done; + if (ret.devices.devices_len > REMOTE_NODE_DEVICE_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many node devices '%d' for limit '%d'"), + ret.devices.devices_len, REMOTE_NODE_DEVICE_LIST_MAX); + goto cleanup; + } + if (devices) { if (VIR_ALLOC_N(tmp_devices, ret.devices.devices_len + 1) < 0) goto cleanup; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 573d63f..21059c8 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -100,8 +100,8 @@ const REMOTE_STORAGE_POOL_LIST_MAX = 4096; /* Upper limit on lists of storage vols. */ const REMOTE_STORAGE_VOL_LIST_MAX = 16384; -/* Upper limit on lists of node device names. */ -const REMOTE_NODE_DEVICE_NAME_LIST_MAX = 16384; +/* Upper limit on lists of node devices. */ +const REMOTE_NODE_DEVICE_LIST_MAX = 16384; /* Upper limit on lists of node device capabilities. */ const REMOTE_NODE_DEVICE_CAPS_LIST_MAX = 65536; @@ -1863,7 +1863,7 @@ struct remote_node_list_devices_args { }; struct remote_node_list_devices_ret { - remote_nonnull_string names<REMOTE_NODE_DEVICE_NAME_LIST_MAX>; /* insert@2 */ + remote_nonnull_string names<REMOTE_NODE_DEVICE_LIST_MAX>; /* insert@2 */ }; struct remote_node_device_lookup_by_name_args { @@ -2708,7 +2708,7 @@ struct remote_connect_list_all_node_devices_args { }; struct remote_connect_list_all_node_devices_ret { - remote_nonnull_node_device devices<>; + remote_nonnull_node_device devices<REMOTE_NODE_DEVICE_LIST_MAX>; unsigned int ret; }; -- 1.8.3.1

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 10/12] Add bounds checking on virConnectListAllNWFilters RPC call

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virConnectListAllNWFilters call were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 7 +++++++ src/remote/remote_driver.c | 7 +++++++ src/remote/remote_protocol.x | 8 ++++---- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 697aa58..e19b42d 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4374,6 +4374,13 @@ remoteDispatchConnectListAllNWFilters(virNetServerPtr server ATTRIBUTE_UNUSED, args->flags)) < 0) goto cleanup; + if (nfilters > REMOTE_NWFILTER_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many network filters '%d' for limit '%d'"), + nfilters, REMOTE_NWFILTER_LIST_MAX); + goto cleanup; + } + if (filters && nfilters) { if (VIR_ALLOC_N(ret->filters.filters_val, nfilters) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b170a06..d486fa5 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3041,6 +3041,13 @@ remoteConnectListAllNWFilters(virConnectPtr conn, (char *) &ret) == -1) goto done; + if (ret.filters.filters_len > REMOTE_NWFILTER_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many network filters '%d' for limit '%d'"), + ret.filters.filters_len, REMOTE_NWFILTER_LIST_MAX); + goto cleanup; + } + if (filters) { if (VIR_ALLOC_N(tmp_filters, ret.filters.filters_len + 1) < 0) goto cleanup; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 21059c8..b2841a7 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -106,8 +106,8 @@ const REMOTE_NODE_DEVICE_LIST_MAX = 16384; /* Upper limit on lists of node device capabilities. */ const REMOTE_NODE_DEVICE_CAPS_LIST_MAX = 65536; -/* Upper limit on lists of network filter names. */ -const REMOTE_NWFILTER_NAME_LIST_MAX = 1024; +/* Upper limit on lists of network filters. */ +const REMOTE_NWFILTER_LIST_MAX = 1024; /* Upper limit on list of scheduler parameters. */ const REMOTE_DOMAIN_SCHEDULER_PARAMETERS_MAX = 16; @@ -1423,7 +1423,7 @@ struct remote_connect_list_nwfilters_args { }; struct remote_connect_list_nwfilters_ret { - remote_nonnull_string names<REMOTE_NWFILTER_NAME_LIST_MAX>; /* insert@1 */ + remote_nonnull_string names<REMOTE_NWFILTER_LIST_MAX>; /* insert@1 */ }; struct remote_nwfilter_lookup_by_uuid_args { @@ -2718,7 +2718,7 @@ struct remote_connect_list_all_nwfilters_args { }; struct remote_connect_list_all_nwfilters_ret { - remote_nonnull_nwfilter filters<>; + remote_nonnull_nwfilter filters<REMOTE_NWFILTER_LIST_MAX>; unsigned int ret; }; -- 1.8.3.1

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 11/12] Add bounds checking on virConnectListAllSecrets RPC call

From: "Daniel P. Berrange" <berrange(a)redhat.com> The return values for the virConnectListAllSecrets call were not bounds checked. This is a robustness issue for clients if something where to cause corruption of the RPC stream data. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- daemon/remote.c | 7 +++++++ src/remote/remote_driver.c | 7 +++++++ src/remote/remote_protocol.x | 6 +++--- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index e19b42d..6ace7af 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -4433,6 +4433,13 @@ remoteDispatchConnectListAllSecrets(virNetServerPtr server ATTRIBUTE_UNUSED, args->flags)) < 0) goto cleanup; + if (nsecrets > REMOTE_SECRET_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many secrets '%d' for limit '%d'"), + nsecrets, REMOTE_SECRET_LIST_MAX); + goto cleanup; + } + if (secrets && nsecrets) { if (VIR_ALLOC_N(ret->secrets.secrets_val, nsecrets) < 0) goto cleanup; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d486fa5..62e77a5 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3107,6 +3107,13 @@ remoteConnectListAllSecrets(virConnectPtr conn, (char *) &ret) == -1) goto done; + if (ret.secrets.secrets_len > REMOTE_SECRET_LIST_MAX) { + virReportError(VIR_ERR_RPC, + _("Too many secrets '%d' for limit '%d'"), + ret.secrets.secrets_len, REMOTE_SECRET_LIST_MAX); + goto cleanup; + } + if (secrets) { if (VIR_ALLOC_N(tmp_secrets, ret.secrets.secrets_len + 1) < 0) goto cleanup; diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index b2841a7..a1c23da 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -188,7 +188,7 @@ const REMOTE_SECRET_VALUE_MAX = 65536; /* * Upper limit on list of secrets. */ -const REMOTE_SECRET_UUID_LIST_MAX = 16384; +const REMOTE_SECRET_LIST_MAX = 16384; /* * Upper limit on list of CPUs accepted when computing a baseline CPU. @@ -2002,7 +2002,7 @@ struct remote_connect_list_secrets_args { }; struct remote_connect_list_secrets_ret { - remote_nonnull_string uuids<REMOTE_SECRET_UUID_LIST_MAX>; /* insert@1 */ + remote_nonnull_string uuids<REMOTE_SECRET_LIST_MAX>; /* insert@1 */ }; struct remote_secret_lookup_by_uuid_args { @@ -2728,7 +2728,7 @@ struct remote_connect_list_all_secrets_args { }; struct remote_connect_list_all_secrets_ret { - remote_nonnull_secret secrets<>; + remote_nonnull_secret secrets<REMOTE_SECRET_LIST_MAX>; unsigned int ret; }; -- 1.8.3.1

Daniel P. Berrange

5:49 a.m.

New subject: [libvirt] [PATCH 12/12] Prohibit unbounded arrays in XDR protocols

From: "Daniel P. Berrange" <berrange(a)redhat.com> The use of <> is a security issue for RPC parameters, since a malicious client can set a huge array length causing arbitrary memory allocation in the daemon. It is also a robustness issue for RPC return values, because if the stream is corrupted, it can cause the client to also allocate arbitrary memory. Use a syntax-check rule to prohibit any use of <> Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com> --- cfg.mk | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cfg.mk b/cfg.mk index 23564f1..9a9616c 100644 --- a/cfg.mk +++ b/cfg.mk @@ -836,6 +836,12 @@ sc_prohibit_config_h_in_headers: halt='headers should not include <config.h>' \ $(_sc_search_regexp) +sc_prohibit_unbounded_arrays_in_rpc: + @prohibit='<>' \ + in_vc_files='\.x$$' \ + halt='Arrays in XDR must have a upper limit set for <NNN>' \ + $(_sc_search_regexp) + # We don't use this feature of maint.mk. prev_version_file = /dev/null -- 1.8.3.1

4081

days inactive

4081

days old

devel@lists.libvirt.org

Manage subscription

17 comments

3 participants

tags (0)

participants (3)

Daniel P. Berrange
Eric Blake
Michal Privoznik

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

[libvirt] [PATCH 00/12] Ensure array bounds checking is present on all RPC calls