diff mbox

[pacman-dev] Check for all return values of _alpm_key_in_keychain

Message ID 20170421040711.GA23261@nibbler
State Superseded, archived
Delegated to: Andrew Gregory
Headers show

Commit Message

David Phillips April 21, 2017, 4:07 a.m. UTC
This fixes a bug I encountered with a GPG keyring where the
key id used to locate a key in the keyring was ambiguous within
my keychain.

This commit ensures that all valid return values are checked to
catch this and related error cases rather than incorrectly taking
an error case to mean the key was found, since this is rarely going
to be the case.
---
 lib/libalpm/be_package.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

Comments

Allan McRae April 24, 2017, 1:06 a.m. UTC | #1
On 21/04/17 14:07, David Phillips wrote:
> This fixes a bug I encountered with a GPG keyring where the
> key id used to locate a key in the keyring was ambiguous within
> my keychain.
> 
> This commit ensures that all valid return values are checked to
> catch this and related error cases rather than incorrectly taking
> an error case to mean the key was found, since this is rarely going
> to be the case.

Can you tell me what happens both before and after this patch with
ambiguous key IDs.   Also, what happens if you only have one of the keys
in the keyring and want to match the other?

Perhaps we also need to look at matching the whole key fingerprint...

Cheers,
Allan
David Phillips April 24, 2017, 2:35 a.m. UTC | #2
Hi,

It now looks like this problem's root cause was a keyring that got corrupted
while I was debugging some problems with refreshing pacman's keyring. That
is to say, it looks like I do not have a genuine "in the wild" key collision.
It looks like my own public key became copied to both me and one of the TUs
in the keyring.

With that said though, I suppose it's still not impossible for long key IDs
to genuinely collide.

On Mon, Apr 24, 2017 at 11:06:05AM +1000, Allan McRae wrote:
> Can you tell me what happens both before and after this patch with
> ambiguous key IDs.

Please find attached two log files before and after. As you can see, pacman
used to blindly run ahead, trying to check the signature even though it could
not get the key from the keychain. A quick look at a backtrace (not attached)
shows this to be a segmentation fault after control is passed to libgpgme.

> Also, what happens if you only have one of the keys
> in the keyring and want to match the other?

I cannot test this, since it turns out to be a corrupt keyring where the key
in question looks to be attached to two people and is exactly the same.

> Perhaps we also need to look at matching the whole key fingerprint...

If the issue with collisions comes up again and doesn't end up being put down
to a corrupt keyring, this would be a good idea. For now, it seems like seeing
this in the wild too narrow a possibility, but there is still a clear hiccup
with tht way pacman behaves before this patch.

Thanks
David
debug: pacman v5.0.1 - libalpm v10.0.1
debug: config: attempting to read file /home/david/pacman-root/etc/pacman.conf
debug: config: new section 'options'
debug: config: HoldPkg: pacman
debug: config: HoldPkg: glibc
debug: config: arch: x86_64
debug: config: finished parsing /home/david/pacman-root/etc/pacman.conf
debug: setup_libalpm called
debug: option 'logfile' = /home/david/pacman-root/var/log/pacman.log
debug: option 'gpgdir' = /home/david/pacman-root/etc/pacman.d/gnupg/
debug: option 'hookdir' = /home/david/pacman-root/etc/pacman.d/hooks/
debug: option 'cachedir' = /home/david/pacman-root/var/cache/pacman/pkg/
debug: GPGME version: 1.9.0
debug: GPGME engine info: file=/usr/bin/gpg2, home=/home/david/pacman-root/etc/pacman.d/gnupg/
debug: looking up key 4BCADD9BD3246795 locally
debug: gpg error: Ambiguous name
debug: sig data: <from .sig>
debug: checking signature for /var/cache/pacman/pkg/metservice-1-1-any.pkg.tar.xz

error: segmentation fault
Please submit a full bug report with --debug if appropriate.
debug: pacman v5.0.1 - libalpm v10.0.1
debug: config: attempting to read file /home/david/pacman-root/etc/pacman.conf
debug: config: new section 'options'
debug: config: HoldPkg: pacman
debug: config: HoldPkg: glibc
debug: config: arch: x86_64
debug: config: finished parsing /home/david/pacman-root/etc/pacman.conf
debug: setup_libalpm called
debug: option 'logfile' = /home/david/pacman-root/var/log/pacman.log
debug: option 'gpgdir' = /home/david/pacman-root/etc/pacman.d/gnupg/
debug: option 'hookdir' = /home/david/pacman-root/etc/pacman.d/hooks/
debug: option 'cachedir' = /home/david/pacman-root/var/cache/pacman/pkg/
debug: GPGME version: 1.9.0
debug: GPGME engine info: file=/usr/bin/gpg2, home=/home/david/pacman-root/etc/pacman.d/gnupg/
debug: looking up key 4BCADD9BD3246795 locally
debug: gpg error: Ambiguous name
error: required key missing from keyring
error: '/var/cache/pacman/pkg/metservice-1-1-any.pkg.tar.xz': unexpected error
debug: unregistering database 'local'
loading packages...
Andrew Gregory Jan. 24, 2019, 9:10 a.m. UTC | #3
On 04/21/17 at 04:07pm, David Phillips wrote:
> This fixes a bug I encountered with a GPG keyring where the
> key id used to locate a key in the keyring was ambiguous within
> my keychain.
> 
> This commit ensures that all valid return values are checked to
> catch this and related error cases rather than incorrectly taking
> an error case to mean the key was found, since this is rarely going
> to be the case.
> ---
>  lib/libalpm/be_package.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c
> index 7e8b7920..1891fa5a 100644
> --- a/lib/libalpm/be_package.c
> +++ b/lib/libalpm/be_package.c
> @@ -754,10 +754,21 @@ int SYMEXPORT alpm_pkg_load(alpm_handle_t *handle, const char *filename, int ful
>  				alpm_list_t *k;
>  				for(k = keys; k; k = k->next) {
>  					char *key = k->data;
> -					if(_alpm_key_in_keychain(handle, key) == 0) {
> -						if(_alpm_key_import(handle, key) == -1) {
> +					switch(_alpm_key_in_keychain(handle, key)) {
> +						case 1:
> +							/* key is known; proceed */
> +							break;
> +						case 0:
> +							/* key is unknown; attempt to import */
> +							if(_alpm_key_import(handle, key) == -1) {
> +								fail = 1;
> +							}
> +							break;
> +						case -1:
> +							/* error finding key in keychain */
> +						default:
>  							fail = 1;

Just below this, an error message is printed if fail is true, saying
that the key is missing.  If we got a generic code, that may not be
the case, and is the exact opposite of the problem that prompted
patch.

I'm wondering if this is necessary at all, though.  This bit of code
is specifically just for importing missing keys.  Any other
significant errors should be caught during the actual validation
attempt, where we already provide the actual gpg error message.  If
the gpgme segmentation fault is no longer an issue, do we gain
anything other than bailing out slightly sooner by adding the extra
check and complexity for printing an appropriate error message here?

> -						}
> +							break;
>  					}
>  				}
>  				FREELIST(keys);
> -- 
> 2.12.2
diff mbox

Patch

diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c
index 7e8b7920..1891fa5a 100644
--- a/lib/libalpm/be_package.c
+++ b/lib/libalpm/be_package.c
@@ -754,10 +754,21 @@  int SYMEXPORT alpm_pkg_load(alpm_handle_t *handle, const char *filename, int ful
 				alpm_list_t *k;
 				for(k = keys; k; k = k->next) {
 					char *key = k->data;
-					if(_alpm_key_in_keychain(handle, key) == 0) {
-						if(_alpm_key_import(handle, key) == -1) {
+					switch(_alpm_key_in_keychain(handle, key)) {
+						case 1:
+							/* key is known; proceed */
+							break;
+						case 0:
+							/* key is unknown; attempt to import */
+							if(_alpm_key_import(handle, key) == -1) {
+								fail = 1;
+							}
+							break;
+						case -1:
+							/* error finding key in keychain */
+						default:
 							fail = 1;
-						}
+							break;
 					}
 				}
 				FREELIST(keys);