Re: pkg/48194: Fixing signed packages in pkg_install and pkgsrc

[Alistair Crooks <agc%pkgsrc.org@localhost>]

On Sun, Sep 08, 2013 at 11:30:00PM +0000, Pierre Pronchery wrote:
> >Description:
> pkgsrc has been supporting signed packages since 2001, with mechanisms
> based on either GPG keys or X509 certificates. pkg_add(1) may however
> fail at installing such packages in some conditions, due to
> uninitialized variables in the code used to extract the package signed
> from its container.

Thanks for the PR.

These aren't GPG signatures, they're PGP signatures. gnupg is just one
implementation of PGP.

> >How-To-Repeat:
> This example uses a GPG key, which has to be generated beforehand.
> 
> Configure pkg_install:
> $ cat /etc/pkg_install.conf
> GPG=/home/khorben/bin/gpg
> GPG_SIGN_AS=root%edgebsd.org@localhost
> VERIFIED_INSTALLATION=always
> 
> Sign a package:
> $ mkdir signed
> $ pkg_admin gpg-sign-package digest-20121220.tgz signed/digest-20121220.tgz
> 
> Try to install the resulting package:
> $ pkg_add -v signed/digest-20121220.tgz
> gpg: Signature made Sun Sep  8 03:32:11 2013 UTC using RSA key ID 6F3AF5E2
> gpg: Good signature from "EdgeBSD packages <root%edgebsd.org@localhost>"
> pkg_add: 1 package addition failed
> 
> >Fix:
> 
> X-Git-Url: 
> http://git.edgebsd.org/gitweb/?p=edgebsd-pkgsrc.git;a=commitdiff_plain;h=1a4a18342a5d49ce9a93ab0689b4aa04dfc40847
> 
> Fixed installation of signed packages (uninitialized variables)
> ---
> 
> diff --git a/pkgtools/pkg_install/files/lib/pkg_signature.c 
> b/pkgtools/pkg_install/files/lib/pkg_signature.c
> index 089234e..5e837be 100644
> --- a/pkgtools/pkg_install/files/lib/pkg_signature.c
> +++ b/pkgtools/pkg_install/files/lib/pkg_signature.c
> @@ -326,6 +326,9 @@ pkg_verify_signature(const char *archive_name, struct 
> archive **archive,
>       *pkgname = NULL;
>  
>       state = xmalloc(sizeof(*state));
> +     state->sign_block_len = 0;
> +     state->sign_block_number = 0;
> +     state->sign_cur_block = 0;
>       state->sign_blocks = NULL;
>       state->sign_buf = NULL;
>       state->archive = NULL;

I'd be mode inclined to initialise with:

        state = xcalloc(1, sizeof(*state));

and avoid all the explicit initialisations. Scales better.
 
Regards,
Al