Patches 1-3 are for the sharness test suite and are only needed if you're running 'make check'. Patch 4 fixes the actual bug. Stack applies cleanly against 0.5.14. Tested on s390x.
From c593d4ff7b1fc37bb67bffaa1e0a896b136fdff6 Mon Sep 17 00:00:00 2001 From: Chris Dunlap <cdun...@llnl.gov> Date: Fri, 4 Dec 2020 17:00:06 -0800 Subject: [PATCH 1/4] Sharness: Add munged_kill_daemon and munged_cleanup
Add munged_kill_daemon() to kill an errant munged process left running in the background from a previous test, and munged_cleanup() which currently only calls munged_kill_daemon(). The situation of an errant munged process is most likely to occur when a munged test is expected to fail and instead erroneously succeeds since those tests do not include a corresponding munged_stop_daemon(). munged_cleanup() should be called at the end of any test script that starts a munged process. It is not necessary to call it or munged_kill_daemon() after every munged process is supposedly stopped since munged_kill_daemon() is now called by munged_start_daemon() to kill any previous munged process named in the $(MUNGE_PIDFILE} before starting a new one. By only checking for a leftover munged process named in the pidfile, munged_kill_daemon() will not interfere with munged processes belonging to other tests or system use. Tested: - Arch Linux - CentOS Stream 8, 8.3.2011, 7.9.2009, 6.10 - Debian sid, 10.8, 9.13, 8.11, 7.11, 6.0.10, 5.0.10, 4.0 - Fedora 33, 32, 31 - FreeBSD 12.2, 11.4 - NetBSD 9.1, 9.0, 8.1 - OpenBSD 6.8, 6.7, 6.6 - openSUSE 15.2, 15.1 - Raspberry Pi OS (Raspbian 10) [armv7l] - Ubuntu 20.10, 20.04.2 LTS, 18.04.5 LTS, 16.04.7 LTS, 14.04.6 LTS, 12.04.5 LTS Tested by calling munged_start_daemon() without a corresponding munged_stop_daemon() in order to leave the munged process running in the background. The test suite was run with debug=t and verbose=t to check for the test_debug() message for the killed munged pid. --- t/0010-basic.t | 4 +++ t/0011-munged-cmdline.t | 6 +++++ t/0012-munge-cmdline.t | 4 +++ t/0013-unmunge-cmdline.t | 4 +++ t/0021-munged-valgrind.t | 4 +++ t/0022-munge-valgrind.t | 4 +++ t/0023-unmunge-valgrind.t | 4 +++ t/0100-munged-lock.t | 8 +++++- t/0101-munged-security-socket.t | 6 +++-- t/0102-munged-security-keyfile.t | 6 +++++ t/0103-munged-security-logfile.t | 6 +++++ t/0104-munged-security-pidfile.t | 6 +++++ t/0105-munged-security-seedfile.t | 6 +++++ t/0110-munged-origin-addr.t | 6 +++++ t/sharness.d/03-munged.sh | 42 +++++++++++++++++++++++++++++-- 15 files changed, 111 insertions(+), 5 deletions(-) diff --git a/t/0010-basic.t b/t/0010-basic.t index 9294bab..1709c3f 100755 --- a/t/0010-basic.t +++ b/t/0010-basic.t @@ -84,4 +84,8 @@ test_expect_unstable 'check logfile for replay' ' grep "Replayed credential" "${MUNGE_LOGFILE}" ' +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0011-munged-cmdline.t b/t/0011-munged-cmdline.t index 95a1dc7..2566ce0 100755 --- a/t/0011-munged-cmdline.t +++ b/t/0011-munged-cmdline.t @@ -65,4 +65,10 @@ test_expect_failure 'finish writing tests' ' false ' +# Clean up after a munged process that may not have terminated. +## +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0012-munge-cmdline.t b/t/0012-munge-cmdline.t index 53d542a..57394f2 100755 --- a/t/0012-munge-cmdline.t +++ b/t/0012-munge-cmdline.t @@ -623,4 +623,8 @@ test_expect_success 'stop munged' ' munged_stop_daemon ' +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0013-unmunge-cmdline.t b/t/0013-unmunge-cmdline.t index c034109..c532123 100755 --- a/t/0013-unmunge-cmdline.t +++ b/t/0013-unmunge-cmdline.t @@ -245,4 +245,8 @@ test_expect_success 'stop munged' ' munged_stop_daemon ' +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0021-munged-valgrind.t b/t/0021-munged-valgrind.t index fb0dc56..071be97 100755 --- a/t/0021-munged-valgrind.t +++ b/t/0021-munged-valgrind.t @@ -40,4 +40,8 @@ test_expect_success 'check valgrind log for errors in munged' ' valgrind_check_log ' +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0022-munge-valgrind.t b/t/0022-munge-valgrind.t index 9e62bdb..ed9a7d2 100755 --- a/t/0022-munge-valgrind.t +++ b/t/0022-munge-valgrind.t @@ -32,4 +32,8 @@ test_expect_success 'check valgrind log for errors in munge' ' valgrind_check_log ' +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0023-unmunge-valgrind.t b/t/0023-unmunge-valgrind.t index e54fbbd..6788ee4 100755 --- a/t/0023-unmunge-valgrind.t +++ b/t/0023-unmunge-valgrind.t @@ -36,4 +36,8 @@ test_expect_success 'check valgrind log for errors in unmunge' ' valgrind_check_log ' +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0100-munged-lock.t b/t/0100-munged-lock.t index a1ab934..117e848 100755 --- a/t/0100-munged-lock.t +++ b/t/0100-munged-lock.t @@ -53,7 +53,7 @@ test_expect_success 'check lockfile permissions' ' # The lockfile should prevent this. ## test_expect_success 'start munged with in-use socket' ' - test_must_fail munged_start_daemon && + test_must_fail munged_start_daemon t-keep-process && egrep "Error:.* Failed to lock \"${MUNGE_LOCKFILE}\"" "${MUNGE_LOGFILE}" ' @@ -201,4 +201,10 @@ test_expect_success SUDO 'stop unprivileged munged as root' ' fi ' +# Clean up after a munged process that may not have terminated. +## +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0101-munged-security-socket.t b/t/0101-munged-security-socket.t index 560e4ac..532dc19 100755 --- a/t/0101-munged-security-socket.t +++ b/t/0101-munged-security-socket.t @@ -213,11 +213,13 @@ test_expect_success 'socket dir inaccessible by all override' ' "${MUNGE_LOGFILE}" ' -# Cleanup detritus from testing. +# Clean up detritus from testing. This may include an errant munged process +# that has not terminated. ## test_expect_success 'cleanup' ' rmdir "${MUNGE_SOCKETDIR}" && - if rmdir "$(dirname "${MUNGE_SOCKETDIR}")" 2>/dev/null; then :; fi + if rmdir "$(dirname "${MUNGE_SOCKETDIR}")" 2>/dev/null; then :; fi && + munged_cleanup ' test_done diff --git a/t/0102-munged-security-keyfile.t b/t/0102-munged-security-keyfile.t index 25e7ce2..5cc1808 100755 --- a/t/0102-munged-security-keyfile.t +++ b/t/0102-munged-security-keyfile.t @@ -358,4 +358,10 @@ test_expect_success 'keyfile dir writable by other with sticky bit' ' chmod 0755 "${MUNGE_KEYDIR}" ' +# Clean up after a munged process that may not have terminated. +## +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0103-munged-security-logfile.t b/t/0103-munged-security-logfile.t index c9887fd..fafd973 100755 --- a/t/0103-munged-security-logfile.t +++ b/t/0103-munged-security-logfile.t @@ -358,4 +358,10 @@ test_expect_success 'logfile failure writes single message to stderr' ' test "${NUM}" -eq 1 2>/dev/null ' +# Clean up after a munged process that may not have terminated. +## +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0104-munged-security-pidfile.t b/t/0104-munged-security-pidfile.t index dbe5825..0c2a505 100755 --- a/t/0104-munged-security-pidfile.t +++ b/t/0104-munged-security-pidfile.t @@ -42,4 +42,10 @@ test_expect_failure 'finish writing tests' ' false ' +# Clean up after a munged process that may not have terminated. +## +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0105-munged-security-seedfile.t b/t/0105-munged-security-seedfile.t index 31debc2..5008239 100755 --- a/t/0105-munged-security-seedfile.t +++ b/t/0105-munged-security-seedfile.t @@ -50,4 +50,10 @@ test_expect_failure 'finish writing tests' ' false ' +# Clean up after a munged process that may not have terminated. +## +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/0110-munged-origin-addr.t b/t/0110-munged-origin-addr.t index 3b4d369..7d0589c 100755 --- a/t/0110-munged-origin-addr.t +++ b/t/0110-munged-origin-addr.t @@ -139,4 +139,10 @@ test_expect_success 'munged --origin non-interface IP address metadata' ' egrep "^ENCODE_HOST:.* 192\.0\.0\.255\>" meta.$$ ' +# Clean up after a munged process that may not have terminated. +## +test_expect_success 'cleanup' ' + munged_cleanup +' + test_done diff --git a/t/sharness.d/03-munged.sh b/t/sharness.d/03-munged.sh index 0168326..6f6e975 100644 --- a/t/sharness.d/03-munged.sh +++ b/t/sharness.d/03-munged.sh @@ -73,19 +73,22 @@ munged_create_key() } ## -# Start munged, removing an existing logfile (from a previous run) if present. +# Start munged, removing an existing logfile or killing an errant munged +# process (from a previous run) if needed. # The following leading args are recognized: # t-exec=ARG - use ARG to exec munged. # t-keep-logfile - do not remove logfile before starting munged. +# t-keep-process - do not kill previous munged process. # Remaining args will be appended to the munged command-line. ## munged_start_daemon() { - local EXEC= KEEP_LOGFILE= && + local EXEC= KEEP_LOGFILE= KEEP_PROCESS= && while true; do case $1 in t-exec=*) EXEC=$(echo "$1" | sed 's/^[^=]*=//');; t-keep-logfile) KEEP_LOGFILE=1;; + t-keep-process) KEEP_PROCESS=1;; *) break;; esac shift @@ -93,6 +96,9 @@ munged_start_daemon() if test "${KEEP_LOGFILE}" != 1; then rm -f "${MUNGE_LOGFILE}" fi && + if test "${KEEP_PROCESS}" != 1; then + munged_kill_daemon + fi && test_debug "echo ${EXEC} \"${MUNGED}\" \ --socket=\"${MUNGE_SOCKET}\" \ --key-file=\"${MUNGE_KEYFILE}\" \ @@ -136,3 +142,35 @@ munged_stop_daemon() --stop \ "$@" } + +## +# Kill an errant munged process running in the background from a previous test. +# This situation is most likely to occur if a munged test is expected to fail +# and instead erroneously succeeds. +# Only check for the pid named in ${MUNGE_PIDFILE} to avoid intefering with +# munged processes belonging to other tests or system use. And check that +# the named pid is a munged process and not one recycled by the system for +# some other running process. +# A SIGTERM is used here instead of "munged --stop" in case the latter has a +# bug introduced that prevents cleanup from occurring. +# A SIGKILL would prevent the munged process from cleaning up which could cause +# other tests to inadvertently fail. +## +munged_kill_daemon() +{ + local PID + PID=$(cat "${MUNGE_PIDFILE}" 2>/dev/null) + if ps -p "${PID}" -ww 2>/dev/null | grep munged; then + kill "${PID}" + test_debug "echo \"Killed munged pid ${PID}\"" + fi +} + +## +# Perform any housekeeping to clean up after munged. This should be called +# at the end of any test script that starts a munged process. +## +munged_cleanup() +{ + munged_kill_daemon +} -- 2.30.0
From 2ad81007d2371f536af9e231490357c928eca53a Mon Sep 17 00:00:00 2001 From: Chris Dunlap <cdun...@llnl.gov> Date: Wed, 2 Dec 2020 09:50:27 -0800 Subject: [PATCH 4/4] HKDF: Fix big-endian bug caused by size_t ptr cast When Fedora updated to 0.5.14 and added the new test suite to their rpm spec's %check, munge successfully built but its test suite failed on s390x for hkdf_test: > FAIL: hkdf_test > =============== > Failed to finalize HKDF MAC ctx for extraction This is caused by the cast of prklenp from a size_t * to an int * in _hkdf_extract(). On s390x, memory ordering is big-endian and size_t is an alias for unsigned long. Thus, a ptr to an 8-byte size_t was being cast to a ptr to a 4-byte int. This worked on little-endian systems (of which all my test systems had been) since the least-significant byte is stored at the smallest memory address (the little end), and the stored value always fit within 4 bytes. But on big-endian systems, the least-significant byte is stored at the largest memory address (the big end) which differs for 4-byte and 8-byte values. Remove the cast by using an int variable as an intermediary. Reference: - https://fedoraproject.org/wiki/Architectures/s390x#Notes_for_application_developers_and_package_maintainers - https://bugzilla.redhat.com/show_bug.cgi?id=1923337 - https://bugs.launchpad.net/bugs/1915457 - https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=982564 Tested: - Arch Linux - CentOS Stream 8, 8.3.2011, 7.9.2009, 6.10 - Debian sid, 10.8, 9.13, 8.11, 7.11, 6.0.10, 5.0.10, 4.0 - Fedora 33 [s390x, x86_64], 32, 31 - FreeBSD 12.2, 11.4 - NetBSD 9.1, 9.0, 8.1 - OpenBSD 6.8, 6.7, 6.6 - openSUSE 15.2, 15.1 - Raspberry Pi OS (Raspbian 10) [armv7l] - Ubuntu 20.10, 20.04.2 LTS, 18.04.5 LTS, 16.04.7 LTS, 14.04.6 LTS, 12.04.5 LTS Closes #91 --- src/common/hkdf.c | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/src/common/hkdf.c b/src/common/hkdf.c index ac7ab6f..364f3e0 100644 --- a/src/common/hkdf.c +++ b/src/common/hkdf.c @@ -32,6 +32,7 @@ #include <assert.h> #include <errno.h> +#include <limits.h> #include <stdio.h> #include <stdlib.h> #include <string.h> @@ -316,6 +317,7 @@ _hkdf_extract (hkdf_ctx_t *ctxp, void *prk, size_t *prklenp) { mac_ctx mac_ctx; int mac_ctx_is_initialized = 0; + int prklen; int rv = 0; assert (ctxp != NULL); @@ -325,6 +327,14 @@ _hkdf_extract (hkdf_ctx_t *ctxp, void *prk, size_t *prklenp) assert (prklenp != NULL); assert (*prklenp > 0); + /* Convert prklen size_t to int for the call to mac_final() since the parm + * is being passed as a ptr, and size of size_t and int may differ. + * *prklenp must be representable as an int because it was assigned + * (via ctxp->mdlen) by mac_size() which returns an int. + */ + assert (*prklenp <= INT_MAX); + prklen = (int) *prklenp; + /* Compute the pseudorandom key. * prk = HMAC (salt, ikm) */ @@ -340,7 +350,7 @@ _hkdf_extract (hkdf_ctx_t *ctxp, void *prk, size_t *prklenp) log_msg (LOG_ERR, "Failed to update HKDF MAC ctx for extraction"); goto err; } - rv = mac_final (&mac_ctx, prk, (int *) prklenp); + rv = mac_final (&mac_ctx, prk, &prklen); if (rv == -1) { log_msg (LOG_ERR, "Failed to finalize HKDF MAC ctx for extraction"); goto err; @@ -352,6 +362,12 @@ err: return -1; } } + /* Update [prklenp] on success. + */ + if (rv >= 0) { + assert (prklen >= 0); + *prklenp = (size_t) prklen; + } return rv; } @@ -371,7 +387,7 @@ _hkdf_expand (hkdf_ctx_t *ctxp, const void *prk, size_t prklen, unsigned char *dstptr; size_t dstlen; unsigned char *okm = NULL; - size_t okmlen; + int okmlen; int num_rounds; const int max_rounds = 255; unsigned char round; @@ -390,8 +406,14 @@ _hkdf_expand (hkdf_ctx_t *ctxp, const void *prk, size_t prklen, /* Allocate buffer for output keying material. * The buffer size is equal to the size of the hash function output. + * Note that okmlen must be an int (and not size_t) for the call to + * mac_final() since the parm is being passed as a ptr, and size of + * size_t and int may differ. + * ctxp->mdlen must be representable as an int because it was assigned + * by mac_size() which returns an int. */ - okmlen = ctxp->mdlen; + assert (ctxp->mdlen <= INT_MAX); + okmlen = (int) ctxp->mdlen; okm = calloc (1, okmlen); if (okm == NULL) { rv = -1; @@ -448,7 +470,7 @@ _hkdf_expand (hkdf_ctx_t *ctxp, const void *prk, size_t prklen, "for expansion round #%u", round); goto err; } - rv = mac_final (&mac_ctx, okm, (int *) &okmlen); + rv = mac_final (&mac_ctx, okm, &okmlen); if (rv == -1) { log_msg (LOG_ERR, "Failed to finalize HKDF MAC ctx " -- 2.30.0
From 014cff3c0ba16fc645eeceeb16eb6be8132c59fd Mon Sep 17 00:00:00 2001 From: Chris Dunlap <cdun...@llnl.gov> Date: Fri, 4 Dec 2020 23:50:39 -0800 Subject: [PATCH 3/4] Sharness: Fix EACCES failure succeeding for root When the test suite is run by root, the following failure occurs in "0103-munged-security-logfile.t": 10 - logfile not writable by user failure This sets the logfile perms to 0400 to check for an error when the logfile is not writable by the user. However, root will not get a "permission denied" error here. Consequently, the expected failure erroneously succeeds. Add a check for whether the test is being run by the root user, and set the ROOT prerequisite when this is true. Furthermore, add the !ROOT prereq to the above test so it will be skipped when run by root. Tested: - Arch Linux - CentOS Stream 8, 8.3.2011, 7.9.2009, 6.10 - Debian sid, 10.8, 9.13, 8.11, 7.11, 6.0.10, 5.0.10, 4.0 - Fedora 33, 32, 31 - FreeBSD 12.2, 11.4 - NetBSD 9.1, 9.0, 8.1 - OpenBSD 6.8, 6.7, 6.6 - openSUSE 15.2, 15.1 - Raspberry Pi OS (Raspbian 10) [armv7l] - Ubuntu 20.10, 20.04.2 LTS, 18.04.5 LTS, 16.04.7 LTS, 14.04.6 LTS, 12.04.5 LTS --- t/0103-munged-security-logfile.t | 4 +++- t/sharness.d/10-root.sh | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 t/sharness.d/10-root.sh diff --git a/t/0103-munged-security-logfile.t b/t/0103-munged-security-logfile.t index 40b59a6..9e951b9 100755 --- a/t/0103-munged-security-logfile.t +++ b/t/0103-munged-security-logfile.t @@ -118,8 +118,10 @@ test_expect_success 'logfile non-regular-file override failure' ' ' # Check for an error when the logfile is not writable by user. +# Skip this test if running as root since the root user will not get the +# expected EACCESS failure. ## -test_expect_success 'logfile not writable by user failure' ' +test_expect_success !ROOT 'logfile not writable by user failure' ' rm -f "${MUNGE_LOGFILE}" && touch "${MUNGE_LOGFILE}" && chmod 0400 "${MUNGE_LOGFILE}" && diff --git a/t/sharness.d/10-root.sh b/t/sharness.d/10-root.sh new file mode 100644 index 0000000..5a2fd28 --- /dev/null +++ b/t/sharness.d/10-root.sh @@ -0,0 +1,6 @@ +## +# Is the test being run by the root user? +## +if test "$(id -u)" = 0; then + test_set_prereq ROOT +fi -- 2.30.0
From f7333277c2709b147e2f2a3ab357ec3a195fb1f5 Mon Sep 17 00:00:00 2001 From: Chris Dunlap <cdun...@llnl.gov> Date: Fri, 4 Dec 2020 21:31:34 -0800 Subject: [PATCH 2/4] Sharness: Fix dup of failing check when run by root When the test suite is run by root, the following two failures occur in "0103-munged-security-logfile.t": 10 - logfile not writable by user failure 31 - logfile failure writes single message to stderr This second test, "logfile failure writes single message to stderr", checks for a regression of a duplicate error message being written to stderr by forcing an expected failure -- namely, setting the logfile perms to 0400 and expecting an error when opening the logfile because the user does not have write-permissions. This expected failure is the check being performed in the first test, "logfile not writable by user failure". Fix the test for "logfile failure writes single message to stderr" by forcing a different error that is not affected by root privileges. In particular, set the logfile perms to 0602 which will fail because the logfile is now writable by other; this will fail regardless of whether or not the user is root. Tested: - Arch Linux - CentOS Stream 8, 8.3.2011, 7.9.2009, 6.10 - Debian sid, 10.8, 9.13, 8.11, 7.11, 6.0.10, 5.0.10, 4.0 - Fedora 33, 32, 31 - FreeBSD 12.2, 11.4 - NetBSD 9.1, 9.0, 8.1 - OpenBSD 6.8, 6.7, 6.6 - openSUSE 15.2, 15.1 - Raspberry Pi OS (Raspbian 10) [armv7l] - Ubuntu 20.10, 20.04.2 LTS, 18.04.5 LTS, 16.04.7 LTS, 14.04.6 LTS, 12.04.5 LTS --- t/0103-munged-security-logfile.t | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/t/0103-munged-security-logfile.t b/t/0103-munged-security-logfile.t index fafd973..40b59a6 100755 --- a/t/0103-munged-security-logfile.t +++ b/t/0103-munged-security-logfile.t @@ -343,14 +343,16 @@ test_expect_success 'logfile dir writable by other with sticky bit' ' chmod 0755 "${MUNGE_LOGDIR}" ' -# Check for a regression of a duplicate error message being written to stderr -# for a failure to open the logfile. +# Check for a regression of a duplicate error message being written to stderr. +# To generate an error, test for the logfile being writable by other since this +# will not be affected by root privileges. +# ## test_expect_success 'logfile failure writes single message to stderr' ' local ERR NUM && rm -f "${MUNGE_LOGFILE}" && touch "${MUNGE_LOGFILE}" && - chmod 0400 "${MUNGE_LOGFILE}" && + chmod 0602 "${MUNGE_LOGFILE}" && test_must_fail munged_start_daemon t-keep-logfile 2>err.$$ && cat err.$$ && ERR=$(sed -n -e "s/.*Error: //p" err.$$ | sort | uniq -c | sort -n -r) && -- 2.30.0