Re: [Xen-devel] [PATCH v2 4/6] tools/dm_restrict: Unshare mount and IPC namespaces on Linux

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 09/24/2018 11:40 AM, Ian Jackson wrote:
> George Dunlap writes ("[PATCH v2 4/6] tools/dm_restrict: Unshare mount and 
> IPC namespaces on Linux"):
>> QEMU running under Xen doesn't need mount or IPC functionality.
>> Create and enter separate namespaces for each of these before
>> executing QEMU, so that in the event that other restrictions fail, the
>> process won't be able to even name system mount points or exsting
>> non-file-based IPC descriptors to attempt to attack them.
> 
>> Unsharing is something a process can only do to itself (it would
>> seem); so add an os-specific "dm_preexec_restrict()" hook just before
>> we exec() the device model.
> ...
>> +
>> +void libxl__local_dm_preexec_restrict(libxl__gc *gc, int stderrfd)
>> +{
>> +    return;
>> +}
> 
> This patch is the point of divergence between Linux dom0's and FreeBSD
> dom0's.
> 
> IMO it's not good enough to simply talk about some restrictions as
> being Linux-only.  Rather, the restrictions are supposed to work
> together as a whole, and the whole syscall API available to qemu needs
> to be considered.  That would need to be done separately for FreeBSD.
> 
> I think that this means we should explicitly write down that the qemu
> depriv implementation is incomplete on FreeBSD.

I think theoretically, an unprivileged user locked in a chroot owned by
root, and with the xenstore fds properly restricted, should be enough to
prevent a guest from gaining control or reading data that it shouldn't
be able to read.  All of the other restrictions are to add extra layers,
in case that first layer should have a bug.

That is, our unprivileged process shouldn't be *able* to call mount;
but, *just in case*, we'll put it in a separate mount space, so that it
can't mount proc or sysfs or whatever.  Our unprivileged process
shouldn't be able to use non-file-based IPC to break into other
processes on the system; but just in case, we'll put it in its own IPC
namespace.  Our unpriveleged process shouldn't be *able* to cause any
mischief with the reboot or readdir system calls, but let's restrict
those with `-sandbox` anyway, just in case.

`-runas` and `-chroot` should work just fine on FreeBSD; and if the
restrict IOCTL is implemented, then that should be enough to say, "At a
basic level this works".  We probably do want someone familiar with
FreeBSD to go through and implement whatever can be implemented there as
far as additional restrictions.

> 
>> +/* 
>> + * Called after forking but before executing the local devicemodel.
>> + * Must call _exit(-1) on failure, printing an error message to
>> + * stderrfd if available.
> 
> IMO it would be better if the single call site handled the errors.  I
> think that can be done in a standard way.  So this function should
> probably be expected to call a LOG macro and then return ERROR_FAIL,
> on failure.

Ack

>> +void libxl__local_dm_preexec_restrict(libxl__gc *gc, int stderrfd)
>> +{
>> +    int rc;
>> +
>> +    /* Unshare mount and IPC namespaces.  These are unused by QEMU. */
>> +    rc = unshare(CLONE_NEWNS | CLONE_NEWIPC);
> 
> Variable name should be r.  rc is used for libxl error codes.
> See tools/libxl/CODING_STYLE.

Yes, sorry I forgot about this. Ack (to this and all similar reminders).

>> +# TEST: Namespace unsharing
>> +#
>> +# Read /proc/<dmpid>/ns/<namespace> and make sure it's not equal to
>> +# the current processes' value
>> +for nsname in ipc mnt; do
>> +    echo -n "Unshare namespace $nsname: "
>> +    dmns=$(readlink /proc/$dmpid/ns/$nsname)
>> +    myns=$(readlink /proc/self/ns/$nsname)
>> +
>> +    if [[ "$dmns" == "$myns" ]] ; then
>> +    echo "FAILED"
>> +    failed="true"
>> +    else
>> +    echo "PASSED"
>> +    fi
>> +done
> 
> I wonder if maybe this output should be in subunit v1 format.

We could do that.

It doesn't look like the "official" subunit code [1] accumulates the
failure and returns it as the status code of the script as a whole.  Is
that something that's valuable, do you think?  Or should we have `status
= OK` mean, "The script itself ran without errors, check the output for
any test failures"?

 -George

[1]
https://github.com/testing-cabal/subunit/blob/master/shell/share/subunit.sh

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel