linux-integrity.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] ima-evm-utils: Add some tests for evmctl
@ 2019-07-29  4:20 Vitaly Chikunov
  2019-07-30  8:27 ` Petr Vorel
  0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Chikunov @ 2019-07-29  4:20 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin, linux-integrity

Run `make check' to execute the tests.
Currently only ima_hash, sign, verify, ima_sign, and ima_verify are
tested.

Signed-off-by: Vitaly Chikunov <vt@altlinux.org>
---
Changelog since v2:
- ima_sign.test and ima_verify.test merged into sign_verify.test
  which is also able to test evm signatures.
- Apply Mimi Zohar suggestions regarding commenting, variable renaming,
  code rearranging, etc. with intent to simplify the code and review.

Changelog since v1:
- Apply suggestions by Petr Vorel:
 - Rename function names and variables to be more understandable.
 - Rename tests/functions to tests/functions.sh.
 - Define exit codes (77, 99, ...) as variables.
- Added more comments and remove single letter variables (for Mimi Zohar).
- Move getfattr check into function.
- Move evmctl run and check into single function.
- Add sign/verify tests for v1 signatures.
- Minor improvements.
- Since I still edit all 5 files I did not split the patch into multiple
  commits to separate the files, otherwise editing will become too
  complicated, as I ought to continuously rebase and edit different
  commits. This was really non-productive suggestion.

Please test the tests.
 .gitignore             |   2 +-
 Makefile.am            |   2 +-
 configure.ac           |   1 +
 tests/Makefile.am      |  12 ++
 tests/functions.sh     | 269 ++++++++++++++++++++++++++++++++++++++
 tests/gen-keys.sh      |  91 +++++++++++++
 tests/ima_hash.test    |  71 ++++++++++
 tests/sign_verify.test | 347 +++++++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 793 insertions(+), 2 deletions(-)
 create mode 100644 tests/Makefile.am
 create mode 100755 tests/functions.sh
 create mode 100755 tests/gen-keys.sh
 create mode 100755 tests/ima_hash.test
 create mode 100755 tests/sign_verify.test

diff --git a/.gitignore b/.gitignore
index cb82166..c579199 100644
--- a/.gitignore
+++ b/.gitignore
@@ -21,7 +21,7 @@ missing
 compile
 libtool
 ltmain.sh
-
+test-driver
 
 # Compiled executables
 *.o
diff --git a/Makefile.am b/Makefile.am
index dba408d..45c6f82 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -1,4 +1,4 @@
-SUBDIRS = src
+SUBDIRS = src tests
 dist_man_MANS = evmctl.1
 
 doc_DATA =  examples/ima-genkey-self.sh examples/ima-genkey.sh examples/ima-gen-local-ca.sh
diff --git a/configure.ac b/configure.ac
index eedf90e..d08d862 100644
--- a/configure.ac
+++ b/configure.ac
@@ -67,6 +67,7 @@ EVMCTL_MANPAGE_DOCBOOK_XSL
 
 AC_CONFIG_FILES([Makefile
 		src/Makefile
+		tests/Makefile
 		packaging/ima-evm-utils.spec
 		])
 AC_OUTPUT
diff --git a/tests/Makefile.am b/tests/Makefile.am
new file mode 100644
index 0000000..029f2ff
--- /dev/null
+++ b/tests/Makefile.am
@@ -0,0 +1,12 @@
+check_SCRIPTS =
+TESTS = $(check_SCRIPTS)
+
+check_SCRIPTS += ima_hash.test sign_verify.test
+
+clean-local:
+	-rm -f *.txt *.out *.sig *.sig2
+
+distclean: distclean-keys
+.PHONY: distclean-keys
+distclean-keys:
+	./gen-keys.sh clean
diff --git a/tests/functions.sh b/tests/functions.sh
new file mode 100755
index 0000000..2e3a7c3
--- /dev/null
+++ b/tests/functions.sh
@@ -0,0 +1,269 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# ima-evm-utils tests bash functions
+#
+# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
+#
+# 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; either version 2, or (at your option)
+# any later version.
+#
+# This program 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 General Public License for more details.
+
+# Tests accounting
+declare -i testspass=0 testsfail=0 testsskip=0
+
+# Exit codes (compatible with automake)
+declare -r OK=0
+declare -r FAIL=1
+declare -r HARDFAIL=99 # hard failure no matter testing mode
+declare -r SKIP=77     # skip test
+
+# You can set env VERBOSE=1 to see more output from evmctl
+V=vvvv
+V=${V:0:$VERBOSE}
+V=${V:+-$V}
+
+# Exit if env FAILEARLY is defined.
+# Used in expect_{pass,fail}.
+exit_early() {
+  if [ $FAILEARLY ]; then
+    exit $1
+  fi
+}
+
+# Require particular executables to be present
+_require() {
+  ret=
+  for i; do
+    if ! type $i; then
+      echo "$i is required for test"
+      ret=1
+    fi
+  done
+  [ $ret ] && exit $HARDFAIL
+}
+
+# Only allow color output on tty
+if tty -s; then
+     RED=$'\e[1;31m'
+   GREEN=$'\e[1;32m'
+  YELLOW=$'\e[1;33m'
+    BLUE=$'\e[1;34m'
+    CYAN=$'\e[1;36m'
+    NORM=$'\e[m'
+fi
+
+# Test mode determined by TFAIL variable:
+#   undefined: to success testing
+#   defined: failure testing
+TFAIL=
+TMODE=+ # mode character to prepend running command in log
+declare -i TNESTED=0 # just for sanity checking
+
+# Run positive test (one that should pass) and account its result
+expect_pass() {
+  local ret
+
+  if [ $TNESTED -gt 0 ]; then
+    echo $RED"expect_pass should not be run nested"$NORM
+    testsfail+=1
+    exit $HARDFAIL
+  fi
+  TFAIL=
+  TMODE=+
+  TNESTED+=1
+  [[ "$VERBOSE" -gt 1 ]] && echo "____ START positive test: $@"
+  "$@"
+  ret=$?
+  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) positive test: $@"
+  TNESTED+=-1
+  case $ret in
+    0)  testspass+=1 ;;
+    77) testsskip+=1 ;;
+    99) testsfail+=1; exit_early 1 ;;
+    *)  testsfail+=1; exit_early 2 ;;
+  esac
+  return $ret
+}
+
+# Eval negative test (one that should fail) and account its result
+expect_fail() {
+  local ret
+
+  if [ $TNESTED -gt 0 ]; then
+    echo $RED"expect_fail should not be run nested"$NORM
+    testsfail+=1
+    exit $HARDFAIL
+  fi
+
+  TFAIL=yes
+  TMODE=-
+  TNESTED+=1
+  [[ "$VERBOSE" -gt 1 ]] && echo "____ START negative test: $@"
+  "$@"
+  ret=$?
+  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) negative test: $@"
+  TNESTED+=-1
+  case $ret in
+    0)  testsfail+=1; exit_early 3 ;;
+    77) testsskip+=1 ;;
+    99) testsfail+=1; exit_early 4 ;;
+    *)  testspass+=1 ;;
+  esac
+  TFAIL= # Restore defaults for tests run without wrappers
+  TMODE=+
+  return $ret
+}
+
+# return true if current test is positive
+_is_expect_pass() {
+  [ ! $TFAIL ]
+}
+
+# return true if current test is negative
+_is_expect_fail() {
+  [ $TFAIL ]
+}
+
+# Show blank line and color following text to red
+# if it's real error (ie we are in expect_pass mode).
+red_if_failure() {
+  if _is_expect_pass; then
+    echo $@ $RED
+    COLOR_RESTORE=1
+  fi
+}
+
+# For hard errors
+red_always() {
+  echo $@ $RED
+  COLOR_RESTORE=1
+}
+
+color_restore() {
+  [ $COLOR_RESTORE ] && echo $@ $NORM
+  COLOR_RESTORE=
+}
+
+ADD_DEL=
+ADD_TEXT_FOR=
+# _evmctl_run should be run as `_evmctl_run ... || return'
+_evmctl_run() {
+  local op=$1 out=$1-$$.out
+  local text_for=${FOR:+for $ADD_TEXT_FOR}
+  # Additional parameters:
+  # ADD_DEL: additional files to rm on failure
+  # ADD_TEXT_FOR: append to text as 'for $ADD_TEXT_FOR'
+
+  cmd="evmctl $V ${ENGINE:+--engine $ENGINE} $@"
+  echo $YELLOW$TMODE $cmd$NORM
+  $cmd >$out 2>&1
+  ret=$?
+
+  # Shell special and signal exit codes (except 255)
+  if [ $ret -ge 126 -a $ret -lt 255 ]; then
+    red_always
+    echo "evmctl $op failed hard with ($ret) $text_for"
+    sed 's/^/  /' $out
+    color_restore
+    rm $out $ADD_DEL
+    ADD_DEL=
+    ADD_TEXT_FOR=
+    return $HARDFAIL
+  elif [ $ret -gt 0 ]; then
+    red_if_failure
+    echo "evmctl $op failed" ${TFAIL:+properly} "with ($ret) $text_for"
+    # Show evmctl output only in verbose mode or if real failure.
+    if _is_expect_pass || [ "$VERBOSE" ]; then
+      sed 's/^/  /' $out
+    fi
+    color_restore
+    rm $out $ADD_DEL
+    ADD_DEL=
+    ADD_TEXT_FOR=
+    return $FAIL
+  elif _is_expect_fail; then
+    red_always
+    echo "evmctl $op wrongly succeeded $text_for"
+    sed 's/^/  /' $out
+    color_restore
+  else
+    [ "$VERBOSE" ] && sed 's/^/  /' $out
+  fi
+  rm $out
+  ADD_DEL=
+  ADD_TEXT_FOR=
+  return $OK
+}
+
+# Extract xattr $attr from $file into $out file skipping $pref'ix
+_extract_xattr() {
+  local file=$1 attr=$2 out=$3 pref=$4
+
+  getfattr -n $attr -e hex $file \
+    | grep "^$attr=" \
+    | sed "s/^$attr=$pref//" \
+    | xxd -r -p > $out
+}
+
+# Test if xattr $attr in $file matches $pref'ix
+# Show error and fail otherwise.
+_test_xattr() {
+  local file=$1 attr=$2 pref=$3
+  local test_for=${ADD_TEXT_FOR:+ for $ADD_TEXT_FOR}
+
+  if ! getfattr -n $attr -e hex $file | egrep -qx "$attr=$pref"; then
+    red_if_failure
+    echo "Did not find expected hash$text_for:"
+    echo "    $attr=$pref"
+    echo ""
+    echo "Actual output below:"
+    getfattr -n $attr -e hex $file | sed 's/^/    /'
+    color_restore
+    rm $file
+    ADD_TEXT_FOR=
+    return $FAIL
+  fi
+  ADD_TEXT_FOR=
+}
+
+# Try to enable gost-engine if needed.
+_enable_gost_engine() {
+  # Do not enable if it's already working (enabled by user)
+  if ! openssl md_gost12_256 /dev/null >/dev/null 2>&1 \
+    && openssl engine gost >/dev/null 2>&1; then
+    ENGINE=gost
+  fi
+}
+
+# Show test stats and exit into automake test system
+# with proper exit code (same as ours).
+_report_exit() {
+  if [ $testsfail -gt 0 ]; then
+    echo "================================="
+    echo " Run with FAILEARLY=1 $0 $@"
+    echo " To stop after first failure"
+    echo "================================="
+  fi
+  [ $testspass -gt 0 ] && echo -n $GREEN || echo -n $NORM
+  echo -n "PASS: $testspass"
+  [ $testsskip -gt 0 ] && echo -n $YELLOW || echo -n $NORM
+  echo -n " SKIP: $testsskip"
+  [ $testsfail -gt 0 ] && echo -n $RED || echo -n $NORM
+  echo " FAIL: $testsfail"
+  echo $NORM
+  if [ $testsfail -gt 0 ]; then
+    exit $FAIL
+  elif [ $testspass -gt 0 ]; then
+    exit $OK
+  else
+    exit $SKIP
+  fi
+}
+
diff --git a/tests/gen-keys.sh b/tests/gen-keys.sh
new file mode 100755
index 0000000..6a31ac9
--- /dev/null
+++ b/tests/gen-keys.sh
@@ -0,0 +1,91 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# Generate keys for the tests
+#
+# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
+#
+# 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; either version 2, or (at your option)
+# any later version.
+#
+# This program 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 General Public License for more details.
+
+cd $(dirname $0)
+PATH=../src:$PATH
+type openssl
+
+log() {
+  echo - "$*"
+  eval "$@"
+}
+
+if [ "$1" = clean ]; then
+  rm -f test-ca.conf
+elif [ ! -e test-ca.conf ]; then
+cat > test-ca.conf <<- EOF
+	[ req ]
+	distinguished_name = req_distinguished_name
+	prompt = no
+	string_mask = utf8only
+	x509_extensions = v3_ca
+
+	[ req_distinguished_name ]
+	O = IMA-CA
+	CN = IMA/EVM certificate signing key
+	emailAddress = ca@ima-ca
+
+	[ v3_ca ]
+	basicConstraints=CA:TRUE
+	subjectKeyIdentifier=hash
+	authorityKeyIdentifier=keyid:always,issuer
+EOF
+fi
+
+# RSA
+# Second key will be used for wrong key tests.
+for m in 1024 2048; do
+  if [ "$1" = clean ]; then
+    rm -f test-rsa$m.cer test-rsa$m.key test-rsa$m.pub
+    continue
+  fi
+  if [ ! -e test-rsa$m.key ]; then
+    log openssl req -verbose -new -nodes -utf8 -sha1 -days 10000 -batch -x509 \
+      -config test-ca.conf \
+      -newkey rsa:$m \
+      -out test-rsa$m.cer -outform DER \
+      -keyout test-rsa$m.key
+    # for v1 signatures
+    log openssl pkey -in test-rsa$m.key -out test-rsa$m.pub -pubout
+  fi
+done
+
+# EC-RDSA
+for m in \
+  gost2012_256:A \
+  gost2012_256:B \
+  gost2012_256:C \
+  gost2012_512:A \
+  gost2012_512:B; do
+    IFS=':' read -r algo param <<< "$m"
+    if [ "$1" = clean ]; then
+      rm -f test-$algo-$param.key test-$algo-$param.cer test-$algo-$param.pub
+      continue
+    fi
+    [ -e test-$algo-$param.key ] && continue
+    log openssl req -nodes -x509 -utf8 -days 10000 -batch \
+      -config test-ca.conf \
+      -newkey $algo \
+      -pkeyopt paramset:$param \
+      -out    test-$algo-$param.cer -outform DER \
+      -keyout test-$algo-$param.key
+    log openssl pkey -in test-$algo-$param.key -out test-$algo-$param.pub -pubout
+done
+
+# This script leaves test-ca.conf, *.cer, *.pub, *.key files for sing/verify tests.
+# They are never deleted except by `make distclean'.
+
diff --git a/tests/ima_hash.test b/tests/ima_hash.test
new file mode 100755
index 0000000..718b868
--- /dev/null
+++ b/tests/ima_hash.test
@@ -0,0 +1,71 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# evmctl ima_hash tests
+#
+# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
+#
+# 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; either version 2, or (at your option)
+# any later version.
+#
+# This program 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 General Public License for more details.
+
+cd $(dirname $0)
+PATH=../src:$PATH
+source ./functions.sh
+_require evmctl openssl getfattr
+
+trap _report_exit EXIT
+set -f # disable globbing
+
+check() {
+  local alg=$1 pref=$2 hash=$3
+  local file=$alg-hash.txt
+
+  rm -f $file
+  touch $file
+  # Generate hash with openssl, if it's failed skip test,
+  # unless it's negative test, then pass to evmctl
+  cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$alg $file"
+  echo - $cmd
+  hash=$(set -o pipefail; $cmd 2>/dev/null | cut -d' ' -f2)
+  if [ $? -ne 0 ] && _is_expect_pass; then
+    echo $CYAN"$alg test is skipped"$NORM
+    rm $file
+    return $SKIP
+  fi
+
+  ADD_TEXT_FOR=$alg ADD_DEL=$file \
+    _evmctl_run ima_hash --hashalgo $alg --xattr-user $file || return
+  ADD_TEXT_FOR=$alg \
+    _test_xattr $file user.ima $pref$hash || return
+  rm $file
+  return $OK
+}
+
+# check args: algo prefix hex-hash
+expect_pass check md4    0x01
+expect_pass check md5    0x01
+expect_pass check sha1   0x01
+expect_fail check SHA1   0x01 # uppercase
+expect_fail check sha512-224 0x01 # valid for pkcs1
+expect_fail check sha512-256 0x01 # valid for pkcs1
+expect_fail check unknown 0x01 # nonexistent
+expect_pass check sha224 0x0407
+expect_pass check sha256 0x0404
+expect_pass check sha384 0x0405
+expect_pass check sha512 0x0406
+expect_pass check rmd160 0x0403
+expect_fail check sm3     0x01
+expect_fail check sm3-256 0x01
+_enable_gost_engine
+expect_pass check md_gost12_256 0x0412
+expect_pass check streebog256   0x0412
+expect_pass check md_gost12_512 0x0413
+expect_pass check streebog512   0x0413
+
diff --git a/tests/sign_verify.test b/tests/sign_verify.test
new file mode 100755
index 0000000..51cf152
--- /dev/null
+++ b/tests/sign_verify.test
@@ -0,0 +1,347 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+#
+# evmctl {,ima_}{sign,verify} tests
+#
+# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
+#
+# 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; either version 2, or (at your option)
+# any later version.
+#
+# This program 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 General Public License for more details.
+
+cd $(dirname $0)
+PATH=../src:$PATH
+source ./functions.sh
+_require evmctl openssl xxd getfattr
+./gen-keys.sh >/dev/null 2>&1
+
+trap _report_exit EXIT
+set -f # disable globbing
+
+# Determine keyid from cert
+_keyid() {
+  local cer=${1%.*}.cer cmd
+
+  cer=test-${cer#test-}
+  cmd="openssl x509 ${ENGINE:+-engine $ENGINE} \
+          -in $cer -inform DER -pubkey -noout"
+  id=$($cmd 2>/dev/null \
+    | openssl asn1parse \
+    | grep BIT.STRING \
+    | cut -d: -f1)
+  if [ -z "$id" ]; then
+    echo - $cmd >&2
+    echo "Cannot asn1parse $cer to determine keyid" >&2
+    exit 1
+  fi
+  openssl x509 ${ENGINE:+-engine $ENGINE} \
+      -in $cer -inform DER -pubkey -noout 2>/dev/null \
+    | openssl asn1parse -strparse $id -out - -noout \
+    | openssl dgst -c -sha1 \
+    | cut -d' ' -f2 \
+    | grep -o ":..:..:..:..$" \
+    | tr -d :
+}
+
+# Convert test $type into evmctl op prefix
+op() {
+  if [ $1 = ima ]; then
+    echo ima_
+  fi
+}
+
+# Convert test $type into xattr name
+xattr() {
+  if [ $1 = ima ]; then
+    echo user.ima
+  else
+    echo user.evm
+  fi
+}
+
+# Check that detached signature matches xattr signature
+_test_sigfile() {
+  local file=$1 attr=$2 file_sig=$3 file_sig2=$4
+
+  if [ ! -e $file_sig ]; then
+    red_always
+    echo "evmctl ima_sign: no detached signature $file_sig"
+    color_restore
+    rm $file
+    return $FAIL
+  fi
+
+  _extract_xattr $file $attr $file_sig2
+  if ! cmp -bl $file_sig $file_sig2; then
+    red_always
+    echo "evmctl ima_sign: xattr signature on $file differ from detached $file_sig"
+    color_restore
+    rm $file $file_sig $file_sig2
+    return $FAIL
+  fi
+
+  rm $file_sig $file_sig2
+}
+
+# Run single sign command
+_evmctl_sign() {
+  local type=$1 key=$2 alg=$3 file=$4 opts=$5
+
+  # Can check --sigfile for ima_sign
+  [ $type = ima ] && opts+=" --sigfile"
+
+  ADD_FOR_TEXT="$alg ($key)" ADD_DEL=$file \
+    _evmctl_run $(op $type)sign $opts \
+    --hashalgo $alg --key $key --xattr-user $file || return
+
+  if [ $type = ima ]; then
+    _test_sigfile $file $(xattr $type) $file.sig $file.sig2
+  fi
+}
+
+# Run and test {ima_,}sign operation
+check_sign() {
+  # Arguments are passed via global vars:
+  # TYPE (ima or evm),
+  # KEY,
+  # ALG (hash algo),
+  # PREF (signature header prefix),
+  # OPTS (additional options for evmctl),
+  # FILE (working file to sign).
+  local "$@"
+  local KEY=${KEY%.*}.key
+  local FILE=${FILE:-$ALG.txt}
+
+  # Normalize key filename
+  KEY=test-${KEY#test-}
+
+  # Append suffix to files for negative tests, because we may
+  # leave only good files for verify tests.
+  _is_expect_fail && FILE+='~'
+
+  rm -f $FILE
+  if ! touch $FILE; then
+    red_always
+    echo "Can't create test file: $FILE"
+    color_restore
+    return $HARDFAIL
+  fi
+
+  if _is_expect_pass; then
+    if [ ! -e $KEY ]; then
+      red_always
+      echo "Key $KEY does not exist."
+      color_restore
+      return $HARDFAIL
+    fi
+
+    # Can openssl work with this digest?
+    cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$ALG $FILE"
+    echo - $cmd
+    if ! $cmd >/dev/null; then
+      echo $CYAN"$ALG ($KEY) test is skipped (openssl is unable to digest)"$NORM
+      return $SKIP
+    fi
+
+    # Can openssl sign with this digest and key?
+    cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$ALG -sign $KEY -hex $FILE"
+    echo - $cmd
+    if ! $cmd >/dev/null; then
+      echo $CYAN"$ALG ($KEY) test is skipped (openssl is unable to sign)"$NORM
+      return $SKIP
+    fi
+  fi
+
+  # Fix keyid in the prefix.
+  if [[ $PREF =~ K ]]; then
+    keyid=$(_keyid $KEY)
+    if [ $? -ne 0 ]; then
+      red_always
+      echo "Unable to determine keyid for $KEY"
+      color_restore
+      return $HARDFAIL
+    fi
+    PREF=$(echo $PREF | sed "s/K/$keyid/")
+  fi
+
+  # Perform signing by evmctl
+  _evmctl_sign $TYPE $KEY $ALG $FILE "$OPTS" || return
+
+  # First simple pattern match the signature.
+  ADD_FOR_TEXT=$ALG \
+    _test_xattr $FILE $(xattr $TYPE) $PREF.* || return
+
+  # This is all we can do for v1 signatures.
+  [[ "$OPTS" =~ --rsa ]] && return $OK
+
+  # This is all we can do for evm.
+  [[ $TYPE =~ evm ]] && return $OK
+
+  # Extract signature to a file
+  _extract_xattr $FILE $(xattr $TYPE) $FILE.sig2 $PREF
+
+  # Verify extracted signature with openssl
+  cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$ALG -verify ${KEY%.*}.pub \
+	-signature $FILE.sig2 $FILE"
+  echo - $cmd
+  if ! $cmd; then
+    red_if_failure
+    echo "Signature v2 verification with openssl is failed."
+    color_restore
+    rm $FILE.sig2
+    return $FAIL
+  fi
+
+  rm $FILE.sig2
+  return $OK
+}
+
+# Test verify operation
+check_verify() {
+  # Arguments are passed via global vars:
+  # TYPE (ima or evm),
+  # KEY,
+  # ALG (hash algo),
+  # OPTS (additional options for evmctl),
+  # FILE (filename to verify).
+  local "$@"
+  local opts=$OPTS
+
+  if ! openssl dgst ${ENGINE:+-engine $ENGINE} -$ALG /dev/null >/dev/null 2>&1; then
+    echo $CYAN"$ALG ($KEY) test is skipped (openssl does not support $ALG)"$NORM
+    return $SKIP
+  fi
+
+  # verify cannot autodetect hash algo
+  [ $TYPE = evm ] && opts+=" --hashalgo $ALG"
+
+  ADD_FOR_TEXT="$FILE ($KEY)" \
+    _evmctl_run $(op $TYPE)verify --key $KEY --xattr-user $opts $FILE
+}
+
+# Perform sign and verify ima and evm testing
+sign_verify() {
+  local key=$1 alg=$2 pref="$3" opts="$4"
+  local file=$alg.txt
+
+  # Set defaults:
+  # Public key is different for v1 and v2 (where x509 cert is used).
+  if [[ $opts =~ --rsa ]]; then
+    KEY=test-$key.pub
+  else
+    KEY=test-$key.cer
+  fi
+  ALG=$alg
+  PREF=$pref
+  OPTS=$opts
+  FILE=$file
+
+  TYPE=ima
+  if expect_pass check_sign; then
+
+    # Normal verify with proper key should pass
+    expect_pass check_verify
+
+    # Multiple files and some don't verify
+    expect_fail check_verify FILE=/dev/null,$file
+  fi
+
+  TYPE=evm
+  if expect_pass check_sign; then
+    # Normal verify with proper key
+    expect_pass check_verify
+    # Verify with wrong key
+    expect_fail check_verify KEY=rsa2048
+  fi
+}
+
+# Test --keys
+try_different_keys() {
+  # v2 signing can work with multiple keys in --key option
+  if [[ ! $OPTS =~ --rsa ]]; then
+    # Have correct key in the key list
+    expect_pass check_verify KEY=test-rsa2048.cer,$KEY
+    expect_pass check_verify KEY=/dev/null,$KEY,
+  fi
+
+  # Try key that is not used for signing
+  expect_fail check_verify KEY=rsa2048
+  # Try completely wrong key files
+  expect_fail check_verify KEY=/dev/null
+  expect_fail check_verify KEY=/dev/zero
+}
+
+try_different_sigs() {
+  # Test --imasig
+  if expect_pass check_sign OPTS="$OPTS --imasig"; then
+    # Verify both evm and ima sigs
+    expect_pass check_verify
+    expect_pass check_verify TYPE=ima
+  fi
+
+  # Test --imahash
+  if expect_pass check_sign OPTS="$OPTS --imahash"; then
+    expect_pass check_verify
+    # IMA hash is not verifiable by ima_verify
+    expect_fail check_verify TYPE=ima
+  fi
+
+  # Test --portable
+  expect_pass check_sign OPTS="$OPTS --portable" PREF=0x05
+  # Cannot be verified
+
+  # Test -i (immutable)
+  expect_pass check_sign OPTS="$OPTS -i" PREF=0x0303
+  # Cannot be verified
+
+}
+
+#######################################################################
+# Single test args: type key hash signature-prefix "evmctl-options"
+# sign_verify args:      key hash signature-prefix "evmctl-options"
+# Only single test can be prefixed with expect_{fail,pass}
+# `sign_verify' can not be prefixed with expect_{fail,pass} because
+# it runs multiple tests inside. See more tests there.
+# signature-prefix can contain `K' which will be resolved to keyid (v2 only)
+
+## Test v1 signatures
+# Signature v1 only supports sha1 and sha256 so any other should fail
+expect_fail \
+  check_sign TYPE=ima KEY=rsa1024 ALG=md5 PREF=0x0301 OPTS=--rsa
+
+sign_verify  rsa1024  sha1    0x0301 --rsa
+sign_verify  rsa1024  sha256  0x0301 --rsa
+  try_different_keys
+  try_different_sigs
+
+## Test v2 signatures with RSA PKCS#1
+# List of allowed hashes much greater but not all are supported.
+sign_verify  rsa1024  md5     0x030201K0080
+sign_verify  rsa1024  sha1    0x030202K0080
+sign_verify  rsa1024  sha224  0x030207K0080
+sign_verify  rsa1024  sha256  0x030204K0080
+  try_different_keys
+  try_different_sigs
+sign_verify  rsa1024  sha384  0x030205K0080
+sign_verify  rsa1024  sha512  0x030206K0080
+sign_verify  rsa1024  rmd160  0x030203K0080
+
+# Test v2 signatures with EC-RDSA
+_enable_gost_engine
+sign_verify  gost2012_256-A md_gost12_256 0x030212K0040
+sign_verify  gost2012_256-B md_gost12_256 0x030212K0040
+sign_verify  gost2012_256-C md_gost12_256 0x030212K0040
+sign_verify  gost2012_512-A md_gost12_512 0x030213K0080
+sign_verify  gost2012_512-B md_gost12_512 0x030213K0080
+# Test if signing with wrong key length does not work.
+expect_fail \
+  check_sign TYPE=ima KEY=gost2012_512-B ALG=md_gost12_256 PREF=0x0302 OPTS=
+expect_fail \
+  check_sign TYPE=ima KEY=gost2012_256-B ALG=md_gost12_512 PREF=0x0302 OPTS=
+
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v3] ima-evm-utils: Add some tests for evmctl
  2019-07-29  4:20 [PATCH v3] ima-evm-utils: Add some tests for evmctl Vitaly Chikunov
@ 2019-07-30  8:27 ` Petr Vorel
  2019-07-30 10:07   ` Vitaly Chikunov
  0 siblings, 1 reply; 4+ messages in thread
From: Petr Vorel @ 2019-07-30  8:27 UTC (permalink / raw)
  To: Vitaly Chikunov; +Cc: Mimi Zohar, Dmitry Kasatkin, linux-integrity

Hi Vitaly,

only v1 runs without failure, when running v2 and v3 sign_verify fails:
(testing on on bash 5.0.7 on openSUSE and 5.0.3 on Debian)

evmctl is ../src/evmctl
openssl is /usr/bin/openssl
xxd is /usr/bin/xxd
getfattr is /usr/bin/getfattr
^[[1;33m- evmctl ima_sign --rsa --sigfile --hashalgo md5 --key test-rsa1024.key --xattr-user md5.txt~^[[m
evmctl ima_sign failed properly with (255) 
- openssl dgst -sha1 sha1.txt
- openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
^[[1;33m+ evmctl ima_sign --rsa --sigfile --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt^[[m
^[[1;33m+ evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa sha1.txt^[[m
^[[1;31m
evmctl ima_verify failed with (1) 
  Failed to d2i_X509_fp key file: test-rsa1024.pub
  openssl: error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
  openssl: error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error
^[[m
^[[1;33m- evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa /dev/null,sha1.txt^[[m
evmctl ima_verify failed properly with (255) 
- openssl dgst -sha1 sha1.txt
- openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
^[[1;33m+ evmctl sign --rsa --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt^[[m
^[[1;31m
evmctl sign failed with (255) 
  Failed to read UUID. Root access might require.
  errno: No data available (61)
^[[m
- openssl dgst -sha256 sha256.txt
- openssl dgst -sha256 -sign test-rsa1024.key -hex sha256.txt
^[[1;33m+ evmctl ima_sign --rsa --sigfile --hashalgo sha256 --key test-rsa1024.key --xattr-user sha256.txt^[[m
^[[1;33m+ evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa sha256.txt^[[m
^[[1;31m
evmctl ima_verify failed with (1) 
  Failed to d2i_X509_fp key file: test-rsa1024.pub
  openssl: error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
  openssl: error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error
^[[m
^[[1;33m- evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa /dev/null,sha256.txt^[[m
evmctl ima_verify failed properly with (255) 
- openssl dgst -sha256 sha256.txt
- openssl dgst -sha256 -sign test-rsa1024.key -hex sha256.txt
^[[1;33m+ evmctl sign --rsa --hashalgo sha256 --key test-rsa1024.key --xattr-user sha256.txt^[[m
^[[1;31m
evmctl sign failed with (255) 
  Failed to read UUID. Root access might require.
  errno: No data available (61)

...
> --- /dev/null
> +++ b/tests/functions.sh
> @@ -0,0 +1,269 @@
> +#!/bin/bash
I'd still vote for /bin/sh and posix syntax. evmctl can be cross compiled for
embedded device. Than, it'd have to be run on the target, which can be tiny
linux customised distro with busybox shell. That's why POSIX shell syntax
instead of relying on bash.

Debian also has nice script that can be used to check for non-portable shell code:
https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl

> +# SPDX-License-Identifier: GPL-2.0
This is the correct SPDX for GPL v2+:
# SPDX-License-Identifier: GPL-2.0-or-later

> +#
> +# ima-evm-utils tests bash functions
> +#
> +# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
> +#
> +# 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; either version 2, or (at your option)
> +# any later version.
> +#
> +# This program 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 General Public License for more details.
SPDX license identifier replaces GPL verbose text. These 2 paragraphs can be
deleted.

> +
> +# Tests accounting
> +declare -i testspass=0 testsfail=0 testsskip=0
> +
> +# Exit codes (compatible with automake)
> +declare -r OK=0
Can you please omit 'declare -r'? It's nice to have read only variables, but
it's a bashism (not working in busybox nor in dash).

> +declare -r FAIL=1
> +declare -r HARDFAIL=99 # hard failure no matter testing mode
> +declare -r SKIP=77     # skip test
Also comment "skip test" might be omitted (it's obvious).

> +
> +# You can set env VERBOSE=1 to see more output from evmctl
> +V=vvvv
> +V=${V:0:$VERBOSE}
> +V=${V:+-$V}
Maybe it's just me but it's not really readable.

> +
> +# Exit if env FAILEARLY is defined.
> +# Used in expect_{pass,fail}.
> +exit_early() {
> +  if [ $FAILEARLY ]; then
> +    exit $1
> +  fi
> +}
> +
> +# Require particular executables to be present
> +_require() {
> +  ret=
> +  for i; do
> +    if ! type $i; then
> +      echo "$i is required for test"
> +      ret=1
> +    fi
> +  done
> +  [ $ret ] && exit $HARDFAIL
> +}
> +
> +# Only allow color output on tty
> +if tty -s; then
BTW I'd prefer the output to file to be plain text (I have vim plugin to show
colors, but not everybody has). Now it does not (forcing by red_always ?).
[ -t 1 ] will do the check.
Or, you could even have variable to allow user to chose, example from LTP [1]

tst_color_enabled()
{
	[ "$LTP_COLORIZE_OUTPUT" = "n" ] || [ "$LTP_COLORIZE_OUTPUT" = "0" ] && return 0
	[ "$LTP_COLORIZE_OUTPUT" = "y" ] || [ "$LTP_COLORIZE_OUTPUT" = "1" ] && return 1
	[ -t 1 ] || return 0
	return 1
}

> +     RED=$'\e[1;31m'
> +   GREEN=$'\e[1;32m'
> +  YELLOW=$'\e[1;33m'
> +    BLUE=$'\e[1;34m'
> +    CYAN=$'\e[1;36m'
> +    NORM=$'\e[m'
> +fi
> +
> +# Test mode determined by TFAIL variable:
> +#   undefined: to success testing
> +#   defined: failure testing
> +TFAIL=
> +TMODE=+ # mode character to prepend running command in log
> +declare -i TNESTED=0 # just for sanity checking
> +
> +# Run positive test (one that should pass) and account its result
> +expect_pass() {
> +  local ret
> +
> +  if [ $TNESTED -gt 0 ]; then
> +    echo $RED"expect_pass should not be run nested"$NORM
> +    testsfail+=1
> +    exit $HARDFAIL
> +  fi
> +  TFAIL=
> +  TMODE=+
> +  TNESTED+=1
> +  [[ "$VERBOSE" -gt 1 ]] && echo "____ START positive test: $@"
Bashism, use [ ]
> +  "$@"
> +  ret=$?
> +  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) positive test: $@"
> +  TNESTED+=-1
> +  case $ret in
> +    0)  testspass+=1 ;;
Bashism, use testspass=$((testspass+1))
> +    77) testsskip+=1 ;;
> +    99) testsfail+=1; exit_early 1 ;;
> +    *)  testsfail+=1; exit_early 2 ;;
> +  esac
> +  return $ret
> +}
> +
> +# Eval negative test (one that should fail) and account its result
> +expect_fail() {
> +  local ret
> +
> +  if [ $TNESTED -gt 0 ]; then
> +    echo $RED"expect_fail should not be run nested"$NORM
IMHO it's safer to quote whole string (here ok, but on some circumstances shell
handles quoted and unquoted string differently):
echo "$REDexpect_fail should not be run nested $NORM"

> +    testsfail+=1
> +    exit $HARDFAIL
> +  fi
> +
> +  TFAIL=yes
> +  TMODE=-
> +  TNESTED+=1
> +  [[ "$VERBOSE" -gt 1 ]] && echo "____ START negative test: $@"
> +  "$@"
> +  ret=$?
> +  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) negative test: $@"
> +  TNESTED+=-1
> +  case $ret in
> +    0)  testsfail+=1; exit_early 3 ;;
> +    77) testsskip+=1 ;;
> +    99) testsfail+=1; exit_early 4 ;;
> +    *)  testspass+=1 ;;
> +  esac
> +  TFAIL= # Restore defaults for tests run without wrappers
> +  TMODE=+
I guess you wanted TMODE=
> +  return $ret
> +}
...

> +ADD_DEL=
> +ADD_TEXT_FOR=
> +# _evmctl_run should be run as `_evmctl_run ... || return'
> +_evmctl_run() {
> +  local op=$1 out=$1-$$.out
> +  local text_for=${FOR:+for $ADD_TEXT_FOR}
> +  # Additional parameters:
> +  # ADD_DEL: additional files to rm on failure
> +  # ADD_TEXT_FOR: append to text as 'for $ADD_TEXT_FOR'
> +
> +  cmd="evmctl $V ${ENGINE:+--engine $ENGINE} $@"
> +  echo $YELLOW$TMODE $cmd$NORM
> +  $cmd >$out 2>&1
> +  ret=$?
> +
> +  # Shell special and signal exit codes (except 255)
> +  if [ $ret -ge 126 -a $ret -lt 255 ]; then
> +    red_always
> +    echo "evmctl $op failed hard with ($ret) $text_for"
> +    sed 's/^/  /' $out
> +    color_restore
I'd reset terminal inside coloring functions (color_restore() and $NORM).
red()
{
	printf '\033[1;31m'
	echo "$@"
	printf '\033[0m'
}

red "evmctl $op failed hard with ($ret) $text_for"

> +    rm $out $ADD_DEL
> +    ADD_DEL=
> +    ADD_TEXT_FOR=
> +    return $HARDFAIL
> +  elif [ $ret -gt 0 ]; then
> +    red_if_failure
> +    echo "evmctl $op failed" ${TFAIL:+properly} "with ($ret) $text_for"
> +    # Show evmctl output only in verbose mode or if real failure.
> +    if _is_expect_pass || [ "$VERBOSE" ]; then
> +      sed 's/^/  /' $out
> +    fi
> +    color_restore
> +    rm $out $ADD_DEL
> +    ADD_DEL=
> +    ADD_TEXT_FOR=
> +    return $FAIL
> +  elif _is_expect_fail; then
> +    red_always
> +    echo "evmctl $op wrongly succeeded $text_for"
> +    sed 's/^/  /' $out
> +    color_restore
> +  else
> +    [ "$VERBOSE" ] && sed 's/^/  /' $out
> +  fi
> +  rm $out
> +  ADD_DEL=
> +  ADD_TEXT_FOR=
> +  return $OK
> +}
...

> diff --git a/tests/ima_hash.test b/tests/ima_hash.test
> new file mode 100755
> index 0000000..718b868
> --- /dev/null
> +++ b/tests/ima_hash.test
> @@ -0,0 +1,71 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# evmctl ima_hash tests
> +#
> +# Copyright (C) 2019 Vitaly Chikunov <vt@altlinux.org>
> +#
> +# 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; either version 2, or (at your option)
> +# any later version.
> +#
> +# This program 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 General Public License for more details.
> +
> +cd $(dirname $0)
> +PATH=../src:$PATH

> +source ./functions.sh
Bashism, use . 

Maybe

. $(dirname $0)/functions.sh

and in functions.sh
export PATH="../src:$PATH"

(that reduces duplicity, but then you'd need to source it also in tests/gen-keys.sh)

> +_require evmctl openssl getfattr
> +
> +trap _report_exit EXIT
> +set -f # disable globbing
> +
> +check() {
> +  local alg=$1 pref=$2 hash=$3
> +  local file=$alg-hash.txt
> +
> +  rm -f $file
> +  touch $file
> +  # Generate hash with openssl, if it's failed skip test,
> +  # unless it's negative test, then pass to evmctl
> +  cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$alg $file"
Why not set ENGINE once at the beginning of the script (or in functions.sh if
needed on more places)?

...
> diff --git a/tests/sign_verify.test b/tests/sign_verify.test
...

> +# Convert test $type into evmctl op prefix
> +op() {
> +  if [ $1 = ima ]; then
> +    echo ima_
> +  fi
Maybe just
[ "$1" = "ima" ] && echo "ima_"

> +}
> +
> +# Convert test $type into xattr name
> +xattr() {
> +  if [ $1 = ima ]; then
> +    echo user.ima
> +  else
> +    echo user.evm
> +  fi
Maybe just:
[ "$1" = "ima" ] && echo "user.ima" || echo "user.evm"

...
> +# Run and test {ima_,}sign operation
> +check_sign() {
> +  # Arguments are passed via global vars:
> +  # TYPE (ima or evm),
> +  # KEY,
> +  # ALG (hash algo),
> +  # PREF (signature header prefix),
> +  # OPTS (additional options for evmctl),
> +  # FILE (working file to sign).
> +  local "$@"
Not sure if this works everywhere as you expect.
In most of the cases you set these global variables anyway, so cleaner would be
to use getopts (instead overwriting global variables).
> +  local KEY=${KEY%.*}.key
> +  local FILE=${FILE:-$ALG.txt}
I'd use lower case for local variables.
> +
> +  # Normalize key filename
> +  KEY=test-${KEY#test-}
> +
> +  # Append suffix to files for negative tests, because we may
> +  # leave only good files for verify tests.
> +  _is_expect_fail && FILE+='~'
> +
> +  rm -f $FILE
> +  if ! touch $FILE; then
> +    red_always
> +    echo "Can't create test file: $FILE"
> +    color_restore
> +    return $HARDFAIL
> +  fi
> +
> +  if _is_expect_pass; then
> +    if [ ! -e $KEY ]; then
> +      red_always
> +      echo "Key $KEY does not exist."
> +      color_restore
> +      return $HARDFAIL
> +    fi
> +
> +    # Can openssl work with this digest?
> +    cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$ALG $FILE"
> +    echo - $cmd
> +    if ! $cmd >/dev/null; then
> +      echo $CYAN"$ALG ($KEY) test is skipped (openssl is unable to digest)"$NORM
> +      return $SKIP
> +    fi
> +
> +    # Can openssl sign with this digest and key?
> +    cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$ALG -sign $KEY -hex $FILE"
> +    echo - $cmd
> +    if ! $cmd >/dev/null; then
> +      echo $CYAN"$ALG ($KEY) test is skipped (openssl is unable to sign)"$NORM
> +      return $SKIP
> +    fi
> +  fi
> +
> +  # Fix keyid in the prefix.
> +  if [[ $PREF =~ K ]]; then
Bashism, could you please use grep instead of [[ =~ ]]?
...


Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/blob/master/testcases/lib/tst_ansi_color.sh

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v3] ima-evm-utils: Add some tests for evmctl
  2019-07-30  8:27 ` Petr Vorel
@ 2019-07-30 10:07   ` Vitaly Chikunov
  2019-07-30 23:24     ` Petr Vorel
  0 siblings, 1 reply; 4+ messages in thread
From: Vitaly Chikunov @ 2019-07-30 10:07 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Mimi Zohar, Dmitry Kasatkin, linux-integrity

Petr,

On Tue, Jul 30, 2019 at 10:27:43AM +0200, Petr Vorel wrote:
> only v1 runs without failure, when running v2 and v3 sign_verify fails:
> (testing on on bash 5.0.7 on openSUSE and 5.0.3 on Debian)

v3 should be run over all my other fixes I sent to pass.

> evmctl is ../src/evmctl
> openssl is /usr/bin/openssl
> xxd is /usr/bin/xxd
> getfattr is /usr/bin/getfattr
> ^[[1;33m- evmctl ima_sign --rsa --sigfile --hashalgo md5 --key test-rsa1024.key --xattr-user md5.txt~^[[m
> evmctl ima_sign failed properly with (255) 
> - openssl dgst -sha1 sha1.txt
> - openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
> ^[[1;33m+ evmctl ima_sign --rsa --sigfile --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt^[[m
> ^[[1;33m+ evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa sha1.txt^[[m
> ^[[1;31m
> evmctl ima_verify failed with (1) 
>   Failed to d2i_X509_fp key file: test-rsa1024.pub
>   openssl: error:0D0680A8:asn1 encoding routines:asn1_check_tlen:wrong tag
>   openssl: error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error

evmctl w/o `-v` does not show why verify is failed.

This "Failed to d2i_X509_fp key file" is failure to read multiple keys
which is happen _before_ verify operation. (This is fixed in
"ima-evm-utils: Do not load keys from x509 certs if user pass --rsa").

You may re-run test with VERBOSE=1 ./sign_verify.test for more output to
see why it's actually failed to verify.

But, if you didn't apply my other fixes it's probably that public key is
not loaded.

So, this was valid bug in evmctl found by the tests.

> ^[[1;33m- evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa /dev/null,sha1.txt^[[m
> evmctl ima_verify failed properly with (255) 
> - openssl dgst -sha1 sha1.txt
> - openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
> ^[[1;33m+ evmctl sign --rsa --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt^[[m
> ^[[1;31m
> evmctl sign failed with (255) 
>   Failed to read UUID. Root access might require.

I never got this error myself (always running the tests under user). This
is error from `blkid'.

I should pass `--uuid ...' for this test, thanks.

> > --- /dev/null
> > +++ b/tests/functions.sh
> > @@ -0,0 +1,269 @@
> > +#!/bin/bash
> I'd still vote for /bin/sh and posix syntax. evmctl can be cross compiled for
> embedded device. Than, it'd have to be run on the target, which can be tiny
> linux customised distro with busybox shell. That's why POSIX shell syntax
> instead of relying on bash.

Tests are not required to compile evmctl.
On tiny embedded box you may not have gcc so lets code on perl instead.
You may not have also asciidoc but it is used in ima-evm-utils build.

> Debian also has nice script that can be used to check for non-portable shell code:
> https://salsa.debian.org/debian/devscripts/raw/master/scripts/checkbashisms.pl

I know, but I disagree to port tests from bash to posix shell.

> > +# SPDX-License-Identifier: GPL-2.0
> This is the correct SPDX for GPL v2+:
> # SPDX-License-Identifier: GPL-2.0-or-later

I am not expert is this, but, as I understand, text in license "or later"
doesn't mean license is GPL-2.0-or-later, because both texts contain "or
later" statement.

  https://spdx.org/licenses/GPL-2.0-only.html
  https://spdx.org/licenses/GPL-2.0-or-later.html

GPL-2.0 is ambiguous enough like real license is.

I agree to change to license which is stated in COPYING of
ima-evm-utils, but what it's correct tag?

> > +# 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; either version 2, or (at your option)
> > +# any later version.
> > +#
> > +# This program 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 General Public License for more details.
> SPDX license identifier replaces GPL verbose text. These 2 paragraphs can be
> deleted.

All countries agree to that?

> > +# Exit codes (compatible with automake)
> > +declare -r OK=0
> Can you please omit 'declare -r'? It's nice to have read only variables, but
> it's a bashism (not working in busybox nor in dash).

Please no dash propaganda.

> > +declare -r FAIL=1
> > +declare -r HARDFAIL=99 # hard failure no matter testing mode
> > +declare -r SKIP=77     # skip test
> Also comment "skip test" might be omitted (it's obvious).

Hm.

> > +# You can set env VERBOSE=1 to see more output from evmctl
> > +V=vvvv
> > +V=${V:0:$VERBOSE}
> > +V=${V:+-$V}
> Maybe it's just me but it's not really readable.

Yeah. But that would you suggest to make number of -v in V the same as
number in $VERBOSE?

> > +# Only allow color output on tty
> > +if tty -s; then
> BTW I'd prefer the output to file to be plain text (I have vim plugin to show
> colors, but not everybody has). Now it does not (forcing by red_always ?).
> [ -t 1 ] will do the check.
> Or, you could even have variable to allow user to chose, example from LTP [1]

Yes I will disable coloring if tests are run inside of make.
I neglected to fix that, because I found colored test-suite.log is still
useful. (Even though less -R does not show red colored parts.)

> > +  [[ "$VERBOSE" -gt 1 ]] && echo "____ START positive test: $@"
> Bashism, use [ ]

This is intentional since ['s -gt will output error if $VERBOSE is not
a number.

> > +  case $ret in
> > +    0)  testspass+=1 ;;
> Bashism, use testspass=$((testspass+1))
> > +    77) testsskip+=1 ;;
> > +    99) testsfail+=1; exit_early 1 ;;
> > +    *)  testsfail+=1; exit_early 2 ;;
> > +  esac
> > +  return $ret
> > +}
> > +
> > +# Eval negative test (one that should fail) and account its result
> > +expect_fail() {
> > +  local ret
> > +
> > +  if [ $TNESTED -gt 0 ]; then
> > +    echo $RED"expect_fail should not be run nested"$NORM
> IMHO it's safer to quote whole string (here ok, but on some circumstances shell
> handles quoted and unquoted string differently):
> echo "$REDexpect_fail should not be run nested $NORM"

I think quoting there is just a matter of style, not of safety.

> > +  TFAIL=yes
> > +  TMODE=-
> > +  TNESTED+=1
> > +  [[ "$VERBOSE" -gt 1 ]] && echo "____ START negative test: $@"
> > +  "$@"
> > +  ret=$?
> > +  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) negative test: $@"
> > +  TNESTED+=-1
> > +  case $ret in
> > +    0)  testsfail+=1; exit_early 3 ;;
> > +    77) testsskip+=1 ;;
> > +    99) testsfail+=1; exit_early 4 ;;
> > +    *)  testspass+=1 ;;
> > +  esac
> > +  TFAIL= # Restore defaults for tests run without wrappers
> > +  TMODE=+
> I guess you wanted TMODE=

No. Comment says I want to restore defaults, and default values for
these are same as for positive testing. I may expend comment.

> > +  if [ $ret -ge 126 -a $ret -lt 255 ]; then
> > +    red_always
> > +    echo "evmctl $op failed hard with ($ret) $text_for"
> > +    sed 's/^/  /' $out
> > +    color_restore
> I'd reset terminal inside coloring functions (color_restore() and $NORM).
> red()
> {
> 	printf '\033[1;31m'
> 	echo "$@"
> 	printf '\033[0m'
> }

What do you mean? Hm...

Argument in `red_always' is just to pass `-n` if needed. But it was
never needed, so I can delete it. Blank line was intentional. And
it enables red for all next lines until `color_restore' sends color 
restore.

> red "evmctl $op failed hard with ($ret) $text_for"

This may be useful for red oneliners, I thought about them, but not
added. I thought it's beneficent that I output only with `echo' and not
with some unusual custom printing function.

> > +cd $(dirname $0)
> > +PATH=../src:$PATH
> 
> > +source ./functions.sh
> Bashism, use . 

Since I don't plan to remove bashisms...

> Maybe
> 
> . $(dirname $0)/functions.sh
> 
> and in functions.sh
> export PATH="../src:$PATH"
> 
> (that reduces duplicity, but then you'd need to source it also in tests/gen-keys.sh)

You forgot to `cd'. To put it in functions.sh would be too strong side
effect.

> > +  cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$alg $file"
> Why not set ENGINE once at the beginning of the script (or in functions.sh if
> needed on more places)?

It could be replaced with two env variables.
  One for openssl with `-engine x'
  and another for evmctl with `--engine x'.
It's possible but this is purely optimisation. Maybe OK.

> > +# Convert test $type into evmctl op prefix
> > +op() {
> > +  if [ $1 = ima ]; then
> > +    echo ima_
> > +  fi
> Maybe just
> [ "$1" = "ima" ] && echo "ima_"

Maybe.

> > +# Convert test $type into xattr name
> > +xattr() {
> > +  if [ $1 = ima ]; then
> > +    echo user.ima
> > +  else
> > +    echo user.evm
> > +  fi
> Maybe just:
> [ "$1" = "ima" ] && echo "user.ima" || echo "user.evm"

Matter of style. Besides `x && y || z` is not too understandable in
shell context for general person. (Yes I use it too in non critical part
to output colors). But, what if second || affects result of echo and not
of [? (Question about ease of understandability).

> > +  local "$@"
> Not sure if this works everywhere as you expect.

It works in bash.

> In most of the cases you set these global variables anyway, so cleaner would be
> to use getopts (instead overwriting global variables).

This would require a lot of rework and make test more complicated?

> > +  local KEY=${KEY%.*}.key
> > +  local FILE=${FILE:-$ALG.txt}
> I'd use lower case for local variables.

But they are globals (becoming locals).

> > +  # Fix keyid in the prefix.
> > +  if [[ $PREF =~ K ]]; then
> Bashism, could you please use grep instead of [[ =~ ]]?

If I were against bashisms I'd suggest using expr.

> Kind regards,

Thanks for the review!

Thanks,

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH v3] ima-evm-utils: Add some tests for evmctl
  2019-07-30 10:07   ` Vitaly Chikunov
@ 2019-07-30 23:24     ` Petr Vorel
  0 siblings, 0 replies; 4+ messages in thread
From: Petr Vorel @ 2019-07-30 23:24 UTC (permalink / raw)
  To: Mimi Zohar, Dmitry Kasatkin, linux-integrity

Hi Vitaly,

> evmctl w/o `-v` does not show why verify is failed.

> This "Failed to d2i_X509_fp key file" is failure to read multiple keys
> which is happen _before_ verify operation. (This is fixed in
> "ima-evm-utils: Do not load keys from x509 certs if user pass --rsa").

> You may re-run test with VERBOSE=1 ./sign_verify.test for more output to
> see why it's actually failed to verify.

> But, if you didn't apply my other fixes it's probably that public key is
> not loaded.
I tested v3 on the top of current next, i.e. on the top of commit cf1b8fd
("ima-evm-utils: Allow EVM verify to determine hash algo").

> So, this was valid bug in evmctl found by the tests.

> > ^[[1;33m- evmctl ima_verify --key test-rsa1024.pub --xattr-user --rsa /dev/null,sha1.txt^[[m
> > evmctl ima_verify failed properly with (255) 
> > - openssl dgst -sha1 sha1.txt
> > - openssl dgst -sha1 -sign test-rsa1024.key -hex sha1.txt
> > ^[[1;33m+ evmctl sign --rsa --hashalgo sha1 --key test-rsa1024.key --xattr-user sha1.txt^[[m
> > ^[[1;31m
> > evmctl sign failed with (255) 
> >   Failed to read UUID. Root access might require.

> I never got this error myself (always running the tests under user). This
> is error from `blkid'.

> I should pass `--uuid ...' for this test, thanks.
With proper cleanup VERBOSE=1 ./sign_verify.test has now only 10 failures, all
about missing key test-gost2012_256-*.key.
Now command blkid -s UUID -o value /dev/block/254:4 works even on non-root.
Not sure what happen, I suppose the problem is somewhere on my side.

> > > --- /dev/null
> > > +++ b/tests/functions.sh
> > > @@ -0,0 +1,269 @@
> > > +#!/bin/bash
> > I'd still vote for /bin/sh and posix syntax. evmctl can be cross compiled for
> > embedded device. Than, it'd have to be run on the target, which can be tiny
> > linux customised distro with busybox shell. That's why POSIX shell syntax
> > instead of relying on bash.

> Tests are not required to compile evmctl.
> On tiny embedded box you may not have gcc so lets code on perl instead.
> You may not have also asciidoc but it is used in ima-evm-utils build.
My point was cross compile on host, but run the test on target.
My assumption was that it makes sense to run tests on target.
I mean if you get a bug report on arm or unusual architecture (I got the
impression that IMA+EVM is used on arm a lot), you ask reporter to copy tests
directory and run tests. Now you have to ask for bash as well (but better than
perl/python :)).

Although, as there is AFAIK no arch dependent code, it might not make sense to
test it on target (enough to run the code compiled natively).
The changes wouldn't be that hard, just using a different style and more
external commands instead of bash specific builtins.
But whatever, I got the message that bash dependency stay, sorry for bothering you with it :).
The main goal is to cover evmctl with tests, which you did very well.

...
> > > +# You can set env VERBOSE=1 to see more output from evmctl
> > > +V=vvvv
> > > +V=${V:0:$VERBOSE}
> > > +V=${V:+-$V}
> > Maybe it's just me but it's not really readable.

> Yeah. But that would you suggest to make number of -v in V the same as
> number in $VERBOSE?
Well, my style would be to use awk (awk is very common and portable), but many
people (including me) consider awk syntax obscure and nonintuitive, so
understand it's not an option for you.

Maybe more descriptive variable would make it clearer.
And mention in docs that VERBOSE can be <1-4>.
But, again, a detail.

> > > +# Only allow color output on tty
> > > +if tty -s; then
> > BTW I'd prefer the output to file to be plain text (I have vim plugin to show
> > colors, but not everybody has). Now it does not (forcing by red_always ?).
> > [ -t 1 ] will do the check.
> > Or, you could even have variable to allow user to chose, example from LTP [1]

> Yes I will disable coloring if tests are run inside of make.
> I neglected to fix that, because I found colored test-suite.log is still
> useful. (Even though less -R does not show red colored parts.)
ok, thanks.

> > > +  [[ "$VERBOSE" -gt 1 ]] && echo "____ START positive test: $@"
> > Bashism, use [ ]

> This is intentional since ['s -gt will output error if $VERBOSE is not
> a number.

> > > +  case $ret in
> > > +    0)  testspass+=1 ;;
> > Bashism, use testspass=$((testspass+1))
> > > +    77) testsskip+=1 ;;
> > > +    99) testsfail+=1; exit_early 1 ;;
> > > +    *)  testsfail+=1; exit_early 2 ;;
> > > +  esac
> > > +  return $ret
> > > +}
> > > +
> > > +# Eval negative test (one that should fail) and account its result
> > > +expect_fail() {
> > > +  local ret
> > > +
> > > +  if [ $TNESTED -gt 0 ]; then
> > > +    echo $RED"expect_fail should not be run nested"$NORM
> > IMHO it's safer to quote whole string (here ok, but on some circumstances shell
> > handles quoted and unquoted string differently):
> > echo "$REDexpect_fail should not be run nested $NORM"

> I think quoting there is just a matter of style, not of safety.

> > > +  TFAIL=yes
> > > +  TMODE=-
> > > +  TNESTED+=1
> > > +  [[ "$VERBOSE" -gt 1 ]] && echo "____ START negative test: $@"
> > > +  "$@"
> > > +  ret=$?
> > > +  [[ "$VERBOSE" -gt 1 ]] && echo "^^^^ STOP ($ret) negative test: $@"
> > > +  TNESTED+=-1
> > > +  case $ret in
> > > +    0)  testsfail+=1; exit_early 3 ;;
> > > +    77) testsskip+=1 ;;
> > > +    99) testsfail+=1; exit_early 4 ;;
> > > +    *)  testspass+=1 ;;
> > > +  esac
> > > +  TFAIL= # Restore defaults for tests run without wrappers
> > > +  TMODE=+
> > I guess you wanted TMODE=

> No. Comment says I want to restore defaults, and default values for
> these are same as for positive testing. I may expend comment.

> > > +  if [ $ret -ge 126 -a $ret -lt 255 ]; then
> > > +    red_always
> > > +    echo "evmctl $op failed hard with ($ret) $text_for"
> > > +    sed 's/^/  /' $out
> > > +    color_restore
> > I'd reset terminal inside coloring functions (color_restore() and $NORM).
> > red()
> > {
> > 	printf '\033[1;31m'
> > 	echo "$@"
> > 	printf '\033[0m'
> > }

> What do you mean? Hm...

> Argument in `red_always' is just to pass `-n` if needed. But it was
> never needed, so I can delete it. Blank line was intentional. And
> it enables red for all next lines until `color_restore' sends color 
> restore.

> > red "evmctl $op failed hard with ($ret) $text_for"

> This may be useful for red oneliners, I thought about them, but not
> added. I thought it's beneficent that I output only with `echo' and not
> with some unusual custom printing function.

> > > +cd $(dirname $0)
> > > +PATH=../src:$PATH

> > > +source ./functions.sh
> > Bashism, use . 

> Since I don't plan to remove bashisms...

> > Maybe

> > . $(dirname $0)/functions.sh

> > and in functions.sh
> > export PATH="../src:$PATH"

> > (that reduces duplicity, but then you'd need to source it also in tests/gen-keys.sh)

> You forgot to `cd'. To put it in functions.sh would be too strong side
> effect.
Agree on both. I'd still prefer put PATH to functions.sh, but that's a detail.

> > > +  cmd="openssl dgst ${ENGINE:+-engine $ENGINE} -$alg $file"
> > Why not set ENGINE once at the beginning of the script (or in functions.sh if
> > needed on more places)?

> It could be replaced with two env variables.
>   One for openssl with `-engine x'
>   and another for evmctl with `--engine x'.
> It's possible but this is purely optimisation. Maybe OK.
I noted it, as simple variable  increases readability a bit.

> > > +# Convert test $type into evmctl op prefix
> > > +op() {
> > > +  if [ $1 = ima ]; then
> > > +    echo ima_
> > > +  fi
> > Maybe just
> > [ "$1" = "ima" ] && echo "ima_"

> Maybe.

> > > +# Convert test $type into xattr name
> > > +xattr() {
> > > +  if [ $1 = ima ]; then
> > > +    echo user.ima
> > > +  else
> > > +    echo user.evm
> > > +  fi
> > Maybe just:
> > [ "$1" = "ima" ] && echo "user.ima" || echo "user.evm"

> Matter of style. Besides `x && y || z` is not too understandable in
> shell context for general person. (Yes I use it too in non critical part
> to output colors). But, what if second || affects result of echo and not
> of [? (Question about ease of understandability).
OK, sorry for bothering with details and pushing my style.

...

Kind regards,
Petr

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-07-30 23:24 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-29  4:20 [PATCH v3] ima-evm-utils: Add some tests for evmctl Vitaly Chikunov
2019-07-30  8:27 ` Petr Vorel
2019-07-30 10:07   ` Vitaly Chikunov
2019-07-30 23:24     ` Petr Vorel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).