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

[Pierre Pronchery <khorben%netbsd.org@localhost>]

The following reply was made to PR pkg/48194; it has been noted by GNATS.

From: Pierre Pronchery <khorben%netbsd.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: Alistair Crooks <agc%pkgsrc.org@localhost>, 
pkg-manager%netbsd.org@localhost, 
 pkgsrc-bugs%netbsd.org@localhost
Subject: Re: pkg/48194: Fixing signed packages in pkg_install and pkgsrc
Date: Tue, 10 Sep 2013 18:25:52 +0200

 This is a multi-part message in MIME format.
 --------------080302090605020106030807
 Content-Type: text/plain; charset=ISO-8859-1
 Content-Transfer-Encoding: 7bit
 
                        Hi there,
 
 On 09/09/2013 05:45, Alistair Crooks wrote:
 > 
 >  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.
 >  
 >  These aren't GPG signatures, they're PGP signatures. gnupg is just one
 >  implementation of PGP.
 
 Is it really so bad to call them GPG signatures and keys? Shouldn't we
 even say "OpenPGP" then instead? In the context of the GPG
 implementation, there are keys and signatures too - hopefully in
 compliance with the standard.
 
 Anyway, I used "GPG" in the patch to be consistent with the existing
 options from pkg_admin(1) and pkg_install.conf(5), which expect an
 implementation of PGP/GPG to be command-line compatible with gnupg.
 
 >  > >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.
 
 Done; the new fix is attached here (for pkg_install in pkgsrc only first).
 
 Cheers,
 -- 
 khorben
 
 --------------080302090605020106030807
 Content-Type: text/plain; charset=ISO-8859-15;
  name="patch-pkg_install_signing.diff"
 Content-Transfer-Encoding: 7bit
 Content-Disposition: attachment;
  filename="patch-pkg_install_signing.diff"
 
 diff --git a/pkgtools/pkg_install/files/lib/pkg_signature.c 
b/pkgtools/pkg_install/files/lib/pkg_signature.c
 index 089234e..79a8092 100644
 --- a/pkgtools/pkg_install/files/lib/pkg_signature.c
 +++ b/pkgtools/pkg_install/files/lib/pkg_signature.c
 @@ -325,10 +325,7 @@ pkg_verify_signature(const char *archive_name, struct 
archive **archive,
  
        *pkgname = NULL;
  
 -      state = xmalloc(sizeof(*state));
 -      state->sign_blocks = NULL;
 -      state->sign_buf = NULL;
 -      state->archive = NULL;
 +      state = xcalloc(sizeof(*state), 1);
  
        r = read_file_from_archive(archive_name, *archive, entry, HASH_FNAME,
            &hash_file, &hash_len);
 
 --------------080302090605020106030807--