[lxc-devel] [PATCH RFC] apparmor: improve behavior when kernel lacks mount restrictions
Dwight Engen
dwight.engen at oracle.com
Sat Sep 20 01:25:15 UTC 2014
On Sun, 14 Sep 2014 03:49:32 +0000
Serge Hallyn <serge.hallyn at ubuntu.com> wrote:
> (If we go this route we can also drop the whole lsm_label_get()
> method and the lsm_label field in the attach context info)
>
> Apparmor policies require mount restrictions to fullfill many of
> their promises - for instance if proc can be mounted anywhere,
> then 'deny /proc/sysrq-trigger w' prevents only accidents, not
> malice.
>
> The mount restrictions are not available in the upstream kernel.
> We can detect their presence through /sys. In the past, when
> we detected it missing, we would not enable apparmor. But that
> prevents apparmor from helping to prevent accidents.
>
> At the same time, if the user accidentaly boots a kernel which
> has regressed, we do not want them starting the container thinking
> they are more protected than they are.
>
> This patch:
>
> 1. adds a lxc.aa_allow_incomplete = 1 container config flag. If
> not set, then any container which is not set to run unconfined
> will refuse to run. If set, then the container will run with
> apparmor protection.
>
> 2. to pass this flag to the apparmor driver, we pass the container
> configuration (lxc_conf) to the lsm_label_set hook, rather than
> just the label.
Hey Serge, it looks good to me though I admit I did not read the test
case code. Attach is the tricky case, especially where the caller is
say the python API and just wants to attach and run some python code
(ie. the LXC_ATTACH_LSM_NOW) case. Anyway, I think what you have should
not effect that.
Acked-by: Dwight Engen <dwight.engen at oracle.com>
> 3. add a testcase. To test the case were a kernel does not
> provide mount restrictions, we mount an empty directory over
> the /sys/kernel/security/apparmor/features/mount directory. In
> order to have that not be unmounted in a new namespace, we must
> test using unprivileged containers (who cannot remove bind mounts
> which hide existing mount contents).
>
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
> ---
> src/lxc/attach.c | 2 +-
> src/lxc/conf.h | 1 +
> src/lxc/confile.c | 14 +++
> src/lxc/lsm/apparmor.c | 32 +++++--
> src/lxc/lsm/lsm.c | 4 +-
> src/lxc/lsm/lsm.h | 8 +-
> src/lxc/lsm/nop.c | 2 +-
> src/lxc/lsm/selinux.c | 6 +-
> src/lxc/start.c | 7 +-
> src/tests/Makefile.am | 1 +
> src/tests/lxc-test-apparmor-mount | 189
> ++++++++++++++++++++++++++++++++++++++ 11 files changed, 245
> insertions(+), 21 deletions(-) create mode 100755
> src/tests/lxc-test-apparmor-mount
>
> diff --git a/src/lxc/attach.c b/src/lxc/attach.c
> index 923c497..9d92240 100644
> --- a/src/lxc/attach.c
> +++ b/src/lxc/attach.c
> @@ -1043,7 +1043,7 @@ static int attach_child_main(void* data)
> int on_exec;
>
> on_exec = options->attach_flags &
> LXC_ATTACH_LSM_EXEC ? 1 : 0;
> - ret = lsm_process_label_set(init_ctx->lsm_label, 0,
> on_exec);
> + ret =
> lsm_process_label_set(init_ctx->container->lxc_conf, 0, on_exec); if
> (ret < 0) { rexit(-1);
> }
> diff --git a/src/lxc/conf.h b/src/lxc/conf.h
> index df66f99..97d9f91 100644
> --- a/src/lxc/conf.h
> +++ b/src/lxc/conf.h
> @@ -311,6 +311,7 @@ struct lxc_conf {
> struct lxc_list hooks[NUM_LXC_HOOKS];
>
> char *lsm_aa_profile;
> + int lsm_aa_allow_incomplete;
> char *lsm_se_context;
> int tmp_umount_proc;
> char *seccomp; // filename with the seccomp rules
> diff --git a/src/lxc/confile.c b/src/lxc/confile.c
> index 5de1241..af55feb 100644
> --- a/src/lxc/confile.c
> +++ b/src/lxc/confile.c
> @@ -58,6 +58,7 @@ static int config_tty(const char *, const char *,
> struct lxc_conf *); static int config_ttydir(const char *, const char
> *, struct lxc_conf *); static int config_kmsg(const char *, const
> char *, struct lxc_conf *); static int config_lsm_aa_profile(const
> char *, const char *, struct lxc_conf *); +static int
> config_lsm_aa_incomplete(const char *, const char *, struct lxc_conf
> *); static int config_lsm_se_context(const char *, const char *,
> struct lxc_conf *); static int config_cgroup(const char *, const char
> *, struct lxc_conf *); static int config_idmap(const char *, const
> char *, struct lxc_conf *); @@ -108,6 +109,7 @@ static struct
> lxc_config_t config[] = { { "lxc.devttydir",
> config_ttydir }, { "lxc.kmsg",
> config_kmsg }, { "lxc.aa_profile",
> config_lsm_aa_profile },
> + { "lxc.aa_allow_incomplete", config_lsm_aa_incomplete },
> { "lxc.se_context", config_lsm_se_context },
> { "lxc.cgroup", config_cgroup },
> { "lxc.id_map", config_idmap },
> @@ -1145,6 +1147,16 @@ static int config_lsm_aa_profile(const char
> *key, const char *value, return
> config_string_item(&lxc_conf->lsm_aa_profile, value); }
>
> +static int config_lsm_aa_incomplete(const char *key, const char
> *value,
> + struct lxc_conf *lxc_conf)
> +{
> + int v = atoi(value);
> +
> + lxc_conf->lsm_aa_allow_incomplete = v == 1 ? 1 : 0;
> +
> + return 0;
> +}
> +
> static int config_lsm_se_context(const char *key, const char *value,
> struct lxc_conf *lxc_conf)
> {
> @@ -2257,6 +2269,8 @@ int lxc_get_config_item(struct lxc_conf *c,
> const char *key, char *retv, return lxc_get_arch_entry(c, retv,
> inlen); else if (strcmp(key, "lxc.aa_profile") == 0)
> v = c->lsm_aa_profile;
> + else if (strcmp(key, "lxc.aa_allow_incomplete") == 0)
> + return lxc_get_conf_int(c, retv, inlen,
> c->lsm_aa_allow_incomplete); else if (strcmp(key, "lxc.se_context")
> == 0) v = c->lsm_se_context;
> else if (strcmp(key, "lxc.logfile") == 0)
> diff --git a/src/lxc/lsm/apparmor.c b/src/lxc/lsm/apparmor.c
> index f4c8d26..3e98a38 100644
> --- a/src/lxc/lsm/apparmor.c
> +++ b/src/lxc/lsm/apparmor.c
> @@ -29,6 +29,7 @@
>
> #include "log.h"
> #include "lsm/lsm.h"
> +#include "conf.h"
>
> lxc_log_define(lxc_apparmor, lxc);
>
> @@ -39,17 +40,23 @@ static int aa_enabled = 0;
> #define AA_MOUNT_RESTR
> "/sys/kernel/security/apparmor/features/mount/mask" #define
> AA_ENABLED_FILE "/sys/module/apparmor/parameters/enabled"
> +static int mount_feature_enabled(void)
> +{
> + struct stat statbuf;
> + int ret;
> + ret = stat(AA_MOUNT_RESTR, &statbuf);
> + if (ret != 0)
> + return 0;
> + return 1;
> +}
> +
> /* aa_getcon is not working right now. Use our hand-rolled version
> below */ static int apparmor_enabled(void)
> {
> - struct stat statbuf;
> FILE *fin;
> char e;
> int ret;
>
> - ret = stat(AA_MOUNT_RESTR, &statbuf);
> - if (ret != 0)
> - return 0;
> fin = fopen(AA_ENABLED_FILE, "r");
> if (!fin)
> return 0;
> @@ -131,9 +138,11 @@ static int apparmor_am_unconfined(void)
> *
> * Notes: This relies on /proc being available.
> */
> -static int apparmor_process_label_set(const char *label, int
> use_default, +static int apparmor_process_label_set(struct lxc_conf
> *conf, int use_default, int on_exec)
> {
> + const char *label = conf->lsm_aa_profile;
> +
> if (!aa_enabled)
> return 0;
>
> @@ -141,9 +150,20 @@ static int apparmor_process_label_set(const char
> *label, int use_default, if (use_default)
> label = AA_DEF_PROFILE;
> else
> - return 0;
> + label = "unconfined";
> + }
> +
> + if (!mount_feature_enabled() && strcmp(label,
> "unconfined") != 0) {
> + WARN("Incomplete AppArmor support in your kernel");
> + if (!conf->lsm_aa_allow_incomplete) {
> + ERROR("If you really want to start this
> container, set");
> + ERROR("lxc.aa_allow_incomplete = 1");
> + ERROR("in your container configuration
> file");
> + return -1;
> + }
> }
>
> +
> if (strcmp(label, "unconfined") == 0 &&
> apparmor_am_unconfined()) { INFO("apparmor profile unchanged");
> return 0;
> diff --git a/src/lxc/lsm/lsm.c b/src/lxc/lsm/lsm.c
> index c994046..54b404c 100644
> --- a/src/lxc/lsm/lsm.c
> +++ b/src/lxc/lsm/lsm.c
> @@ -85,13 +85,13 @@ char *lsm_process_label_get(pid_t pid)
> return drv->process_label_get(pid);
> }
>
> -int lsm_process_label_set(const char *label, int use_default, int
> on_exec) +int lsm_process_label_set(struct lxc_conf *conf, int
> use_default, int on_exec) {
> if (!drv) {
> ERROR("LSM driver not inited");
> return -1;
> }
> - return drv->process_label_set(label, use_default, on_exec);
> + return drv->process_label_set(conf, use_default, on_exec);
> }
>
> #endif
> diff --git a/src/lxc/lsm/lsm.h b/src/lxc/lsm/lsm.h
> index 93f5b87..0d21b4c 100644
> --- a/src/lxc/lsm/lsm.h
> +++ b/src/lxc/lsm/lsm.h
> @@ -33,7 +33,7 @@ struct lsm_drv {
>
> int (*enabled)(void);
> char *(*process_label_get)(pid_t pid);
> - int (*process_label_set)(const char *label, int
> use_default,
> + int (*process_label_set)(struct lxc_conf *conf, int
> use_default, int on_exec);
> };
>
> @@ -42,13 +42,15 @@ void lsm_init(void);
> int lsm_enabled(void);
> const char *lsm_name(void);
> char *lsm_process_label_get(pid_t pid);
> -int lsm_process_label_set(const char *label, int
> use_default, int on_exec); +int lsm_process_label_set(struct
> lxc_conf *conf,
> + int use_default, int on_exec);
> #else
> static inline void lsm_init(void) { }
> static inline int lsm_enabled(void) { return 0; }
> static inline const char *lsm_name(void) { return "none"; }
> static inline char *lsm_process_label_get(pid_t pid) { return
> NULL; } -static inline int lsm_process_label_set(const char
> *label, int use_default, int on_exec) { return 0; } +static inline
> int lsm_process_label_set(struct lxc-conf *conf,
> + int use_default, int on_exec) { return 0; }
> #endif
>
> #endif
> diff --git a/src/lxc/lsm/nop.c b/src/lxc/lsm/nop.c
> index e5db124..5e10f4e 100644
> --- a/src/lxc/lsm/nop.c
> +++ b/src/lxc/lsm/nop.c
> @@ -29,7 +29,7 @@ static char *nop_process_label_get(pid_t pid)
> return NULL;
> }
>
> -static int nop_process_label_set(const char *label, int use_default,
> +static int nop_process_label_set(struct lxc_conf *conf, int
> use_default, int on_exec)
> {
> return 0;
> diff --git a/src/lxc/lsm/selinux.c b/src/lxc/lsm/selinux.c
> index f8dad95..9e33055 100644
> --- a/src/lxc/lsm/selinux.c
> +++ b/src/lxc/lsm/selinux.c
> @@ -29,6 +29,7 @@
>
> #include "log.h"
> #include "lsm/lsm.h"
> +#include "conf.h"
>
> #define DEFAULT_LABEL "unconfined_t"
>
> @@ -61,7 +62,7 @@ static char *selinux_process_label_get(pid_t pid)
> /*
> * selinux_process_label_set: Set SELinux context of a process
> *
> - * @label : the context to set
> + * @conf : the container configuration
> * @default : use the default context if label is NULL
> * @on_exec : the new context will take effect on exec(2) not
> immediately *
> @@ -69,9 +70,10 @@ static char *selinux_process_label_get(pid_t pid)
> *
> * Notes: This relies on /proc being available.
> */
> -static int selinux_process_label_set(const char *label, int
> use_default, +static int selinux_process_label_set(struct lxc_conf
> *conf, int use_default, int on_exec)
> {
> + const char *label = conf->lsm_se_context;
> if (!label) {
> if (use_default)
> label = DEFAULT_LABEL;
> diff --git a/src/lxc/start.c b/src/lxc/start.c
> index d9fbc8c..d4dcd56 100644
> --- a/src/lxc/start.c
> +++ b/src/lxc/start.c
> @@ -615,7 +615,6 @@ static int do_start(void *data)
> {
> struct lxc_list *iterator;
> struct lxc_handler *handler = data;
> - const char *lsm_label = NULL;
>
> if (sigprocmask(SIG_SETMASK, &handler->oldmask, NULL)) {
> SYSERROR("failed to set sigprocmask");
> @@ -695,11 +694,7 @@ static int do_start(void *data)
> return -1;
>
> /* Set the label to change to when we exec(2) the
> container's init */
> - if (!strcmp(lsm_name(), "AppArmor"))
> - lsm_label = handler->conf->lsm_aa_profile;
> - else if (!strcmp(lsm_name(), "SELinux"))
> - lsm_label = handler->conf->lsm_se_context;
> - if (lsm_process_label_set(lsm_label, 1, 1) < 0)
> + if (lsm_process_label_set(handler->conf, 1, 1) < 0)
> goto out_warn_father;
>
> /* Some init's such as busybox will set sane tty settings on
> stdin, diff --git a/src/tests/Makefile.am b/src/tests/Makefile.am
> index 35bc1b3..e0969eb 100644
> --- a/src/tests/Makefile.am
> +++ b/src/tests/Makefile.am
> @@ -52,6 +52,7 @@ bin_SCRIPTS = lxc-test-autostart
>
> if DISTRO_UBUNTU
> bin_SCRIPTS += \
> + lxc-test-apparmor-mount \
> lxc-test-checkpoint-restore \
> lxc-test-ubuntu \
> lxc-test-unpriv \
> diff --git a/src/tests/lxc-test-apparmor-mount
> b/src/tests/lxc-test-apparmor-mount new file mode 100755
> index 0000000..503381e
> --- /dev/null
> +++ b/src/tests/lxc-test-apparmor-mount
> @@ -0,0 +1,189 @@
> +#!/bin/sh
> +
> +# apparmor_mount: test proper handling of apparmor in kernels
> +# without mount features
> +
> +# These require the ubuntu lxc package to be installed.
> +
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +# Lesser General Public License for more details.
> +
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
> 02110-1301 USA +
> +# This test assumes an Ubuntu host
> +
> +set -e
> +
> +FAIL() {
> + echo -n "Failed " >&2
> + echo "$*" >&2
> + exit 1
> +}
> +
> +run_cmd() {
> + sudo -i -u $TUSER \
> + env http_proxy=${http_proxy:-}
> https_proxy=${https_proxy:-} \
> + XDG_RUNTIME_DIR=/run/user/$(id -u $TUSER) $*
> +}
> +
> +DONE=0
> +MOUNTSR=/sys/kernel/security/apparmor/features/mount
> +dnam=`mktemp -d`
> +cname=`basename $dnam`
> +cleanup() {
> + run_cmd lxc-destroy -f -n $cname || true
> + umount -l $MOUNTSR || true
> + rmdir $dnam || true
> + pkill -u $(id -u $TUSER) -9
> + sed -i '/lxcunpriv/d' /run/lxc/nics /etc/lxc/lxc-usernet
> + sed -i '/^lxcunpriv:/d' /etc/subuid /etc/subgid
> + rm -Rf $HDIR /run/user/$(id -u $TUSER)
> + deluser $TUSER
> + if [ $DONE -eq 0 ]; then
> + echo "FAIL"
> + exit 1
> + fi
> + echo "PASS"
> +}
> +
> +trap cleanup exit
> +
> +# Only run on a normally configured ubuntu lxc system
> +if [ ! -d /sys/class/net/lxcbr0 ]; then
> + echo "lxcbr0 is not configured."
> + exit 1
> +fi
> +if [ "$(id -u)" != "0" ]; then
> + echo "ERROR: Must run as root."
> + exit 1
> +fi
> +
> +# This would be much simpler if we could run it as
> +# root. However, in order to not have the bind mount
> +# of an empty directory over the securitfs 'mount' directory
> +# be removed, we need to do this as non-root.
> +
> +which newuidmap >/dev/null 2>&1 || { echo "'newuidmap' command is
> missing" >&2; exit 1; } +# create a test user
> +TUSER=lxcunpriv
> +HDIR=/home/$TUSER
> +
> +ARCH=i386
> +if type dpkg >/dev/null 2>&1; then
> + ARCH=$(dpkg --print-architecture)
> +fi
> +
> +deluser $TUSER && rm -Rf $HDIR || true
> +useradd $TUSER
> +
> +mkdir -p $HDIR
> +echo "$TUSER veth lxcbr0 2" > /etc/lxc/lxc-usernet
> +sed -i '/^lxcunpriv:/d' /etc/subuid /etc/subgid
> +
> +usermod -v 910000-919999 -w 910000-919999 $TUSER
> +
> +mkdir -p $HDIR/.config/lxc/
> +cat > $HDIR/.config/lxc/default.conf << EOF
> +lxc.network.type = veth
> +lxc.network.link = lxcbr0
> +lxc.id_map = u 0 910000 9999
> +lxc.id_map = g 0 910000 9999
> +EOF
> +chown -R $TUSER: $HDIR
> +
> +mkdir -p /run/user/$(id -u $TUSER)
> +chown -R $TUSER: /run/user/$(id -u $TUSER)
> +
> +cd $HDIR
> +
> +cgm create all $TUSER
> +cgm chown all $TUSER $(id -u $TUSER) $(id -g $TUSER)
> +cgm movepid all $TUSER $$
> +
> +run_cmd mkdir -p $HDIR/.cache/lxc
> +[ -d /var/cache/lxc/download ] && \
> + cp -R /var/cache/lxc/download $HDIR/.cache/lxc && \
> + chown -R $TUSER: $HDIR/.cache/lxc
> +
> +run_cmd lxc-create -t download -n $cname -- -d ubuntu -r trusty -a
> $ARCH +
> +echo "test default confined container"
> +run_cmd lxc-start -n $cname -d
> +run_cmd lxc-wait -n $cname -s RUNNING
> +pid=`run_cmd lxc-info -p -H -n $cname`
> +profile=`cat /proc/$pid/attr/current`
> +if [ "x$profile" != "xlxc-container-default (enforce)" ]; then
> + echo "FAIL: confined container was in profile $profile"
> + exit 1
> +fi
> +run_cmd lxc-stop -n $cname
> +
> +echo "test regular unconfined container"
> +echo "lxc.aa_profile = unconfined" >>
> $HDIR/.local/share/lxc/$cname/config +run_cmd lxc-start -n $cname -d
> +run_cmd lxc-wait -n $cname -s RUNNING
> +pid=`run_cmd lxc-info -p -H -n $cname`
> +profile=`cat /proc/$pid/attr/current`
> +if [ "x$profile" != "xunconfined" ]; then
> + echo "FAIL: unconfined container was in profile $profile"
> + exit 1
> +fi
> +run_cmd lxc-stop -n $cname
> +
> +echo "masking $MOUNTSR"
> +mount --bind $dnam $MOUNTSR
> +
> +echo "test default confined container"
> +sed -i '/aa_profile/d' $HDIR/.local/share/lxc/$cname/config
> +run_cmd lxc-start -n $cname -d || true
> +sleep 3
> +pid=`run_cmd lxc-info -p -H -n $cname` || true
> +if [ -n "$pid" -a "$pid" != "-1" ]; then
> + echo "FAIL: confined container started without mount
> restrictions"
> + echo "pid was $pid"
> + exit 1
> +fi
> +
> +echo "test regular unconfined container"
> +echo "lxc.aa_profile = unconfined" >>
> $HDIR/.local/share/lxc/$cname/config +run_cmd lxc-start -n $cname -d
> +run_cmd lxc-wait -n $cname -s RUNNING
> +pid=`run_cmd lxc-info -p -H -n $cname`
> +if [ "$pid" = "-1" ]; then
> + echo "FAIL: unconfined container failed to start without
> mount restrictions"
> + exit 1
> +fi
> +profile=`cat /proc/$pid/attr/current`
> +if [ "x$profile" != "xunconfined" ]; then
> + echo "FAIL: confined container was in profile $profile"
> + exit 1
> +fi
> +run_cmd lxc-stop -n $cname
> +
> +echo "testing override"
> +sed -i '/aa_profile/d' $HDIR/.local/share/lxc/$cname/config
> +echo "lxc.aa_allow_incomplete = 1" >>
> $HDIR/.local/share/lxc/$cname/config +run_cmd lxc-start -n $cname -d
> +run_cmd lxc-wait -n $cname -s RUNNING
> +pid=`run_cmd lxc-info -p -H -n $cname`
> +if [ "$pid" = "-1" ]; then
> + echo "FAIL: excepted container failed to start without mount
> restrictions"
> + exit 1
> +fi
> +profile=`cat /proc/$pid/attr/current`
> +if [ "x$profile" != "xlxc-container-default (enforce)" ]; then
> + echo "FAIL: confined container was in profile $profile"
> + exit 1
> +fi
> +run_cmd lxc-stop -n $cname
> +
> +DONE=1
More information about the lxc-devel
mailing list