[PATCH 0/2] Reject Xen VM config containing nwfilter references
This is essentially V2 of a small series inspired by a report on the security list about nwfilters not working with Xen VMs. V1 was posted to the security list, so no public reference. The libxl driver simply does not support nwfilters, so the report is really a RFE vs a security issue. I'm now moving the discussion to the public devel list. I don't have time to add nwfilter support to the libxl driver, but agree the documentation could be improved. Given the perceived security implications, I also think it's worth considering rejecting Xen VM <interface> configuration containing <filterref>, even though libvirt tends to ignore unsupported XML config. Patch1 improves the documentation. I also considered adding a "Limitations" section to docs/drvxen.rst, but none of the other drivers have such section. Also, for the xen one, I wasn't sure where to start with listing limitations :-P. Patch2 rejects Xen VM config containg <filterref> in their <interface> definitions. Jim Fehlig (2): docs: Clarify hypervisor support for nwfilter profiles libxl: Reject VM config referencing nwfilters docs/formatdomain.rst | 8 ++++---- src/libxl/libxl_domain.c | 7 +++++++ 2 files changed, 11 insertions(+), 4 deletions(-) -- 2.35.3
Enhance the 'since' annotation of <filterref> documentation to note it's only supported by the QEMU, LXC, and ch hypervisor drivers. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- docs/formatdomain.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 47d3e2125e..8e8a9660fc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. Traffic filtering with NWFilter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain -interface, which allows configuring traffic filter rules for the virtual -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more -complete details. +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an ``nwfilter`` +profile can be assigned to a domain interface, which allows configuring traffic +filter rules for the virtual machine. See the `nwfilter <formatnwfilter.html>`__ +documentation for more complete details. :: -- 2.35.3
On Wed, Sep 11, 2024 at 03:02:41PM -0600, Jim Fehlig wrote:
Enhance the 'since' annotation of <filterref> documentation to note it's only supported by the QEMU, LXC, and ch hypervisor drivers.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- docs/formatdomain.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 47d3e2125e..8e8a9660fc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. Traffic filtering with NWFilter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain -interface, which allows configuring traffic filter rules for the virtual -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more -complete details. +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an ``nwfilter`` +profile can be assigned to a domain interface, which allows configuring traffic +filter rules for the virtual machine. See the `nwfilter <formatnwfilter.html>`__ +documentation for more complete details.
::
-- 2.35.3
Should this have a Suggested-by: Demi Marie Obenour <demi@invisiblethingslab.com> tag? In any case: Acked-by: Demi Marie Obenour <demi@invisiblethingslab.com> -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab
On 9/11/24 15:54, Demi Marie Obenour wrote:
On Wed, Sep 11, 2024 at 03:02:41PM -0600, Jim Fehlig wrote:
Enhance the 'since' annotation of <filterref> documentation to note it's only supported by the QEMU, LXC, and ch hypervisor drivers.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- docs/formatdomain.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 47d3e2125e..8e8a9660fc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. Traffic filtering with NWFilter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain -interface, which allows configuring traffic filter rules for the virtual -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more -complete details. +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an ``nwfilter`` +profile can be assigned to a domain interface, which allows configuring traffic +filter rules for the virtual machine. See the `nwfilter <formatnwfilter.html>`__ +documentation for more complete details.
::
-- 2.35.3
Should this have a
Suggested-by: Demi Marie Obenour <demi@invisiblethingslab.com>
tag?
I've added that.
In any case:
Acked-by: Demi Marie Obenour <demi@invisiblethingslab.com>
I don't recall use of 'Acked-by' in libvirt for a long time. 'git log' seems to confirm that. Neither the contributor or committer guidelines say much about it https://libvirt.org/hacking.html https://libvirt.org/committer-guidelines.html I'm happy to add it, or 'Reviewed-by' if you're fine with that, but would like to hear opinions from other maintainers on this topic. Regards, Jim
On 9/11/24 6:44 PM, Jim Fehlig via Devel wrote:
On 9/11/24 15:54, Demi Marie Obenour wrote:
On Wed, Sep 11, 2024 at 03:02:41PM -0600, Jim Fehlig wrote:
Enhance the 'since' annotation of <filterref> documentation to note it's only supported by the QEMU, LXC, and ch hypervisor drivers.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- docs/formatdomain.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 47d3e2125e..8e8a9660fc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. Traffic filtering with NWFilter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain -interface, which allows configuring traffic filter rules for the virtual -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more -complete details. +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an ``nwfilter`` +profile can be assigned to a domain interface, which allows configuring traffic +filter rules for the virtual machine. See the `nwfilter <formatnwfilter.html>`__ +documentation for more complete details. :: -- 2.35.3
Should this have a
Suggested-by: Demi Marie Obenour <demi@invisiblethingslab.com>
tag?
I've added that.
In any case:
Acked-by: Demi Marie Obenour <demi@invisiblethingslab.com>
I don't recall use of 'Acked-by' in libvirt for a long time. 'git log' seems to confirm that. Neither the contributor or committer guidelines say much about it
https://libvirt.org/hacking.html https://libvirt.org/committer-guidelines.html
I'm happy to add it, or 'Reviewed-by' if you're fine with that, but would like to hear opinions from other maintainers on this topic.
A *long* time ago before we used the Reviewed-by: tag we used to approve patches by replying "ACK" in an email, but we've always put Reviewed-by: in git.
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
Enhance the 'since' annotation of <filterref> documentation to note it's only supported by the QEMU, LXC, and ch hypervisor drivers.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- docs/formatdomain.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 47d3e2125e..8e8a9660fc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. Traffic filtering with NWFilter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain -interface, which allows configuring traffic filter rules for the virtual -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more -complete details. +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an ``nwfilter`` +profile can be assigned to a domain interface, which allows configuring traffic +filter rules for the virtual machine. See the `nwfilter <formatnwfilter.html>`__ +documentation for more complete details.
It's preexisting, but I would have said "allows configuring network traffic filter rules". Reviewed-by: Laine Stump <laine@redhat.com>
On 9/11/24 16:08, Laine Stump wrote:
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
Enhance the 'since' annotation of <filterref> documentation to note it's only supported by the QEMU, LXC, and ch hypervisor drivers.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- docs/formatdomain.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 47d3e2125e..8e8a9660fc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. Traffic filtering with NWFilter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain -interface, which allows configuring traffic filter rules for the virtual -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more -complete details. +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an ``nwfilter`` +profile can be assigned to a domain interface, which allows configuring traffic +filter rules for the virtual machine. See the `nwfilter <formatnwfilter.html>`__ +documentation for more complete details.
It's preexisting, but I would have said "allows configuring network traffic filter rules".
I've added 'network' to that sentence in my local branch.
Reviewed-by: Laine Stump <laine@redhat.com>
Thanks! Regards, Jim
On 9/11/24 16:47, Jim Fehlig wrote:
On 9/11/24 16:08, Laine Stump wrote:
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
Enhance the 'since' annotation of <filterref> documentation to note it's only supported by the QEMU, LXC, and ch hypervisor drivers.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- docs/formatdomain.rst | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 47d3e2125e..8e8a9660fc 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -6205,10 +6205,10 @@ hypervisor tries to reconnect. Traffic filtering with NWFilter ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -:since:`Since 0.8.0` an ``nwfilter`` profile can be assigned to a domain -interface, which allows configuring traffic filter rules for the virtual -machine. See the `nwfilter <formatnwfilter.html>`__ documentation for more -complete details. +:since:`Since 0.8.0 (QEMU), 0.9.3 (LXC), 10.1.0 (Cloud Hypervisor)` an ``nwfilter`` +profile can be assigned to a domain interface, which allows configuring traffic +filter rules for the virtual machine. See the `nwfilter <formatnwfilter.html>`__ +documentation for more complete details.
It's preexisting, but I would have said "allows configuring network traffic filter rules".
I've added 'network' to that sentence in my local branch.
Reviewed-by: Laine Stump <laine@redhat.com>
I've pushed this patch now. I'll send V2 of patch2 in a bit. Regards, Jim
The Xen libxl driver does not support nwfilter. Add a check for nwfilters to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if any are found. It's generally preferred for drivers to ignore unsupported XML features, but ignoring a user's request to filter VM network traffic can be viewed as a security issue. Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0f129ec69c..2f6cebb8ae 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) { + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("filterref is not supported in %1$s"), + virDomainVirtTypeToString(def->virtType)); + return -1; + } + if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE && -- 2.35.3
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
The Xen libxl driver does not support nwfilter. Add a check for nwfilters to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if any are found.
It's generally preferred for drivers to ignore unsupported XML features,
I would instead characterize it as "drivers generally ignore *unrecognized* XML", but it's quite common for a bit of XML that's understood and supported in one context within libvirt to generate an UNSUPPORTED error when attempting to use it in a place where it isn't supported.
but ignoring a user's request to filter VM network traffic can be viewed as a security issue.
Definitely.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0f129ec69c..2f6cebb8ae 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) { + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("filterref is not supported in %1$s"), + virDomainVirtTypeToString(def->virtType)); + return -1; + } +
This more properly should be in a function called libxlValidateDomainDeviceDef(), which would look something like qemuValidateDomainDeviceDef() and be added into libxlDomainDefParserConfig with this initialization: .deviceValidateCallback = libxlValidateDomainDeviceDef, (my understanding of the purpose of the two has always been that the PostParse callback is intended for performing post-parse modifications/fixups to the domain def, while the Validate only checks for correctness, without modifying anything. There are multiple cases of having validation in the postparse function, but I think those are the 1) leftovers from before the introduction of the validate callbacks, and 2) the result of misunderstanding and/or sloth (e.g. in cases where you want to validate something, but the driver you're adding this validation to doesn't already have a deviceValidateCallback)
if (dev->type == VIR_DOMAIN_DEVICE_CHR && dev->data.chr->deviceType == VIR_DOMAIN_CHR_DEVICE_TYPE_CONSOLE && dev->data.chr->targetType == VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_NONE &&
On 9/11/24 16:24, Laine Stump wrote:
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
The Xen libxl driver does not support nwfilter. Add a check for nwfilters to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if any are found.
It's generally preferred for drivers to ignore unsupported XML features,
I would instead characterize it as "drivers generally ignore *unrecognized* XML", but it's quite common for a bit of XML that's understood and supported in one context within libvirt to generate an UNSUPPORTED error when attempting to use it in a place where it isn't supported.
but ignoring a user's request to filter VM network traffic can be viewed as a security issue.
Definitely.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0f129ec69c..2f6cebb8ae 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) { + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("filterref is not supported in %1$s"), + virDomainVirtTypeToString(def->virtType)); + return -1; + } +
This more properly should be in a function called libxlValidateDomainDeviceDef(), which would look something like qemuValidateDomainDeviceDef() and be added into libxlDomainDefParserConfig with this initialization:
.deviceValidateCallback = libxlValidateDomainDeviceDef,
Yes, good point! The libxl driver already has domainValidateCallback, but now needs a deviceValidateCallback for this code. I'll make that change in V2. Before sending another version, I'd like to hear opinions on Demi's question about the other hypervisor drivers. Do they need a similar change? Regards, Jim
On Wed, Sep 11, 2024 at 05:09:03PM -0600, Jim Fehlig wrote:
On 9/11/24 16:24, Laine Stump wrote:
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
The Xen libxl driver does not support nwfilter. Add a check for nwfilters to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if any are found.
It's generally preferred for drivers to ignore unsupported XML features,
I would instead characterize it as "drivers generally ignore *unrecognized* XML", but it's quite common for a bit of XML that's understood and supported in one context within libvirt to generate an UNSUPPORTED error when attempting to use it in a place where it isn't supported.
but ignoring a user's request to filter VM network traffic can be viewed as a security issue.
Definitely.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0f129ec69c..2f6cebb8ae 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) { + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("filterref is not supported in %1$s"), + virDomainVirtTypeToString(def->virtType)); + return -1; + } +
This more properly should be in a function called libxlValidateDomainDeviceDef(), which would look something like qemuValidateDomainDeviceDef() and be added into libxlDomainDefParserConfig with this initialization:
.deviceValidateCallback = libxlValidateDomainDeviceDef,
Yes, good point! The libxl driver already has domainValidateCallback, but now needs a deviceValidateCallback for this code. I'll make that change in V2.
Before sending another version, I'd like to hear opinions on Demi's question about the other hypervisor drivers. Do they need a similar change?
I'm not familiar with how libvirt works internally, but to me it seems like one option would be to have a flag set on the drivers that support network filtering. Generic code would then check the flag and fail if filtering is requested with a driver that doesn't support it. This has the advantage of not requiring changes to each and every driver. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab
On 9/11/24 7:42 PM, Demi Marie Obenour wrote:
On Wed, Sep 11, 2024 at 05:09:03PM -0600, Jim Fehlig wrote:
On 9/11/24 16:24, Laine Stump wrote:
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
The Xen libxl driver does not support nwfilter. Add a check for nwfilters to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if any are found.
It's generally preferred for drivers to ignore unsupported XML features,
I would instead characterize it as "drivers generally ignore *unrecognized* XML", but it's quite common for a bit of XML that's understood and supported in one context within libvirt to generate an UNSUPPORTED error when attempting to use it in a place where it isn't supported.
but ignoring a user's request to filter VM network traffic can be viewed as a security issue.
Definitely.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0f129ec69c..2f6cebb8ae 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) { + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("filterref is not supported in %1$s"), + virDomainVirtTypeToString(def->virtType)); + return -1; + } +
This more properly should be in a function called libxlValidateDomainDeviceDef(), which would look something like qemuValidateDomainDeviceDef() and be added into libxlDomainDefParserConfig with this initialization:
.deviceValidateCallback = libxlValidateDomainDeviceDef,
Yes, good point! The libxl driver already has domainValidateCallback, but now needs a deviceValidateCallback for this code. I'll make that change in V2.
Before sending another version, I'd like to hear opinions on Demi's question about the other hypervisor drivers. Do they need a similar change?
Now that we're talking more about this, I'm having flashbacks of new features being added in, and the author (e.g. me) basically updating the hypervisor drivers they were personally interested in to support the feature (or else explicitly log an error), but leaving the other drivers untouched because "I can't test it, so I don't want to add code that could potentially end up failing when somebody actually *can* test it" (or some such other copout :-P) (e.g. in my case that used to be qemu and lxc, but for a long time has been only qemu).
I'm not familiar with how libvirt works internally, but to me it seems like one option would be to have a flag set on the drivers that support network filtering. Generic code would then check the flag and fail if filtering is requested with a driver that doesn't support it.
This has the advantage of not requiring changes to each and every driver.
An interesting idea, but then we would really want to do that for every individual XML knob; essentially generic "capabilities flags" similar to the QEMU capabilities flags we use to determine whether a particular feature is available for a particular QEMU binary.
On Wed, Sep 11, 2024 at 09:16:41PM -0400, Laine Stump wrote:
On 9/11/24 7:42 PM, Demi Marie Obenour wrote:
On Wed, Sep 11, 2024 at 05:09:03PM -0600, Jim Fehlig wrote:
On 9/11/24 16:24, Laine Stump wrote:
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
The Xen libxl driver does not support nwfilter. Add a check for nwfilters to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if any are found.
It's generally preferred for drivers to ignore unsupported XML features,
I would instead characterize it as "drivers generally ignore *unrecognized* XML", but it's quite common for a bit of XML that's understood and supported in one context within libvirt to generate an UNSUPPORTED error when attempting to use it in a place where it isn't supported.
but ignoring a user's request to filter VM network traffic can be viewed as a security issue.
Definitely.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0f129ec69c..2f6cebb8ae 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) { + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("filterref is not supported in %1$s"), + virDomainVirtTypeToString(def->virtType)); + return -1; + } +
This more properly should be in a function called libxlValidateDomainDeviceDef(), which would look something like qemuValidateDomainDeviceDef() and be added into libxlDomainDefParserConfig with this initialization:
.deviceValidateCallback = libxlValidateDomainDeviceDef,
Yes, good point! The libxl driver already has domainValidateCallback, but now needs a deviceValidateCallback for this code. I'll make that change in V2.
Before sending another version, I'd like to hear opinions on Demi's question about the other hypervisor drivers. Do they need a similar change?
Now that we're talking more about this, I'm having flashbacks of new features being added in, and the author (e.g. me) basically updating the hypervisor drivers they were personally interested in to support the feature (or else explicitly log an error), but leaving the other drivers untouched because "I can't test it, so I don't want to add code that could potentially end up failing when somebody actually *can* test it" (or some such other copout :-P) (e.g. in my case that used to be qemu and lxc, but for a long time has been only qemu).
In this case I think this should be pretty low-risk, but I would leave that to the authors of the other drivers to decide. For what it is worth, the only other driver I can think of for which filtering could potentially be added is VirtualBox on Linux, unless libvirt wants to interface with whatever Hyper-V, VMware ESXi, VMware on desktop, and other platforms use for their filtering.
I'm not familiar with how libvirt works internally, but to me it seems like one option would be to have a flag set on the drivers that support network filtering. Generic code would then check the flag and fail if filtering is requested with a driver that doesn't support it.
This has the advantage of not requiring changes to each and every driver.
An interesting idea, but then we would really want to do that for every individual XML knob; essentially generic "capabilities flags" similar to the QEMU capabilities flags we use to determine whether a particular feature is available for a particular QEMU binary.
This could be the first, but I also understand if this is out of scope for the current change. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab
On Wed, Sep 11, 2024 at 18:24:07 -0400, Laine Stump wrote:
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
The Xen libxl driver does not support nwfilter. Add a check for nwfilters to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if any are found.
It's generally preferred for drivers to ignore unsupported XML features,
I would instead characterize it as "drivers generally ignore *unrecognized* XML", but it's quite common for a bit of XML that's understood and supported in one context within libvirt to generate an UNSUPPORTED error when attempting to use it in a place where it isn't supported.
but ignoring a user's request to filter VM network traffic can be viewed as a security issue.
Definitely.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0f129ec69c..2f6cebb8ae 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) { + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("filterref is not supported in %1$s"), + virDomainVirtTypeToString(def->virtType)); + return -1; + } +
This more properly should be in a function called libxlValidateDomainDeviceDef(), which would look something like qemuValidateDomainDeviceDef() and be added into libxlDomainDefParserConfig with this initialization:
.deviceValidateCallback = libxlValidateDomainDeviceDef,
(my understanding of the purpose of the two has always been that the PostParse callback is intended for performing post-parse modifications/fixups to the domain def, while the Validate only checks for correctness, without modifying anything. There are multiple cases of having validation in the postparse function, but I think those are the 1) leftovers from before the introduction of the validate callbacks, and 2) the result of misunderstanding and/or sloth (e.g. in cases where you want to validate something, but the driver you're adding this validation to doesn't already have a deviceValidateCallback)
The main difference between the two is that 'postParse' is called on every parse of a XML. That means also for parsing the XMLs of persistently defined domains. If you reject parsing of a XML in a 'postParse' it fails to load the persistent definition so the VM "vanishes", which is something we don't want to do. Thus doing validation in postParse is really valid only when it's a new attribute or configuration that can't exist in "defined" state. In contrast the validate callback is applied in a limited set of XML entrypoints which then don't make the VM to vanish and certain other situations. For that reason the 'Validate' callback/step needs to be re-tried when starting the VM. Other than that, what you've said is correct.
On 9/12/24 01:37, Peter Krempa wrote:
On Wed, Sep 11, 2024 at 18:24:07 -0400, Laine Stump wrote:
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
The Xen libxl driver does not support nwfilter. Add a check for nwfilters to the devicesPostParseCallback, returning VIR_ERR_CONFIG_UNSUPPORTED if any are found.
It's generally preferred for drivers to ignore unsupported XML features,
I would instead characterize it as "drivers generally ignore *unrecognized* XML", but it's quite common for a bit of XML that's understood and supported in one context within libvirt to generate an UNSUPPORTED error when attempting to use it in a place where it isn't supported.
but ignoring a user's request to filter VM network traffic can be viewed as a security issue.
Definitely.
Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- src/libxl/libxl_domain.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/libxl/libxl_domain.c b/src/libxl/libxl_domain.c index 0f129ec69c..2f6cebb8ae 100644 --- a/src/libxl/libxl_domain.c +++ b/src/libxl/libxl_domain.c @@ -131,6 +131,13 @@ libxlDomainDeviceDefPostParse(virDomainDeviceDef *dev, void *opaque G_GNUC_UNUSED, void *parseOpaque G_GNUC_UNUSED) { + if (dev->type == VIR_DOMAIN_DEVICE_NET && dev->data.net->filter) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("filterref is not supported in %1$s"), + virDomainVirtTypeToString(def->virtType)); + return -1; + } +
This more properly should be in a function called libxlValidateDomainDeviceDef(), which would look something like qemuValidateDomainDeviceDef() and be added into libxlDomainDefParserConfig with this initialization:
.deviceValidateCallback = libxlValidateDomainDeviceDef,
(my understanding of the purpose of the two has always been that the PostParse callback is intended for performing post-parse modifications/fixups to the domain def, while the Validate only checks for correctness, without modifying anything. There are multiple cases of having validation in the postparse function, but I think those are the 1) leftovers from before the introduction of the validate callbacks, and 2) the result of misunderstanding and/or sloth (e.g. in cases where you want to validate something, but the driver you're adding this validation to doesn't already have a deviceValidateCallback)
The main difference between the two is that 'postParse' is called on every parse of a XML. That means also for parsing the XMLs of persistently defined domains.
If you reject parsing of a XML in a 'postParse' it fails to load the persistent definition so the VM "vanishes", which is something we don't want to do.
Nod. I had already seen that in my testing. Not nice, but I was prepared to swallow the pill.
Thus doing validation in postParse is really valid only when it's a new attribute or configuration that can't exist in "defined" state.
In contrast the validate callback is applied in a limited set of XML entrypoints which then don't make the VM to vanish and certain other situations. For that reason the 'Validate' callback/step needs to be re-tried when starting the VM.
The validate callback plus a similar check at VM start is much nicer! I'll add that to V2. Thanks! Regards, Jim
On Thu, Sep 12, 2024 at 15:54:04 -0600, Jim Fehlig wrote:
On 9/12/24 01:37, Peter Krempa wrote:
On Wed, Sep 11, 2024 at 18:24:07 -0400, Laine Stump wrote:
On 9/11/24 5:02 PM, Jim Fehlig via Devel wrote:
[...]
Thus doing validation in postParse is really valid only when it's a new attribute or configuration that can't exist in "defined" state.
In contrast the validate callback is applied in a limited set of XML entrypoints which then don't make the VM to vanish and certain other situations. For that reason the 'Validate' callback/step needs to be re-tried when starting the VM.
The validate callback plus a similar check at VM start is much nicer! I'll add that to V2. Thanks!
At startup time the code should simply re-call the validate infrastructure. We do that in the qemu driver so that there's no duplication. I'm not sure whether this was done in libxl, but that would be the proper solution.
On Wed, Sep 11, 2024 at 03:02:40PM -0600, Jim Fehlig wrote:
This is essentially V2 of a small series inspired by a report on the security list about nwfilters not working with Xen VMs. V1 was posted to the security list, so no public reference. The libxl driver simply does not support nwfilters, so the report is really a RFE vs a security issue.
I'm now moving the discussion to the public devel list. I don't have time to add nwfilter support to the libxl driver, but agree the documentation could be improved. Given the perceived security implications, I also think it's worth considering rejecting Xen VM <interface> configuration containing <filterref>, even though libvirt tends to ignore unsupported XML config.
Patch1 improves the documentation. I also considered adding a "Limitations" section to docs/drvxen.rst, but none of the other drivers have such section. Also, for the xen one, I wasn't sure where to start with listing limitations :-P.
Does the Xen driver have a lot of limitations compared to other drivers?
Patch2 rejects Xen VM config containg <filterref> in their <interface> definitions.
Should something similar be added to the other drivers without <filterref> support? I think it would be best if <filterref> was known to all drivers and explicitly rejected by the ones that do not support it.
Jim Fehlig (2): docs: Clarify hypervisor support for nwfilter profiles libxl: Reject VM config referencing nwfilters
docs/formatdomain.rst | 8 ++++---- src/libxl/libxl_domain.c | 7 +++++++ 2 files changed, 11 insertions(+), 4 deletions(-)
-- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab
On 9/11/24 15:49, Demi Marie Obenour wrote:
On Wed, Sep 11, 2024 at 03:02:40PM -0600, Jim Fehlig wrote:
This is essentially V2 of a small series inspired by a report on the security list about nwfilters not working with Xen VMs. V1 was posted to the security list, so no public reference. The libxl driver simply does not support nwfilters, so the report is really a RFE vs a security issue.
I'm now moving the discussion to the public devel list. I don't have time to add nwfilter support to the libxl driver, but agree the documentation could be improved. Given the perceived security implications, I also think it's worth considering rejecting Xen VM <interface> configuration containing <filterref>, even though libvirt tends to ignore unsupported XML config.
Patch1 improves the documentation. I also considered adding a "Limitations" section to docs/drvxen.rst, but none of the other drivers have such section. Also, for the xen one, I wasn't sure where to start with listing limitations :-P.
Does the Xen driver have a lot of limitations compared to other drivers?
Not necessarily. It just feels that way since I'm one of the few contributors, and haven't contributed much in quite a while.
Patch2 rejects Xen VM config containg <filterref> in their <interface> definitions.
Should something similar be added to the other drivers without <filterref> support? I think it would be best if <filterref> was known to all drivers and explicitly rejected by the ones that do not support it.
That's a good point, and might be part of the reason libvirt drivers have traditionally ignored unsupported XML config. Let's see what other maintainers have to say about this patch. Regards, Jim
participants (5)
-
Demi Marie Obenour -
Jim Fehlig -
Laine Stump -
Laine Stump -
Peter Krempa