Validate annotations that refer to binaries #3005

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#3004 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#3004 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

There was an extra 'p' in addHypervisorVirtioFsOverrides. Fixes: kata-containers#3004 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#3004 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

Replace strange negative logic (!ok -> continue) with positive logic (ok -> do it) Fixes: kata-containers#3004 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#3004 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#3004 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#3004 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#3004 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

This path could be used to overwrite data on the host. Fixes: kata-containers#3004 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#3004 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#3004 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#3004 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#3004 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#3004 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#3004 Suggested-by: Julio Montes <julio.montes@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

The name is shorter and more specific Fixes: kata-containers#3004 Suggested-by: James O.D. Hunt <james.o.hunt@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

Use more meaningful variable names for clarity. Fixes: kata-containers#3004 Suggested-by: James O.D. Hunt james.o.hunt@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

The entries used to be things like PATH_LIST, which are too generic. Replace them with more precise name with a distinguishing keyword, namely VALID. For example valid_hypervisor_paths. Fixes: kata-containers#3004 Suggested-by: James O.D. Hunt <james.o.hunt@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

This was discovered while checking a massive change in variables. The root cause for the error is a very long list of manual replacements, that is best replaced with a $(foreach). All individual variables in the output configuration files were checked against the old build using diff. Fixes: kata-containers#2943 Fixes: kata-containers#3004 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

The build was setting a `FCVALIDPATHS` variable for firecracker, but that was never being used. Conversely, the firecracker configuration template was expecting a `FCVALIDHYPERVISORPATHS`, but that variable was never being set. Resolve by only setting the `FCVALIDHYPERVISORPATHS` variable to ensure the generated firecracker config is valid once again. Fixes: kata-containers#3005 From kata-containers/kata-containers#1002 Fixing kata-containers/kata-containers#1001 Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

James O.D Hunt: "But also, regexpContains() and checkPathIsInGlobList() seem like good candidates for some unit tests. The "look" obvious, but a few boundary condition tests would be useful I think (filenames with spaces, backslashes, special characters, and relative & absolute paths are also an interesting thought here)." There aren't that many boundary conditions on a list with regexps, if you assume the regexp match function itself works. However, the tests is useful in documenting expectations. Fixes: kata-containers#3004 Suggested-by: James O.D. Hunt <james.o.hunt@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

There are a few interesting corner cases to consider for this function. Fixes: kata-containers#3004 Suggested-by: James O.D. Hunt <james.o.hunt@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

Warning from gocyclo during make check: virtcontainers/pkg/oci/utils.go:404:1: cyclomatic complexity 37 of func `addHypervisorConfigOverrides` is high (> 30) (gocyclo) func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, runtime RuntimeConfig) error { ^ Fixes: kata-containers#3004 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

Add the verification of some basic protections, namely that: - EnableAnnotations is honored - Dangerous paths cannot be modified if no match - Errors are returned when expected Fixes: kata-containers#3004 Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

During review, Julio Montes pointed out that there was some missing text in the comments for configuration-xyz.toml.in relative to the virtiofs version. Fixes: kata-containers#3005 Suggested-by: Julio Montes <julio.montes@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

During review, Julio Montes found the term "annotation value" ambiguous. Specify each time that this is a path. Also fixed a grammatical error (annotations value -> annotation value) Suggested-by: Julio Montes <julio.montes@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

This was suggested for consistency in other code. For consistency within the file, this also changes the existing usage in that file, where `func SandboxConfig` used `runtime` for a long time. Suggested-by: Archana Shinde <archana.m.shinde@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

With the annotation filtering infrastructure in place, annotations are filtered as defined by the `enable_annotations` setting. In order to not cause regressions from the earlier versions of the code, accept all hypervisor annotations by default. Suggested-by: Archana Shinde <archana.m.shinde@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

The changes introduced in PR kata-containers#3031 require additional changes. Code lifted from kata-containers/kata-containers#1086. Fixes: kata-containers#3005 Suggested-by: James O.D. Hunt <james.o.hunt@intel.com> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate annotations that refer to binaries #3005

Validate annotations that refer to binaries #3005

Commits on Nov 10, 2020