2019-01-31 18:57:42

by Mimi Zohar

[permalink] [raw]
Subject: [PATCH 0/3] selftest/ima: add kexec_file_load test

The kernel can be configured to verify PE signed kernel images, IMA
kernel image signatures, both types of signatures, or none. Verify
only properly signed kernel images are loaded into memory, based on
the kernel configuration and runtime policies.

Mimi Zohar (3):
selftest/ima: cleanup the kexec selftest
scripts/ima: define a set of common functions
selftests/ima: kexec_file_load syscall test

tools/testing/selftests/ima/Makefile | 2 +-
tools/testing/selftests/ima/common_lib.sh | 20 ++
.../testing/selftests/ima/test_kexec_file_load.sh | 250 +++++++++++++++++++++
tools/testing/selftests/ima/test_kexec_load.sh | 31 +--
4 files changed, 281 insertions(+), 22 deletions(-)
create mode 100755 tools/testing/selftests/ima/common_lib.sh
create mode 100755 tools/testing/selftests/ima/test_kexec_file_load.sh

--
2.7.5



2019-01-31 18:56:57

by Mimi Zohar

[permalink] [raw]
Subject: [PATCH 2/3] scripts/ima: define a set of common functions

Define and move get_secureboot_mode() to a common file for use by other
tests.

Signed-off-by: Mimi Zohar <[email protected]>
---
tools/testing/selftests/ima/common_lib.sh | 20 ++++++++++++++++++++
tools/testing/selftests/ima/test_kexec_load.sh | 17 +++--------------
2 files changed, 23 insertions(+), 14 deletions(-)
create mode 100755 tools/testing/selftests/ima/common_lib.sh

diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
new file mode 100755
index 000000000000..ae097a634da5
--- /dev/null
+++ b/tools/testing/selftests/ima/common_lib.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0+
+
+get_secureboot_mode()
+{
+ EFIVARFS="/sys/firmware/efi/efivars"
+ # Make sure that efivars is mounted in the normal location
+ if ! grep -q "^\S\+ $EFIVARFS efivarfs" /proc/mounts; then
+ echo "$TEST: efivars is not mounted on $EFIVARFS" >&2
+ exit $ksft_skip
+ fi
+
+ # Get secureboot mode
+ file="$EFIVARFS/SecureBoot-*"
+ if [ ! -e $file ]; then
+ echo "$TEST: unknown secureboot mode" >&2
+ exit $ksft_skip
+ fi
+ return `hexdump $file | awk '{print substr($4,length($4),1)}'`
+}
diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
index 74423c4229e2..5e3566738888 100755
--- a/tools/testing/selftests/ima/test_kexec_load.sh
+++ b/tools/testing/selftests/ima/test_kexec_load.sh
@@ -5,7 +5,7 @@
# is booted in secureboot mode.

TEST="$0"
-EFIVARFS="/sys/firmware/efi/efivars"
+. ./common_lib.sh
rc=0

# Kselftest framework requirement - SKIP code is 4.
@@ -17,19 +17,8 @@ if [ $(id -ru) != 0 ]; then
exit $ksft_skip
fi

-# Make sure that efivars is mounted in the normal location
-if ! grep -q "^\S\+ $EFIVARFS efivarfs" /proc/mounts; then
- echo "$TEST: efivars is not mounted on $EFIVARFS" >&2
- exit $ksft_skip
-fi
-
-# Get secureboot mode
-file="$EFIVARFS/SecureBoot-*"
-if [ ! -e $file ]; then
- echo "$TEST: unknown secureboot mode" >&2
- exit $ksft_skip
-fi
-secureboot=`hexdump $file | awk '{print substr($4,length($4),1)}'`
+get_secureboot_mode
+secureboot=$?

# kexec_load should fail in secure boot mode
KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
--
2.7.5


2019-01-31 18:57:28

by Mimi Zohar

[permalink] [raw]
Subject: [PATCH 1/3] selftest/ima: cleanup the kexec selftest

Remove the few bashisms in the script and use the complete option name
for clarity.

Signed-off-by: Mimi Zohar <[email protected]>
---
tools/testing/selftests/ima/test_kexec_load.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
index 1c10093fb526..74423c4229e2 100755
--- a/tools/testing/selftests/ima/test_kexec_load.sh
+++ b/tools/testing/selftests/ima/test_kexec_load.sh
@@ -1,7 +1,7 @@
#!/bin/sh
# SPDX-License-Identifier: GPL-2.0+
# Loading a kernel image via the kexec_load syscall should fail
-# when the kerne is CONFIG_KEXEC_VERIFY_SIG enabled and the system
+# when the kernel is CONFIG_KEXEC_VERIFY_SIG enabled and the system
# is booted in secureboot mode.

TEST="$0"
@@ -12,7 +12,7 @@ rc=0
ksft_skip=4

# kexec requires root privileges
-if [ $UID != 0 ]; then
+if [ $(id -ru) != 0 ]; then
echo "$TEST: must be run as root" >&2
exit $ksft_skip
fi
@@ -33,17 +33,17 @@ secureboot=`hexdump $file | awk '{print substr($4,length($4),1)}'`

# kexec_load should fail in secure boot mode
KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
-kexec -l $KERNEL_IMAGE &>> /dev/null
-if [ $? == 0 ]; then
- kexec -u
- if [ "$secureboot" == "1" ]; then
+kexec --load $KERNEL_IMAGE 2>&1 /dev/null
+if [ $? -eq 0 ]; then
+ kexec --unload
+ if [ $secureboot -eq 1 ]; then
echo "$TEST: kexec_load succeeded [FAIL]"
rc=1
else
echo "$TEST: kexec_load succeeded [PASS]"
fi
else
- if [ "$secureboot" == "1" ]; then
+ if [ $secureboot -eq 1 ]; then
echo "$TEST: kexec_load failed [PASS]"
else
echo "$TEST: kexec_load failed [FAIL]"
--
2.7.5


2019-01-31 18:58:30

by Mimi Zohar

[permalink] [raw]
Subject: [PATCH 3/3] selftests/ima: kexec_file_load syscall test

The kernel can be configured to verify PE signed kernel images, IMA
kernel image signatures, both types of signatures, or none. This test
verifies only properly signed kernel images are loaded into memory,
based on the kernel configuration and runtime policies.

Signed-off-by: Mimi Zohar <[email protected]>
---
tools/testing/selftests/ima/Makefile | 2 +-
.../testing/selftests/ima/test_kexec_file_load.sh | 250 +++++++++++++++++++++
2 files changed, 251 insertions(+), 1 deletion(-)
create mode 100755 tools/testing/selftests/ima/test_kexec_file_load.sh

diff --git a/tools/testing/selftests/ima/Makefile b/tools/testing/selftests/ima/Makefile
index 0b3adf5444b6..945fd203744c 100644
--- a/tools/testing/selftests/ima/Makefile
+++ b/tools/testing/selftests/ima/Makefile
@@ -4,7 +4,7 @@ uname_M := $(shell uname -m 2>/dev/null || echo not)
ARCH ?= $(shell echo $(uname_M) | sed -e s/i.86/x86/ -e s/x86_64/x86/)

ifeq ($(ARCH),x86)
-TEST_PROGS := test_kexec_load.sh
+TEST_PROGS := test_kexec_load.sh test_kexec_file_load.sh

include ../lib.mk

diff --git a/tools/testing/selftests/ima/test_kexec_file_load.sh b/tools/testing/selftests/ima/test_kexec_file_load.sh
new file mode 100755
index 000000000000..70819662ed6f
--- /dev/null
+++ b/tools/testing/selftests/ima/test_kexec_file_load.sh
@@ -0,0 +1,250 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0+
+#
+# Loading a kernel image via the kexec_file_load syscall can verify either
+# the IMA signature stored in the security.ima xattr or the PE signature,
+# both signatures depending on the IMA policy, or none.
+#
+# To determine whether the kernel image is signed, this test depends
+# on pesign and getfattr. This test also requires the kernel to be
+# built with CONFIG_IKCONFIG enabled and either CONFIG_IKCONFIG_PROC
+# enabled or access to the extract-ikconfig script.
+
+VERBOSE=1
+EXTRACT_IKCONFIG=$(ls /lib/modules/`uname -r`/source/scripts/extract-ikconfig)
+IKCONFIG=/tmp/config-`uname -r`
+PROC_CONFIG="/proc/config.gz"
+KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
+PESIGN=/usr/bin/pesign
+GETFATTR=/usr/bin/getfattr
+
+TEST="$0"
+. ./common_lib.sh
+
+# Kselftest framework requirement - SKIP code is 4.
+ksft_skip=4
+
+kconfig_enabled()
+{
+ RC=0
+ egrep -q $1 $IKCONFIG
+ if [ $? -eq 0 ]; then
+ RC=1
+ fi
+ return $RC
+}
+
+# policy rule format: action func=<keyword> [appraise_type=<type>]
+check_ima_policy()
+{
+ IMA_POLICY=/sys/kernel/security/ima/policy
+
+ RC=0
+ if [ $# -eq 3 ]; then
+ grep -e $2 $IMA_POLICY | grep -e "^$1.*$3" 2>&1 >/dev/null
+ else
+ grep -e $2 $IMA_POLICY | grep -e "^$1" 2>&1 >/dev/null
+ fi
+ if [ $? -eq 0 ]; then
+ RC=1
+ fi
+ return $RC
+}
+
+check_kconfig_options()
+{
+ # Attempt to get the kernel config first via proc, and then by
+ # extracting it from the kernel image using scripts/extract-ikconfig.
+ if [ ! -f $PROC_CONFIG ]; then
+ modprobe configs 2>/dev/null
+ fi
+ if [ -f $PROC_CONFIG ]; then
+ cat $PROC_CONFIG > $IKCONFIG
+ fi
+
+ if [ ! -f $IKCONFIG ]; then
+ if [ ! -f $EXTRACT_IKCONFIG ]; then
+ echo "$TEST: requires access to extract-ikconfig" >&2
+ exit $ksft_skip
+ fi
+
+ $EXTRACT_IKCONFIG $KERNEL_IMAGE > $IKCONFIG
+ kconfig_enabled "CONFIG_IKCONFIG=y"
+ if [ $? -eq 0 ]; then
+ echo "$TEST: requires the kernel to be built with CONFIG_IKCONFIG" >&2
+ exit $ksft_skip
+ fi
+ fi
+
+ kconfig_enabled "CONFIG_KEXEC_BZIMAGE_VERIFY_SIG=y"
+ pe_sig_required=$?
+ if [ $VERBOSE -ne 0 ] && [ $pe_sig_required -eq 1 ]; then
+ echo "$TEST: [INFO] PE signed kernel image required"
+ fi
+
+ kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS=y"
+ ima_sig_required=$?
+ if [ $VERBOSE -ne 0 ] && [ $ima_sig_required -eq 1 ]; then
+ echo "$TEST: [INFO] IMA kernel image signature required"
+ fi
+
+ kconfig_enabled "CONFIG_IMA_ARCH_POLICY=y"
+ arch_policy=$?
+ if [ $VERBOSE -ne 0 ] && [ $arch_policy -eq 1 ]; then
+ echo "$TEST: [INFO] architecture specific policy enabled"
+ fi
+
+ kconfig_enabled "CONFIG_INTEGRITY_PLATFORM_KEYRING=y"
+ platform_keyring=$?
+ if [ $VERBOSE -ne 0 ] && [ $platform_keyring -eq 1 ]; then
+ echo "$TEST: [INFO] platform kerying enabled"
+ fi
+
+ kconfig_enabled "CONFIG_IMA_READ_POLICY=y"
+ ima_read_policy=$?
+ if [ $VERBOSE -ne 0 ] && [ $ima_read_policy -eq 1 ]; then
+ echo "$TEST: [INFO] userspace can read IMA policy"
+ fi
+ rm $IKCONFIG
+}
+
+check_for_apps()
+{
+ if [ ! -f $PESIGN ]; then
+ PESIGN=$(which pesign 2>/dev/null)
+ if [ $? -eq 1 ]; then
+ echo "$TEST: requires pesign" >&2
+ exit $ksft_skip
+ else
+ echo "$TEST: [INFO] found $PESIGN"
+ fi
+ fi
+
+ if [ ! -f $GETFATTR ]; then
+ GETFATTR=$(which getfattr 2>/dev/null)
+ if [ $? -eq 1 ]; then
+ echo "$TEST: requires getfattr" >&2
+ exit $ksft_skip
+ else
+ echo "$TEST: [INFO] found $GETFATTR"
+ fi
+ fi
+}
+
+check_runtime()
+{
+ get_secureboot_mode
+ secureboot=$?
+ if [ $VERBOSE -ne 0 ] && [ $secureboot -eq 1 ]; then
+ echo "$TEST: [INFO] secure boot mode enabled"
+ fi
+ # The builtin "secure_boot" or custom policies might require an
+ # IMA signature. Check the runtime appraise policy rules
+ # (eg. <securityfs>/ima/policy). Policy rules are walked
+ # sequentially. As a result, a policy rule may be defined,
+ # but might not necessarily be used. This test assumes if a
+ # policy rule is specified, that is the intent.
+ if [ $ima_sig_required -eq 0 ] && [ $ima_read_policy -eq 1 ]; then
+ check_ima_policy "appraise" "func=KEXEC_KERNEL_CHECK" \
+ "appraise_type=imasig"
+ ima_sig_required=$?
+ if [ $VERBOSE -ne 0 ] && [ $ima_sig_required -eq 1 ]; then
+ echo "$TEST: [INFO] IMA signature required"
+ fi
+ fi
+}
+
+check_for_sigs()
+{
+ pe_signed=0
+ $PESIGN -i $KERNEL_IMAGE --show-signature | grep -q "No signatures"
+ pe_signed=$?
+ if [ $VERBOSE -ne 0 ]; then
+ if [ $pe_signed -eq 1 ]; then
+ echo "$TEST: [INFO] kexec kernel image PE signed"
+ else
+ echo "$TEST: [INFO] kexec kernel image not PE signed"
+ fi
+ fi
+
+ ima_signed=0
+ line=$($GETFATTR -n security.ima -e hex --absolute-names $KERNEL_IMAGE 2>&1)
+ echo $line | grep -q "security.ima=0x03"
+ if [ $? -eq 0 ]; then
+ ima_signed=1
+ if [ $VERBOSE -ne 0 ] ; then
+ echo "$TEST: [INFO] kexec kernel image IMA signed"
+ fi
+ elif [ $VERBOSE -ne 0 ]; then
+ echo "$TEST: [INFO] kexec kernel image not IMA signed"
+ fi
+}
+
+kexec_file_load_test()
+{
+ succeed_msg="$TEST: kexec_file_load succeeded "
+ failed_msg="$TEST: kexec_file_load failed "
+ platformkey_msg="try enabling the CONFIG_INTEGRITY_PLATFORM_KEYRING"
+ rc=0
+
+ line=$(kexec --load --kexec-file-syscall $KERNEL_IMAGE 2>&1)
+
+ # kexec_file_load succeeded. In secureboot mode with an architecture
+ # specific policy, make sure either an IMA or PE signature exists.
+ if [ $? -eq 0 ]; then
+ kexec --unload --kexec-file-syscall
+ if [ $arch_policy -eq 1 ] && [ $ima_signed -eq 0 ] && \
+ [ $pe_signed -eq 0 ]; then
+ echo $succeed_msg "(missing sigs) [FAIL]"
+ rc=1
+ elif [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
+ echo $succeed_msg "(missing imasig) [FAIL]"
+ rc=1
+ elif [ $pe_sig_required -eq 1 ] && [ $pe_signed -eq 0 ]; then
+ echo $succeed_msg "(missing PE sig) [FAIL]"
+ rc=1
+ elif [ $ima_read_policy -eq 0 ] && [ $ima_sig_required -eq 0 ] \
+ && [ $ima_signed -eq 0]; then
+ echo $succeed_msg "[UNKNOWN]"
+ else
+ echo $succeed_msg "[PASS]"
+ fi
+ return $rc
+ fi
+
+ # Check the reason for the kexec_file_load failure
+ echo $line | grep -q "Required key not available"
+ if [ $? -eq 0 ]; then
+ rc=1
+ if [ $platform_keyring -eq 0 ]; then
+ echo $failed_msg "(-ENOKEY)," $platformkey_msg
+ else
+ echo $failed_msg "(-ENOKEY)"
+ fi
+ elif [ $ima_sig_required -eq 1 ] && [ $ima_signed -eq 0 ]; then
+ echo $TEST: $failed_msg "[PASS]"
+ elif [ $pe_sig_required -eq 1 ] && [ $pe_signed -eq 0 ]; then
+ echo $TEST: $failed_msg "[PASS]"
+ elif [ $ima_read_policy -eq 0 ] && [ $ima_sig_required -eq 0 ] && \
+ [ $ima_signed -eq 0]; then
+ echo $failed_msg "[UNKNOWN]"
+ else
+ echo $TEST: $failed_msg "[FAIL]"
+ rc=1
+ fi
+ return $rc
+}
+
+# kexec requires root privileges
+if [ $(id -ru) != 0 ]; then
+ echo "$TEST: Requires root privileges" >&2
+ exit $ksft_skip
+fi
+
+check_kconfig_options
+check_for_apps
+check_runtime
+check_for_sigs
+kexec_file_load_test
+rc=$?
+exit $rc
--
2.7.5


2019-02-03 20:55:49

by Petr Vorel

[permalink] [raw]
Subject: Re: [PATCH 1/3] selftest/ima: cleanup the kexec selftest

Hi Mimi,

> Remove the few bashisms in the script and use the complete option name
> for clarity.

> Signed-off-by: Mimi Zohar <[email protected]>
Reviewed-by: Petr Vorel <[email protected]>
> ---
> tools/testing/selftests/ima/test_kexec_load.sh | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)

> diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
> index 1c10093fb526..74423c4229e2 100755
> --- a/tools/testing/selftests/ima/test_kexec_load.sh
> +++ b/tools/testing/selftests/ima/test_kexec_load.sh
> @@ -1,7 +1,7 @@
> #!/bin/sh
> # SPDX-License-Identifier: GPL-2.0+
# SPDX-License-Identifier: GPL-2.0-or-later
According to [1] GPL-2.0+ has been deprecated (but who cares).

...
> - if [ "$secureboot" == "1" ]; then
> +kexec --load $KERNEL_IMAGE 2>&1 /dev/null
kexec --load $KERNEL_IMAGE 2>&1 >/dev/null
missing redirection.

> +if [ $? -eq 0 ]; then
> + kexec --unload
> + if [ $secureboot -eq 1 ]; then
> echo "$TEST: kexec_load succeeded [FAIL]"
> rc=1
> else
> echo "$TEST: kexec_load succeeded [PASS]"
> fi
It'd be nice, if selftest has some main library with helpers (like LTP has [2]),
to have unified output and reduce duplicity.


Kind regards,
Petr

[1] https://spdx.org/licenses/
[2] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_test.sh

2019-02-03 21:27:08

by Petr Vorel

[permalink] [raw]
Subject: Re: [PATCH 2/3] scripts/ima: define a set of common functions

Hi Mimi,

> Define and move get_secureboot_mode() to a common file for use by other
> tests.

> Signed-off-by: Mimi Zohar <[email protected]>
Reviewed-by: Petr Vorel <[email protected]>
> ---
> tools/testing/selftests/ima/common_lib.sh | 20 ++++++++++++++++++++
> tools/testing/selftests/ima/test_kexec_load.sh | 17 +++--------------
> 2 files changed, 23 insertions(+), 14 deletions(-)
> create mode 100755 tools/testing/selftests/ima/common_lib.sh

> diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
> new file mode 100755
> index 000000000000..ae097a634da5
> --- /dev/null
> +++ b/tools/testing/selftests/ima/common_lib.sh
> @@ -0,0 +1,20 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0+
# SPDX-License-Identifier: GPL-2.0-or-later

> +
> +get_secureboot_mode()
> +{
> + EFIVARFS="/sys/firmware/efi/efivars"
local efivarfs="/sys/firmware/efi/efivars"
local file
It's a good practise to use local keyword and lower case the name of the
variable for variables used only locally (if you treat $EFIVARFS as constant,
I'd move it outside of get_secureboot_mode()).
I personally try to avoid using global variables (except constant like).

> + # Make sure that efivars is mounted in the normal location
> + if ! grep -q "^\S\+ $EFIVARFS efivarfs" /proc/mounts; then
> + echo "$TEST: efivars is not mounted on $EFIVARFS" >&2
> + exit $ksft_skip
> + fi
There could be helper function printing error and exit in selftest library.

> + # Get secureboot mode
> + file="$EFIVARFS/SecureBoot-*"
file="$efivarfs/SecureBoot-*"

...
> KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
Another candidate for helper for potential selftest library.

Kind regards,
Petr

2019-02-03 22:25:16

by Petr Vorel

[permalink] [raw]
Subject: Re: [PATCH 3/3] selftests/ima: kexec_file_load syscall test

Hi Mimi,

> The kernel can be configured to verify PE signed kernel images, IMA
> kernel image signatures, both types of signatures, or none. This test
> verifies only properly signed kernel images are loaded into memory,
> based on the kernel configuration and runtime policies.

> Signed-off-by: Mimi Zohar <[email protected]>
Reviewed-by: Petr Vorel <[email protected]>

...
> +++ b/tools/testing/selftests/ima/test_kexec_file_load.sh
> @@ -0,0 +1,250 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0+
# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Loading a kernel image via the kexec_file_load syscall can verify either
> +# the IMA signature stored in the security.ima xattr or the PE signature,
> +# both signatures depending on the IMA policy, or none.
> +#
> +# To determine whether the kernel image is signed, this test depends
> +# on pesign and getfattr. This test also requires the kernel to be
> +# built with CONFIG_IKCONFIG enabled and either CONFIG_IKCONFIG_PROC
> +# enabled or access to the extract-ikconfig script.
> +
> +VERBOSE=1
Maybe allow to disable verbose without source change?
VERBOSE="${VERBOSE:-1}"

> +EXTRACT_IKCONFIG=$(ls /lib/modules/`uname -r`/source/scripts/extract-ikconfig)
> +IKCONFIG=/tmp/config-`uname -r`
> +PROC_CONFIG="/proc/config.gz"
> +KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
> +PESIGN=/usr/bin/pesign
> +GETFATTR=/usr/bin/getfattr
> +
> +TEST="$0"
> +. ./common_lib.sh
> +
> +# Kselftest framework requirement - SKIP code is 4.
> +ksft_skip=4
> +
> +kconfig_enabled()
> +{
> + RC=0
> + egrep -q $1 $IKCONFIG
> + if [ $? -eq 0 ]; then
> + RC=1
> + fi
> + return $RC
> +}
This would be enough (grep with -e returns only 0 or 1):
kconfig_enabled()
{
grep -E -q $1 $IKCONFIG
}
> +
> +# policy rule format: action func=<keyword> [appraise_type=<type>]
> +check_ima_policy()
> +{
> + IMA_POLICY=/sys/kernel/security/ima/policy
> +
> + RC=0
> + if [ $# -eq 3 ]; then
> + grep -e $2 $IMA_POLICY | grep -e "^$1.*$3" 2>&1 >/dev/null
> + else
> + grep -e $2 $IMA_POLICY | grep -e "^$1" 2>&1 >/dev/null
> + fi
> + if [ $? -eq 0 ]; then
> + RC=1
> + fi
> + return $RC
> +}
This would be enough and more descriptive:
check_ima_policy()
{
local action="$1"
local keyword="$2"
local type="$3"

[ -n "$type" ] && type="appraise_type=$type"
grep -q "^$action.*func=$keyword.*$type" /sys/kernel/security/ima/policy
}

> +
> +check_kconfig_options()
> +{
> + # Attempt to get the kernel config first via proc, and then by
> + # extracting it from the kernel image using scripts/extract-ikconfig.
> + if [ ! -f $PROC_CONFIG ]; then
> + modprobe configs 2>/dev/null
> + fi
> + if [ -f $PROC_CONFIG ]; then
> + cat $PROC_CONFIG > $IKCONFIG
> + fi
> +
> + if [ ! -f $IKCONFIG ]; then
> + if [ ! -f $EXTRACT_IKCONFIG ]; then
> + echo "$TEST: requires access to extract-ikconfig" >&2
> + exit $ksft_skip
> + fi
> +
> + $EXTRACT_IKCONFIG $KERNEL_IMAGE > $IKCONFIG
> + kconfig_enabled "CONFIG_IKCONFIG=y"
> + if [ $? -eq 0 ]; then
> + echo "$TEST: requires the kernel to be built with CONFIG_IKCONFIG" >&2
> + exit $ksft_skip
> + fi
> + fi
> +
> + kconfig_enabled "CONFIG_KEXEC_BZIMAGE_VERIFY_SIG=y"
> + pe_sig_required=$?
> + if [ $VERBOSE -ne 0 ] && [ $pe_sig_required -eq 1 ]; then
> + echo "$TEST: [INFO] PE signed kernel image required"
> + fi
Checks for $VERBOSE here and in other kconfig_enabled usages bellow are a bit
redundant. And you check for assigned variable now and then later on,
you use these variables as global (and reset $ima_sig_required in
check_runtime().

How about using functions instead:
log_info()
{
echo "$TEST: [INFO] $1"
}
(Reducing some duplicity, IMHO helper functions in shell library used in all
selftest tests would be useful)

kconfig_enabled()
{
local config="$1"
local msg="$2"
local ret

grep -E -q $config $IKCONFIG
ret=$?
[ $VERBOSE -ne 0 ] && [ $ret -eq 1 ] && log_info "$msg"
return $ret
}

ima_sig_enabled()
{
kconfig_enabled "CONFIG_KEXEC_BZIMAGE_VERIFY_SIG=y" \
"PE signed kernel image required"
}

ima_sig_enabled()
{
kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS=y" \
"IMA kernel image signature required"
}
Warning is printed each time, but that's deliberate.
If it's not wanted, it can be moved into setup.

...
> +check_kconfig_options
> +check_for_apps
> +check_runtime
> +check_for_sigs
> +kexec_file_load_test

> +rc=$?
> +exit $rc
These two are redundant.

Kind regards,
Petr

2019-02-04 14:31:09

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH 3/3] selftests/ima: kexec_file_load syscall test

On Sun, 2019-02-03 at 23:02 +0100, Petr Vorel wrote:
> Hi Mimi,
>
> > The kernel can be configured to verify PE signed kernel images, IMA
> > kernel image signatures, both types of signatures, or none. This test
> > verifies only properly signed kernel images are loaded into memory,
> > based on the kernel configuration and runtime policies.
>
> > Signed-off-by: Mimi Zohar <[email protected]>
> Reviewed-by: Petr Vorel <[email protected]>

Thank you for the specific and generic suggestions to simplify/clean
up the tests!  The suggestions, below, and the "print" helpers will
really make a difference.

Mimi

>
> ...
> > +++ b/tools/testing/selftests/ima/test_kexec_file_load.sh
> > @@ -0,0 +1,250 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0+
> # SPDX-License-Identifier: GPL-2.0-or-later
> > +#
> > +# Loading a kernel image via the kexec_file_load syscall can verify either
> > +# the IMA signature stored in the security.ima xattr or the PE signature,
> > +# both signatures depending on the IMA policy, or none.
> > +#
> > +# To determine whether the kernel image is signed, this test depends
> > +# on pesign and getfattr. This test also requires the kernel to be
> > +# built with CONFIG_IKCONFIG enabled and either CONFIG_IKCONFIG_PROC
> > +# enabled or access to the extract-ikconfig script.
> > +
> > +VERBOSE=1
> Maybe allow to disable verbose without source change?
> VERBOSE="${VERBOSE:-1}"
>
> > +EXTRACT_IKCONFIG=$(ls /lib/modules/`uname -r`/source/scripts/extract-ikconfig)
> > +IKCONFIG=/tmp/config-`uname -r`
> > +PROC_CONFIG="/proc/config.gz"
> > +KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
> > +PESIGN=/usr/bin/pesign
> > +GETFATTR=/usr/bin/getfattr
> > +
> > +TEST="$0"
> > +. ./common_lib.sh
> > +
> > +# Kselftest framework requirement - SKIP code is 4.
> > +ksft_skip=4
> > +
> > +kconfig_enabled()
> > +{
> > + RC=0
> > + egrep -q $1 $IKCONFIG
> > + if [ $? -eq 0 ]; then
> > + RC=1
> > + fi
> > + return $RC
> > +}
> This would be enough (grep with -e returns only 0 or 1):
> kconfig_enabled()
> {
> grep -E -q $1 $IKCONFIG
> }
> > +
> > +# policy rule format: action func=<keyword> [appraise_type=<type>]
> > +check_ima_policy()
> > +{
> > + IMA_POLICY=/sys/kernel/security/ima/policy
> > +
> > + RC=0
> > + if [ $# -eq 3 ]; then
> > + grep -e $2 $IMA_POLICY | grep -e "^$1.*$3" 2>&1 >/dev/null
> > + else
> > + grep -e $2 $IMA_POLICY | grep -e "^$1" 2>&1 >/dev/null
> > + fi
> > + if [ $? -eq 0 ]; then
> > + RC=1
> > + fi
> > + return $RC
> > +}
> This would be enough and more descriptive:
> check_ima_policy()
> {
> local action="$1"
> local keyword="$2"
> local type="$3"
>
> [ -n "$type" ] && type="appraise_type=$type"
> grep -q "^$action.*func=$keyword.*$type" /sys/kernel/security/ima/policy
> }
>
> > +
> > +check_kconfig_options()
> > +{
> > + # Attempt to get the kernel config first via proc, and then by
> > + # extracting it from the kernel image using scripts/extract-ikconfig.
> > + if [ ! -f $PROC_CONFIG ]; then
> > + modprobe configs 2>/dev/null
> > + fi
> > + if [ -f $PROC_CONFIG ]; then
> > + cat $PROC_CONFIG > $IKCONFIG
> > + fi
> > +
> > + if [ ! -f $IKCONFIG ]; then
> > + if [ ! -f $EXTRACT_IKCONFIG ]; then
> > + echo "$TEST: requires access to extract-ikconfig" >&2
> > + exit $ksft_skip
> > + fi
> > +
> > + $EXTRACT_IKCONFIG $KERNEL_IMAGE > $IKCONFIG
> > + kconfig_enabled "CONFIG_IKCONFIG=y"
> > + if [ $? -eq 0 ]; then
> > + echo "$TEST: requires the kernel to be built with CONFIG_IKCONFIG" >&2
> > + exit $ksft_skip
> > + fi
> > + fi
> > +
> > + kconfig_enabled "CONFIG_KEXEC_BZIMAGE_VERIFY_SIG=y"
> > + pe_sig_required=$?
> > + if [ $VERBOSE -ne 0 ] && [ $pe_sig_required -eq 1 ]; then
> > + echo "$TEST: [INFO] PE signed kernel image required"
> > + fi
> Checks for $VERBOSE here and in other kconfig_enabled usages bellow are a bit
> redundant. And you check for assigned variable now and then later on,
> you use these variables as global (and reset $ima_sig_required in
> check_runtime().
>
> How about using functions instead:
> log_info()
> {
> echo "$TEST: [INFO] $1"
> }
> (Reducing some duplicity, IMHO helper functions in shell library used in all
> selftest tests would be useful)
>
> kconfig_enabled()
> {
> local config="$1"
> local msg="$2"
> local ret
>
> grep -E -q $config $IKCONFIG
> ret=$?
> [ $VERBOSE -ne 0 ] && [ $ret -eq 1 ] && log_info "$msg"
> return $ret
> }
>
> ima_sig_enabled()
> {
> kconfig_enabled "CONFIG_KEXEC_BZIMAGE_VERIFY_SIG=y" \
> "PE signed kernel image required"
> }
>
> ima_sig_enabled()
> {
> kconfig_enabled "CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS=y" \
> "IMA kernel image signature required"
> }
> Warning is printed each time, but that's deliberate.
> If it's not wanted, it can be moved into setup.
>
> ...
> > +check_kconfig_options
> > +check_for_apps
> > +check_runtime
> > +check_for_sigs
> > +kexec_file_load_test
>
> > +rc=$?
> > +exit $rc
> These two are redundant.
>
> Kind regards,
> Petr
>


2019-02-28 13:43:11

by Dave Young

[permalink] [raw]
Subject: Re: [PATCH 2/3] scripts/ima: define a set of common functions

Hi Mimi,

Sorry for jumping in late, just noticed this kexec selftests, I think we
also need a kexec load test not only for ima, but for general kexec

On 01/31/19 at 01:55pm, Mimi Zohar wrote:
> Define and move get_secureboot_mode() to a common file for use by other
> tests.
>
> Signed-off-by: Mimi Zohar <[email protected]>
> ---
> tools/testing/selftests/ima/common_lib.sh | 20 ++++++++++++++++++++
> tools/testing/selftests/ima/test_kexec_load.sh | 17 +++--------------
> 2 files changed, 23 insertions(+), 14 deletions(-)
> create mode 100755 tools/testing/selftests/ima/common_lib.sh
>
> diff --git a/tools/testing/selftests/ima/common_lib.sh b/tools/testing/selftests/ima/common_lib.sh
> new file mode 100755
> index 000000000000..ae097a634da5
> --- /dev/null
> +++ b/tools/testing/selftests/ima/common_lib.sh
> @@ -0,0 +1,20 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0+
> +
> +get_secureboot_mode()
> +{
> + EFIVARFS="/sys/firmware/efi/efivars"
> + # Make sure that efivars is mounted in the normal location
> + if ! grep -q "^\S\+ $EFIVARFS efivarfs" /proc/mounts; then
> + echo "$TEST: efivars is not mounted on $EFIVARFS" >&2
> + exit $ksft_skip
> + fi
> +
> + # Get secureboot mode
> + file="$EFIVARFS/SecureBoot-*"
> + if [ ! -e $file ]; then
> + echo "$TEST: unknown secureboot mode" >&2
> + exit $ksft_skip
> + fi
> + return `hexdump $file | awk '{print substr($4,length($4),1)}'`
> +}

Do you want to get the Secureboot status here?
I got some advice from Peter Jones previously, thus we have below
in our kdump scripts:
https://src.fedoraproject.org/cgit/rpms/kexec-tools.git/tree/kdump-lib.sh

See the function is_secure_boot_enforced(), probably you can refer to
that function and check setup mode as well.

> diff --git a/tools/testing/selftests/ima/test_kexec_load.sh b/tools/testing/selftests/ima/test_kexec_load.sh
> index 74423c4229e2..5e3566738888 100755
> --- a/tools/testing/selftests/ima/test_kexec_load.sh
> +++ b/tools/testing/selftests/ima/test_kexec_load.sh
> @@ -5,7 +5,7 @@
> # is booted in secureboot mode.
>
> TEST="$0"
> -EFIVARFS="/sys/firmware/efi/efivars"
> +. ./common_lib.sh
> rc=0
>
> # Kselftest framework requirement - SKIP code is 4.
> @@ -17,19 +17,8 @@ if [ $(id -ru) != 0 ]; then
> exit $ksft_skip
> fi
>
> -# Make sure that efivars is mounted in the normal location
> -if ! grep -q "^\S\+ $EFIVARFS efivarfs" /proc/mounts; then
> - echo "$TEST: efivars is not mounted on $EFIVARFS" >&2
> - exit $ksft_skip
> -fi
> -
> -# Get secureboot mode
> -file="$EFIVARFS/SecureBoot-*"
> -if [ ! -e $file ]; then
> - echo "$TEST: unknown secureboot mode" >&2
> - exit $ksft_skip
> -fi
> -secureboot=`hexdump $file | awk '{print substr($4,length($4),1)}'`
> +get_secureboot_mode
> +secureboot=$?
>
> # kexec_load should fail in secure boot mode
> KERNEL_IMAGE="/boot/vmlinuz-`uname -r`"
> --
> 2.7.5
>
Thanks
Dave

2019-02-28 16:35:04

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH 2/3] scripts/ima: define a set of common functions

Hi Dave,

On Thu, 2019-02-28 at 21:41 +0800, Dave Young wrote:
> Hi Mimi,
>
> Sorry for jumping in late, just noticed this kexec selftests, I think we
> also need a kexec load test not only for ima, but for general kexec

The IMA kselftest tests are for the coordination between the different
methods of verifying file signatures.  In particular, for the kexec
kernel image and kernel module signatures.

The initial IMA kselftest just verifies that in an environment
requiring signed kexec kernel images, the kexec_load syscall fails. 

This week I posted additional IMA kselftests[1][2], including one for
the kexec_file_load syscall.  I would really appreciate these
kselftests being reviewed/acked.

Mimi

[1] Subject: [PATCH v2 0/5] selftests/ima: add kexec and kernel module tests
[2] Patches available from the "next-queued-testing" branch
https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/


2019-03-08 02:45:38

by Dave Young

[permalink] [raw]
Subject: Re: [PATCH 2/3] scripts/ima: define a set of common functions

On 02/28/19 at 10:05am, Mimi Zohar wrote:
> Hi Dave,
>
> On Thu, 2019-02-28 at 21:41 +0800, Dave Young wrote:
> > Hi Mimi,
> >
> > Sorry for jumping in late, just noticed this kexec selftests, I think we
> > also need a kexec load test not only for ima, but for general kexec
>
> The IMA kselftest tests are for the coordination between the different
> methods of verifying file signatures. ?In particular, for the kexec
> kernel image and kernel module signatures.
>
> The initial IMA kselftest just verifies that in an environment
> requiring signed kexec kernel images, the kexec_load syscall fails.?
>
> This week I posted additional IMA kselftests[1][2], including one for
> the kexec_file_load syscall. ?I would really appreciate these
> kselftests being reviewed/acked.
>
> Mimi
>
> [1] Subject: [PATCH v2 0/5] selftests/ima: add kexec and kernel module tests
> [2] Patches available from the "next-queued-testing" branch
> https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git/
>

Hi Mimi,

Still did not get change to have a look at V2, but seems you missed the
last chunk of comments about the secure boot mode in previous reply?

I just copy it hear:
'''
Do you want to get the Secureboot status here?
I got some advice from Peter Jones previously, thus we have below
in our kdump scripts:
https://src.fedoraproject.org/cgit/rpms/kexec-tools.git/tree/kdump-lib.sh

See the function is_secure_boot_enforced(), probably you can refer to
that function and check setup mode as well.
'''

Thanks
Dave

2019-03-08 13:48:11

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH 2/3] scripts/ima: define a set of common functions

On Fri, 2019-03-08 at 10:44 +0800, Dave Young wrote:
> Hi Mimi,
>
> Still did not get change to have a look at V2, but seems you missed the
> last chunk of comments about the secure boot mode in previous reply?
>
> I just copy it hear:
> '''
> Do you want to get the Secureboot status here?
> I got some advice from Peter Jones previously, thus we have below
> in our kdump scripts:
> https://src.fedoraproject.org/cgit/rpms/kexec-tools.git/tree/kdump-lib.sh
>
> See the function is_secure_boot_enforced(), probably you can refer to
> that function and check setup mode as well.
> '''

Thank you for the pointer to the kdump scripts and reminder.

Mimi