Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate runtime annotations #902

Merged
merged 24 commits into from Oct 16, 2020
Merged

Validate runtime annotations #902

merged 24 commits into from Oct 16, 2020

Commits on Oct 14, 2020

  1. config: Add 'List' alternates for hypervisor configuration paths 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    8c75de1 View commit details
    Browse the repository at this point in the history

  2. config: Protect virtio_fs_daemon annotation 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    bf13ff0 View commit details
    Browse the repository at this point in the history

  3. config: Fix typo in function name 

    There was an extra 'p' in addHypervisorVirtioFsOverrides.

Fixes: kata-containers#901

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    2e093df View commit details
    Browse the repository at this point in the history

  4. config: Add hypervisor path override through annotations 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    2ca9ca8 View commit details
    Browse the repository at this point in the history

  5. annotations: Simplify negative logic 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    2d431c6 View commit details
    Browse the repository at this point in the history

  6. config: Add examples for path_list configuration 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    0766901 View commit details
    Browse the repository at this point in the history

  7. config: Protect jailer_path annotation 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    27b6620 View commit details
    Browse the repository at this point in the history

  8. config: Protect ctlpath from annotation attack 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    b21a829 View commit details
    Browse the repository at this point in the history

  9. config: Add security warning on configuration examples 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    5588165 View commit details
    Browse the repository at this point in the history

  10. config: Protect vhost_user_store_path against annotation attacks 

    This path could be used to overwrite data on the host.

Fixes: kata-containers#901

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    aae9656 View commit details
    Browse the repository at this point in the history

  11. config: Protect file_mem_backend against annotation attacks 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    4e89b88 View commit details
    Browse the repository at this point in the history

  12. config: Add makefile variables for path lists 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    c16cdcb View commit details
    Browse the repository at this point in the history

  13. annotations: Fix typo in comment 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    11b9c90 View commit details
    Browse the repository at this point in the history

  14. config: Use glob instead of regexp to match paths in annotations 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    f047fce View commit details
    Browse the repository at this point in the history

  15. config: Whitelist hypervisor annotations by name 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    7c6aede View commit details
    Browse the repository at this point in the history

  16. config: Add better comments in the template files 

    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>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    d65a7d1 View commit details
    Browse the repository at this point in the history

  17. annotations: Rename checkPathIsInGlobList with checkPathIsInGlobs 

    The name is shorter and more specific

Fixes: kata-containers#901

Suggested-by: James O.D. Hunt <james.o.hunt@intel.com>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    b5db114 View commit details
    Browse the repository at this point in the history

  18. annotations: Give better names to local variabes in search functions 

    Use more meaningful variable names for clarity.

Fixes: kata-containers#901

Suggested-by: James O.D. Hunt james.o.hunt@intel.com>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    b119427 View commit details
    Browse the repository at this point in the history

  19. makefile: Improve names of config entries for annotation checks 

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

Suggested-by: James O.D. Hunt <james.o.hunt@intel.com>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    be6ee25 View commit details
    Browse the repository at this point in the history

  20. makefile: Add missing generated vars to USER_VARS 

    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.

This is a forward port of a makefile fix included in
PR kata-containers/runtime#3004
for issue kata-containers/runtime#2943

Fixes: kata-containers#901

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    966bd57 View commit details
    Browse the repository at this point in the history

  21. annotations: Add unit test for regexpContains function 

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

Suggested-by: James O.D. Hunt <james.o.hunt@intel.com>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    6f52179 View commit details
    Browse the repository at this point in the history

  22. annotations: Add unit test for checkPathIsInGlobs 

    There are a few interesting corner cases to consider for this
function.

Fixes: kata-containers#901

Suggested-by: James O.D. Hunt <james.o.hunt@intel.com>
Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    b2b3bc7 View commit details
    Browse the repository at this point in the history

  23. annotations: Split addHypervisorOverrides to reduce complexity 

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

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    398d791 View commit details
    Browse the repository at this point in the history

  24. annotations: Correct unit tests to validate new protections 

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

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
    @c3d
    c3d committed Oct 14, 2020
    Configuration menu
    Copy the full SHA
    c5771be View commit details
    Browse the repository at this point in the history