Validate runtime annotations #902
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/test |
|
Thanks @c3d ! Travis failed on UTs. Please take a look. |
c3d
added
forward-port
c3d
force-pushed
the
bug-v2/launchpad-1878234-access
branch
from
9a023e3
to
669a3d3
Compare
|
@bergwolf Thanks. It looks like the new |
c3d
force-pushed
the
bug-v2/launchpad-1878234-access
branch
2 times, most recently
from
e83748b
to
138c318
Compare
c3d
force-pushed
the
bug-v2/launchpad-1878234-access
branch
from
138c318
to
6751f71
Compare
Paths mentioned in the hypervisor configuration can be overriden using annotations, which is potentially dangerous. For each path, add a 'List' variant that specifies the list of acceptable values from annotations. Bug: https://bugs.launchpad.net/katacontainers.io/+bug/1878234 Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
Sending the virtio_fs_daemon annotation can be used to execute arbitrary code on the host. In order to prevent this, restrict the values of the annotation to a list provided by the configuration file. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
There was an extra 'p' in addHypervisorVirtioFsOverrides. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
The annotation is provided, so it should be respected. Furthermore, it is important to implement it with the appropriate protetions similar to what was done for virtiofsd. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
Replace strange negative logic (!ok -> continue) with positive logic (ok -> do it) Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
The path_list configuration gives a series of regular expressions that limit which values are acceptable through annotations in order to avoid kata launching arbitrary binaries on the host when receiving an annotation. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
The jailer_path annotation can be used to execute arbitrary code on the host. Add a jailer_path_list configuration entry providing a list of regular expressions that can be used to filter annotations that represent valid file names. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
This also adds annotation for ctlpath which were not present before. It's better to implement the code consistenly right now to make sure that we don't end up with a leaky implementation tacked on later. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
Add the following text explaining the risk of using regular expressions in path lists: Each member of the list can be a regular expression, but prefer names. Otherwise, please read and understand the following carefully. SECURITY WARNING: If you use regular expressions, be mindful that an attacker could craft an annotation that uses .. to escape the paths you gave. For example, if your regexp is /bin/qemu.* then if there is a directory named /bin/qemu.d/, then an attacker can pass an annotation containing /bin/qemu.d/../put-any-binary-name-here and attack your host. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
This path could be used to overwrite data on the host. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
This one could theoretically be used to overwrite data on the host. It seems somewhat less risky than the earlier ones for a number of reasons, but worth protecting a little anyway. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
Add variables to override defaults at build time for the various lists used to control path annotations. Fixes: kata-containers#901 Suggested-by: Fabiano Fidencio <fidencio@redhat.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
A comment talking about runtime related annotations describes them as being related to the agent. A similar comment for the agent annotations is missing. Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
When filtering annotations that correspond to paths, e.g. hypervisor.path, it is better to use a glob syntax than a regexp syntax, as it is more usual for paths, and prevents classes of matches that are undesirable in our case, such as matching .. against .* Fixes: kata-containers#901 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
Add a field "enable_annotations" to the runtime configuration that can be used to whitelist annotations using a list of regular expressions, which are used to match any part of the base annotation name, i.e. the part after "io.katacontainers.config.hypervisor." For example, the following configuraiton will match "virtio_fs_daemon", "initrd" and "jailer_path", but not "path" nor "firmware": enable_annotations = [ "virtio.*", "initrd", "_path" ] The default is an empty list of enabled annotations, which disables annotations entirely. If an anontation is rejected, the message is something like: annotation io.katacontainers.config.hypervisor.virtio_fs_daemon is not enabled Fixes: kata-containers#901 Suggested-by: Peng Tao <tao.peng@linux.alibaba.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
When there is a default value from the code (usually empty) that differs from a possible suggested value from the distro, then the wording "default: empty" is confusing. Fixes: kata-containers#901 Suggested-by: Julio Montes <julio.montes@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
|
@bergwolf - any further comments before we merge? |
|
lgtm, thanks @c3d ! |
c3d
added a commit
to c3d/kata-containers
that referenced
this pull request
Oct 19, 2020
In commit 966bd57 for PR kata-containers#902, the makefile was changed to automate the replacement of user variables. However, one variable was treated specially in the original `sed` replacements, namely `RUNTIME_NAME` which was replaced by `$(TARGET)`. This commit adds the `RUNTIME_NAME` variable to the makefile in order to ensure that the replacement works correctly. Fixes: kata-containers#993 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
to c3d/kata-containers
that referenced
this pull request
Oct 19, 2020
In commit 966bd57 for PR kata-containers#902, the makefile was changed to automate the replacement of user variables. However, one variable was treated specially in the original `sed` replacements, namely `RUNTIME_NAME` which was replaced by `$(TARGET)`. This commit adds the `RUNTIME_NAME` variable to the makefile in order to ensure that the replacement works correctly. Fixes: kata-containers#993 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
to c3d/kata-containers
that referenced
this pull request
Oct 19, 2020
In commit 966bd57 for PR kata-containers#902, the makefile was changed to automate the replacement of user variables. However, one variable was treated specially in the original `sed` replacements, namely `RUNTIME_NAME` which was replaced by `$(TARGET)`. This commit adds the `RUNTIME_NAME` variable to the makefile in order to ensure that the replacement works correctly. Fixes: kata-containers#993 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
to c3d/kata-containers
that referenced
this pull request
Oct 28, 2020
Document restricted annotations, as implemented in kata-containers#902 Fixes: kata-containers#1044 Forward-port-of: kata-containers/documentation#755 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
to c3d/kata-containers
that referenced
this pull request
Oct 28, 2020
Document restricted annotations, as implemented in kata-containers#902 Fixes: kata-containers#1044 Forward-port-of: kata-containers/documentation#755 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
to c3d/kata-containers
that referenced
this pull request
Oct 28, 2020
Document restricted annotations, as implemented in kata-containers#902 Fixes: kata-containers#1044 Forward-port-of: kata-containers/documentation#755 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
to c3d/kata-containers
that referenced
this pull request
Nov 9, 2020
Document restricted annotations, as implemented in kata-containers#902 Fixes: kata-containers#1044 Forward-port-of: kata-containers/documentation#755 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
that referenced
this pull request
Apr 12, 2021
In commit 966bd57 for PR #902, the makefile was changed to automate the replacement of user variables. However, one variable was treated specially in the original `sed` replacements, namely `RUNTIME_NAME` which was replaced by `$(TARGET)`. This commit adds the `RUNTIME_NAME` variable to the makefile in order to ensure that the replacement works correctly. Fixes: #993 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
that referenced
this pull request
Apr 12, 2021
Document restricted annotations, as implemented in #902 Fixes: #1044 Forward-port-of: kata-containers/documentation#755 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
to c3d/kata-containers
that referenced
this pull request
Apr 12, 2021
In commit 966bd57 for PR kata-containers#902, the makefile was changed to automate the replacement of user variables. However, one variable was treated specially in the original `sed` replacements, namely `RUNTIME_NAME` which was replaced by `$(TARGET)`. This commit adds the `RUNTIME_NAME` variable to the makefile in order to ensure that the replacement works correctly. Fixes: kata-containers#993 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
c3d
added a commit
to c3d/kata-containers
that referenced
this pull request
Apr 27, 2021
In commit 966bd57 for PR kata-containers#902, the makefile was changed to automate the replacement of user variables. However, one variable was treated specially in the original `sed` replacements, namely `RUNTIME_NAME` which was replaced by `$(TARGET)`. This commit adds the `RUNTIME_NAME` variable to the makefile in order to ensure that the replacement works correctly. Fixes: kata-containers#993 Fixes: kata-containers#1164 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com> (cherry-picked from 96a4ed7)
dgibson
added a commit
to dgibson/kata-containers
that referenced
this pull request
May 28, 2022
…default Since kata-containers#902 the `io.katacontainers.config.hypervisor` pod annotations have only been permitted if explicitly allowed in the global configuration. The default global configuration allows no such annotations. That's important because several of those annotations would cause Kata to execute arbitrary binaries, and so were wildly unsafe. However, this is inconvenient for the `io.katacontainers.config.hypervisor.enable_iommu` annotation specifically, which controls whether the sandbox VM includes a vIOMMU. A guest side vIOMMU is necessary to implement VFIO passthrough devices with `vfio_mode = vfio`, so enabling that mode of operation currently requires a global configuration change, and can't just be enabled per-pod. Unlike some of the other hypervisor annotations, the `enable_iommu` annotation is quite safe. By default the vIOMMU is not present, so allowing a user to override it for a pod only improves their facilities for isolation. Even if the global default were changed to enable the vIOMMU, that doesn't compel the guest kernel to use it, so allowing a user to disable the vIOMMU doesn't materially affect isolation either. Therefore, allow the io.katacontainers.config.hypervisor.enable_iommu annotation to work in the default configurations. fixes kata-containers#4330 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
dgibson
added a commit
to dgibson/kata-containers
that referenced
this pull request
May 28, 2022
Since kata-containers#902 the `io.katacontainers.config.hypervisor` pod annotations have only been permitted if explicitly allowed in the global configuration. The default global configuration allows no such annotations. That's important because several of those annotations would cause Kata to execute arbitrary binaries, and so were wildly unsafe. However, this is inconvenient for the `io.katacontainers.config.hypervisor.enable_iommu` annotation specifically, which controls whether the sandbox VM includes a vIOMMU. A guest side vIOMMU is necessary to implement VFIO passthrough devices with `vfio_mode = vfio`, so enabling that mode of operation currently requires a global configuration change, and can't just be enabled per-pod. Unlike some of the other hypervisor annotations, the `enable_iommu` annotation is quite safe. By default the vIOMMU is not present, so allowing a user to override it for a pod only improves their facilities for isolation. Even if the global default were changed to enable the vIOMMU, that doesn't compel the guest kernel to use it, so allowing a user to disable the vIOMMU doesn't materially affect isolation either. Therefore, allow the io.katacontainers.config.hypervisor.enable_iommu annotation to work in the default configurations. fixes kata-containers#4330 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
dgibson
added a commit
to dgibson/kata-containers
that referenced
this pull request
May 28, 2022
Since kata-containers#902 the `io.katacontainers.config.hypervisor` pod annotations have only been permitted if explicitly allowed in the global configuration. The default global configuration allows no such annotations. That's important because several of those annotations would cause Kata to execute arbitrary binaries, and so were wildly unsafe. However, this is inconvenient for the `io.katacontainers.config.hypervisor.enable_iommu` annotation specifically, which controls whether the sandbox VM includes a vIOMMU. A guest side vIOMMU is necessary to implement VFIO passthrough devices with `vfio_mode = vfio`, so enabling that mode of operation currently requires a global configuration change, and can't just be enabled per-pod. Unlike some of the other hypervisor annotations, the `enable_iommu` annotation is quite safe. By default the vIOMMU is not present, so allowing a user to override it for a pod only improves their facilities for isolation. Even if the global default were changed to enable the vIOMMU, that doesn't compel the guest kernel to use it, so allowing a user to disable the vIOMMU doesn't materially affect isolation either. Therefore, allow the io.katacontainers.config.hypervisor.enable_iommu annotation to work in the default configurations. fixes kata-containers#4330 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
dgibson
added a commit
to dgibson/kata-containers
that referenced
this pull request
May 28, 2022
Since kata-containers#902 the `io.katacontainers.config.hypervisor` pod annotations have only been permitted if explicitly allowed in the global configuration. The default global configuration allows no such annotations. That's important because several of those annotations would cause Kata to execute arbitrary binaries, and so were wildly unsafe. However, this is inconvenient for the `io.katacontainers.config.hypervisor.enable_iommu` annotation specifically, which controls whether the sandbox VM includes a vIOMMU. A guest side vIOMMU is necessary to implement VFIO passthrough devices with `vfio_mode = vfio`, so enabling that mode of operation currently requires a global configuration change, and can't just be enabled per-pod. Unlike some of the other hypervisor annotations, the `enable_iommu` annotation is quite safe. By default the vIOMMU is not present, so allowing a user to override it for a pod only improves their facilities for isolation. Even if the global default were changed to enable the vIOMMU, that doesn't compel the guest kernel to use it, so allowing a user to disable the vIOMMU doesn't materially affect isolation either. Therefore, allow the io.katacontainers.config.hypervisor.enable_iommu annotation to work in the default configurations. fixes kata-containers#4330 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
dgibson
added a commit
to dgibson/kata-containers
that referenced
this pull request
May 30, 2022
Since kata-containers#902 the `io.katacontainers.config.hypervisor` pod annotations have only been permitted if explicitly allowed in the global configuration. The default global configuration allows no such annotations. That's important because several of those annotations would cause Kata to execute arbitrary binaries, and so were wildly unsafe. However, this is inconvenient for the `io.katacontainers.config.hypervisor.enable_iommu` annotation specifically, which controls whether the sandbox VM includes a vIOMMU. A guest side vIOMMU is necessary to implement VFIO passthrough devices with `vfio_mode = vfio`, so enabling that mode of operation currently requires a global configuration change, and can't just be enabled per-pod. Unlike some of the other hypervisor annotations, the `enable_iommu` annotation is quite safe. By default the vIOMMU is not present, so allowing a user to override it for a pod only improves their facilities for isolation. Even if the global default were changed to enable the vIOMMU, that doesn't compel the guest kernel to use it, so allowing a user to disable the vIOMMU doesn't materially affect isolation either. Therefore, allow the io.katacontainers.config.hypervisor.enable_iommu annotation to work in the default configurations. fixes kata-containers#4330 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
dgibson
added a commit
to dgibson/kata-containers
that referenced
this pull request
May 31, 2022
Since kata-containers#902 the `io.katacontainers.config.hypervisor` pod annotations have only been permitted if explicitly allowed in the global configuration. The default global configuration allows no such annotations. That's important because several of those annotations would cause Kata to execute arbitrary binaries, and so were wildly unsafe. However, this is inconvenient for the `io.katacontainers.config.hypervisor.enable_iommu` annotation specifically, which controls whether the sandbox VM includes a vIOMMU. A guest side vIOMMU is necessary to implement VFIO passthrough devices with `vfio_mode = vfio`, so enabling that mode of operation currently requires a global configuration change, and can't just be enabled per-pod. Unlike some of the other hypervisor annotations, the `enable_iommu` annotation is quite safe. By default the vIOMMU is not present, so allowing a user to override it for a pod only improves their facilities for isolation. Even if the global default were changed to enable the vIOMMU, that doesn't compel the guest kernel to use it, so allowing a user to disable the vIOMMU doesn't materially affect isolation either. Therefore, allow the io.katacontainers.config.hypervisor.enable_iommu annotation to work in the default configurations. fixes kata-containers#4330 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
dgibson
added a commit
to dgibson/kata-containers
that referenced
this pull request
Jun 4, 2022
Since kata-containers#902 the `io.katacontainers.config.hypervisor` pod annotations have only been permitted if explicitly allowed in the global configuration. The default global configuration allows no such annotations. That's important because several of those annotations would cause Kata to execute arbitrary binaries, and so were wildly unsafe. However, this is inconvenient for the `io.katacontainers.config.hypervisor.enable_iommu` annotation specifically, which controls whether the sandbox VM includes a vIOMMU. A guest side vIOMMU is necessary to implement VFIO passthrough devices with `vfio_mode = vfio`, so enabling that mode of operation currently requires a global configuration change, and can't just be enabled per-pod. Unlike some of the other hypervisor annotations, the `enable_iommu` annotation is quite safe. By default the vIOMMU is not present, so allowing a user to override it for a pod only improves their facilities for isolation. Even if the global default were changed to enable the vIOMMU, that doesn't compel the guest kernel to use it, so allowing a user to disable the vIOMMU doesn't materially affect isolation either. Therefore, allow the io.katacontainers.config.hypervisor.enable_iommu annotation to work in the default configurations. fixes kata-containers#4330 Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes #901 and https://bugs.launchpad.net/katacontainers.io/+bug/1878234.
Need to add a link to code reviews once this is merged.