[lxc-devel] [PATCH] apparmor: improve behavior when kernel lacks mount restrictions (v2)
Stéphane Graber
stgraber at ubuntu.com
Mon Sep 22 21:26:19 UTC 2014
On Sat, Sep 20, 2014 at 03:47:38AM +0000, Serge Hallyn wrote:
> (Dwight, I took the liberty of adding your Ack but the code did
> change a bit to continue passing the char *label from attach.
> Tested that "lxc-start -n u1 -s lxc.aa_profile=p2; lxc-attach -n u1"
> does attach you to the p2 profile)
>
> 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.
>
> 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>
> Acked-by: Dwight Engen <dwight.engen at oracle.com>
As usual, you've missed adding the new testcase to extra_DIST, I'll take
care of it :)
Acked-by: Stéphane Graber <stgraber at ubuntu.com>
>
> ---
> src/lxc/attach.c | 3 +-
> src/lxc/conf.h | 1 +
> src/lxc/confile.c | 14 +++
> src/lxc/lsm/apparmor.c | 35 +++++--
> src/lxc/lsm/lsm.c | 5 +-
> src/lxc/lsm/lsm.h | 10 +-
> src/lxc/lsm/nop.c | 4 +-
> src/lxc/lsm/selinux.c | 9 +-
> src/lxc/start.c | 7 +-
> src/tests/Makefile.am | 1 +
> src/tests/lxc-test-apparmor-mount | 189 ++++++++++++++++++++++++++++++++++++++
> 11 files changed, 253 insertions(+), 25 deletions(-)
> create mode 100755 src/tests/lxc-test-apparmor-mount
>
> diff --git a/src/lxc/attach.c b/src/lxc/attach.c
> index 923c497..7cadcbd 100644
> --- a/src/lxc/attach.c
> +++ b/src/lxc/attach.c
> @@ -1043,7 +1043,8 @@ 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->lsm_label,
> + 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..8de0115 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;
> @@ -124,6 +131,7 @@ static int apparmor_am_unconfined(void)
> * apparmor_process_label_set: Set AppArmor process profile
> *
> * @label : the profile to set
> + * @conf : the container configuration to use @label is NULL
> * @default : use the default profile if label is NULL
> * @on_exec : this is ignored. Apparmor profile will be changed immediately
> *
> @@ -131,9 +139,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,
> - int on_exec)
> +static int apparmor_process_label_set(const char *inlabel, struct lxc_conf *conf,
> + int use_default, int on_exec)
> {
> + const char *label = inlabel ? inlabel : conf->lsm_aa_profile;
> +
> if (!aa_enabled)
> return 0;
>
> @@ -141,9 +151,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..79f837f 100644
> --- a/src/lxc/lsm/lsm.c
> +++ b/src/lxc/lsm/lsm.c
> @@ -85,13 +85,14 @@ 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(const char *label, 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(label, conf, use_default, on_exec);
> }
>
> #endif
> diff --git a/src/lxc/lsm/lsm.h b/src/lxc/lsm/lsm.h
> index 93f5b87..84e992d 100644
> --- a/src/lxc/lsm/lsm.h
> +++ b/src/lxc/lsm/lsm.h
> @@ -33,8 +33,8 @@ struct lsm_drv {
>
> int (*enabled)(void);
> char *(*process_label_get)(pid_t pid);
> - int (*process_label_set)(const char *label, int use_default,
> - int on_exec);
> + int (*process_label_set)(const char *label, struct lxc_conf *conf,
> + int use_default, int on_exec);
> };
>
> #if HAVE_APPARMOR || HAVE_SELINUX
> @@ -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(const char *label, 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(const char *label,
> + 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..c13d8f5 100644
> --- a/src/lxc/lsm/nop.c
> +++ b/src/lxc/lsm/nop.c
> @@ -29,8 +29,8 @@ static char *nop_process_label_get(pid_t pid)
> return NULL;
> }
>
> -static int nop_process_label_set(const char *label, int use_default,
> - int on_exec)
> +static int nop_process_label_set(const char *label, 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..46554d8 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,8 @@ static char *selinux_process_label_get(pid_t pid)
> /*
> * selinux_process_label_set: Set SELinux context of a process
> *
> - * @label : the context to set
> + * @label : label string
> + * @conf : the container configuration to use @label is NULL
> * @default : use the default context if label is NULL
> * @on_exec : the new context will take effect on exec(2) not immediately
> *
> @@ -69,9 +71,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,
> - int on_exec)
> +static int selinux_process_label_set(const char *inlabel, struct lxc_conf *conf,
> + int use_default, int on_exec)
> {
> + const char *label = inlabel ? inlabel : 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..d7d6708 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(NULL, 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
> --
> 2.1.0
>
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel
--
Stéphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20140922/bc3d04c3/attachment.sig>
More information about the lxc-devel
mailing list