[PATCH] libmakepkg/integrity: handle PGP signature files containing multiple signatures
Levente Polyak
anthraxx at archlinux.org
Wed Jun 8 19:15:59 UTC 2022
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 at 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() {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.archlinux.org/pipermail/pacman-dev/attachments/20220608/49d98af4/attachment.sig>
More information about the pacman-dev
mailing list