Validate annotations that refer to binaries #3004

c3d · 2020-10-09T16:06:08Z

The description of the problem is in https://bugs.launchpad.net/katacontainers.io/+bug/1878234.

This issue is a placeholder to allow the fixes to be submitted.

It also contains a fix for #2943

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>

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>