Limiting HostConfig.Binds and HostConfig.Mounts is extremely easy to defeat

[bviktor]

Consider this policy:

package docker.authz

default allow = false

allow
{
    valid_bind_mapping_whitelist = true
    valid_mount_mapping_whitelist = true
}

# prohibit access to the host file system outside /home
# which would essentially grant root privileges to the user

valid_host_path_prefixes = {"home/"}

# binds
# `docker run -v /:/host-root`

host_bind_paths[trimmed]
{
    input.Body.HostConfig.Binds[_] = bind
    split(bind, ":", parts)
    trim(parts[0], "/", trimmed)
}

valid_host_bind_paths[host_path]
{
    host_bind_paths[host_path]
    startswith(host_path, valid_host_path_prefixes[_])
}

valid_bind_mapping_whitelist
{
    invalid_paths = host_bind_paths - valid_host_bind_paths
    count(invalid_paths, 0)
}

# bind mounts
# `docker run --mount type=bind,source=/,target=/host-root`

host_mount_paths[trimmed]
{
    input.Body.HostConfig.Mounts[_] = mount
    trim(mount.Source, "/", trimmed)
}

valid_host_mount_paths[host_path]
{
    host_mount_paths[host_path]
    startswith(host_path, valid_host_path_prefixes[_])
}

valid_mount_mapping_whitelist
{
    invalid_paths = host_mount_paths - valid_host_mount_paths
    count(invalid_paths, 0)
}

Now run this command:

$ docker run -it --mount type=bind,source=/,target=/host-root ubuntu bash
docker: Error response from daemon: authorization denied by plugin openpolicyagent/opa-docker-authz-v2:0.5: request rejected by administrative policy.
See 'docker run --help'.

All is well. Right? No, not quite.

$ mkdir ~/root
$ ln -s / ~/root
$ docker run -it --mount type=bind,source=/home/bviktor/root,target=/host-root ubuntu bash
root@d6dbce5be851:/# ls host-root/
bin   dev  home        initrd.img.old  lib32  lost+found  mnt  proc  run   snap  sys  usr  vmlinuz
boot  etc  initrd.img  lib             lib64  media       opt  root  sbin  srv   tmp  var  vmlinuz.old

Wow, just wow.

So, the solution seems obvious: dereference all paths defined on the command-line before running the AuthZ checks on them.

[bviktor]

A patch like this might suffice:

diff --git a/vendor/github.com/open-policy-agent/opa/storage/path.go b/vendor/github.com/open-policy-agent/opa/storage/path.go
index 5379bbd..3c495d0 100644
--- a/vendor/github.com/open-policy-agent/opa/storage/path.go
+++ b/vendor/github.com/open-policy-agent/opa/storage/path.go
@@ -9,6 +9,7 @@ import (
        "net/url"
        "strconv"
        "strings"
+       "path/filepath"
 
        "github.com/open-policy-agent/opa/ast"
 )
@@ -17,7 +18,8 @@ import (
 type Path []string
 
 // ParsePath returns a new path for the given str.
-func ParsePath(str string) (path Path, ok bool) {
+func ParsePath(rawStr string) (path Path, ok bool) {
+       str, _ := filepath.EvalSymlinks(rawStr)
        if len(str) == 0 {
                return nil, false
        }

But I'm unable to test it. If I simply replace the built file, docker complains it can't validate the file. If I try to install from local file, it says invalid reference format... Anyone?

[bviktor]

Any update? It'd be great to add a simple check if the target is symlink or not. As of now, this omission renders the plugin quite pointless :(

[anderseknert]

@bviktor I've been working some on this plugin for the past couple of days, and I stumbled on this issue now. Do I get it right that in your example, the Source attribute here is a symlink to something else entirely, and since there's no way of determining that from from inside of your policy, you'd want to have that check added before it's passed to OPA for evaluation?

"Mounts": [
          {
            "Source": "/home/ubuntu/root",
            "Target": "/host-root",
            "Type": "bind"
          }
]

I don't really understand the suggested patch though. Would this not be better fixed in the plugin than in OPA?

[flixr]

@anderseknert I have the same problem.
If we could resolve symlinks for absolute paths given in input.Body.HostConfig.Binds and input.Body.HostConfig.Mounts[_].Source, then we could check the actual path in the policy file.
From what I understand, this could work if we e.g. put the Binds and Mounts into extra fields like e.g. input.Binds and input.Mounts and call filepath.EvalSymlinks on those entries that are absolute paths.
Then in the policy file one could check input.Binds instead of the non-resolved input.Body.HostConfig.Binds

[flixr]

How about something like this (non-functional):

        var bindMounts []string
	for _, b := range body["HostConfig"]["Binds"] {
		if strings.HasPrefix(b, "/") {
			abs, _ := filepath.EvalSymlinks(b)
			bindMounts = append(bindMounts, abs)
		}
	}

	input := map[string]interface{}{
		"Headers":    r.RequestHeaders,
		"Path":       r.RequestURI,
		"PathPlain":  u.Path,
		"PathArr":    strings.Split(u.Path, "/"),
		"Query":      u.Query(),
		"Method":     r.RequestMethod,
		"Body":       body,
		"User":       r.User,
		"AuthMethod": r.UserAuthNMethod,
		"BindMounts": bindMounts,
	}

I'm new to go, so this is not correct, you can't access the body.HostConfig.Binds like that...
Not sure how to access this properly, but IMHO something like this should do the job here in principle.
Suggestions welcome!

[anderseknert]

Thanks @flixr! I'll take a shot at this, we'll see how it goes :)

[flixr]

I started this in #61, hopefully will have some more time to test it this week...

[anderseknert]

Sure! Let me know how it goes and if you need some help later 👍

[bviktor]

@bviktor I've been working some on this plugin for the past couple of days, and I stumbled on this issue now. Do I get it right that in your example, the Source attribute here is a symlink to something else entirely, and since there's no way of determining that from from inside of your policy, you'd want to have that check added before it's passed to OPA for evaluation?

"Mounts": [
          {
            "Source": "/home/ubuntu/root",
            "Target": "/host-root",
            "Type": "bind"
          }
]

I don't really understand the suggested patch though. Would this not be better fixed in the plugin than in OPA?

Hi,

Yes. You restrict bind mounts to say, /home/, then the user can just ln -s / /home/jane/foo and that's it, the protection is circumvented already.

Nevermind the patch, I realized since then that it's completely not the way to do it.

In the meanwhile, it also came up that this interferes with volumes in docker-compose too, due to the ........ idiotic way Docker does things. If you start Docker from the CLI, this is the Binds section:

"Binds":["/home/bviktor/data:/data"]

This is after a docker-compose run with volumes:

"Binds":["test_shared_vol:/home/bviktor/data:rw"]

Yes, you read that right. The structure is different. In the first case its source:target, in the second case it's name:source:target. Good luck parsing that, especially with rego...

This is my current policy file, which already deals with simple volumes, like:

docker run --rm -it -v $(docker volume create):/foo ubuntu:18.04

But not with docker-compose. Ouch.

---

For the record, this is the full docker JSON after running a container with a volume attached:

{
  "AttachStderr":false,
  "AttachStdin":false,
  "AttachStdout":false,
  "HostConfig":{
    "Binds":[
      "test_shared_vol:/home/bviktor/data:rw"
    ],
    "Links":[

    ],
    "LogConfig":{
      "Config":{

      },
      "Type":""
    },
    "NetworkMode":"test_default",
    "PortBindings":{

    },
    "VolumesFrom":[

    ]
  },
  "Image":"alpine:latest",
  "Labels":{
    "com.docker.compose.config-hash":"e2adc96bf2a08494c66de013b9e8be8d0a651ad9d06ec3e0b112da12876de9b0",
    "com.docker.compose.container-number":"1",
    "com.docker.compose.oneoff":"False",
    "com.docker.compose.project":"test",
    "com.docker.compose.project.config_files":"docker-compose.yml",
    "com.docker.compose.project.working_dir":"/home/bviktor/Downloads/test",
    "com.docker.compose.service":"myapp",
    "com.docker.compose.version":"1.29.2"
  },
  "NetworkDisabled":false,
  "NetworkingConfig":{
    "EndpointsConfig":{
      "test_default":{
        "Aliases":[
          "myapp"
        ],
        "IPAMConfig":{

        }
      }
    }
  },
  "OpenStdin":false,
  "StdinOnce":false,
  "Tty":false,
  "Volumes":{
    "/home/bviktor/data":{

    }
  }
}

[flixr]

@bviktor see #61 which will only put absolute source paths (so not named volumes) in BindMounts after resolving them...

[bviktor]

Yes I see, one step at a time :) Still an awesome improvement! I'm really grateful for all the work guys. I'll prolly have to open separate tickets for all the quirks I just mentioned :)

[anderseknert]

@bviktor please take a look at the work done by @mdcurtis in #65

I do believe this should resolve this issue, at least to the extent possible. WDYT?