libmakepkg/integrity: handle PGP signature files containing multiple signatures

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

Commit Message

Jonas Witschel June 8, 2022, 5:32 p.m. UTC
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.

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(-)

Comments

Levente Polyak June 8, 2022, 7:15 p.m. UTC | #1
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() {
Jonas Witschel June 8, 2022, 9:16 p.m. UTC | #2
Thank you very much for your review, Levente! See below for my answers to the
points you raised.

On 2022-06-08 21:15, Levente Polyak wrote:
> 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.

Having thought about this for a bit, I agree with your reasoning and have
implemented the "any" logic, which I will post as an updated version of the
patch.

> 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`.

Done. Note that due to the changed logic it is not a "set_difference" any more,
but an "arrays_intersect" function because we need to check whether any of the
fingerprints is also in validpgpkeys to find a valid signature.

> > +	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.

The variables are now called _arrays_intersect_{first,second} to avoid naming
collisions.

Best,
Jonas
Allan McRae June 25, 2022, 5:18 a.m. UTC | #3
On 9/6/22 05:15, Levente Polyak wrote:
> 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.

I have read this argument multiple times and I just don't understand it...

Switching to any signature being valid rather than all means we could 
have two signatures saying "bad file, do not use" and one saying "all is 
good", and we trust the good one.  I would not use the source fine in 
this case.

Allan
Jonas Witschel June 25, 2022, 11:44 a.m. UTC | #4
On 2022-06-25 15:18, Allan McRae wrote:
> > 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.
> 
> I have read this argument multiple times and I just don't understand it...
> 
> Switching to any signature being valid rather than all means we could have
> two signatures saying "bad file, do not use" and one saying "all is good",
> and we trust the good one.  I would not use the source fine in this case.

If there is one good and one bad signature, this could mean that one of the two
maintainers has gone rogue and uploaded a malicious release, but was not able
to produce a valid signature by the other maintainer. However, nothing stops
them from just uploading a release that has only a single valid signature made
by their own key: there is no way to tell pacman (or gpgv, for that matter)
that a release must be signed by both keys, since there is no way to specify a
threshold on the number of required signatures. So in this scenario, requiring
both signatures to be valid does not offer increased signature benefits since a
new release with just a single signature would be recognised as valid just as
well.

On the other hand, upstreams might be signing releases with an old, deprecated
key in addition to the current main signing key. In this case, if the old key
gets revoked at some point we would not want to invalidate releases just
because they are still signed by the old key, which is what would happen when
requiring all signatures to be valid.
Even worse, if we require all signatures to be valid, the old key *must* be
added to the validpgpkeys array because a release signed by both keys could not
be validated without trusting that key.
However, a new release signed by only the old key would then also be considered
to be valid, see above. So requiring all keys to be valid forces maintainers to
add less trusted keys as valid just to make the validation pass.

Levente's argument is that we do not gain much from validating both signatures
in the first scenario, while in the second case requiring both signatures even
makes the situation actively worse. Therefore without being able to specify
more detailed key usage policies in the PKGBUILD, trusting any valid signature
seems to be a reasonable default, and is also in line with the current approach
of trusting *any* of the keys in the validpgpkeys array.

One way the situation could be improved is to add a threshold for the number of
required valid signatures. In the first scenario, this threshold could be set
to 2 in order to always require both signatures to be present and valid. In the
second scenario, one would set the threshold to 1 instead and only add the
newer key to validpgpkeys, which prevents releases that are only signed by the
deprecated key to be considered as valid.
However, it is not entirely clear what the default value for this new threshold
would be: setting it to the number of keys in the signature file does not make
sense since this can arbitrarily vary between releases as shown by the first
scenario above. Therefore the only reasonable default threshold would be 1,
which exactly corresponds to the approach taken in v2 of the proposed patch to
require at least one valid signature.
Therefore I would argue that having such a threshold would be more of an
improvement that could be based on v2 of the patch rather than a separate
implementation: it could be easily achieved by changing arrays_intersect() to
return the number of elements in the intersection and to compare that number
with a new threshold variable specified in the PKGBUILD (which would default to
1 if unset in order to be backwards compatible).

Best,
Jonas
Allan McRae June 25, 2022, 12:09 p.m. UTC | #5
On 25/6/22 21:44, Jonas Witschel wrote:

<snip>

> Therefore without being able to specify
> more detailed key usage policies in the PKGBUILD, trusting any valid signature
> seems to be a reasonable default, and is also in line with the current approach
> of trusting *any* of the keys in the validpgpkeys array.

Somewhat offtopic - I'd argue that it is poor packaging to have more 
validpgpkeys in a PKGBUILD than the key used to verify the source for 
that particular package version.  Lazily just adding more keys to a 
PKGBUILD and not removing unneeded ones in case they are needed in the 
future is not ideal.

<snip>

> Therefore I would argue that having such a threshold would be more of an
> improvement that could be based on v2 of the patch rather than a separate
> implementation: it could be easily achieved by changing arrays_intersect() to
> return the number of elements in the intersection and to compare that number
> with a new threshold variable specified in the PKGBUILD (which would default to
> 1 if unset in order to be backwards compatible).

How about...   all signatures from trusted keys (either through PGP web 
of trust, or via being in validpgpkeys) need to validate?

That does not solve the rogue maintainer issue (it is up to a packager 
to ask why the number of signatures dropped...), but it does address not 
needing to validate a legacy key.

Allan
Jonas Witschel June 25, 2022, 1:40 p.m. UTC | #6
On 2022-06-25 22:09, Allan McRae wrote:
> Somewhat offtopic - I'd argue that it is poor packaging to have more
> validpgpkeys in a PKGBUILD than the key used to verify the source for that
> particular package version.  Lazily just adding more keys to a PKGBUILD and
> not removing unneeded ones in case they are needed in the future is not
> ideal.

We should indeed be more careful in actively removing old keys from
validpgpkeys. However some upstreams have multiple active maintainers that
"arbitrarily" sign new releases, in which case keeping all of them in the array
can be helpful to convey an existing relationship of trust (without having to
go through the revision control system to figure out whether the key of a new
release has been already used in the past, or whether some other means of
establishing a chain of trust is necessary).

> How about...   all signatures from trusted keys (either through PGP web of
> trust, or via being in validpgpkeys) need to validate?
> 
> That does not solve the rogue maintainer issue (it is up to a packager to
> ask why the number of signatures dropped...), but it does address not
> needing to validate a legacy key.

From a security standpoint I am fine with this approach (CC Levente for his
opinion).

From an implementation standpoint I will note that this complicates things a
bit due to the structure of the GnuPG status file: for a valid signature, we
are guaranteed to get a VALIDSIG status code containing the full key
fingerprint [1], matching the information that we record in validpgpkeys.
For an invalid signature, we are only guaranteed to get a BADSIG/ERRSIG
containing the (long) key ID of the key [2] (because the key might not even be
available locally and the signature packet alone only contains the key ID
instead of the full fingerprint). So in order to check whether a bad signature
was produced by a key in validpgpkeys, we need to collect the corresponding key
ID and check whether there is a *partial* match with one of the fingerprints in
the validpgpkeys array.

I will note that it is easy to find colliding key IDs [3], so normally one
should not rely on the key ID alone (even if it is the "long" 64 bit instead of
the "short" 32 bit ID). In this case I think using the ID should be fine
though, since we only use it for "negative" matching as an additional security
check rather than relying on the key ID for "positive" matching of trusted
keys: if someone manages to upload a signature file containing a bad signature
with a spoofed key ID, the validation would still fail (rightfully so), while a
good signature from a spoofed key ID would not be considered as valid because
the fingerprint in VALIDSIG does not match.

If this seems like a reasonable approach to everybody, I would be happy to
implement the necessary partial comparison of the key IDs of invalid signatures
in a v3 of the patch. However I would like to hear Levente's opinion first
since it was he who initially brought up the concerns regarding the handling of
files containing multiple signatures.

Best,
Jonas

[1] https://github.com/gpg/gnupg/blob/master/doc/DETAILS#validsig-args
[2] https://github.com/gpg/gnupg/blob/master/doc/DETAILS#badsig--long_keyid_or_fpr--username
[3] https://nullprogram.com/blog/2019/07/22/
Levente Polyak June 26, 2022, 9:46 p.m. UTC | #7
On 6/25/22 15:40, Jonas Witschel wrote:
> On 2022-06-25 22:09, Allan McRae wrote:
>> Somewhat offtopic - I'd argue that it is poor packaging to have more
>> validpgpkeys in a PKGBUILD than the key used to verify the source for that
>> particular package version.  Lazily just adding more keys to a PKGBUILD and
>> not removing unneeded ones in case they are needed in the future is not
>> ideal.
>  > We should indeed be more careful in actively removing old keys from
> validpgpkeys. However some upstreams have multiple active maintainers that
> "arbitrarily" sign new releases, in which case keeping all of them in the array
> can be helpful to convey an existing relationship of trust (without having to
> go through the revision control system to figure out whether the key of a new
> release has been already used in the past, or whether some other means of
> establishing a chain of trust is necessary).
> 

While I agree with old keys should be thrown out and revisited regularly
whether upstream deprecated any, I disagree with only declaring the set
needed for a single particular package version. If we require packagers
to fiddle with the validpgpkeys array for virtually every release in a
multi-maintainer project, this will weaken effective security in the
long run.

Requiring to re-check history and the established chain of trust for the
current set of keys on each iteration will lead to a bigger window for
errors and ultimately lead to fatigue related to carefully handling any
changes to the validpgpkeys array.

When such a security property becomes a burden to maintain, humans by
nature will find a way to make it less annoying. Touching the
validpgpkeys array would become the quick easygoing norm instead of a
careful exception. This has also happened to pinned git sources where
packagers over time simply switched back to refnames -- but this will
hopefully get resolved by the immutable git sources patch.


>> How about...   all signatures from trusted keys (either through PGP web of
>> trust, or via being in validpgpkeys) need to validate?
>>
>> That does not solve the rogue maintainer issue (it is up to a packager to
>> ask why the number of signatures dropped...), but it does address not
>> needing to validate a legacy key.
> >  From a security standpoint I am fine with this approach (CC Levente 
for his
> opinion).
> 
>  From an implementation standpoint I will note that this complicates things a
> bit due to the structure of the GnuPG status file: for a valid signature, we
> are guaranteed to get a VALIDSIG status code containing the full key
> fingerprint [1], matching the information that we record in validpgpkeys.
> For an invalid signature, we are only guaranteed to get a BADSIG/ERRSIG
> containing the (long) key ID of the key [2] (because the key might not even be
> available locally and the signature packet alone only contains the key ID
> instead of the full fingerprint). So in order to check whether a bad signature
> was produced by a key in validpgpkeys, we need to collect the corresponding key
> ID and check whether there is a *partial* match with one of the fingerprints in
> the validpgpkeys array.
> 
> I will note that it is easy to find colliding key IDs [3], so normally one
> should not rely on the key ID alone (even if it is the "long" 64 bit instead of
> the "short" 32 bit ID). In this case I think using the ID should be fine
> though, since we only use it for "negative" matching as an additional security
> check rather than relying on the key ID for "positive" matching of trusted
> keys: if someone manages to upload a signature file containing a bad signature
> with a spoofed key ID, the validation would still fail (rightfully so), while a
> good signature from a spoofed key ID would not be considered as valid because
> the fingerprint in VALIDSIG does not match.
> 
> If this seems like a reasonable approach to everybody, I would be happy to
> implement the necessary partial comparison of the key IDs of invalid signatures
> in a v3 of the patch. However I would like to hear Levente's opinion first
> since it was he who initially brought up the concerns regarding the handling of
> files containing multiple signatures.
> 

This is quite exactly what went through my mind while thinking about the
proposal, hence I second what Jonas summarized.

As Jonas' previous mail (before the one I'm replying to) described: A
rouge maintainer would need to swap both, the tarball as well as the
signature. As a simple alternative, a new version containing just a
single signature could be released. Hence the signature would not need
to contain any BADSIG/ERRSIG from other maintainers in both scenarios.

The only real advantage that would result from the proposed changes
would be if a trusted attacker is able to replace the sources but only
append signatures at storage or in transit. -- which kind of sounds
unlikely.

I currently do not see any major disadvantages with the proposed changes
besides complexity. But considering that the net benefit of this
approach would be uncertain, I feel kind of mixed if that additional
check would bring any effective security.


Cheers,
Levente

Patch

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() {
+	local -n first=$1
+	local -n second=$2
+	local -n result=$3
+	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() {