[v2] libmakepkg/integrity: handle PGP signature files containing multiple signatures

Message ID 20220608212111.328157-1-diabonas@archlinux.org
State New
Headers show
Series [v2] libmakepkg/integrity: handle PGP signature files containing multiple signatures | expand

Commit Message

Jonas Witschel June 8, 2022, 9:21 p.m. UTC
PGP signature files can contain multiple signatures from different keys, see
e.g. the source tarballs of recent GnuPG releases like version 2.2.35.
parse_gpg_statusfile() is currently not equipped to handle the gpg output for
such signature files: it assumes that the file contains only one signature and
therefore overwrites the verification results of previous signatures,
effectively only considering the last signature in the file.

It is not clearly documented how a signature file containing multiple
signatures should be handled. The gpgv verification tools opts to check *all*
signatures and to fail if any of them are not valid or untrusted. At first
glance this appears to make sense, but there are severe disadvantages: in order
to verify a signature file with multiple signatures, all its keys have to be
added to validpgpkeys. This means old or otherwise untrusted keys might need to
be added just for the verification to succeed. On the other hand, there is no
way to set a threshold of keys to be used for the verification in the PKGBUILD.
This means that a signature file only containing a signature by one of these
additional keys would now be accepted as valid as well. In conclusion, it is
better to leave it to packagers to choose which of the keys in a signature file
they want to trust and to consider a signature file as valid if *any* of the
signatures it contains are valid.

To implement this, the logic in parse_gpg_statusfile() needs to be refined so
that $success and $trusted are true if any valid/trusted signature is found. We
also need to store the verification results per key (in order to present more
specific error messages) and all the validated fingerprints (in order to check
whether there is an intersection with validpgpkeys). Furthermore, the error
handling of missing/revoked/bad keys in check_pgpsigs() needs to be moved so
that it can be non-fatal as long as there is a single valid signature.

Signed-off-by: Jonas Witschel <diabonas@archlinux.org>
---
 .../integrity/verify_signature.sh.in          | 103 ++++++++++--------
 scripts/libmakepkg/util/util.sh.in            |  12 ++
 2 files changed, 68 insertions(+), 47 deletions(-)

Patch

diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in
index ad5bb66d..f77d1189 100644
--- a/scripts/libmakepkg/integrity/verify_signature.sh.in
+++ b/scripts/libmakepkg/integrity/verify_signature.sh.in
@@ -32,7 +32,7 @@  check_pgpsigs() {
 
 	msg "$(gettext "Verifying source file signatures with %s...")" "gpg"
 
-	local netfile proto pubkey success status fingerprint trusted
+	local netfile proto pubkey success status fingerprints trusted
 	local warnings=0
 	local errors=0
 	local statusfile=$(mktemp)
@@ -57,49 +57,59 @@  check_pgpsigs() {
 
 		# these variables are assigned values in parse_gpg_statusfile
 		success=0
-		status=
-		pubkey=
-		fingerprint=
-		trusted=
+		declare -A status
+		fingerprints=()
+		trusted=0
 		parse_gpg_statusfile "$statusfile"
 		if (( ! $success )); then
 			printf '%s' "$(gettext "FAILED")" >&2
-			case "$status" in
-				"missingkey")
-					printf ' (%s)' "$(gettext "unknown public key") $pubkey" >&2
-					;;
-				"revokedkey")
-					printf " ($(gettext "public key %s has been revoked"))" "$pubkey" >&2
-					;;
-				"bad")
-					printf ' (%s)' "$(gettext "bad signature from public key") $pubkey" >&2
-					;;
-				"error")
-					printf ' (%s)' "$(gettext "error during signature verification")" >&2
-					;;
-			esac
 			errors=1
 		else
 			if (( ${#validpgpkeys[@]} == 0 && !trusted )); then
-				printf "%s ($(gettext "the public key %s is not trusted"))" $(gettext "FAILED") "$fingerprint" >&2
+				printf '%s' "$(gettext "FAILED")" >&2
+				for pubkey in "${fingerprints[@]}"; do
+					printf " ($(gettext "the public key %s is not trusted"))" "$pubkey" >&2
+				done
 				errors=1
-			elif (( ${#validpgpkeys[@]} > 0 )) && ! in_array "$fingerprint" "${validpgpkeys[@]}"; then
-				printf "%s (%s %s)" "$(gettext "FAILED")" "$(gettext "invalid public key")" "$fingerprint" >&2
+			elif (( ${#validpgpkeys[@]} > 0 )) && ! arrays_intersect fingerprints validpgpkeys; then
+				printf '%s' "$(gettext "FAILED")" >&2
+				for pubkey in "${fingerprints[@]}"; do
+					printf " (%s %s)" "$(gettext "invalid public key")" "$pubkey" >&2
+				done
 				errors=1
 			else
 				printf '%s' "$(gettext "Passed")" >&2
-				case "$status" in
+			fi
+		fi
+
+		if (( warnings )); then
+			for pubkey in "${!status[@]}"; do
+				case "${status["$pubkey"]}" in
+					"good")
+						printf ' (%s)' "$(gettext "good signature from public key") $pubkey" >&2
+						;;
+					"missingkey")
+						printf ' (%s)' "$(gettext "unknown public key") $pubkey" >&2
+						;;
+					"revokedkey")
+						printf " ($(gettext "public key %s has been revoked"))" "$pubkey" >&2
+						;;
+					"bad")
+						printf ' (%s)' "$(gettext "bad signature from public key") $pubkey" >&2
+						;;
+					"error")
+						printf ' (%s)' "$(gettext "error during signature verification")" >&2
+						;;
 					"expired")
-						printf ' (%s)' "$(gettext "WARNING:") $(gettext "the signature has expired.")" >&2
-						warnings=1
+						printf " (%s $(gettext "the signature by key %s has expired."))" "$(gettext "WARNING:")" "$pubkey" >&2
 						;;
 					"expiredkey")
-						printf ' (%s)' "$(gettext "WARNING:") $(gettext "the key has expired.")" >&2
-						warnings=1
+						printf " (%s $(gettext "the key %s has expired."))" "$(gettext "WARNING:")" "$pubkey" >&2
 						;;
 				esac
-			fi
+			done
 		fi
+
 		printf '\n' >&2
 	done
 
@@ -204,50 +214,49 @@  parse_gpg_statusfile() {
 	while read -r _ type arg1 _ _ _ _ arg6 _ _ _ arg10 _; do
 		case "$type" in
 			GOODSIG)
-				pubkey=$arg1
 				success=1
-				status="good"
+				status["$arg1"]="good"
 				;;
 			EXPSIG)
-				pubkey=$arg1
 				success=1
-				status="expired"
+				warnings=1
+				status["$arg1"]="expired"
 				;;
 			EXPKEYSIG)
-				pubkey=$arg1
 				success=1
-				status="expiredkey"
+				warnings=1
+				status["$arg1"]="expiredkey"
 				;;
 			REVKEYSIG)
-				pubkey=$arg1
-				success=0
-				status="revokedkey"
+				warnings=1
+				status["$arg1"]="revokedkey"
 				;;
 			BADSIG)
-				pubkey=$arg1
-				success=0
-				status="bad"
+				warnings=1
+				status["$arg1"]="bad"
 				;;
 			ERRSIG)
-				pubkey=$arg1
-				success=0
+				warnings=1
 				if [[ $arg6 == 9 ]]; then
-					status="missingkey"
+					status["$arg1"]="missingkey"
 				else
-					status="error"
+					status["$arg1"]="error"
 				fi
 				;;
 			VALIDSIG)
 				if [[ $arg10 ]]; then
 					# If the file was signed with a subkey, arg10 contains
 					# the fingerprint of the primary key
-					fingerprint=$arg10
+					fingerprints+=("$arg10")
 				else
-					fingerprint=$arg1
+					fingerprints+=("$arg1")
 				fi
 				;;
 			TRUST_UNDEFINED|TRUST_NEVER)
-				trusted=0
+				# If the signature file contains multiple signatures, we
+				# consider the file to be valid if any of the signatures is
+				# made by a trusted key, so having other untrusted signatures
+				# is not a fatal error
 				;;
 			TRUST_MARGINAL|TRUST_FULLY|TRUST_ULTIMATE)
 				trusted=1
diff --git a/scripts/libmakepkg/util/util.sh.in b/scripts/libmakepkg/util/util.sh.in
index 6c0e589a..15499227 100644
--- a/scripts/libmakepkg/util/util.sh.in
+++ b/scripts/libmakepkg/util/util.sh.in
@@ -53,6 +53,18 @@  is_array() {
 	return $ret
 }
 
+# Returns 0 if the two arrays passed by name have intersecting elements,
+# 1 otherwise
+arrays_intersect() {
+	local -n _arrays_intersect_first=$1
+	local -n _arrays_intersect_second=$2
+	local element
+	for element in "${_arrays_intersect_first[@]}"; do
+		in_array "$element" "${_arrays_intersect_second[@]}" && return 0
+	done
+	return 1
+}
+
 # Canonicalize a directory path if it exists
 canonicalize_path() {
 	local path="$1"