[prev in list] [next in list] [prev in thread] [next in thread]
List: apparmor-dev
Subject: Re: [apparmor] [PATCH 2/2] tests: Fix onexec.sh races by using the transition test program
From: Seth Arnold <seth.arnold () canonical ! com>
Date: 2016-06-25 3:24:29
Message-ID: 20160625032429.GB21074 () hunt
[Download RAW message or body]
[Attachment #2 (multipart/signed)]
On Fri, Jun 24, 2016 at 05:15:54PM -0500, Tyler Hicks wrote:
> The onexec.sh test has periodically exhibited unexplicable failures that
> are possibly due to race conditions when onexec.sh is verifying the
> /proc/PID/attr/{current,exec} values of the process under test. This
> patch attempts to solve the flaky test failures by removing the need for
> IPC to coordinate between the test script and the test program.
>
> The old onexec test program is removed and the transition test program
> is used instead. This allows for the test script to tell the transition
> test program what its current and exec procattr labels should be via
> command line options.
>
> Since IPC is no longer needed, the signal:ALL allow rule can be dropped
> from the test profile. A new allow rule is needed to grant reading of
> /proc/*/attr/{current,exec} since transition must verify the contents of
> these files.
>
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
for 2.9, 2.10, trunk, etc
Thanks
> ---
> tests/regression/apparmor/Makefile | 1 -
> tests/regression/apparmor/onexec.c | 63 ----------------
> tests/regression/apparmor/onexec.sh | 147 +++++++++---------------------------
> 3 files changed, 35 insertions(+), 176 deletions(-)
> delete mode 100644 tests/regression/apparmor/onexec.c
>
> diff --git a/tests/regression/apparmor/Makefile \
> b/tests/regression/apparmor/Makefile index 8d75c69..198ca42 100644
> --- a/tests/regression/apparmor/Makefile
> +++ b/tests/regression/apparmor/Makefile
> @@ -73,7 +73,6 @@ SRC=access.c \
> at_secure.c \
> introspect.c \
> changeprofile.c \
> - onexec.c \
> changehat.c \
> changehat_fork.c \
> changehat_misc.c \
> diff --git a/tests/regression/apparmor/onexec.c \
> b/tests/regression/apparmor/onexec.c deleted file mode 100644
> index f9beb3a..0000000
> --- a/tests/regression/apparmor/onexec.c
> +++ /dev/null
> @@ -1,63 +0,0 @@
> -/*
> - * Copyright (C) 2002-2005 Novell/SUSE
> - *
> - * This program is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU General Public License as
> - * published by the Free Software Foundation, version 2 of the
> - * License.
> - */
> -
> -#include <stdio.h>
> -#include <unistd.h>
> -#include <errno.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
> -#include <fcntl.h>
> -#include <string.h>
> -#include <stdlib.h>
> -#include <signal.h>
> -#include <linux/unistd.h>
> -
> -#include <sys/apparmor.h>
> -#include "changehat.h"
> -
> -int main(int argc, char *argv[])
> -{
> - int rc = 0;
> -
> - extern char **environ;
> -
> - if (argc < 3){
> - fprintf(stderr, "usage: %s profile executable args\n",
> - argv[0]);
> - return 1;
> - }
> -
> - /* change profile if profile name != nochange */
> - if (strcmp(argv[1], "nochange") != 0){
> - rc = aa_change_onexec(argv[1]);
> - if (rc == -1){
> - fprintf(stderr, "FAIL: change_onexec %s failed - %s\n",
> - argv[1], strerror(errno));
> - exit(errno);
> - }
> - }
> -
> - /* stop after onexec and wait to for continue before exec so
> - * caller can introspect task */
> - rc = kill(getpid(), SIGSTOP);
> - if (rc == -1){
> - fprintf(stderr, "FAIL: signal to self failed - %s\n",
> - strerror(errno));
> - exit(errno);
> - }
> -
> - (void)execve(argv[2], &argv[2], environ);
> - /* exec failed, kill outselves to flag parent */
> -
> - rc = errno;
> -
> - fprintf(stderr, "FAIL: exec to '%s' failed\n", argv[2]);
> -
> - return rc;
> -}
> diff --git a/tests/regression/apparmor/onexec.sh \
> b/tests/regression/apparmor/onexec.sh index 9bfacd6..922ea25 100644
> --- a/tests/regression/apparmor/onexec.sh
> +++ b/tests/regression/apparmor/onexec.sh
> @@ -18,6 +18,7 @@ bin=$pwd
>
> . $bin/prologue.inc
>
> +settest transition
> file=$tmpdir/file
> subfile=$tmpdir/file2
> okperm=rw
> @@ -29,63 +30,11 @@ fqsubtest="$fqsubbase//$subtest"
> subtest2="$pwd//sub2"
> subtest3="$pwd//sub3"
>
> -onexec="/proc/*/attr/exec"
> +exec_w="/proc/*/attr/exec:w"
> +attrs_r="/proc/*/attr/{current,exec}:r"
>
> touch $file $subfile
>
> -check_exec()
> -{
> - local rc
> - local actual
> - local desc="$1"
> - local pid="$2"
> - local expected="$3"
> - actual=`cat /proc/${pid}/attr/exec 2>/dev/null`
> - rc=$?
> -
> - # /proc/${pid}/attr/exec returns invalid argument if onexec has not been \
> called
> - if [ $rc -ne 0 ] ; then
> - if [ "${expected}" == "nochange" ] ; then
> - return 0
> - fi
> - echo "ONEXEC (${desc}) - exec transition not set"
> - return $rc
> - fi
> - if [ "${actual% (*)}" != "${expected}" ] ; then
> - echo "ONEXEC (${desc}) - check exec '${actual% (*)}' != expected '${expected}'"
> - return 1
> - fi
> -
> - return 0
> -}
> -
> -check_current()
> -{
> - local rc
> - local actual
> - local desc="$1"
> - local pid="$2"
> - local expected="$3"
> - actual=`cat /proc/${pid}/attr/current 2>/dev/null`
> - rc=$?
> -
> - # /proc/${pid}/attr/current return enoent if the onexec process already exited \
> due to error
> - if [ $rc -ne 0 ] ; then
> - # These assume a check has already been done to see if the
> - # task is still around
> - echo -n "ONEXEC - check current ($1): "
> - cat /proc/${pid}/attr/current
> - return $rc
> - fi
> -
> - if [ "${actual% (*)}" != "${expected}" ] ; then
> - echo "ONEXEC - check current (${desc}) '${actual% (*)}' != expected \
> '${expected}'"
> - return 1
> - fi
> -
> - return 0
> -}
> -
> do_test()
> {
> local desc="$1"
> @@ -94,34 +43,12 @@ do_test()
> local res="$4"
> shift 4
>
> - #ignore prologue.inc error trapping that catches our subfn return values
> -
> - runtestbg "ONEXEC $desc ($prof -> $target_prof)" $res $target_prof "$@"
> - # check that transition does not happen before exec, and that transition
> - # is set
> -
> - # give the onexec process a chance to run
> - sleep 0.05
> -
> - # check that task hasn't exited because change_onexec failed
> - if ! [ -d "/proc/${_pid}" ] ; then
> - checktestfg
> - return
> - fi
> -
> - if ! check_current "${desc}" $_pid $prof ; then
> - checktestfg
> - return
> - fi
> -
> - if ! check_exec "${desc}" $_pid $target_prof ; then
> - checktestfg
> - return
> + desc="ONEXEC $desc ($prof -> $target_prof)"
> + if [ "$target_prof" == "nochange" ] ; then
> + runchecktest "$desc" $res -l "$prof" -- "$@"
> + else
> + runchecktest "$desc" $res -O "$target_prof" -l "$prof" -L "$target_prof" \
> -- "$@" fi
> -
> - kill -CONT $_pid
> -
> - checktestbg
> }
>
>
> @@ -146,59 +73,55 @@ do_test "override px" unconfined $bin/rw pass $bin/open $file
>
> #------
>
> -# NOTE: test program pauses for the driver script to catch up by sending
> -# and recieving SIGSTOP/SIGCONT, so the onexec program needs access to
> -# signals (this is not a script to test signal mediation)
> -
> # ONEXEC from CONFINED - don't change profile, open can't exec
> -genprofile 'change_profile->':$bin/rw $onexec:w signal:ALL
> -do_test "no px perm" $bin/onexec nochange fail $bin/open $file
> +genprofile 'change_profile->':$bin/rw $exec_w $attrs_r
> +do_test "no px perm" $test nochange fail $bin/open $file
>
> # ONEXEC from CONFINED - don't change profile, open is run unconfined
> -genprofile 'change_profile->':$bin/rw $bin/open:rux $onexec:w signal:ALL
> -do_test "nochange rux" $bin/onexec nochange pass $bin/open $file
> +genprofile 'change_profile->':$bin/rw $bin/open:rux $exec_w $attrs_r
> +do_test "nochange rux" $test nochange pass $bin/open $file
>
> # ONEXEC from CONFINED - don't change profile, open is run confined without \
> necessary perms
> -genprofile 'change_profile->':$bin/rw $onexec:w signal:ALL -- image=$bin/open \
> $file:rw
> -do_test "nochange px - no px perm" $bin/onexec nochange fail $bin/open $file
> +genprofile 'change_profile->':$bin/rw $exec_w $attrs_r -- image=$bin/open $file:rw
> +do_test "nochange px - no px perm" $test nochange fail $bin/open $file
>
> # ONEXEC from CONFINED - don't change profile, open is run confined without \
> necessary perms
> -genprofile 'change_profile->':$bin/rw $bin/open:rpx $onexec:w signal:ALL -- \
> image=$bin/open
> -do_test "nochange px - no file perm" $bin/onexec nochange fail $bin/open $file
> +genprofile 'change_profile->':$bin/rw $bin/open:rpx $exec_w $attrs_r -- \
> image=$bin/open +do_test "nochange px - no file perm" $test nochange fail $bin/open \
> $file
> # ONEXEC from CONFINED - target does NOT exist
> -genprofile 'change_profile->':$bin/open $onexec:w signal:ALL -- image=$bin/rw \
> $bin/open:rix $file:rw -- image=$bin/open
> -do_test "noexist px" $bin/onexec noexist fail $bin/open $file
> +genprofile 'change_profile->':$bin/open $exec_w $attrs_r -- image=$bin/rw \
> $bin/open:rix $file:rw -- image=$bin/open +do_test "noexist px" $test noexist fail \
> $bin/open $file
> # ONEXEC from CONFINED - change to rw profile, no exec profile to override
> -genprofile 'change_profile->':$bin/rw $onexec:w signal:ALL -- image=$bin/rw \
> $bin/open:rix $file:rw
> -do_test "change profile - override rix" $bin/onexec $bin/rw pass $bin/open $file
> +genprofile 'change_profile->':$bin/rw $exec_w $attrs_r -- image=$bin/rw \
> $bin/open:rix $file:rw +do_test "change profile - override rix" $test $bin/rw pass \
> $bin/open $file
> -# ONEXEC from CONFINED - change to rw profile, no exec profile to override, no \
> explicit access to /proc/*/attr/exec
> -genprofile 'change_profile->':$bin/rw signal:ALL -- image=$bin/rw $bin/open:rix \
> $file:rw
> -do_test "change profile - no onexec:w" $bin/onexec $bin/rw pass $bin/open $file
> +# ONEXEC from CONFINED - change to rw profile, no exec profile to override, no \
> explicit write access to /proc/*/attr/exec +genprofile 'change_profile->':$bin/rw \
> $attrs_r -- image=$bin/rw $bin/open:rix $file:rw +do_test "change profile - no \
> exec_w" $test $bin/rw pass $bin/open $file
> # ONEXEC from CONFINED - don't change profile, make sure exec profile is applied
> -genprofile 'change_profile->':$bin/rw $onexec:w $bin/open:rpx signal:ALL -- \
> image=$bin/rw $bin/open:rix $file:rw -- image=$bin/open $file:rw
> -do_test "nochange px" $bin/onexec nochange pass $bin/open $file
> +genprofile 'change_profile->':$bin/rw $exec_w $attrs_r $bin/open:rpx -- \
> image=$bin/rw $bin/open:rix $file:rw -- image=$bin/open $file:rw +do_test \
> "nochange px" $test nochange pass $bin/open $file
> # ONEXEC from CONFINED - change to rw profile, override regular exec profile, exec \
> profile doesn't have perms
> -genprofile 'change_profile->':$bin/rw $onexec:w signal:ALL -- image=$bin/rw \
> $bin/open:rix $file:rw -- image=$bin/open
> -do_test "override px" $bin/onexec $bin/rw pass $bin/open $file
> +genprofile 'change_profile->':$bin/rw $exec_w $attrs_r -- image=$bin/rw \
> $bin/open:rix $file:rw -- image=$bin/open +do_test "override px" $test $bin/rw \
> pass $bin/open $file
> # ONEXEC from - change to rw profile, override regular exec profile, exec profile \
> has perms, rw doesn't
> -genprofile 'change_profile->':$bin/rw $onexec:w signal:ALL -- image=$bin/rw \
> $bin/open:rix -- image=$bin/open $file:rw
> -do_test "override px" $bin/onexec $bin/rw fail $bin/open $file
> +genprofile 'change_profile->':$bin/rw $exec_w $attrs_r -- image=$bin/rw \
> $bin/open:rix -- image=$bin/open $file:rw +do_test "override px" $test $bin/rw \
> fail $bin/open $file
> # ONEXEC from COFINED - change to rw profile via glob rule, override exec profile, \
> exec profile doesn't have perms
> -genprofile 'change_profile->':/** $onexec:w signal:ALL -- image=$bin/rw \
> $bin/open:rix $file:rw -- image=$bin/open
> -do_test "glob override px" $bin/onexec $bin/rw pass $bin/open $file
> +genprofile 'change_profile->':/** $exec_w $attrs_r -- image=$bin/rw $bin/open:rix \
> $file:rw -- image=$bin/open +do_test "glob override px" $test $bin/rw pass \
> $bin/open $file
> # ONEXEC from COFINED - change to exec profile via glob rule, override exec \
> profile, exec profile doesn't have perms
> -genprofile 'change_profile->':/** $onexec:w signal:ALL -- image=$bin/rw \
> $bin/open:rix $file:rw -- image=$bin/open
> -do_test "glob override px" $bin/onexec $bin/open fail $bin/open $file
> +genprofile 'change_profile->':/** $exec_w $attrs_r -- image=$bin/rw $bin/open:rix \
> $file:rw -- image=$bin/open +do_test "glob override px" $test $bin/open fail \
> $bin/open $file
> # ONEXEC from COFINED - change to exec profile via glob rule, override exec \
> profile, exec profile has perms
> -genprofile 'change_profile->':/** $onexec:w signal:ALL -- image=$bin/rw \
> $bin/open:rix $file:rw -- image=$bin/open $file:rw
> -do_test "glob override px" $bin/onexec $bin/rw pass $bin/open $file
> +genprofile 'change_profile->':/** $exec_w $attrs_r -- image=$bin/rw $bin/open:rix \
> $file:rw -- image=$bin/open $file:rw +do_test "glob override px" $test $bin/rw \
> pass $bin/open $file
["signature.asc" (application/pgp-signature)]
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic