From 47a2662bccb8e6f2f192acf46c26d862fe3bbcfb Mon Sep 17 00:00:00 2001 From: Evgeny Kolesnikov Date: Fri, 17 Jan 2020 10:24:07 +0100 Subject: [PATCH 1/2] Covscan fixes Error: FORWARD_NULL (CWE-476): [#def17] xccdf_policy_remediate.c:383: var_compare_op: Comparing "rr" to null implies that "rr" might be null. xccdf_policy_remediate.c:384: var_deref_model: Passing null pointer "rr" to "_rule_add_info_message", which dereferences it. Error: FORWARD_NULL (CWE-476): [#def18] test_fsdev_is_local_fs.c:35: assign_zero: Assigning: "ment.mnt_fsname" = "NULL". test_fsdev_is_local_fs.c:37: var_deref_model: Passing "&ment" to "is_local_fs", which dereferences null "ment.mnt_fsname". --- src/OVAL/probes/fsdev.c | 4 ++++ src/XCCDF_POLICY/xccdf_policy_remediate.c | 12 ++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/OVAL/probes/fsdev.c b/src/OVAL/probes/fsdev.c index bd8e52fbf..a6b36f5e0 100644 --- a/src/OVAL/probes/fsdev.c +++ b/src/OVAL/probes/fsdev.c @@ -97,6 +97,10 @@ static int is_local_fs(struct mntent *ment) return 0; } + if (ment->mnt_fsname == NULL) { + return 0; + } + s = ment->mnt_fsname; /* If the fsname begins with "//", it is probably CIFS. */ if (s[0] == '/' && s[1] == '/') diff --git a/src/XCCDF_POLICY/xccdf_policy_remediate.c b/src/XCCDF_POLICY/xccdf_policy_remediate.c index 389a7d1bd..f59737727 100644 --- a/src/XCCDF_POLICY/xccdf_policy_remediate.c +++ b/src/XCCDF_POLICY/xccdf_policy_remediate.c @@ -380,7 +380,11 @@ static inline int _xccdf_fix_decode_xml(struct xccdf_fix *fix, char **result) #if defined(unix) || defined(__unix__) || defined(__unix) static inline int _xccdf_fix_execute(struct xccdf_rule_result *rr, struct xccdf_fix *fix) { - if (fix == NULL || rr == NULL || oscap_streq(xccdf_fix_get_content(fix), NULL)) { + if (rr == NULL) { + return 1; + } + + if (fix == NULL || oscap_streq(xccdf_fix_get_content(fix), NULL)) { _rule_add_info_message(rr, "No fix available."); return 1; } @@ -481,7 +485,11 @@ static inline int _xccdf_fix_execute(struct xccdf_rule_result *rr, struct xccdf_ #else static inline int _xccdf_fix_execute(struct xccdf_rule_result *rr, struct xccdf_fix *fix) { - if (fix == NULL || rr == NULL || oscap_streq(xccdf_fix_get_content(fix), NULL)) { + if (rr == NULL) { + return 1; + } + + if (fix == NULL || oscap_streq(xccdf_fix_get_content(fix), NULL)) { _rule_add_info_message(rr, "No fix available."); return 1; } else { From 7bccc09eabd30e0581cf0fdf4f20fa481db12e91 Mon Sep 17 00:00:00 2001 From: Evgeny Kolesnikov Date: Fri, 17 Jan 2020 11:04:13 +0100 Subject: [PATCH 2/2] Covscan fixes (SHELLCHECK), small refactoring in Shell wrappers Error: SHELLCHECK_WARNING: warning: die references arguments, but none are ever passed. [SC2120] Error: SHELLCHECK_WARNING: warning: Use 'cd ... || exit' or 'cd ... || return' in case cd fails. [SC2164] Error: SHELLCHECK_WARNING: warning: Declare and assign separately to avoid masking return values. [SC2155] --- utils/oscap-chroot | 20 ++++++++++++-------- utils/oscap-podman | 42 +++++++++++++++++++++--------------------- utils/oscap-ssh | 39 ++++++++++++++++++++++----------------- utils/oscap-vm | 19 +++++++++++-------- 4 files changed, 66 insertions(+), 54 deletions(-) diff --git a/utils/oscap-chroot b/utils/oscap-chroot index 6518d7a2c..318f55a91 100755 --- a/utils/oscap-chroot +++ b/utils/oscap-chroot @@ -25,6 +25,13 @@ function die() exit 1 } +function invalid() +{ + echo -e "$*\n" >&2 + usage + exit 1 +} + function usage() { echo "oscap-chroot -- Tool for offline SCAP evaluation of filesystems mounted in arbitrary paths." @@ -74,26 +81,23 @@ function usage() } if [ $# -lt 1 ]; then - echo "No arguments provided." - usage - die + invalid "No arguments provided." elif [ "$1" == "-h" ] || [ "$1" == "--help" ]; then usage - die + exit 0 elif [ "$#" -gt 1 ]; then true else - echo "Invalid arguments provided." - usage - die + invalid "Invalid arguments provided." fi # Learn more at https://www.redhat.com/archives/open-scap-list/2013-July/msg00000.html export OSCAP_PROBE_ROOT -OSCAP_PROBE_ROOT="$(cd "$1"; pwd)" +OSCAP_PROBE_ROOT="$(cd "$1" && pwd)" || die "Invalid CHROOT_PATH argument." export OSCAP_EVALUATION_TARGET="chroot://$OSCAP_PROBE_ROOT" shift 1 oscap "$@" EXIT_CODE=$? + exit $EXIT_CODE diff --git a/utils/oscap-podman b/utils/oscap-podman index 32ec0cfcb..6b9f4a3de 100755 --- a/utils/oscap-podman +++ b/utils/oscap-podman @@ -16,13 +16,19 @@ # License along with this library; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA - function die() { echo "$*" >&2 exit 1 } +function invalid() +{ + echo -e "$*\n" >&2 + usage + exit 1 +} + function usage() { echo "oscap-podman -- Tool for SCAP evaluation of Podman images and containers." @@ -39,30 +45,24 @@ function usage() OSCAP_BINARY=oscap if [ $# -lt 1 ]; then - echo "No arguments provided." - usage - die + invalid "No arguments provided." elif [ "$1" == "-h" ] || [ "$1" == "--help" ]; then usage - die + exit 0 elif [[ "$1" == --oscap=* ]] && [ $# -gt 2 ]; then OSCAP_BINARY=${1#"--oscap="} shift elif [ "$#" -gt 1 ]; then true else - echo "Invalid arguments provided." - usage - die + invalid "Invalid arguments provided." fi if [ $(id -u) -ne 0 ]; then - echo "This script cannot run in rootless mode." >&2 - die + die "This script cannot run in rootless mode." fi if grep -q "\-\-remediate" <<< "$@"; then - echo "This script does not support '--remediate' option." >&2 - die + die "This script does not support '--remediate' option." fi IMAGE_NAME=$(podman image exists "$1" \ @@ -72,14 +72,13 @@ CONTAINER_NAME=$(podman container exists "$1" \ if [ -n "$IMAGE_NAME" ] && [ -n "$CONTAINER_NAME" ]; then echo "Ambiguous target, container image and container with the same name detected: '$1'." >&2 - echo "Please rather use an unique ID to specify the target of the scan." >&2 - die + die "Please rather use an unique ID to specify the target of the scan." fi # Check if the target of scan is image or container. CLEANUP=0 if [ -n "$IMAGE_NAME" ]; then - ID=$(podman create $1) || die + ID=$(podman create $1) || die "Unable to create a container." TARGET="podman-image://$IMAGE_NAME" CLEANUP=1 elif [ -n "$CONTAINER_NAME" ]; then @@ -87,14 +86,13 @@ elif [ -n "$CONTAINER_NAME" ]; then ID=$1 TARGET="podman-container://$CONTAINER_NAME" else - echo "Target of the scan not found: '$1'." >&2 - die + die "Target of the scan not found: '$1'." fi # podman init creates required files such as: /run/.containerenv - we don't care about output and exit code podman init $ID &> /dev/null || true -DIR=$(podman mount $ID) || die +DIR=$(podman mount $ID) || die "Failed to mount." if [ ! -f "$DIR/run/.containerenv" ]; then # ubi8-init image does not create .containerenv when running podman init, but we need to make sure that the file is there @@ -105,14 +103,16 @@ for VAR in `podman inspect $ID --format '{{join .Config.Env " "}}'`; do eval "export OSCAP_OFFLINE_$VAR" done -export OSCAP_PROBE_ROOT="$(cd "$DIR"; pwd)" +export OSCAP_PROBE_ROOT +OSCAP_PROBE_ROOT="$(cd "$DIR" && pwd)" || die "Unable to change current directory to OSCAP_PROBE_ROOT (DIR)." export OSCAP_EVALUATION_TARGET="$TARGET" shift 1 $OSCAP_BINARY "$@" EXIT_CODE=$? -podman umount $ID > /dev/null || die + +podman umount $ID > /dev/null || die "Failed to unmount." if [ $CLEANUP -eq 1 ]; then - podman rm $ID > /dev/null || die + podman rm $ID > /dev/null || die "Failed to clean up." fi exit $EXIT_CODE diff --git a/utils/oscap-ssh b/utils/oscap-ssh index 08c8bcd2b..cd3600180 100755 --- a/utils/oscap-ssh +++ b/utils/oscap-ssh @@ -22,9 +22,12 @@ function die() exit 1 } -hash ssh 2> /dev/null || die "Cannot find ssh, please install the OpenSSH client." -hash scp 2> /dev/null || die "Cannot find scp, please install the OpenSSH client." -hash mktemp 2> /dev/null || die "Cannot find mktemp, please install coreutils." +function invalid() +{ + echo -e "$*\n" >&2 + usage + exit 1 +} function usage() { @@ -87,10 +90,6 @@ function usage() echo "See \`man oscap\` to learn more about semantics of these options." } -OSCAP_SUDO="" -# SSH_ADDITIONAL_OPTIONS may be defined in the calling shell -SSH_TTY_ALLOCATION_OPTION="" - # $1, $2, ... SSH options (pass them as separate arguments) function ssh_execute_with_options { ssh -o ControlPath="$MASTER_SOCKET" $SSH_ADDITIONAL_OPTIONS "$@" -p "$SSH_PORT" "$SSH_HOST" @@ -118,22 +117,20 @@ function scp_retreive_from_temp_dir { # Returns: String, where individual command components are double-quoted, so they are not interpreted by the shell. # For example, an array ('-p' '(all)') will be transformed to "\"-p\" \"(all)\"", so after the shell expansion, it will end up as "-p" "(all)". function command_array_to_string { - eval "printf '\"%s\" ' \"\${$1[@]}\"" + eval "printf '\"%s\" ' \"\${$1[@]}\"" } function first_argument_is_sudo { - [ "$1" == "sudo" ] || [ "$1" == "--sudo" ] - return $? + [ "$1" == "sudo" ] || [ "$1" == "--sudo" ] + return $? } function sanity_check_arguments { if [ $# -lt 1 ]; then - echo "No arguments provided." - usage - die + invalid "No arguments provided." elif [ "$1" == "-h" ] || [ "$1" == "--help" ]; then usage - die + exit 0 elif first_argument_is_sudo "$@"; then OSCAP_SUDO="sudo" # force pseudo-tty allocation so that users can type their password if necessary @@ -141,9 +138,7 @@ function sanity_check_arguments { shift fi if [ $# -lt 2 ]; then - echo "Missing ssh host and ssh port." - usage - die + invalid "Missing ssh host and ssh port." fi } @@ -165,6 +160,16 @@ function check_oscap_arguments { fi } + +hash ssh 2> /dev/null || die "Cannot find ssh, please install the OpenSSH client." +hash scp 2> /dev/null || die "Cannot find scp, please install the OpenSSH client." +hash mktemp 2> /dev/null || die "Cannot find mktemp, please install coreutils." + + +OSCAP_SUDO="" +# SSH_ADDITIONAL_OPTIONS may be defined in the calling shell +SSH_TTY_ALLOCATION_OPTION="" + sanity_check_arguments "$@" first_argument_is_sudo "$@" && shift diff --git a/utils/oscap-vm b/utils/oscap-vm index 02f8c6396..6557eb3a7 100755 --- a/utils/oscap-vm +++ b/utils/oscap-vm @@ -22,6 +22,13 @@ function die() exit 1 } +function invalid() +{ + echo -e "$*\n" >&2 + usage + exit 1 +} + function usage() { echo "oscap-vm -- Tool for offline SCAP evaluation of virtual machines." @@ -76,12 +83,10 @@ function usage() OSCAP_BINARY=oscap if [ $# -lt 1 ]; then - echo "No arguments provided." - usage - die + invalid "No arguments provided." elif [ "$1" == "-h" ] || [ "$1" == "--help" ]; then usage - die + exit 0 elif [[ "$1" == --oscap=* ]] && [ $# -gt 3 ]; then OSCAP_BINARY=${1#"--oscap="} shift @@ -90,9 +95,7 @@ elif [ "$1" == "image" ] && [ $# -gt 2 ]; then elif [ "$1" == "domain" ] && [ $# -gt 2 ]; then true else - echo "Invalid arguments provided." - usage - die + invalid "Invalid arguments provided." fi hash guestmount 2> /dev/null || die "Cannot find guestmount, please install libguestfs utilities." @@ -128,7 +131,7 @@ fi # Learn more at https://www.redhat.com/archives/open-scap-list/2013-July/msg00000.html export OSCAP_PROBE_ROOT -OSCAP_PROBE_ROOT="$(cd "$MOUNTPOINT"; pwd)" +OSCAP_PROBE_ROOT="$(cd "$MOUNTPOINT" && pwd)" || die "Unable to change current directory to OSCAP_PROBE_ROOT (MOUNTPOINT)." export OSCAP_EVALUATION_TARGET="oscap-vm $1 $2" shift 2