Nice spot and great patch :) Review disclaimer: patch not field tested yet On 6/8/22 19:32, Jonas Witschel wrote:
PGP signature files can contain multiple signatures from different keys. In this case, verification is considered to be successful if *all* signatures are valid, compare e.g. the behaviour of the gpgv verification tool.
No hard feelings here at all, but my thoughts are as follows: I'm not too sure we really want to mimic gpgv behavior. Validating all signatures sounds great in theory, but the only real security guarantee we can give with the current control mechanisms and options in makepkg is basically "source has any signature available in validpgpkeys". As we have no constraints to specify a signature threshold, there is no way for makepkg to consider tree, two or one available signature as more or less trustworthy. For instance a rogue maintainer can just distribute whatever they want using a single trusted signature which makepkg will happily consume. Hence the only guarantee we can provide currently is to have `any` signature instead of `all`. While this is the case, this may lead to potential issues like "random" secondary signatures or potentially even old superseded keys that are still used as a multi signature which we don't actually wanna forcefully trust anymore. Taking this into account, I'd suggest we go with an `any` approach instead of `all` for the time being. If makepkg ever gets finer trust control per source, such adjustments should be reflected here as well.
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.
Therefore the logic needs to be refined so that $success and $trusted are only true if all signatures are valid and trusted, respectively. 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 they are in validpgpkeys).
The signatures for the source tarballs of recent GnuPG releases, e.g. version 2.2.35, are examples of such signature files.
Signed-off-by: Jonas Witschel <diabonas@archlinux.org> --- .../integrity/verify_signature.sh.in | 115 ++++++++++-------- 1 file changed, 65 insertions(+), 50 deletions(-)
diff --git a/scripts/libmakepkg/integrity/verify_signature.sh.in b/scripts/libmakepkg/integrity/verify_signature.sh.in index ad5bb66d..4aa14d94 100644 --- a/scripts/libmakepkg/integrity/verify_signature.sh.in +++ b/scripts/libmakepkg/integrity/verify_signature.sh.in @@ -26,6 +26,18 @@ LIBRARY=${LIBRARY:-'@libmakepkgdir@'} source "$LIBRARY/util/message.sh" source "$LIBRARY/util/pkgbuild.sh"
+# Returns the elements of the first array that are not found in the second +# array as the third array. All arrays are passed by name. +set_difference() {
I believe a better place in libmakepkg would be to move `set_difference` into `scripts/libmakepkg/util/util.sh.in` next to `in_array`, `is_array` and friends and hence make it an exposed util function. Potentially naming it array instead of set would make sense for such an util function, like `array_diff` or `array_difference`.
+ local -n first=$1 + local -n second=$2 + local -n result=$3
namerefs yield an error in bash if they are named equal to a variable in the caller scope. For all nameref's I would suggest to choose something very unique, possible with an underscore prefix. I'd maybe go with something like `_first_diff_array`, `_result_diff_array` or similar vars.
+ result=() + for element in "${first[@]}"; do + ! in_array "$element" "${second[@]}" && result+=("$element") + done +} + check_pgpsigs() { (( SKIPPGPCHECK )) && return 0 ! source_has_signatures && return 0 @@ -56,48 +68,53 @@ check_pgpsigs() { fi
# these variables are assigned values in parse_gpg_statusfile - success=0 - status= - pubkey= - fingerprint= + success= + declare -A status + fingerprints=() trusted= 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 + for pubkey in "${!status[@]}"; do + case "${status["$pubkey"]}" 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 + done errors=1 else + local invalid_keys + set_difference fingerprints validpgpkeys invalid_keys if (( ${#validpgpkeys[@]} == 0 && !trusted )); then - printf "%s ($(gettext "the public key %s is not trusted"))" $(gettext "FAILED") "$fingerprint" >&2 + printf "%s ($(gettext "the public key %s is not trusted"))" $(gettext "FAILED") "${fingerprints[*]}" >&2 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 )) && (( ${#invalid_keys[@]} > 0 )); then + printf "%s (%s %s)" "$(gettext "FAILED")" "$(gettext "invalid public key")" "${invalid_keys[*]}" >&2 errors=1 else printf '%s' "$(gettext "Passed")" >&2 - case "$status" in - "expired") - printf ' (%s)' "$(gettext "WARNING:") $(gettext "the signature has expired.")" >&2 - warnings=1 - ;; - "expiredkey") - printf ' (%s)' "$(gettext "WARNING:") $(gettext "the key has expired.")" >&2 - warnings=1 - ;; - esac + for pubkey in "${!status[@]}"; do + case "${status["$pubkey"]}" in + "expired") + printf " (%s $(gettext "the signature by key %s has expired."))" "$(gettext "WARNING:")" "$pubkey" >&2 + warnings=1 + ;; + "expiredkey") + printf " (%s $(gettext "the key %s has expired."))" "$(gettext "WARNING:")" "$pubkey" >&2 + warnings=1 + ;; + esac + done fi fi printf '\n' >&2 @@ -201,59 +218,57 @@ verify_git_signature() { parse_gpg_statusfile() { local type arg1 arg6 arg10
+ success=-1 + trusted=-1 while read -r _ type arg1 _ _ _ _ arg6 _ _ _ arg10 _; do case "$type" in GOODSIG) - pubkey=$arg1 - success=1 - status="good" + success=$((success!=0)) + status["$arg1"]="good" ;; EXPSIG) - pubkey=$arg1 - success=1 - status="expired" + success=$((success!=0)) + status["$arg1"]="expired" ;; EXPKEYSIG) - pubkey=$arg1 - success=1 - status="expiredkey" + success=$((success!=0)) + status["$arg1"]="expiredkey" ;; REVKEYSIG) - pubkey=$arg1 success=0 - status="revokedkey" + status["$arg1"]="revokedkey" ;; BADSIG) - pubkey=$arg1 success=0 - status="bad" + status["$arg1"]="bad" ;; ERRSIG) - pubkey=$arg1 success=0 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 ;; TRUST_MARGINAL|TRUST_FULLY|TRUST_ULTIMATE) - trusted=1 + trusted=$((trusted!=0)) ;; esac done < "$1" + success=$((success>0)) + trusted=$((trusted>0)) }
source_has_signatures() {