factory: fix FIFO permissions

[cyphar]

Fix overly permissive FIFO permissions for the start process. Since
we're chown-ing the FIFO we can deal with both userns and full root
containers (as well as rootless containers). The two cases are:

1. We are in a userns. The waiting side (r) owns the file and so doesn't
   need any additional group or other permissions. The "start" end does
   have root privileges (or has the same UID as the .HostUID()) and thus
   has the right permissions to start (w) the container.
2. We are root. In which case anything goes and the permissions don't
   actually matter.

However, by setting the FIFO to 0622, any user on the system could start
any other users container (including full root containers). This is
clearly a potential security hole (and violates least-suprise as well as
breaking the semantics of the signal-based create-start split that
actually had sane ACLs built into it).

This further fixes #912 and #886.

Signed-off-by: Aleksa Sarai [email protected]

[LK4D4]

LGTM

[cyphar]

@crosbymichael This is what I was referring to with the permissions stuff. PTAL.

[crosbymichael]

This change does not work:

standard_init_linux.go:164: openat exec fifo caused "permission denied"

[cyphar]

How are you running it? I just tried it with my rootless containers patchset, and it works (ignore the broken terminal). The config is the default generated one by runc spec --rootless:

% sudo ./runc create test
% sudo ./runc start test
sh: cannot set terminal process group (-1): Inappropriate ioctl for device
sh: no job control in this shell
sh-4.3# 

[crosbymichael]

here is the config.

The uid,gid 1:1 is the trick to making this fail

{
    "ociVersion": "0.5.0-dev",
    "platform": {
        "os": "linux",
        "arch": "amd64"
    },
    "process": {
        "terminal": false,
        "args": [
            "sleep", "30"
        ],
        "user": {"uid":1, "gid": 1},
        "env": [
                "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
                "TERM=xterm"
            ],
            "cwd": "/",
            "capabilities": [
                "CAP_AUDIT_WRITE",
                "CAP_KILL",
                "CAP_NET_BIND_SERVICE"
            ],
        "rlimits": [
            {
                "type": "RLIMIT_NOFILE",
                "hard": 1024,
                "soft": 1024
            }
        ],
        "noNewPrivileges": true
    },
        "annotations": {
        "test": "test"
        },
    "root": {
        "path": "rootfs",
        "readonly": false
    },
    "hostname": "runc",
    "mounts": [
        {
            "destination": "/proc",
            "type": "proc",
            "source": "proc"
        },
        {
            "destination": "/dev",
            "type": "tmpfs",
            "source": "tmpfs",
            "options": [
                "nosuid",
                "strictatime",
                "mode=755",
                "size=65536k"
            ]
        },
        {
            "destination": "/dev/pts",
            "type": "devpts",
            "source": "devpts",
            "options": [
                "nosuid",
                "noexec",
                "newinstance",
                "ptmxmode=0666",
                "mode=0620",
                "gid=5"
            ]
        },
        {
            "destination": "/dev/shm",
            "type": "tmpfs",
            "source": "shm",
            "options": [
                "nosuid",
                "noexec",
                "nodev",
                "mode=1777",
                "size=65536k"
            ]
        }
    ],
    "hooks": {},
    "linux": {
        "resources": {
            "memory": {
                "limit": 1000000
            },
            "devices": [
                {
                    "allow": false,
                    "access": "rwm"
                }
            ]
        },
        "namespaces": [
            {
                "type": "pid"
            },
            {
                "type": "ipc"
            },
            {
                "type": "uts"
            },
            {
                "type": "mount"
            }
        ],
        "maskedPaths": [
            "/proc/kcore",
            "/proc/latency_stats",
            "/proc/timer_stats",
            "/proc/sched_debug"
        ],
        "readonlyPaths": [
            "/proc/asound",
            "/proc/bus",
            "/proc/fs",
            "/proc/irq",
            "/proc/sys",
            "/proc/sysrq-trigger"
        ]
 }
}

[cyphar]

Oh, it's because you've set uid and gid to be a different user. Right, so the problem is that we shouldn't change the global ACL for running containers because of this -- all we need to do is open the file before we setupUser (though this is slightly more complicated than I would like).

While we could in principle set the directory permission to 722 this would result in people being able to create other files there. We could try doing something like linkat(2) but then you have the exact same permission issue as before.

Also we need to add a test where we set uid and gid in the spec.

[crosbymichael]

I think we have resolved these issues with the userns changes for non root containers. Going to close for now.